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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210615174634.GB604521@p14s>
Date:   Tue, 15 Jun 2021 11:46:34 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Ohad Ben-Cohen <ohad@...ery.com>,
        linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH 3/4] rpmsg: ctrl: Add check on rpmsg device removability
 from user space

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@...s.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
>