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

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

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
>>>>