Re: [PATCH 1/2] vhost-scsi: use kvzalloc for vq array allocation

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `max_io_vqs` module parameter is parsed as an unsigned integer but assigned to a signed `int` in `vhost_scsi_open()`. Setting this parameter to a large unsigned value (e.g., `4294967295`) results in a negative value (e.g., `-1`) that bypasses the maximum bound check. This leads to an undersized allocation followed by statically indexed Out-Of-Bounds (OOB) writes.
--

--- Patch [1]: [PATCH 1/2] vhost-scsi: use kvzalloc for vq array allocation ---
commit 3573b684f78f79b1ae3460e4a7625cb917f76044
Author: Dongli Zhang <[email protected]>

vhost-scsi: use kvzalloc for vq array allocation

vhost_scsi_open() allocates one struct vhost_scsi_virtqueue for each
virtqueue. With large max_io_vqs values, this array can require a
high-order contiguous allocation and trigger a page allocator warning.
This patch replaces kmalloc_objs with kvzalloc_objs to avoid the
warning.

> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 9a1253b9d8c50..0c0634eea1443 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -2295,7 +2295,7 @@ static int vhost_scsi_open(struct inode *inode, struct file *f)
>  	if (!vs->old_inflight)
>  		goto err_inflight;
>  
> -	vs->vqs = kmalloc_objs(*vs->vqs, nvqs, GFP_KERNEL | __GFP_ZERO);
> +	vs->vqs = kvzalloc_objs(*vs->vqs, nvqs);

[Severity: High]
This is a pre-existing issue, but could this allocation be undersized due to
an integer overflow in the nvqs calculation?

If a privileged user sets the max_io_vqs module parameter to a value that
overflows the signed integer nvqs (e.g., 4294967295), nvqs could become
negative (e.g., -1). Because it is signed, this bypasses the maximum bounds
check against VHOST_SCSI_MAX_IO_VQ.

When VHOST_SCSI_VQ_IO (2) is added, nvqs becomes 1. The allocation is then
sized for exactly 1 element:

    vs->vqs = kvzalloc_objs(*vs->vqs, nvqs);

However, the code later unconditionally writes to index 1 (VHOST_SCSI_VQ_EVT),
which would cause a heap out-of-bounds write:

    vqs[VHOST_SCSI_VQ_EVT] = &vs->vqs[VHOST_SCSI_VQ_EVT].vq;

Would it be safer to use an unsigned type for nvqs and related variables to
prevent this bounds check bypass?

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