| 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 |
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 >
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 >>
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 > >>
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 >>>>
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 >>>>>
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 --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; };
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(-)