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

From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com
Subject: Re: [PATCH 3/4] rpmsg: ctrl: Add check on rpmsg device removability from user space
Date: Wed, 16 Jun 2021 11:15:24 -0600	[thread overview]
Message-ID: <20210616171524.GA637642@p14s> (raw)
In-Reply-To: <b7dc5207-643b-b5e6-2bee-106b2eb87555@foss.st.com>

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