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

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2