linux-kernel - Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD

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: <7e983702-6662-465d-86ac-d515849d731d@ti.com>
Date: Fri, 30 May 2025 15:15:28 +0530
From: Beleswar Prasad Padhi <b-padhi@...com>
To: Dawei Li <dawei.li@...ux.dev>, <andersson@...nel.org>,
        <mathieu.poirier@...aro.org>
CC: <linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <set_pte_at@...look.com>
Subject: Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL
 uAPI

Hi Dawei,

On 19/05/25 20:38, Dawei Li wrote:
> Implement RPMSG_CREATE_EPT_FD_IOCTL, new uAPI for rpmsg ctrl, which
> shares most of operations of RPMSG_CREATE_EPT_IOCTL except that it
> returns fd representing eptdev to userspace directly.
>
> Possible calling procedures for userspace are:
> - fd = open("/dev/rpmsg_ctrlX")
> - ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &info);
> - fd_ep = info.fd


We are returning a new fd to userspace from inside an IOCTL itself. Is this a
standard way of doing things in Kernel space? (see below related comment)

> - operations on fd_ep(write, read, poll ioctl)
> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> - close(fd_ep)


Can we rely on the userspace to close() the fd_ep? (if not done, could be a
memory leak..).. Opposed to fd, which we can rely on the userspace to
close() since they initiated the open() call. I am just trying to understand if
this is a standard way of doing things...

> - close(fd)
>
> Signed-off-by: Dawei Li <dawei.li@...ux.dev>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 38 ++++++++++++++++++++++++++++++--------
>  include/uapi/linux/rpmsg.h | 24 ++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 28f57945ccd9..9f2f118ceb7b 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -75,19 +75,32 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  				unsigned long arg)
>  {
>  	struct rpmsg_ctrldev *ctrldev = fp->private_data;
> +	struct rpmsg_endpoint_fd_info ept_fd_info;
>  	void __user *argp = (void __user *)arg;
>  	struct rpmsg_endpoint_info eptinfo;
>  	struct rpmsg_channel_info chinfo;
>  	struct rpmsg_device *rpdev;
>  	int ret = 0;
> -
> -	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> -		return -EFAULT;
> -
> -	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> -	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> -	chinfo.src = eptinfo.src;
> -	chinfo.dst = eptinfo.dst;
> +	int fd = -1;
> +
> +	if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
> +	    cmd == RPMSG_RELEASE_DEV_IOCTL) {
> +		if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> +			return -EFAULT;
> +
> +		memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> +		chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> +		chinfo.src = eptinfo.src;
> +		chinfo.dst = eptinfo.dst;
> +	} else if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {


Maybe we can put this 'else if condition' in the first 'if' and treat other
conditions under 'else', as 'RPMSG_CREATE_EPT_FD_IOCTL' is the only
ioctl with a different struct type.

Thanks,
Beleswar

> +		if (copy_from_user(&ept_fd_info, argp, sizeof(ept_fd_info)))
> +			return -EFAULT;
> +
> +		memcpy(chinfo.name, ept_fd_info.name, RPMSG_NAME_SIZE);
> +		chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> +		chinfo.src = ept_fd_info.src;
> +		chinfo.dst = ept_fd_info.dst;
> +	}
>  
>  	mutex_lock(&ctrldev->ctrl_lock);
>  	switch (cmd) {
> @@ -110,6 +123,15 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  				chinfo.name, ret);
>  		break;
>  
> +	case RPMSG_CREATE_EPT_FD_IOCTL:
> +		ret = rpmsg_anonymous_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo,
> +						    ept_fd_info.flags, &fd);
> +		if (!ret) {
> +			ept_fd_info.fd = fd;
> +			ret = copy_to_user(argp, &ept_fd_info, sizeof(ept_fd_info));
> +		}
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  	}
> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> index f0c8da2b185b..e7057bd23577 100644
> --- a/include/uapi/linux/rpmsg.h
> +++ b/include/uapi/linux/rpmsg.h
> @@ -53,4 +53,28 @@ struct rpmsg_endpoint_info {
>   */
>  #define RPMSG_SET_INCOMING_FLOWCONTROL _IOR(0xb5, 0x6, int)
>  
> +/**
> + * struct rpmsg_endpoint_fd_info - endpoint & fd info representation
> + * @name: name of service
> + * @src: local address. To set to RPMSG_ADDR_ANY if not used.
> + * @dst: destination address. To set to RPMSG_ADDR_ANY if not used.
> + * @flags: file flags of endpoint device, valid flags:
> + *         O_RDONLY/O_WRONLY/O_RDWR
> + *         O_NONBLOCK
> + *         O_CLOEXEC
> + * @fd: fd returned from driver
> + */
> +struct rpmsg_endpoint_fd_info {
> +	char name[32];
> +	__u32 src;
> +	__u32 dst;
> +	__u32 flags;
> +	__s32 fd;
> +};
> +
> +/**
> + * Instantiate a new rmpsg endpoint which is represented by fd
> + */
> +#define RPMSG_CREATE_EPT_FD_IOCTL _IOWR(0xb5, 0x7, struct rpmsg_endpoint_fd_info)
> +
>  #endif