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