linux-kernel - Re: [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl

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: <aX1kAVu7gqM4zI-D@fedora>
Date: Sat, 31 Jan 2026 10:08:01 +0800
From: Ming Lei <ming.lei@...hat.com>
To: Caleb Sander Mateos <csander@...estorage.com>
Cc: Jens Axboe <axboe@...nel.dk>,
	Govindarajulu Varadarajan <govind.varadar@...il.com>,
	linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] ublk: use READ_ONCE() to read struct
 ublksrv_ctrl_cmd

On Fri, Jan 30, 2026 at 10:14:13AM -0700, Caleb Sander Mateos wrote:
> struct ublksrv_ctrl_cmd is part of the io_uring_sqe, which may lie in
> userspace-mapped memory. It's racy to access its fields with normal
> loads, as userspace may write to them concurrently. Use READ_ONCE() to
> copy the ublksrv_ctrl_cmd from the io_uring_sqe to the stack. Use the
> local copy in place of the one in the io_uring_sqe.
> 
> Fixes: 87213b0d847c ("ublk: allow non-blocking ctrl cmds in IO_URING_F_NONBLOCK issue")
> Signed-off-by: Caleb Sander Mateos <csander@...estorage.com>
> ---
>  drivers/block/ublk_drv.c | 56 ++++++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 01088194c8d3..8122b012a7ae 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -4729,16 +4729,15 @@ static int ublk_ctrl_del_dev(struct ublk_device **p_ub, bool wait)
>  	if (wait && wait_event_interruptible(ublk_idr_wq, ublk_idr_freed(idx)))
>  		return -EINTR;
>  	return 0;
>  }
>  
> -static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd)
> +static inline void ublk_ctrl_cmd_dump(u32 cmd_op,
> +				      const struct ublksrv_ctrl_cmd *header)
>  {
> -	const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
> -
>  	pr_devel("%s: cmd_op %x, dev id %d qid %d data %llx buf %llx len %u\n",
> -			__func__, cmd->cmd_op, header->dev_id, header->queue_id,
> +			__func__, cmd_op, header->dev_id, header->queue_id,
>  			header->data[0], header->addr, header->len);
>  }
>  
>  static void ublk_ctrl_stop_dev(struct ublk_device *ub)
>  {
> @@ -5117,13 +5116,12 @@ static int ublk_char_dev_permission(struct ublk_device *ub,
>  	path_put(&path);
>  	return err;
>  }
>  
>  static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
> -		struct io_uring_cmd *cmd)
> +		u32 cmd_op, struct ublksrv_ctrl_cmd *header)
>  {
> -	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)io_uring_sqe_cmd(cmd->sqe);
>  	bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
>  	void __user *argp = (void __user *)(unsigned long)header->addr;
>  	char *dev_path = NULL;
>  	int ret = 0;
>  	int mask;
> @@ -5135,11 +5133,11 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
>  		 * The new added command of UBLK_CMD_GET_DEV_INFO2 includes
>  		 * char_dev_path in payload too, since userspace may not
>  		 * know if the specified device is created as unprivileged
>  		 * mode.
>  		 */
> -		if (_IOC_NR(cmd->cmd_op) != UBLK_CMD_GET_DEV_INFO2)
> +		if (_IOC_NR(cmd_op) != UBLK_CMD_GET_DEV_INFO2)
>  			return 0;
>  	}
>  
>  	/*
>  	 * User has to provide the char device path for unprivileged ublk
> @@ -5156,11 +5154,11 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
>  	dev_path = memdup_user_nul(argp, header->dev_path_len);
>  	if (IS_ERR(dev_path))
>  		return PTR_ERR(dev_path);
>  
>  	ret = -EINVAL;
> -	switch (_IOC_NR(cmd->cmd_op)) {
> +	switch (_IOC_NR(cmd_op)) {
>  	case UBLK_CMD_GET_DEV_INFO:
>  	case UBLK_CMD_GET_DEV_INFO2:
>  	case UBLK_CMD_GET_QUEUE_AFFINITY:
>  	case UBLK_CMD_GET_PARAMS:
>  	case (_IOC_NR(UBLK_U_CMD_GET_FEATURES)):
> @@ -5186,11 +5184,11 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
>  	if (!ret) {
>  		header->len -= header->dev_path_len;
>  		header->addr += header->dev_path_len;
>  	}
>  	pr_devel("%s: dev id %d cmd_op %x uid %d gid %d path %s ret %d\n",
> -			__func__, ub->ub_number, cmd->cmd_op,
> +			__func__, ub->ub_number, cmd_op,
>  			ub->dev_info.owner_uid, ub->dev_info.owner_gid,
>  			dev_path, ret);
>  exit:
>  	kfree(dev_path);
>  	return ret;
> @@ -5210,11 +5208,13 @@ static bool ublk_ctrl_uring_cmd_may_sleep(u32 cmd_op)
>  }
>  
>  static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
>  		unsigned int issue_flags)
>  {
> -	const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
> +	/* May point to userspace-mapped memory */
> +	const struct ublksrv_ctrl_cmd *ub_src = io_uring_sqe_cmd(cmd->sqe);
> +	struct ublksrv_ctrl_cmd header;

It is cleaner to initialize header variable here, otherwise this patch
looks fine:

Reviewed-by: Ming Lei <ming.lei@...hat.com>


Thanks, 
Ming