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

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

+ virtualization@lists.linux.dev.

On 2026-07-31 3:24 PM, sashiko-bot@kernel.org wrote:
> 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.
> --


I think this pre-existing issue is implicitly fixed by PATCH 2/2, where
max_io_vqs is validated by param_set_uint_minmax(val, kp, 1,
VHOST_SCSI_MAX_IO_VQ) before the value is stored.

Changing the local nvqs variable from int to unsigned int in PATCH 2/2 would
make the type flow clearer, but the issue should no longer be reachable once
both patches are applied.

Dongli Zhang

> 
> --- Patch [1]: [PATCH 1/2] vhost-scsi: use kvzalloc for vq array allocation ---
> commit 3573b684f78f79b1ae3460e4a7625cb917f76044
> Author: Dongli Zhang <dongli.zhang@oracle.com>
> 
> 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?
>