Re: [PATCH 2/2] vhost-scsi: reject invalid max_io_vqs module parameter

[Dongli Zhang <dongli.zhang@oracle.com>]

+ virtualization@lists.linux.dev

On 2026-07-31 3:26 PM, sashiko-bot@kernel.org 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 <dongli.zhang@oracle.com>
> 
> 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