[PATCH 0/4] ublk: fix struct ublksrv_ctrl_cmd accesses

[PATCH 0/4] 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. Access the
ublksrv_ctrl_cmd fields with READ_ONCE(), as userspace may write to them
concurrently.

Caleb Sander Mateos (3):
  ublk: don't write to struct ublksrv_ctrl_cmd
  ublk: use READ_ONCE() to read struct ublksrv_ctrl_cmd
  ublk: drop ublk_ctrl_{start,end}_recovery() header argument

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

 drivers/block/ublk_drv.c | 163 +++++++++++++++++++--------------------
 1 file changed, 80 insertions(+), 83 deletions(-)

-- 
2.45.2

[PATCH 1/4] 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>
---
 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 7981decd1cee..72ee83ae303d 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -5130,14 +5130,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 2/4] ublk: don't write to struct ublksrv_ctrl_cmd

[Caleb Sander Mateos]

ublk_ctrl_uring_cmd_permission() writes to struct ublksrv_ctrl_cmd's
addr and len fields, which is racy because ublksrv_ctrl_cmd is part of
the io_uring_sqe, which may lie in userspace-mapped memory. Store the
values of addr in len in local variables instead to avoid the race.

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 | 84 ++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 41 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 72ee83ae303d..29c6942450c2 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -4346,24 +4346,24 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
 	mutex_unlock(&ub->mutex);
 	return ret;
 }
 
 static int ublk_ctrl_get_queue_affinity(struct ublk_device *ub,
-		const struct ublksrv_ctrl_cmd *header)
+		const struct ublksrv_ctrl_cmd *header, u64 addr, u16 len)
 {
-	void __user *argp = (void __user *)(unsigned long)header->addr;
+	void __user *argp = (void __user *)addr;
 	cpumask_var_t cpumask;
 	unsigned long queue;
 	unsigned int retlen;
 	unsigned int i;
 	int ret;
 
-	if (header->len * BITS_PER_BYTE < nr_cpu_ids)
+	if (len * BITS_PER_BYTE < nr_cpu_ids)
 		return -EINVAL;
-	if (header->len & (sizeof(unsigned long)-1))
+	if (len & (sizeof(unsigned long)-1))
 		return -EINVAL;
-	if (!header->addr)
+	if (!addr)
 		return -EINVAL;
 
 	queue = header->data[0];
 	if (queue >= ub->dev_info.nr_hw_queues)
 		return -EINVAL;
@@ -4375,15 +4375,15 @@ static int ublk_ctrl_get_queue_affinity(struct ublk_device *ub,
 		if (ub->tag_set.map[HCTX_TYPE_DEFAULT].mq_map[i] == queue)
 			cpumask_set_cpu(i, cpumask);
 	}
 
 	ret = -EFAULT;
-	retlen = min_t(unsigned short, header->len, cpumask_size());
+	retlen = min_t(unsigned short, len, cpumask_size());
 	if (copy_to_user(argp, cpumask, retlen))
 		goto out_free_cpumask;
-	if (retlen != header->len &&
-	    clear_user(argp + retlen, header->len - retlen))
+	if (retlen != len &&
+	    clear_user(argp + retlen, len - retlen))
 		goto out_free_cpumask;
 
 	ret = 0;
 out_free_cpumask:
 	free_cpumask_var(cpumask);
@@ -4396,18 +4396,19 @@ static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info)
 			info->dev_id, info->flags);
 	pr_devel("\t nr_hw_queues %d queue_depth %d\n",
 			info->nr_hw_queues, info->queue_depth);
 }
 
-static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
+static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header,
+			     u64 addr, u16 len)
 {
-	void __user *argp = (void __user *)(unsigned long)header->addr;
+	void __user *argp = (void __user *)addr;
 	struct ublksrv_ctrl_dev_info info;
 	struct ublk_device *ub;
 	int ret = -EINVAL;
 
-	if (header->len < sizeof(info) || !header->addr)
+	if (len < sizeof(info) || !addr)
 		return -EINVAL;
 	if (header->queue_id != (u16)-1) {
 		pr_warn("%s: queue_id is wrong %x\n",
 			__func__, header->queue_id);
 		return -EINVAL;
@@ -4682,16 +4683,15 @@ static int ublk_ctrl_try_stop_dev(struct ublk_device *ub)
 out:
 	ublk_put_disk(disk);
 	return ret;
 }
 
-static int ublk_ctrl_get_dev_info(struct ublk_device *ub,
-		const struct ublksrv_ctrl_cmd *header)
+static int ublk_ctrl_get_dev_info(struct ublk_device *ub, u64 addr, u16 len)
 {
-	void __user *argp = (void __user *)(unsigned long)header->addr;
+	void __user *argp = (void __user *)addr;
 
-	if (header->len < sizeof(struct ublksrv_ctrl_dev_info) || !header->addr)
+	if (len < sizeof(struct ublksrv_ctrl_dev_info) || !addr)
 		return -EINVAL;
 
 	if (copy_to_user(argp, &ub->dev_info, sizeof(ub->dev_info)))
 		return -EFAULT;
 
@@ -4712,24 +4712,23 @@ static void ublk_ctrl_fill_params_devt(struct ublk_device *ub)
 		ub->params.devt.disk_minor = 0;
 	}
 	ub->params.types |= UBLK_PARAM_TYPE_DEVT;
 }
 
-static int ublk_ctrl_get_params(struct ublk_device *ub,
-		const struct ublksrv_ctrl_cmd *header)
+static int ublk_ctrl_get_params(struct ublk_device *ub, u64 addr, u16 len)
 {
-	void __user *argp = (void __user *)(unsigned long)header->addr;
+	void __user *argp = (void __user *)addr;
 	struct ublk_params_header ph;
 	int ret;
 
-	if (header->len <= sizeof(ph) || !header->addr)
+	if (len <= sizeof(ph) || !addr)
 		return -EINVAL;
 
 	if (copy_from_user(&ph, argp, sizeof(ph)))
 		return -EFAULT;
 
-	if (ph.len > header->len || !ph.len)
+	if (ph.len > len || !ph.len)
 		return -EINVAL;
 
 	if (ph.len > sizeof(struct ublk_params))
 		ph.len = sizeof(struct ublk_params);
 
@@ -4742,24 +4741,23 @@ static int ublk_ctrl_get_params(struct ublk_device *ub,
 	mutex_unlock(&ub->mutex);
 
 	return ret;
 }
 
-static int ublk_ctrl_set_params(struct ublk_device *ub,
-		const struct ublksrv_ctrl_cmd *header)
+static int ublk_ctrl_set_params(struct ublk_device *ub, u64 addr, u16 len)
 {
-	void __user *argp = (void __user *)(unsigned long)header->addr;
+	void __user *argp = (void __user *)addr;
 	struct ublk_params_header ph;
 	int ret = -EFAULT;
 
-	if (header->len <= sizeof(ph) || !header->addr)
+	if (len <= sizeof(ph) || !addr)
 		return -EINVAL;
 
 	if (copy_from_user(&ph, argp, sizeof(ph)))
 		return -EFAULT;
 
-	if (ph.len > header->len || !ph.len || !ph.types)
+	if (ph.len > len || !ph.len || !ph.types)
 		return -EINVAL;
 
 	if (ph.len > sizeof(struct ublk_params))
 		ph.len = sizeof(struct ublk_params);
 
@@ -4857,16 +4855,16 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
  out_unlock:
 	mutex_unlock(&ub->mutex);
 	return ret;
 }
 
-static int ublk_ctrl_get_features(const struct ublksrv_ctrl_cmd *header)
+static int ublk_ctrl_get_features(u64 addr, u16 len)
 {
-	void __user *argp = (void __user *)(unsigned long)header->addr;
+	void __user *argp = (void __user *)addr;
 	u64 features = UBLK_F_ALL;
 
-	if (header->len != UBLK_FEATURES_LEN || !header->addr)
+	if (len != UBLK_FEATURES_LEN || !addr)
 		return -EINVAL;
 
 	if (copy_to_user(argp, &features, UBLK_FEATURES_LEN))
 		return -EFAULT;
 
@@ -5028,15 +5026,15 @@ 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)
+		struct io_uring_cmd *cmd, u64 *addr, u16 *len)
 {
-	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)io_uring_sqe_cmd(cmd->sqe);
+	const struct ublksrv_ctrl_cmd *header = 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;
+	void __user *argp = (void __user *)*addr;
 	char *dev_path = NULL;
 	int ret = 0;
 	int mask;
 
 	if (!unprivileged) {
@@ -5059,11 +5057,11 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
 	 * header->dev_path_len records length of dev path buffer.
 	 */
 	if (!header->dev_path_len || header->dev_path_len > PATH_MAX)
 		return -EINVAL;
 
-	if (header->len < header->dev_path_len)
+	if (*len < header->dev_path_len)
 		return -EINVAL;
 
 	dev_path = memdup_user_nul(argp, header->dev_path_len);
 	if (IS_ERR(dev_path))
 		return PTR_ERR(dev_path);
@@ -5093,12 +5091,12 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
 		goto exit;
 	}
 
 	ret = ublk_char_dev_permission(ub, dev_path, mask);
 	if (!ret) {
-		header->len -= header->dev_path_len;
-		header->addr += header->dev_path_len;
+		*len -= header->dev_path_len;
+		*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,
 			ub->dev_info.owner_uid, ub->dev_info.owner_gid,
 			dev_path, ret);
@@ -5125,36 +5123,40 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 {
 	const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
 	struct ublk_device *ub = NULL;
 	u32 cmd_op = cmd->cmd_op;
 	int ret = -EINVAL;
+	u64 addr;
+	u16 len;
 
 	if (ublk_ctrl_uring_cmd_may_sleep(cmd_op) &&
 	    issue_flags & IO_URING_F_NONBLOCK)
 		return -EAGAIN;
 
 	if (!(issue_flags & IO_URING_F_SQE128))
 		return -EINVAL;
 
+	addr = READ_ONCE(header->addr);
+	len = READ_ONCE(header->len);
 	ublk_ctrl_cmd_dump(cmd);
 
 	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(addr, len);
 		goto out;
 	}
 
 	if (_IOC_NR(cmd_op) != UBLK_CMD_ADD_DEV) {
 		ret = -ENODEV;
 		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, &addr, &len);
 		if (ret)
 			goto put_dev;
 	}
 
 	switch (_IOC_NR(cmd_op)) {
@@ -5165,29 +5167,29 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 		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, addr, len);
 		break;
 	case UBLK_CMD_ADD_DEV:
-		ret = ublk_ctrl_add_dev(header);
+		ret = ublk_ctrl_add_dev(header, addr, len);
 		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, addr, len);
 		break;
 	case UBLK_CMD_GET_PARAMS:
-		ret = ublk_ctrl_get_params(ub, header);
+		ret = ublk_ctrl_get_params(ub, addr, len);
 		break;
 	case UBLK_CMD_SET_PARAMS:
-		ret = ublk_ctrl_set_params(ub, header);
+		ret = ublk_ctrl_set_params(ub, addr, len);
 		break;
 	case UBLK_CMD_START_USER_RECOVERY:
 		ret = ublk_ctrl_start_recovery(ub, header);
 		break;
 	case UBLK_CMD_END_USER_RECOVERY:
-- 
2.45.2

[PATCH 3/4] 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() for
all the ublksrv_ctrl_cmd field accesses to avoid the race.

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 | 77 +++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 40 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 29c6942450c2..49510216832f 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -4188,15 +4188,13 @@ static struct ublk_device *ublk_get_device_from_id(int idx)
 	spin_unlock(&ublk_idr_lock);
 
 	return ub;
 }
 
-static int ublk_ctrl_start_dev(struct ublk_device *ub,
-		const struct ublksrv_ctrl_cmd *header)
+static int ublk_ctrl_start_dev(struct ublk_device *ub, int ublksrv_pid)
 {
 	const struct ublk_param_basic *p = &ub->params.basic;
-	int ublksrv_pid = (int)header->data[0];
 	struct queue_limits lim = {
 		.logical_block_size	= 1 << p->logical_bs_shift,
 		.physical_block_size	= 1 << p->physical_bs_shift,
 		.io_min			= 1 << p->io_min_shift,
 		.io_opt			= 1 << p->io_opt_shift,
@@ -4346,15 +4344,14 @@ static int ublk_ctrl_start_dev(struct ublk_device *ub,
 	mutex_unlock(&ub->mutex);
 	return ret;
 }
 
 static int ublk_ctrl_get_queue_affinity(struct ublk_device *ub,
-		const struct ublksrv_ctrl_cmd *header, u64 addr, u16 len)
+		u64 queue, u64 addr, u16 len)
 {
 	void __user *argp = (void __user *)addr;
 	cpumask_var_t cpumask;
-	unsigned long queue;
 	unsigned int retlen;
 	unsigned int i;
 	int ret;
 
 	if (len * BITS_PER_BYTE < nr_cpu_ids)
@@ -4362,11 +4359,10 @@ static int ublk_ctrl_get_queue_affinity(struct ublk_device *ub,
 	if (len & (sizeof(unsigned long)-1))
 		return -EINVAL;
 	if (!addr)
 		return -EINVAL;
 
-	queue = header->data[0];
 	if (queue >= ub->dev_info.nr_hw_queues)
 		return -EINVAL;
 
 	if (!zalloc_cpumask_var(&cpumask, GFP_KERNEL))
 		return -ENOMEM;
@@ -4396,23 +4392,22 @@ static inline void ublk_dump_dev_info(struct ublksrv_ctrl_dev_info *info)
 			info->dev_id, info->flags);
 	pr_devel("\t nr_hw_queues %d queue_depth %d\n",
 			info->nr_hw_queues, info->queue_depth);
 }
 
-static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header,
-			     u64 addr, u16 len)
+static int ublk_ctrl_add_dev(u32 dev_id, u16 qid, u64 addr, u16 len)
 {
 	void __user *argp = (void __user *)addr;
 	struct ublksrv_ctrl_dev_info info;
 	struct ublk_device *ub;
 	int ret = -EINVAL;
 
 	if (len < sizeof(info) || !addr)
 		return -EINVAL;
-	if (header->queue_id != (u16)-1) {
+	if (qid != (u16)-1) {
 		pr_warn("%s: queue_id is wrong %x\n",
-			__func__, header->queue_id);
+			__func__, qid);
 		return -EINVAL;
 	}
 
 	if (copy_from_user(&info, argp, sizeof(info)))
 		return -EFAULT;
@@ -4473,17 +4468,17 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header,
 		return -EINVAL;
 
 	/* the created device is always owned by current user */
 	ublk_store_owner_uid_gid(&info.owner_uid, &info.owner_gid);
 
-	if (header->dev_id != info.dev_id) {
+	if (dev_id != info.dev_id) {
 		pr_warn("%s: dev id not match %u %u\n",
-			__func__, header->dev_id, info.dev_id);
+			__func__, dev_id, info.dev_id);
 		return -EINVAL;
 	}
 
-	if (header->dev_id != U32_MAX && header->dev_id >= UBLK_MAX_UBLKS) {
+	if (dev_id != U32_MAX && dev_id >= UBLK_MAX_UBLKS) {
 		pr_warn("%s: dev id is too large. Max supported is %d\n",
 			__func__, UBLK_MAX_UBLKS - 1);
 		return -EINVAL;
 	}
 
@@ -4505,11 +4500,11 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header,
 	mutex_init(&ub->mutex);
 	spin_lock_init(&ub->lock);
 	mutex_init(&ub->cancel_mutex);
 	INIT_WORK(&ub->partition_scan_work, ublk_partition_scan_work);
 
-	ret = ublk_alloc_dev_number(ub, header->dev_id);
+	ret = ublk_alloc_dev_number(ub, dev_id);
 	if (ret < 0)
 		goto out_free_ub;
 
 	memcpy(&ub->dev_info, &info, sizeof(info));
 
@@ -4641,17 +4636,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, u32 dev_id, u16 qid,
+				      u64 data, u64 addr, u16 len)
 {
-	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,
-			header->data[0], header->addr, header->len);
+			__func__, cmd_op, dev_id, qid, data, addr, len);
 }
 
 static void ublk_ctrl_stop_dev(struct ublk_device *ub)
 {
 	ublk_stop_dev(ub);
@@ -4819,13 +4812,12 @@ static int ublk_ctrl_start_recovery(struct ublk_device *ub,
 	mutex_unlock(&ub->mutex);
 	return ret;
 }
 
 static int ublk_ctrl_end_recovery(struct ublk_device *ub,
-		const struct ublksrv_ctrl_cmd *header)
+		const struct ublksrv_ctrl_cmd *header, int ublksrv_pid)
 {
-	int ublksrv_pid = (int)header->data[0];
 	int ret = -EINVAL;
 
 	pr_devel("%s: Waiting for all FETCH_REQs, dev id %d...\n", __func__,
 		 header->dev_id);
 
@@ -4869,14 +4861,13 @@ static int ublk_ctrl_get_features(u64 addr, u16 len)
 		return -EFAULT;
 
 	return 0;
 }
 
-static void ublk_ctrl_set_size(struct ublk_device *ub, const struct ublksrv_ctrl_cmd *header)
+static void ublk_ctrl_set_size(struct ublk_device *ub, u64 new_size)
 {
 	struct ublk_param_basic *p = &ub->params.basic;
-	u64 new_size = header->data[0];
 
 	mutex_lock(&ub->mutex);
 	p->dev_sectors = new_size;
 	set_capacity_and_notify(ub->ub_disk, p->dev_sectors);
 	mutex_unlock(&ub->mutex);
@@ -4950,15 +4941,13 @@ static int ublk_wait_for_idle_io(struct ublk_device *ub,
 		ret = 0;
 
 	return ret;
 }
 
-static int ublk_ctrl_quiesce_dev(struct ublk_device *ub,
-				 const struct ublksrv_ctrl_cmd *header)
+static int ublk_ctrl_quiesce_dev(struct ublk_device *ub, u64 timeout_ms)
 {
 	/* zero means wait forever */
-	u64 timeout_ms = header->data[0];
 	struct gendisk *disk;
 	int ret = -ENODEV;
 
 	if (!(ub->dev_info.flags & UBLK_F_QUIESCE))
 		return -EOPNOTSUPP;
@@ -5032,10 +5021,11 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
 {
 	const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
 	bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
 	void __user *argp = (void __user *)*addr;
 	char *dev_path = NULL;
+	u16 dev_path_len;
 	int ret = 0;
 	int mask;
 
 	if (!unprivileged) {
 		if (!capable(CAP_SYS_ADMIN))
@@ -5054,17 +5044,18 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
 	 * User has to provide the char device path for unprivileged ublk
 	 *
 	 * header->addr always points to the dev path buffer, and
 	 * header->dev_path_len records length of dev path buffer.
 	 */
-	if (!header->dev_path_len || header->dev_path_len > PATH_MAX)
+	dev_path_len = READ_ONCE(header->dev_path_len);
+	if (!dev_path_len || dev_path_len > PATH_MAX)
 		return -EINVAL;
 
-	if (*len < header->dev_path_len)
+	if (*len < dev_path_len)
 		return -EINVAL;
 
-	dev_path = memdup_user_nul(argp, header->dev_path_len);
+	dev_path = memdup_user_nul(argp, dev_path_len);
 	if (IS_ERR(dev_path))
 		return PTR_ERR(dev_path);
 
 	ret = -EINVAL;
 	switch (_IOC_NR(cmd->cmd_op)) {
@@ -5091,12 +5082,12 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
 		goto exit;
 	}
 
 	ret = ublk_char_dev_permission(ub, dev_path, mask);
 	if (!ret) {
-		*len -= header->dev_path_len;
-		*addr += header->dev_path_len;
+		*len -= dev_path_len;
+		*addr += 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,
 			ub->dev_info.owner_uid, ub->dev_info.owner_gid,
 			dev_path, ret);
@@ -5123,23 +5114,29 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 {
 	const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
 	struct ublk_device *ub = NULL;
 	u32 cmd_op = cmd->cmd_op;
 	int ret = -EINVAL;
+	u32 dev_id;
+	u16 qid;
+	u64 data;
 	u64 addr;
 	u16 len;
 
 	if (ublk_ctrl_uring_cmd_may_sleep(cmd_op) &&
 	    issue_flags & IO_URING_F_NONBLOCK)
 		return -EAGAIN;
 
 	if (!(issue_flags & IO_URING_F_SQE128))
 		return -EINVAL;
 
+	dev_id = READ_ONCE(header->dev_id);
+	qid = READ_ONCE(header->queue_id);
+	data = READ_ONCE(header->data[0]);
 	addr = READ_ONCE(header->addr);
 	len = READ_ONCE(header->len);
-	ublk_ctrl_cmd_dump(cmd);
+	ublk_ctrl_cmd_dump(cmd_op, dev_id, qid, data, addr, len);
 
 	ret = ublk_check_cmd_op(cmd_op);
 	if (ret)
 		goto out;
 
@@ -5148,42 +5145,42 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 		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(dev_id);
 		if (!ub)
 			goto out;
 
 		ret = ublk_ctrl_uring_cmd_permission(ub, cmd, &addr, &len);
 		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, data);
 		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, addr, len);
 		break;
 	case UBLK_CMD_ADD_DEV:
-		ret = ublk_ctrl_add_dev(header, addr, len);
+		ret = ublk_ctrl_add_dev(dev_id, qid, addr, len);
 		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, addr, len);
+		ret = ublk_ctrl_get_queue_affinity(ub, data, addr, len);
 		break;
 	case UBLK_CMD_GET_PARAMS:
 		ret = ublk_ctrl_get_params(ub, addr, len);
 		break;
 	case UBLK_CMD_SET_PARAMS:
@@ -5191,18 +5188,18 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 		break;
 	case UBLK_CMD_START_USER_RECOVERY:
 		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, data);
 		break;
 	case UBLK_CMD_UPDATE_SIZE:
-		ublk_ctrl_set_size(ub, header);
+		ublk_ctrl_set_size(ub, data);
 		ret = 0;
 		break;
 	case UBLK_CMD_QUIESCE_DEV:
-		ret = ublk_ctrl_quiesce_dev(ub, header);
+		ret = ublk_ctrl_quiesce_dev(ub, data);
 		break;
 	case UBLK_CMD_TRY_STOP_DEV:
 		ret = ublk_ctrl_try_stop_dev(ub);
 		break;
 	default:
@@ -5213,11 +5210,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, dev_id, qid);
 	return ret;
 }
 
 static const struct file_operations ublk_ctl_fops = {
 	.open		= nonseekable_open,
-- 
2.45.2

[PATCH 4/4] ublk: drop ublk_ctrl_{start,end}_recovery() header argument

[Caleb Sander Mateos]

ublk_ctrl_start_recovery() and ublk_ctrl_end_recovery() only use their
const struct ublksrv_ctrl_cmd *header arguments 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 arguments.

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

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 49510216832f..a0d1285d24d1 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -4773,12 +4773,11 @@ static int ublk_ctrl_set_params(struct ublk_device *ub, u64 addr, u16 len)
 	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))
@@ -4803,31 +4802,30 @@ 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;
 }
 
-static int ublk_ctrl_end_recovery(struct ublk_device *ub,
-		const struct ublksrv_ctrl_cmd *header, int ublksrv_pid)
+static int ublk_ctrl_end_recovery(struct ublk_device *ub, int ublksrv_pid)
 {
 	int ret = -EINVAL;
 
 	pr_devel("%s: Waiting for all FETCH_REQs, dev id %d...\n", __func__,
-		 header->dev_id);
+		 ub->ub_number);
 
 	if (wait_for_completion_interruptible(&ub->completion))
 		return -EINTR;
 
 	pr_devel("%s: All FETCH_REQs received, dev id %d\n", __func__,
-		 header->dev_id);
+		 ub->ub_number);
 
 	if (ub->ublksrv_tgid != ublksrv_pid)
 		return -EINVAL;
 
 	mutex_lock(&ub->mutex);
@@ -4839,11 +4837,11 @@ static int ublk_ctrl_end_recovery(struct ublk_device *ub,
 		goto out_unlock;
 	}
 	ub->dev_info.ublksrv_pid = ublksrv_pid;
 	ub->dev_info.state = UBLK_S_DEV_LIVE;
 	pr_devel("%s: new ublksrv_pid %d, dev id %d\n",
-			__func__, ublksrv_pid, header->dev_id);
+			__func__, ublksrv_pid, ub->ub_number);
 	blk_mq_kick_requeue_list(ub->ub_disk->queue);
 	ret = 0;
  out_unlock:
 	mutex_unlock(&ub->mutex);
 	return ret;
@@ -5185,14 +5183,14 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 		break;
 	case UBLK_CMD_SET_PARAMS:
 		ret = ublk_ctrl_set_params(ub, addr, len);
 		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, data);
+		ret = ublk_ctrl_end_recovery(ub, data);
 		break;
 	case UBLK_CMD_UPDATE_SIZE:
 		ublk_ctrl_set_size(ub, data);
 		ret = 0;
 		break;
-- 
2.45.2

Re: [PATCH 1/4] ublk: Validate SQE128 flag before accessing the cmd

[Ming Lei]

On Thu, Jan 29, 2026 at 03:46:14PM -0700, Caleb Sander Mateos wrote:
> 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>


Thanks,
Ming

Re: [PATCH 2/4] ublk: don't write to struct ublksrv_ctrl_cmd

[Ming Lei]

On Thu, Jan 29, 2026 at 03:46:15PM -0700, Caleb Sander Mateos wrote:
> ublk_ctrl_uring_cmd_permission() writes to struct ublksrv_ctrl_cmd's
> addr and len fields, which is racy because ublksrv_ctrl_cmd is part of
> the io_uring_sqe, which may lie in userspace-mapped memory. Store the
> values of addr in len in local variables instead to avoid the race.
> 
> Fixes: 87213b0d847c ("ublk: allow non-blocking ctrl cmds in IO_URING_F_NONBLOCK issue")
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>

The simpler approach is to define local `header` variable and copy data to
it.

Given it is introduced in v6.19-rc1, backport should be easy, so this patch
looks fine:

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

Thanks,
Ming

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

[Ming Lei]

On Thu, Jan 29, 2026 at 03:46:16PM -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() for
> all the ublksrv_ctrl_cmd field accesses to avoid the race.
> 
> 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 | 77 +++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 40 deletions(-)

I'd suggest to define local `struct ublksrv_ctrl_cmd` variable in
ublk_ctrl_uring_cmd() for avoiding READ_ONCE:

- it is slow control cod path

- READ_ONCE is supposed to be used for fast path and be carefully

- it is sync command

Defining one local data can be simpler to avoid the issue, and more
reliable.


Thanks,
Ming

Re: [PATCH 2/4] ublk: don't write to struct ublksrv_ctrl_cmd

[Ming Lei]

On Fri, Jan 30, 2026 at 11:48 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Thu, Jan 29, 2026 at 03:46:15PM -0700, Caleb Sander Mateos wrote:
> > ublk_ctrl_uring_cmd_permission() writes to struct ublksrv_ctrl_cmd's
> > addr and len fields, which is racy because ublksrv_ctrl_cmd is part of
> > the io_uring_sqe, which may lie in userspace-mapped memory. Store the
> > values of addr in len in local variables instead to avoid the race.
> >
> > Fixes: 87213b0d847c ("ublk: allow non-blocking ctrl cmds in IO_URING_F_NONBLOCK issue")
> > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
>
> The simpler approach is to define local `header` variable and copy data to
> it.

Given READ_ONCE() is still needed in patch 3, I'd suggest to fix by adding
local `struct ublksrv_ctrl_cmd` shadow variable in an easy & reliable way.

Thanks,