[PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses

[PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses

[Caleb Sander Mateos]

struct ublksrv_ctrl_cmd is part of the io_uring_sqe. Since commit
87213b0d847c ("ublk: allow non-blocking ctrl cmds in IO_URING_F_NONBLOCK
issue") allowed some commands to be handled in the non-blocking issue,
the SQE may lie in userspace-mapped memory. Validate that the SQE size
is the expected 128 bytes before dereferencing it. Use READ_ONCE() to
copy the ublksrv_ctrl_cmd from the SQE to a local variable. This avoids
data races if userspace writes to the SQE concurrently.

v2:
- Make a local copy of the struct ublksrv_ctrl_cmd (Ming)
- Add Reviewed-by tag (Ming)

Caleb Sander Mateos (2):
  ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd
  ublk: drop ublk_ctrl_start_recovery() header argument

Govindarajulu Varadarajan (1):
  ublk: Validate SQE128 flag before accessing the cmd

 drivers/block/ublk_drv.c | 65 +++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

-- 
2.45.2

[PATCH v2 1/3] ublk: Validate SQE128 flag before accessing the cmd

[Caleb Sander Mateos]

From: Govindarajulu Varadarajan <govind.varadar@gmail.com>

ublk_ctrl_cmd_dump() accesses (header *)sqe->cmd before
IO_URING_F_SQE128 flag check. This could cause out of boundary memory
access.

Move the SQE128 flag check earlier in ublk_ctrl_uring_cmd() to return
-EINVAL immediately if the flag is not set.

Fixes: 71f28f3136af ("ublk_drv: add io_uring based userspace block driver")
Signed-off-by: Govindarajulu Varadarajan <govind.varadar@gmail.com>
Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 5efaf53261ce..01088194c8d3 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -5219,14 +5219,14 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 
 	if (ublk_ctrl_uring_cmd_may_sleep(cmd_op) &&
 	    issue_flags & IO_URING_F_NONBLOCK)
 		return -EAGAIN;
 
-	ublk_ctrl_cmd_dump(cmd);
-
 	if (!(issue_flags & IO_URING_F_SQE128))
-		goto out;
+		return -EINVAL;
+
+	ublk_ctrl_cmd_dump(cmd);
 
 	ret = ublk_check_cmd_op(cmd_op);
 	if (ret)
 		goto out;
 
-- 
2.45.2

[PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd

[Caleb Sander Mateos]

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@purestorage.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;
 	struct ublk_device *ub = NULL;
 	u32 cmd_op = cmd->cmd_op;
 	int ret = -EINVAL;
 
 	if (ublk_ctrl_uring_cmd_may_sleep(cmd_op) &&
@@ -5222,74 +5222,80 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 		return -EAGAIN;
 
 	if (!(issue_flags & IO_URING_F_SQE128))
 		return -EINVAL;
 
-	ublk_ctrl_cmd_dump(cmd);
+	header.dev_id = READ_ONCE(ub_src->dev_id);
+	header.queue_id = READ_ONCE(ub_src->queue_id);
+	header.len = READ_ONCE(ub_src->len);
+	header.addr = READ_ONCE(ub_src->addr);
+	header.data[0] = READ_ONCE(ub_src->data[0]);
+	header.dev_path_len = READ_ONCE(ub_src->dev_path_len);
+	ublk_ctrl_cmd_dump(cmd_op, &header);
 
 	ret = ublk_check_cmd_op(cmd_op);
 	if (ret)
 		goto out;
 
 	if (cmd_op == UBLK_U_CMD_GET_FEATURES) {
-		ret = ublk_ctrl_get_features(header);
+		ret = ublk_ctrl_get_features(&header);
 		goto out;
 	}
 
 	if (_IOC_NR(cmd_op) != UBLK_CMD_ADD_DEV) {
 		ret = -ENODEV;
-		ub = ublk_get_device_from_id(header->dev_id);
+		ub = ublk_get_device_from_id(header.dev_id);
 		if (!ub)
 			goto out;
 
-		ret = ublk_ctrl_uring_cmd_permission(ub, cmd);
+		ret = ublk_ctrl_uring_cmd_permission(ub, cmd_op, &header);
 		if (ret)
 			goto put_dev;
 	}
 
 	switch (_IOC_NR(cmd_op)) {
 	case UBLK_CMD_START_DEV:
-		ret = ublk_ctrl_start_dev(ub, header);
+		ret = ublk_ctrl_start_dev(ub, &header);
 		break;
 	case UBLK_CMD_STOP_DEV:
 		ublk_ctrl_stop_dev(ub);
 		ret = 0;
 		break;
 	case UBLK_CMD_GET_DEV_INFO:
 	case UBLK_CMD_GET_DEV_INFO2:
-		ret = ublk_ctrl_get_dev_info(ub, header);
+		ret = ublk_ctrl_get_dev_info(ub, &header);
 		break;
 	case UBLK_CMD_ADD_DEV:
-		ret = ublk_ctrl_add_dev(header);
+		ret = ublk_ctrl_add_dev(&header);
 		break;
 	case UBLK_CMD_DEL_DEV:
 		ret = ublk_ctrl_del_dev(&ub, true);
 		break;
 	case UBLK_CMD_DEL_DEV_ASYNC:
 		ret = ublk_ctrl_del_dev(&ub, false);
 		break;
 	case UBLK_CMD_GET_QUEUE_AFFINITY:
-		ret = ublk_ctrl_get_queue_affinity(ub, header);
+		ret = ublk_ctrl_get_queue_affinity(ub, &header);
 		break;
 	case UBLK_CMD_GET_PARAMS:
-		ret = ublk_ctrl_get_params(ub, header);
+		ret = ublk_ctrl_get_params(ub, &header);
 		break;
 	case UBLK_CMD_SET_PARAMS:
-		ret = ublk_ctrl_set_params(ub, header);
+		ret = ublk_ctrl_set_params(ub, &header);
 		break;
 	case UBLK_CMD_START_USER_RECOVERY:
-		ret = ublk_ctrl_start_recovery(ub, header);
+		ret = ublk_ctrl_start_recovery(ub, &header);
 		break;
 	case UBLK_CMD_END_USER_RECOVERY:
-		ret = ublk_ctrl_end_recovery(ub, header);
+		ret = ublk_ctrl_end_recovery(ub, &header);
 		break;
 	case UBLK_CMD_UPDATE_SIZE:
-		ublk_ctrl_set_size(ub, header);
+		ublk_ctrl_set_size(ub, &header);
 		ret = 0;
 		break;
 	case UBLK_CMD_QUIESCE_DEV:
-		ret = ublk_ctrl_quiesce_dev(ub, header);
+		ret = ublk_ctrl_quiesce_dev(ub, &header);
 		break;
 	case UBLK_CMD_TRY_STOP_DEV:
 		ret = ublk_ctrl_try_stop_dev(ub);
 		break;
 	default:
@@ -5300,11 +5306,11 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
  put_dev:
 	if (ub)
 		ublk_put_device(ub);
  out:
 	pr_devel("%s: cmd done ret %d cmd_op %x, dev id %d qid %d\n",
-			__func__, ret, cmd->cmd_op, header->dev_id, header->queue_id);
+			__func__, ret, cmd_op, header.dev_id, header.queue_id);
 	return ret;
 }
 
 static const struct file_operations ublk_ctl_fops = {
 	.open		= nonseekable_open,
-- 
2.45.2

Re: [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd

[Ming Lei]

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@purestorage.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@redhat.com>


Thanks, 
Ming

Re: [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd

[Caleb Sander Mateos]

On Fri, Jan 30, 2026 at 6:08 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> 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@purestorage.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:

It wouldn't be correct to dereference ub_src here; see the prior patch
"ublk: Validate SQE128 flag before accessing the cmd".

Best,
Caleb

[PATCH v2 3/3] ublk: drop ublk_ctrl_start_recovery() header argument

[Caleb Sander Mateos]

ublk_ctrl_start_recovery() only uses its const struct ublksrv_ctrl_cmd *
header argument to log the dev_id. But this value is already available
in struct ublk_device's ub_number field. So log ub_number instead and
drop the unused header argument.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 8122b012a7ae..60d07480a24c 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -4870,12 +4870,11 @@ static int ublk_ctrl_set_params(struct ublk_device *ub,
 	mutex_unlock(&ub->mutex);
 
 	return ret;
 }
 
-static int ublk_ctrl_start_recovery(struct ublk_device *ub,
-		const struct ublksrv_ctrl_cmd *header)
+static int ublk_ctrl_start_recovery(struct ublk_device *ub)
 {
 	int ret = -EINVAL;
 
 	mutex_lock(&ub->mutex);
 	if (ublk_nosrv_should_stop_dev(ub))
@@ -4900,11 +4899,11 @@ static int ublk_ctrl_start_recovery(struct ublk_device *ub,
 	 */
 	if (test_bit(UB_STATE_OPEN, &ub->state) || !ublk_dev_in_recoverable_state(ub)) {
 		ret = -EBUSY;
 		goto out_unlock;
 	}
-	pr_devel("%s: start recovery for dev id %d.\n", __func__, header->dev_id);
+	pr_devel("%s: start recovery for dev id %d\n", __func__, ub->ub_number);
 	init_completion(&ub->completion);
 	ret = 0;
  out_unlock:
 	mutex_unlock(&ub->mutex);
 	return ret;
@@ -5281,11 +5280,11 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 		break;
 	case UBLK_CMD_SET_PARAMS:
 		ret = ublk_ctrl_set_params(ub, &header);
 		break;
 	case UBLK_CMD_START_USER_RECOVERY:
-		ret = ublk_ctrl_start_recovery(ub, &header);
+		ret = ublk_ctrl_start_recovery(ub);
 		break;
 	case UBLK_CMD_END_USER_RECOVERY:
 		ret = ublk_ctrl_end_recovery(ub, &header);
 		break;
 	case UBLK_CMD_UPDATE_SIZE:
-- 
2.45.2

Re: [PATCH v2 3/3] ublk: drop ublk_ctrl_start_recovery() header argument

[Ming Lei]

On Fri, Jan 30, 2026 at 10:14:14AM -0700, Caleb Sander Mateos wrote:
> ublk_ctrl_start_recovery() only uses its const struct ublksrv_ctrl_cmd *
> header argument to log the dev_id. But this value is already available
> in struct ublk_device's ub_number field. So log ub_number instead and
> drop the unused header argument.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>

Looks fine,

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


Thanks,
Ming

Re: [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses

[Jens Axboe]

On Fri, 30 Jan 2026 10:14:11 -0700, Caleb Sander Mateos wrote:
> struct ublksrv_ctrl_cmd is part of the io_uring_sqe. Since commit
> 87213b0d847c ("ublk: allow non-blocking ctrl cmds in IO_URING_F_NONBLOCK
> issue") allowed some commands to be handled in the non-blocking issue,
> the SQE may lie in userspace-mapped memory. Validate that the SQE size
> is the expected 128 bytes before dereferencing it. Use READ_ONCE() to
> copy the ublksrv_ctrl_cmd from the SQE to a local variable. This avoids
> data races if userspace writes to the SQE concurrently.
> 
> [...]

Applied, thanks!

[1/3] ublk: Validate SQE128 flag before accessing the cmd
      commit: da7e4b75e50c087d2031a92f6646eb90f7045a67
[2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd
      commit: ed9f54cc1e335096733aed03c2a46de3d58922ed
[3/3] ublk: drop ublk_ctrl_start_recovery() header argument
      commit: 373df2c0255da77f0842368708afce771e1330ca

Best regards,
-- 
Jens Axboe