[PATCH v2] vhost-scsi: initialize command pools for late I/O virtqueues

[PATCH v2] vhost-scsi: initialize command pools for late I/O virtqueues

[Linfeng Sun]

vhost_scsi_set_endpoint() allocates command pools only for I/O virtqueues
that are already set up, but publishes the backend for every virtqueue. If
userspace finishes configuring a skipped queue later, a kick reaches
vhost_scsi_get_cmd() with scsi_tags uninitialized and triggers the
!sb->alloc_hint warning in sbitmap_get().

Leave skipped I/O virtqueues inactive at SET_ENDPOINT. When SET_VRING_ADDR
later completes their setup, allocate the command pools and activate only
that queues. Roll back both if access initialization fails.

Fixes: 25b98b64e284 ("vhost scsi: alloc cmds per vq instead of session")
Signed-off-by: Linfeng Sun <linfeng.sun.dev@gmail.com>
---
I checked QEMU and found no evidence it's affected. I haven't checked 
VirtualBox because I am not familiar with it. However, the virtio spec 
doesn't require a specific operation order, so a new VMM could still 
trigger this issue. 

Crash log:
[   34.698961] ------------[ cut here ]------------
[   34.699385] WARNING: lib/sbitmap.c:286 at sbitmap_get+0x227/0x270, CPU#0: vhost-82/88
[   34.700542] Modules linked in:
[   34.701309] CPU: 0 UID: 0 PID: 88 Comm: vhost-82 Not tainted 7.3.0-rc2+ #14 PREEMPT(full)
[   34.701958] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   34.702717] RIP: 0010:sbitmap_get+0x227/0x270
[   34.703247] Code: 41 8d 5d 01 89 df 44 89 fe e8 f5 3a 46 ff 31 c0 44 39 fb 0f 43 d8 e8 38 46 46 ff 65 41 89 1c 24 e9 25 ff ff ff e8 29 46 46 ff <0f> 0b eb ba e8 20 46 46 ff 44 89 fe 31 ff 45 31 e4 e8 c3 3b 46 ff
[   34.704234] RSP: 0018:ffffc90000ec3bb0 EFLAGS: 00000246
[   34.704677] RAX: 0000000000000000 RBX: ffff88800500dee8 RCX: 0000000000000000
[   34.705166] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88800500dee8
[   34.705584] RBP: ffffc90000ec3be0 R08: 0000000000000000 R09: 0000000000000000
[   34.705988] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000006
[   34.706477] R13: 0000000000000000 R14: 0000000000000000 R15: ffff8880050094d0
[   34.706955] FS:  0000000014d05380(0000) GS:ffff888099c36000(0000) knlGS:0000000000000000
[   34.707463] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   34.707854] CR2: 00007c0c3c20e000 CR3: 0000000008e47000 CR4: 00000000000006f0
[   34.708592] Call Trace:
[   34.709259]  <TASK>
[   34.709822]  vhost_scsi_handle_vq+0x4a9/0x1b20
[   34.710437]  ? update_cfs_rq_load_avg+0x2e/0x230
[   34.710874]  vhost_scsi_handle_kick+0x2c/0x40
[   34.711292]  vhost_run_work_list+0xa6/0xf0
[   34.711673]  vhost_task_fn+0x76/0x1b0
[   34.712003]  ? __sanitizer_cov_trace_const_cmp1+0x1a/0x30
[   34.712506]  ? recalc_sigpending+0xe1/0x130
[   34.712890]  ? __pfx_vhost_task_fn+0x10/0x10
[   34.713325]  ret_from_fork+0x37a/0x630
[   34.713673]  ? __pfx_vhost_task_fn+0x10/0x10
[   34.714031]  ret_from_fork_asm+0x1a/0x30
[   34.714535] RIP: 0033:0x0
[   34.715201] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
[   34.715587] RSP: 002b:0000000000000000 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[   34.716158] RAX: 0000000000000000 RBX: 000000000048bb20 RCX: 000000000041ca3d
[   34.716571] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
[   34.716957] RBP: 00007ffcac47c4e0 R08: 00000000ffffffff R09: 0000000000000000
[   34.717418] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
[   34.717826] R13: 00007ffcac47c778 R14: 00000000004af868 R15: 0000000000000001
[   34.718363]  </TASK>
---
Changes in v2:
- Activate only the virtqueue specified by VHOST_SET_VRING_ADDR.
- Mask the userspace provided index before accessing the virtqueue array.
- Link to v1: https://patch.msgid.link/20260908-fix-warning-in-sbitmap_get-v1-1-767c2c211a2b@gmail.com
---
 drivers/vhost/scsi.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 4f8c0260bc9e..efca34362666 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -2008,6 +2008,41 @@ static int vhost_scsi_setup_vq_cmds(struct vhost_virtqueue *vq, int max_cmds)
 	return -ENOMEM;
 }
 
+/* Callers must hold dev mutex. */
+static int vhost_scsi_activate_vq(struct vhost_scsi *vs, unsigned int index)
+{
+	struct vhost_virtqueue *vq;
+	int ret = 0;
+
+	if (!vs->vs_tpg)
+		return 0;
+	if (index >= vs->dev.nvqs)
+		return -EINVAL;
+	index = array_index_nospec(index, vs->dev.nvqs);
+	if (index < VHOST_SCSI_VQ_IO)
+		return 0;
+
+	vq = &vs->vqs[index].vq;
+	mutex_lock(&vq->mutex);
+	if (vhost_vq_get_backend(vq) || !vhost_vq_is_setup(vq))
+		goto unlock;
+
+	ret = vhost_scsi_setup_vq_cmds(vq, vq->num);
+	if (ret)
+		goto unlock;
+
+	vhost_vq_set_backend(vq, vs->vs_tpg);
+	ret = vhost_vq_init_access(vq);
+	if (ret) {
+		vhost_vq_set_backend(vq, NULL);
+		vhost_scsi_destroy_vq_cmds(vq);
+	}
+unlock:
+	mutex_unlock(&vq->mutex);
+
+	return ret;
+}
+
 /*
  * Called from vhost_scsi_ioctl() context to walk the list of available
  * vhost_scsi_tpg with an active struct vhost_scsi_nexus
@@ -2105,6 +2140,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
 
 		for (i = 0; i < vs->dev.nvqs; i++) {
 			vq = &vs->vqs[i].vq;
+			if (i >= VHOST_SCSI_VQ_IO && !vs->vqs[i].scsi_cmds)
+				continue;
+
 			mutex_lock(&vq->mutex);
 			vhost_vq_set_backend(vq, vs_tpg);
 			vhost_vq_init_access(vq);
@@ -2409,6 +2447,7 @@ vhost_scsi_ioctl(struct file *f,
 	u64 __user *featurep = argp;
 	u32 __user *eventsp = argp;
 	u32 events_missed;
+	u32 vq_index = 0;
 	u64 features;
 	int r, abi_version = VHOST_SCSI_ABI_VERSION;
 	struct vhost_virtqueue *vq = &vs->vqs[VHOST_SCSI_VQ_EVT].vq;
@@ -2464,12 +2503,19 @@ vhost_scsi_ioctl(struct file *f,
 		mutex_unlock(&vs->dev.mutex);
 		return r;
 	default:
+		if (ioctl == VHOST_SET_VRING_ADDR &&
+		    get_user(vq_index, (u32 __user *)argp))
+			return -EFAULT;
+
 		mutex_lock(&vs->dev.mutex);
 		r = vhost_dev_ioctl(&vs->dev, ioctl, argp);
-		if (r == -ENOIOCTLCMD)
+		if (r == -ENOIOCTLCMD) {
 			r = vhost_vring_ioctl(&vs->dev, ioctl, argp);
-		else
+			if (!r && ioctl == VHOST_SET_VRING_ADDR)
+				r = vhost_scsi_activate_vq(vs, vq_index);
+		} else {
 			vhost_scsi_flush(vs);
+		}
 		mutex_unlock(&vs->dev.mutex);
 		return r;
 	}

---
base-commit: dd886653d2038a510a21bff476fa1be0422a4fb9

Best regards,
-- 
Linfeng Sun <linfeng.sun.dev@gmail.com>

Re: [PATCH v2] vhost-scsi: initialize command pools for late I/O virtqueues

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vhost-scsi: double fetch of vq_index in SET_VRING_ADDR
--

commit 9fca211b81e6c842310a6804151d7dcf766af409
Author: Linfeng Sun <linfeng.sun.dev@gmail.com>

vhost-scsi: initialize command pools for late I/O virtqueues

This commit delays the allocation of command pools for skipped I/O
virtqueues during VHOST_SCSI_SET_ENDPOINT. When VHOST_SET_VRING_ADDR
subsequently completes their setup, it allocates the pools and activates
only the newly configured queue.

Link: https://patch.msgid.link/20260908-fix-warning-in-sbitmap_get-v1-1-767c2c211a2b@gmail.com

> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 4f8c0260bc9e7..efca343626664 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c

[ ... ]

> @@ -2464,12 +2503,19 @@ vhost_scsi_ioctl(struct file *f,
>  		mutex_unlock(&vs->dev.mutex);
>  		return r;
>  	default:
> +		if (ioctl == VHOST_SET_VRING_ADDR &&
> +		    get_user(vq_index, (u32 __user *)argp))
> +			return -EFAULT;
> +
>  		mutex_lock(&vs->dev.mutex);
>  		r = vhost_dev_ioctl(&vs->dev, ioctl, argp);
> -		if (r == -ENOIOCTLCMD)
> +		if (r == -ENOIOCTLCMD) {
>  			r = vhost_vring_ioctl(&vs->dev, ioctl, argp);
> -		else
> +			if (!r && ioctl == VHOST_SET_VRING_ADDR)
> +				r = vhost_scsi_activate_vq(vs, vq_index);

[Severity: Medium]
Could this introduce a double fetch vulnerability?

The index is fetched first here in vhost_scsi_ioctl():
    get_user(vq_index, (u32 __user *)argp)

And then later in the call chain it fetches the index again from the
same user pointer:

vhost_vring_ioctl() -> vhost_get_vq_from_user():
    get_user(idx, idxp);

If a malicious userspace thread (such as a VMM) modifies the index in the
payload between these two fetches, could it decouple virtqueue configuration
from activation? This might lead to state confusion where one virtqueue gets
its addresses configured by vhost_vring_set_num_addr(), while a completely
different virtqueue index (vq_index from the first fetch) is unexpectedly
activated by vhost_scsi_activate_vq().

While array_index_nospec() in vhost_scsi_activate_vq() prevents out-of-bounds
accesses, could activating a queue that wasn't actually targeted for address
configuration lead to unintended behavior?

> +		} else {
>  			vhost_scsi_flush(vs);
> +		}
>  		mutex_unlock(&vs->dev.mutex);
>  		return r;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-fix-warning-in-sbitmap_get-v2-1-e61a09047089@gmail.com?part=1