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.2On 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
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
© 2016 - 2026 Red Hat, Inc.