* [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses
@ 2026-01-30 17:14 Caleb Sander Mateos
2026-01-30 17:14 ` [PATCH v2 1/3] ublk: Validate SQE128 flag before accessing the cmd Caleb Sander Mateos
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Caleb Sander Mateos @ 2026-01-30 17:14 UTC (permalink / raw)
To: Ming Lei, Jens Axboe
Cc: Govindarajulu Varadarajan, linux-block, linux-kernel,
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] ublk: Validate SQE128 flag before accessing the cmd
2026-01-30 17:14 [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses Caleb Sander Mateos
@ 2026-01-30 17:14 ` Caleb Sander Mateos
2026-01-30 17:14 ` [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd Caleb Sander Mateos
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Caleb Sander Mateos @ 2026-01-30 17:14 UTC (permalink / raw)
To: Ming Lei, Jens Axboe
Cc: Govindarajulu Varadarajan, linux-block, linux-kernel,
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd
2026-01-30 17:14 [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses Caleb Sander Mateos
2026-01-30 17:14 ` [PATCH v2 1/3] ublk: Validate SQE128 flag before accessing the cmd Caleb Sander Mateos
@ 2026-01-30 17:14 ` Caleb Sander Mateos
2026-01-31 2:08 ` Ming Lei
2026-01-30 17:14 ` [PATCH v2 3/3] ublk: drop ublk_ctrl_start_recovery() header argument Caleb Sander Mateos
2026-01-31 13:49 ` [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses Jens Axboe
3 siblings, 1 reply; 8+ messages in thread
From: Caleb Sander Mateos @ 2026-01-30 17:14 UTC (permalink / raw)
To: Ming Lei, Jens Axboe
Cc: Govindarajulu Varadarajan, linux-block, linux-kernel,
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] ublk: drop ublk_ctrl_start_recovery() header argument
2026-01-30 17:14 [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses Caleb Sander Mateos
2026-01-30 17:14 ` [PATCH v2 1/3] ublk: Validate SQE128 flag before accessing the cmd Caleb Sander Mateos
2026-01-30 17:14 ` [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd Caleb Sander Mateos
@ 2026-01-30 17:14 ` Caleb Sander Mateos
2026-01-31 2:09 ` Ming Lei
2026-01-31 13:49 ` [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses Jens Axboe
3 siblings, 1 reply; 8+ messages in thread
From: Caleb Sander Mateos @ 2026-01-30 17:14 UTC (permalink / raw)
To: Ming Lei, Jens Axboe
Cc: Govindarajulu Varadarajan, linux-block, linux-kernel,
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd
2026-01-30 17:14 ` [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd Caleb Sander Mateos
@ 2026-01-31 2:08 ` Ming Lei
2026-02-02 15:38 ` Caleb Sander Mateos
0 siblings, 1 reply; 8+ messages in thread
From: Ming Lei @ 2026-01-31 2:08 UTC (permalink / raw)
To: Caleb Sander Mateos
Cc: Jens Axboe, Govindarajulu Varadarajan, linux-block, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/3] ublk: drop ublk_ctrl_start_recovery() header argument
2026-01-30 17:14 ` [PATCH v2 3/3] ublk: drop ublk_ctrl_start_recovery() header argument Caleb Sander Mateos
@ 2026-01-31 2:09 ` Ming Lei
0 siblings, 0 replies; 8+ messages in thread
From: Ming Lei @ 2026-01-31 2:09 UTC (permalink / raw)
To: Caleb Sander Mateos
Cc: Jens Axboe, Govindarajulu Varadarajan, linux-block, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses
2026-01-30 17:14 [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses Caleb Sander Mateos
` (2 preceding siblings ...)
2026-01-30 17:14 ` [PATCH v2 3/3] ublk: drop ublk_ctrl_start_recovery() header argument Caleb Sander Mateos
@ 2026-01-31 13:49 ` Jens Axboe
3 siblings, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2026-01-31 13:49 UTC (permalink / raw)
To: Ming Lei, Caleb Sander Mateos
Cc: Govindarajulu Varadarajan, linux-block, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd
2026-01-31 2:08 ` Ming Lei
@ 2026-02-02 15:38 ` Caleb Sander Mateos
0 siblings, 0 replies; 8+ messages in thread
From: Caleb Sander Mateos @ 2026-02-02 15:38 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, Govindarajulu Varadarajan, linux-block, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-02-02 15:38 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-30 17:14 [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses Caleb Sander Mateos
2026-01-30 17:14 ` [PATCH v2 1/3] ublk: Validate SQE128 flag before accessing the cmd Caleb Sander Mateos
2026-01-30 17:14 ` [PATCH v2 2/3] ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd Caleb Sander Mateos
2026-01-31 2:08 ` Ming Lei
2026-02-02 15:38 ` Caleb Sander Mateos
2026-01-30 17:14 ` [PATCH v2 3/3] ublk: drop ublk_ctrl_start_recovery() header argument Caleb Sander Mateos
2026-01-31 2:09 ` Ming Lei
2026-01-31 13:49 ` [PATCH v2 0/3] ublk: fix struct ublksrv_ctrl_cmd accesses Jens Axboe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.