+ [email protected] On 2026-07-31 3:26 PM, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [Medium] Missing READ_ONCE() on module parameter allows compiler > rematerialization, leading to TOCTOU array out-of-bounds access. > -- > > --- Patch [2]: [PATCH 2/2] vhost-scsi: reject invalid max_io_vqs module > parameter --- > commit 93f3d55f364233d31773cdb70b64bd78dafbf480 > Author: Dongli Zhang <[email protected]> > > vhost-scsi: reject invalid max_io_vqs module parameter > > This commit shifts the validation of the max_io_vqs module parameter from > device open time to when the parameter is written. It removes the > bounds-checking code from vhost_scsi_open() and uses a module parameter > callback to reject invalid sysfs writes instead. > >> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c >> index 0c0634eea1443..c138c29ce3dd4 100644 >> --- a/drivers/vhost/scsi.c >> +++ b/drivers/vhost/scsi.c > [ ... ] >> @@ -2273,22 +2286,14 @@ static int vhost_scsi_open(struct inode *inode, >> struct file *f) >> struct vhost_scsi_virtqueue *svq; >> struct vhost_scsi *vs; >> struct vhost_virtqueue **vqs; >> - int r = -ENOMEM, i, nvqs = vhost_scsi_max_io_vqs; >> + int r = -ENOMEM, i, nvqs; >> >> vs = kvzalloc_obj(*vs); >> if (!vs) >> goto err_vs; >> vs->inline_sg_cnt = vhost_scsi_inline_sg_cnt; >> >> - if (nvqs > VHOST_SCSI_MAX_IO_VQ) { >> - pr_err("Invalid max_io_vqs of %d. Using %d.\n", nvqs, >> - VHOST_SCSI_MAX_IO_VQ); >> - nvqs = VHOST_SCSI_MAX_IO_VQ; >> - } else if (nvqs == 0) { >> - pr_err("Invalid max_io_vqs of %d. Using 1.\n", nvqs); >> - nvqs = 1; >> - } >> - nvqs += VHOST_SCSI_VQ_IO; >> + nvqs = vhost_scsi_max_io_vqs + VHOST_SCSI_VQ_IO; > > [Severity: Medium] > Since vhost_scsi_max_io_vqs is modifiable via sysfs (0644 permissions), could > the compiler rematerialize nvqs by reloading the global variable from memory > multiple times in this function? > > Without a READ_ONCE(), the compiler is permitted to reload > vhost_scsi_max_io_vqs instead of spilling nvqs to the stack across the > subsequent kmalloc_objs() and kvzalloc_objs() calls. If a concurrent sysfs > write occurs, the reloaded values can differ. > > Could this cause the array allocation sizes to be smaller than the nvqs > value used in the subsequent initialization loop, resulting in an > out-of-bounds access? > > For example, if the compiler decides to reload the variable: > > vs->old_inflight = kmalloc_objs(*vs->old_inflight, nvqs, ...); > // ... > vs->vqs = kvzalloc_objs(*vs->vqs, nvqs); > // ... > for (i = VHOST_SCSI_VQ_IO; i < nvqs; i++) { > svq = &vs->vqs[i]; > > Previously, the complex bounds-checking block likely prevented the compiler > from reloading the variable, but with the check removed, the value is read > directly. Should READ_ONCE(vhost_scsi_max_io_vqs) be used here? >
I can make the change below, either in v2 of PATCH 2/2, or as an additional follow-up patch, depending on whether the maintainer would prefer a v2: nvqs = READ_ONCE(vhost_scsi_max_io_vqs) + VHOST_SCSI_VQ_IO; Thank you very much! Dongli Zhang
