diff mbox series

[3/4] rpmsg: ctrl: Add check on rpmsg device removability from user space

Message ID 20210604091406.15901-4-arnaud.pouliquen@foss.st.com (mailing list archive)
State Superseded
Headers show
Series rpmsg: ctrl: Add ability to instantiate rpmsg device locally | expand

Commit Message

Arnaud Pouliquen June 4, 2021, 9:14 a.m. UTC
Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).

Add a new field to store the removability of the device.

By default the rpmsg device can not be removed by user space. It is
set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
could also be set by an rpmsg driver during probe.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
 include/linux/rpmsg.h      |  2 ++
 2 files changed, 18 insertions(+), 1 deletion(-)

Comments

Mathieu Poirier June 15, 2021, 5:46 p.m. UTC | #1
On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
> 
> Add a new field to store the removability of the device.
> 
> By default the rpmsg device can not be removed by user space. It is
> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
> could also be set by an rpmsg driver during probe.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
>  include/linux/rpmsg.h      |  2 ++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index cb19e32d05e1..e93c6ec49038 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  	struct rpmsg_endpoint_info eptinfo;
>  	struct rpmsg_channel_info chinfo;
>  	struct rpmsg_device *rpdev;
> +	struct device *dev;
>  	int ret = 0;
>  
>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  		if (!rpdev) {
>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>  			ret = -ENXIO;
> +		} else {
> +			/* Allow user space to release the device. */
> +			rpdev->us_removable = 1;

As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
can attain the same result by looking at chinfo->name, chinfo->src and
chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
operation is refused.

That way we don't introduce a new flag and there is also no need to call
rpmsg_find_device() twice.

Thanks,
Mathieu

>  		}
>  		break;
>  
>  	case RPMSG_RELEASE_DEV_IOCTL:
> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
> +		if (!dev)
> +			ret =  -ENXIO;
> +
> +		/* Verify that rpmsg device removal is allowed. */
> +		if (!ret) {
> +			rpdev = to_rpmsg_device(dev);
> +			if (!rpdev->us_removable)
> +				ret = -EACCES;
> +		}
> +		if (!ret)
> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>  		if (ret)
>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
>  				chinfo.name, ret);
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index d97dcd049f18..3642aad1a789 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
>   * @ept: the rpmsg endpoint of this channel
>   * @announce: if set, rpmsg will announce the creation/removal of this channel
>   * @little_endian: True if transport is using little endian byte representation
> + * @us_removable: True if userspace application has permission to remove the rpmsg device
>   */
>  struct rpmsg_device {
>  	struct device dev;
> @@ -57,6 +58,7 @@ struct rpmsg_device {
>  	struct rpmsg_endpoint *ept;
>  	bool announce;
>  	bool little_endian;
> +	bool us_removable;
>  
>  	const struct rpmsg_device_ops *ops;
>  };
> -- 
> 2.17.1
>
Arnaud Pouliquen June 16, 2021, 9:30 a.m. UTC | #2
On 6/15/21 7:46 PM, Mathieu Poirier wrote:
> On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
>> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
>> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
>>
>> Add a new field to store the removability of the device.
>>
>> By default the rpmsg device can not be removed by user space. It is
>> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
>> could also be set by an rpmsg driver during probe.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
>>  include/linux/rpmsg.h      |  2 ++
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> index cb19e32d05e1..e93c6ec49038 100644
>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>  	struct rpmsg_endpoint_info eptinfo;
>>  	struct rpmsg_channel_info chinfo;
>>  	struct rpmsg_device *rpdev;
>> +	struct device *dev;
>>  	int ret = 0;
>>  
>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>  		if (!rpdev) {
>>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>>  			ret = -ENXIO;
>> +		} else {
>> +			/* Allow user space to release the device. */
>> +			rpdev->us_removable = 1;
> 
> As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
> can attain the same result by looking at chinfo->name, chinfo->src and
> chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
> something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
> to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
> operation is refused.

Something must have escaped me, because i turn around your your proposal,
without understand it.

The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
that have not to be released by user application. Either because there are core
( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
rpmsg devices.

look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
by the RPMSG_RELEASE_DEV_IOCTL.

Please, could you clarify what you have in mind here?

Thanks,
Arnaud

> 
> That way we don't introduce a new flag and there is also no need to call
> rpmsg_find_device() twice.



> 
> Thanks,
> Mathieu
> 
>>  		}
>>  		break;
>>  
>>  	case RPMSG_RELEASE_DEV_IOCTL:
>> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
>> +		if (!dev)
>> +			ret =  -ENXIO;
>> +
>> +		/* Verify that rpmsg device removal is allowed. */
>> +		if (!ret) {
>> +			rpdev = to_rpmsg_device(dev);
>> +			if (!rpdev->us_removable)
>> +				ret = -EACCES;
>> +		}
>> +		if (!ret)
>> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>  		if (ret)
>>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
>>  				chinfo.name, ret);
>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>> index d97dcd049f18..3642aad1a789 100644
>> --- a/include/linux/rpmsg.h
>> +++ b/include/linux/rpmsg.h
>> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
>>   * @ept: the rpmsg endpoint of this channel
>>   * @announce: if set, rpmsg will announce the creation/removal of this channel
>>   * @little_endian: True if transport is using little endian byte representation
>> + * @us_removable: True if userspace application has permission to remove the rpmsg device
>>   */
>>  struct rpmsg_device {
>>  	struct device dev;
>> @@ -57,6 +58,7 @@ struct rpmsg_device {
>>  	struct rpmsg_endpoint *ept;
>>  	bool announce;
>>  	bool little_endian;
>> +	bool us_removable;
>>  
>>  	const struct rpmsg_device_ops *ops;
>>  };
>> -- 
>> 2.17.1
>>
Mathieu Poirier June 16, 2021, 5:15 p.m. UTC | #3
On Wed, Jun 16, 2021 at 11:30:51AM +0200, Arnaud POULIQUEN wrote:
> 
> 
> On 6/15/21 7:46 PM, Mathieu Poirier wrote:
> > On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
> >> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
> >> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
> >>
> >> Add a new field to store the removability of the device.
> >>
> >> By default the rpmsg device can not be removed by user space. It is
> >> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
> >> could also be set by an rpmsg driver during probe.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
> >>  include/linux/rpmsg.h      |  2 ++
> >>  2 files changed, 18 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> >> index cb19e32d05e1..e93c6ec49038 100644
> >> --- a/drivers/rpmsg/rpmsg_ctrl.c
> >> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> >> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >>  	struct rpmsg_endpoint_info eptinfo;
> >>  	struct rpmsg_channel_info chinfo;
> >>  	struct rpmsg_device *rpdev;
> >> +	struct device *dev;
> >>  	int ret = 0;
> >>  
> >>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> >> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >>  		if (!rpdev) {
> >>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
> >>  			ret = -ENXIO;
> >> +		} else {
> >> +			/* Allow user space to release the device. */
> >> +			rpdev->us_removable = 1;
> > 
> > As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
> > can attain the same result by looking at chinfo->name, chinfo->src and
> > chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
> > something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
> > to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
> > operation is refused.
> 
> Something must have escaped me, because i turn around your your proposal,
> without understand it.
> 
> The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
> that have not to be released by user application. Either because there are core
> ( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
> rpmsg devices.
>

I don't see how the current patch would allow a driver to prevent user space
from releasing a rpmsg device since the sysfs attribute can be changed at will.
So even if the driver sets the flag user space can still revert it.

> look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
> by the RPMSG_RELEASE_DEV_IOCTL.

That is correct.  I did not address rpmsg_ns to keep things simple but it would
also have to be handled properly.

> 
> Please, could you clarify what you have in mind here?

Other than rpmsg_ctrl and rpmsg_ns I don't think we should introduce any
mechanism to prevent users from releasing an rpmsg.  Doing so needs root access
- if a user space process with root privileges can't be trusted then we have
bigger problems than unwanted releases of registered rpmsg devices.

> 
> Thanks,
> Arnaud
> 
> > 
> > That way we don't introduce a new flag and there is also no need to call
> > rpmsg_find_device() twice.
> 
> 
> 
> > 
> > Thanks,
> > Mathieu
> > 
> >>  		}
> >>  		break;
> >>  
> >>  	case RPMSG_RELEASE_DEV_IOCTL:
> >> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> >> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
> >> +		if (!dev)
> >> +			ret =  -ENXIO;
> >> +
> >> +		/* Verify that rpmsg device removal is allowed. */
> >> +		if (!ret) {
> >> +			rpdev = to_rpmsg_device(dev);
> >> +			if (!rpdev->us_removable)
> >> +				ret = -EACCES;
> >> +		}
> >> +		if (!ret)
> >> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> >>  		if (ret)
> >>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
> >>  				chinfo.name, ret);
> >> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> >> index d97dcd049f18..3642aad1a789 100644
> >> --- a/include/linux/rpmsg.h
> >> +++ b/include/linux/rpmsg.h
> >> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
> >>   * @ept: the rpmsg endpoint of this channel
> >>   * @announce: if set, rpmsg will announce the creation/removal of this channel
> >>   * @little_endian: True if transport is using little endian byte representation
> >> + * @us_removable: True if userspace application has permission to remove the rpmsg device
> >>   */
> >>  struct rpmsg_device {
> >>  	struct device dev;
> >> @@ -57,6 +58,7 @@ struct rpmsg_device {
> >>  	struct rpmsg_endpoint *ept;
> >>  	bool announce;
> >>  	bool little_endian;
> >> +	bool us_removable;
> >>  
> >>  	const struct rpmsg_device_ops *ops;
> >>  };
> >> -- 
> >> 2.17.1
> >>
Arnaud Pouliquen June 17, 2021, 8:02 a.m. UTC | #4
Hello Mathieu,

On 6/16/21 7:15 PM, Mathieu Poirier wrote:
> On Wed, Jun 16, 2021 at 11:30:51AM +0200, Arnaud POULIQUEN wrote:
>>
>>
>> On 6/15/21 7:46 PM, Mathieu Poirier wrote:
>>> On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
>>>> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
>>>> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
>>>>
>>>> Add a new field to store the removability of the device.
>>>>
>>>> By default the rpmsg device can not be removed by user space. It is
>>>> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
>>>> could also be set by an rpmsg driver during probe.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> ---
>>>>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
>>>>  include/linux/rpmsg.h      |  2 ++
>>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>>>> index cb19e32d05e1..e93c6ec49038 100644
>>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>>>> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>>  	struct rpmsg_endpoint_info eptinfo;
>>>>  	struct rpmsg_channel_info chinfo;
>>>>  	struct rpmsg_device *rpdev;
>>>> +	struct device *dev;
>>>>  	int ret = 0;
>>>>  
>>>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>>> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>>  		if (!rpdev) {
>>>>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>>>>  			ret = -ENXIO;
>>>> +		} else {
>>>> +			/* Allow user space to release the device. */
>>>> +			rpdev->us_removable = 1;
>>>
>>> As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
>>> can attain the same result by looking at chinfo->name, chinfo->src and
>>> chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
>>> something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
>>> to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
>>> operation is refused.
>>
>> Something must have escaped me, because i turn around your your proposal,
>> without understand it.
>>
>> The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
>> that have not to be released by user application. Either because there are core
>> ( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
>> rpmsg devices.
>>
> 
> I don't see how the current patch would allow a driver to prevent user space
> from releasing a rpmsg device since the sysfs attribute can be changed at will.
> So even if the driver sets the flag user space can still revert it.


The patch [4/4] define the a read only attribute using the rpmsg_show_attr
declaration[1]. So the userspace can't change it.

This also has the advantage of not allowing the new IOCTRL API to be used by
default for legacy RPMSg devices without a specific patch.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_core.c#L362

> 
>> look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
>> by the RPMSG_RELEASE_DEV_IOCTL.
> 
> That is correct.  I did not address rpmsg_ns to keep things simple but it would
> also have to be handled properly.
> 
>>
>> Please, could you clarify what you have in mind here?
> 
> Other than rpmsg_ctrl and rpmsg_ns I don't think we should introduce any
> mechanism to prevent users from releasing an rpmsg.  Doing so needs root access
> - if a user space process with root privileges can't be trusted then we have
> bigger problems than unwanted releases of registered rpmsg devices.

That's make sense. If we go on this way we could also trust the root application
for the rpmsg_ns and only protect the rpmsg_ctrl which can not release itself,
as you proposed.

Thanks,

Arnaud

> 
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> That way we don't introduce a new flag and there is also no need to call
>>> rpmsg_find_device() twice.
>>
>>
>>
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>>  		}
>>>>  		break;
>>>>  
>>>>  	case RPMSG_RELEASE_DEV_IOCTL:
>>>> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>>> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
>>>> +		if (!dev)
>>>> +			ret =  -ENXIO;
>>>> +
>>>> +		/* Verify that rpmsg device removal is allowed. */
>>>> +		if (!ret) {
>>>> +			rpdev = to_rpmsg_device(dev);
>>>> +			if (!rpdev->us_removable)
>>>> +				ret = -EACCES;
>>>> +		}
>>>> +		if (!ret)
>>>> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>>>  		if (ret)
>>>>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
>>>>  				chinfo.name, ret);
>>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>> index d97dcd049f18..3642aad1a789 100644
>>>> --- a/include/linux/rpmsg.h
>>>> +++ b/include/linux/rpmsg.h
>>>> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
>>>>   * @ept: the rpmsg endpoint of this channel
>>>>   * @announce: if set, rpmsg will announce the creation/removal of this channel
>>>>   * @little_endian: True if transport is using little endian byte representation
>>>> + * @us_removable: True if userspace application has permission to remove the rpmsg device
>>>>   */
>>>>  struct rpmsg_device {
>>>>  	struct device dev;
>>>> @@ -57,6 +58,7 @@ struct rpmsg_device {
>>>>  	struct rpmsg_endpoint *ept;
>>>>  	bool announce;
>>>>  	bool little_endian;
>>>> +	bool us_removable;
>>>>  
>>>>  	const struct rpmsg_device_ops *ops;
>>>>  };
>>>> -- 
>>>> 2.17.1
>>>>
Arnaud Pouliquen June 17, 2021, 4:55 p.m. UTC | #5
On 6/17/21 10:02 AM, Arnaud POULIQUEN wrote:
> Hello Mathieu,
> 
> On 6/16/21 7:15 PM, Mathieu Poirier wrote:
>> On Wed, Jun 16, 2021 at 11:30:51AM +0200, Arnaud POULIQUEN wrote:
>>>
>>>
>>> On 6/15/21 7:46 PM, Mathieu Poirier wrote:
>>>> On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
>>>>> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
>>>>> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
>>>>>
>>>>> Add a new field to store the removability of the device.
>>>>>
>>>>> By default the rpmsg device can not be removed by user space. It is
>>>>> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
>>>>> could also be set by an rpmsg driver during probe.
>>>>>
>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>>> ---
>>>>>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
>>>>>  include/linux/rpmsg.h      |  2 ++
>>>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>>>>> index cb19e32d05e1..e93c6ec49038 100644
>>>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
>>>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>>>>> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>>>  	struct rpmsg_endpoint_info eptinfo;
>>>>>  	struct rpmsg_channel_info chinfo;
>>>>>  	struct rpmsg_device *rpdev;
>>>>> +	struct device *dev;
>>>>>  	int ret = 0;
>>>>>  
>>>>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>>>>> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>>>>  		if (!rpdev) {
>>>>>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
>>>>>  			ret = -ENXIO;
>>>>> +		} else {
>>>>> +			/* Allow user space to release the device. */
>>>>> +			rpdev->us_removable = 1;
>>>>
>>>> As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
>>>> can attain the same result by looking at chinfo->name, chinfo->src and
>>>> chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
>>>> something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
>>>> to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
>>>> operation is refused.
>>>
>>> Something must have escaped me, because i turn around your your proposal,
>>> without understand it.
>>>
>>> The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
>>> that have not to be released by user application. Either because there are core
>>> ( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
>>> rpmsg devices.
>>>
>>
>> I don't see how the current patch would allow a driver to prevent user space
>> from releasing a rpmsg device since the sysfs attribute can be changed at will.
>> So even if the driver sets the flag user space can still revert it.
> 
> 
> The patch [4/4] define the a read only attribute using the rpmsg_show_attr
> declaration[1]. So the userspace can't change it.
> 
> This also has the advantage of not allowing the new IOCTRL API to be used by
> default for legacy RPMSg devices without a specific patch.
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_core.c#L362
> 
>>
>>> look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
>>> by the RPMSG_RELEASE_DEV_IOCTL.
>>
>> That is correct.  I did not address rpmsg_ns to keep things simple but it would
>> also have to be handled properly.
>>
>>>
>>> Please, could you clarify what you have in mind here?
>>
>> Other than rpmsg_ctrl and rpmsg_ns I don't think we should introduce any
>> mechanism to prevent users from releasing an rpmsg.  Doing so needs root access
>> - if a user space process with root privileges can't be trusted then we have
>> bigger problems than unwanted releases of registered rpmsg devices.
> 
> That's make sense. If we go on this way we could also trust the root application
> for the rpmsg_ns and only protect the rpmsg_ctrl which can not release itself,
> as you proposed.

As discussed in the OpenAMP by-weekly meeting, I will send a new revision,
without the attribute.

Thanks,
Arnaud

> 
> Thanks,
> 
> Arnaud
> 
>>
>>>
>>> Thanks,
>>> Arnaud
>>>
>>>>
>>>> That way we don't introduce a new flag and there is also no need to call
>>>> rpmsg_find_device() twice.
>>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Mathieu
>>>>
>>>>>  		}
>>>>>  		break;
>>>>>  
>>>>>  	case RPMSG_RELEASE_DEV_IOCTL:
>>>>> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>>>> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
>>>>> +		if (!dev)
>>>>> +			ret =  -ENXIO;
>>>>> +
>>>>> +		/* Verify that rpmsg device removal is allowed. */
>>>>> +		if (!ret) {
>>>>> +			rpdev = to_rpmsg_device(dev);
>>>>> +			if (!rpdev->us_removable)
>>>>> +				ret = -EACCES;
>>>>> +		}
>>>>> +		if (!ret)
>>>>> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
>>>>>  		if (ret)
>>>>>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
>>>>>  				chinfo.name, ret);
>>>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>>> index d97dcd049f18..3642aad1a789 100644
>>>>> --- a/include/linux/rpmsg.h
>>>>> +++ b/include/linux/rpmsg.h
>>>>> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
>>>>>   * @ept: the rpmsg endpoint of this channel
>>>>>   * @announce: if set, rpmsg will announce the creation/removal of this channel
>>>>>   * @little_endian: True if transport is using little endian byte representation
>>>>> + * @us_removable: True if userspace application has permission to remove the rpmsg device
>>>>>   */
>>>>>  struct rpmsg_device {
>>>>>  	struct device dev;
>>>>> @@ -57,6 +58,7 @@ struct rpmsg_device {
>>>>>  	struct rpmsg_endpoint *ept;
>>>>>  	bool announce;
>>>>>  	bool little_endian;
>>>>> +	bool us_removable;
>>>>>  
>>>>>  	const struct rpmsg_device_ops *ops;
>>>>>  };
>>>>> -- 
>>>>> 2.17.1
>>>>>
Mathieu Poirier June 17, 2021, 4:56 p.m. UTC | #6
On Thu, Jun 17, 2021 at 10:02:14AM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
> 
> On 6/16/21 7:15 PM, Mathieu Poirier wrote:
> > On Wed, Jun 16, 2021 at 11:30:51AM +0200, Arnaud POULIQUEN wrote:
> >>
> >>
> >> On 6/15/21 7:46 PM, Mathieu Poirier wrote:
> >>> On Fri, Jun 04, 2021 at 11:14:05AM +0200, Arnaud Pouliquen wrote:
> >>>> Using the RPMSG_RELEASE_DEV_IOCTL is possible to remove any
> >>>> rpmsg device (such as the rpmsg ns or the rpmsg ctrldev).
> >>>>
> >>>> Add a new field to store the removability of the device.
> >>>>
> >>>> By default the rpmsg device can not be removed by user space. It is
> >>>> set to 1 by the rpmsg ctrl on RPMSG_CREATE_DEV_IOCTL request, but
> >>>> could also be set by an rpmsg driver during probe.
> >>>>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>>> ---
> >>>>  drivers/rpmsg/rpmsg_ctrl.c | 17 ++++++++++++++++-
> >>>>  include/linux/rpmsg.h      |  2 ++
> >>>>  2 files changed, 18 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> >>>> index cb19e32d05e1..e93c6ec49038 100644
> >>>> --- a/drivers/rpmsg/rpmsg_ctrl.c
> >>>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> >>>> @@ -74,6 +74,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >>>>  	struct rpmsg_endpoint_info eptinfo;
> >>>>  	struct rpmsg_channel_info chinfo;
> >>>>  	struct rpmsg_device *rpdev;
> >>>> +	struct device *dev;
> >>>>  	int ret = 0;
> >>>>  
> >>>>  	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> >>>> @@ -95,11 +96,25 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >>>>  		if (!rpdev) {
> >>>>  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
> >>>>  			ret = -ENXIO;
> >>>> +		} else {
> >>>> +			/* Allow user space to release the device. */
> >>>> +			rpdev->us_removable = 1;
> >>>
> >>> As a rule of thumb I try really hard to avoid introducing new flags.  In this case we
> >>> can attain the same result by looking at chinfo->name, chinfo->src and
> >>> chinfo->dst.  I would introduce a new inline function in rpmsg_internal.h,
> >>> something like rpmsg_chrdev_is_ctrl_dev(), and compare the specifics in chinfo
> >>> to rpdev->id.name, rpdev->src and rpdev->dst.  If they all match then the
> >>> operation is refused.
> >>
> >> Something must have escaped me, because i turn around your your proposal,
> >> without understand it.
> >>
> >> The "us_removable" flag is not only for the rpmsg_ctrl, but for any rpmsg device
> >> that have not to be released by user application. Either because there are core
> >> ( rpmsg_ctrl, rpmsg_ns) or because a rpmsg driver don't allow to unbind its
> >> rpmsg devices.
> >>
> > 
> > I don't see how the current patch would allow a driver to prevent user space
> > from releasing a rpmsg device since the sysfs attribute can be changed at will.
> > So even if the driver sets the flag user space can still revert it.
> 
> 
> The patch [4/4] define the a read only attribute using the rpmsg_show_attr
> declaration[1]. So the userspace can't change it.
> 

You are correct - I overlooked the RO attribute in the rpmsg_show_attr() macro.

> This also has the advantage of not allowing the new IOCTRL API to be used by
> default for legacy RPMSg devices without a specific patch.
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_core.c#L362
> 
> > 
> >> look to me that rpmsg_chrdev_is_ctrl_dev just prevents rpmsg ctrl to be released
> >> by the RPMSG_RELEASE_DEV_IOCTL.
> > 
> > That is correct.  I did not address rpmsg_ns to keep things simple but it would
> > also have to be handled properly.
> > 
> >>
> >> Please, could you clarify what you have in mind here?
> > 
> > Other than rpmsg_ctrl and rpmsg_ns I don't think we should introduce any
> > mechanism to prevent users from releasing an rpmsg.  Doing so needs root access
> > - if a user space process with root privileges can't be trusted then we have
> > bigger problems than unwanted releases of registered rpmsg devices.
> 
> That's make sense. If we go on this way we could also trust the root application
> for the rpmsg_ns and only protect the rpmsg_ctrl which can not release itself,
> as you proposed.

I think we should protect both of them or neither of them.  I'd be fine with either
solution.

> 
> Thanks,
> 
> Arnaud
> 
> > 
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>>
> >>> That way we don't introduce a new flag and there is also no need to call
> >>> rpmsg_find_device() twice.
> >>
> >>
> >>
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>>  		}
> >>>>  		break;
> >>>>  
> >>>>  	case RPMSG_RELEASE_DEV_IOCTL:
> >>>> -		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> >>>> +		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
> >>>> +		if (!dev)
> >>>> +			ret =  -ENXIO;
> >>>> +
> >>>> +		/* Verify that rpmsg device removal is allowed. */
> >>>> +		if (!ret) {
> >>>> +			rpdev = to_rpmsg_device(dev);
> >>>> +			if (!rpdev->us_removable)
> >>>> +				ret = -EACCES;
> >>>> +		}
> >>>> +		if (!ret)
> >>>> +			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
> >>>>  		if (ret)
> >>>>  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
> >>>>  				chinfo.name, ret);
> >>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> >>>> index d97dcd049f18..3642aad1a789 100644
> >>>> --- a/include/linux/rpmsg.h
> >>>> +++ b/include/linux/rpmsg.h
> >>>> @@ -47,6 +47,7 @@ struct rpmsg_channel_info {
> >>>>   * @ept: the rpmsg endpoint of this channel
> >>>>   * @announce: if set, rpmsg will announce the creation/removal of this channel
> >>>>   * @little_endian: True if transport is using little endian byte representation
> >>>> + * @us_removable: True if userspace application has permission to remove the rpmsg device
> >>>>   */
> >>>>  struct rpmsg_device {
> >>>>  	struct device dev;
> >>>> @@ -57,6 +58,7 @@ struct rpmsg_device {
> >>>>  	struct rpmsg_endpoint *ept;
> >>>>  	bool announce;
> >>>>  	bool little_endian;
> >>>> +	bool us_removable;
> >>>>  
> >>>>  	const struct rpmsg_device_ops *ops;
> >>>>  };
> >>>> -- 
> >>>> 2.17.1
> >>>>
diff mbox series

Patch

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index cb19e32d05e1..e93c6ec49038 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -74,6 +74,7 @@  static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 	struct rpmsg_endpoint_info eptinfo;
 	struct rpmsg_channel_info chinfo;
 	struct rpmsg_device *rpdev;
+	struct device *dev;
 	int ret = 0;
 
 	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
@@ -95,11 +96,25 @@  static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 		if (!rpdev) {
 			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
 			ret = -ENXIO;
+		} else {
+			/* Allow user space to release the device. */
+			rpdev->us_removable = 1;
 		}
 		break;
 
 	case RPMSG_RELEASE_DEV_IOCTL:
-		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
+		dev = rpmsg_find_device(ctrldev->rpdev->dev.parent, &chinfo);
+		if (!dev)
+			ret =  -ENXIO;
+
+		/* Verify that rpmsg device removal is allowed. */
+		if (!ret) {
+			rpdev = to_rpmsg_device(dev);
+			if (!rpdev->us_removable)
+				ret = -EACCES;
+		}
+		if (!ret)
+			ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
 		if (ret)
 			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
 				chinfo.name, ret);
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index d97dcd049f18..3642aad1a789 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -47,6 +47,7 @@  struct rpmsg_channel_info {
  * @ept: the rpmsg endpoint of this channel
  * @announce: if set, rpmsg will announce the creation/removal of this channel
  * @little_endian: True if transport is using little endian byte representation
+ * @us_removable: True if userspace application has permission to remove the rpmsg device
  */
 struct rpmsg_device {
 	struct device dev;
@@ -57,6 +58,7 @@  struct rpmsg_device {
 	struct rpmsg_endpoint *ept;
 	bool announce;
 	bool little_endian;
+	bool us_removable;
 
 	const struct rpmsg_device_ops *ops;
 };