Re: [PATCH] vhost-scsi: Prevent OOM from invalid protection SGL count

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] A malicious guest can reliably crash the host kernel by providing a virtio-scsi request where the T10 Protection Information length equals or exceeds the total data payload size, triggering a BUG_ON() during SGL allocation.
--

--- Patch [1]: [PATCH] vhost-scsi: Prevent OOM from invalid protection SGL count ---
commit ee69ef365d50119dd5831e263cc68413e6e2bee1
Author: Jia Jia <physicalmtea@gmail.com>

vhost-scsi: Prevent OOM from invalid protection SGL count

The protection SGL path passes the result of vhost_scsi_calc_sgls()
directly to sg_alloc_table_chained(). The helper returns a negative
errno when the iterator is invalid or the request exceeds the segment
limit. The negative errno is then treated as a very large unsigned count
and sends the request into the SGL allocation path with an invalid size.

Cc: stable@vger.kernel.org

> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 9a1253b9d8c50..8486652fd31e8 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -972,6 +972,9 @@ vhost_scsi_mapal(struct vhost_scsi *vs, struct vhost_scsi_cmd *cmd,
>  	if (prot_bytes) {
>  		sgl_count = vhost_scsi_calc_sgls(prot_iter, prot_bytes,
>  						 VHOST_SCSI_PREALLOC_PROT_SGLS);
> +		if (sgl_count < 0)
> +			return sgl_count;
> +

[Severity: Critical]
This is a pre-existing issue, but could sgl_count evaluate to exactly 0 here?

If a guest provides a virtio-scsi request where the protection information
length (prot_bytes) equals or exceeds the total data payload size
(exp_data_len), vhost_scsi_handle_vq() subtracts it without checking for
underflow or equality:

drivers/vhost/scsi.c:vhost_scsi_handle_vq() {
        ...
        if (prot_bytes) {
                exp_data_len -= prot_bytes;
                prot_iter = data_iter;
                iov_iter_truncate(&prot_iter, prot_bytes);
                iov_iter_advance(&data_iter, prot_bytes);
        }
        ...
}

This can reduce the data iterator's count to 0. When vhost_scsi_mapal() calls
vhost_scsi_calc_sgls() on the remaining iterators, iov_iter_npages() evaluates
to 0, returning an sgl_count of 0.

Because the error check added here only tests for < 0, a 0 value will slip
through and be passed as nents to sg_alloc_table_chained(), which will panic
the host kernel:

lib/sg_pool.c:sg_alloc_table_chained() {
        ...
        BUG_ON(!nents);
        ...
}

Would it make sense to change this check to if (sgl_count <= 0) (and
similarly for the data SGL check below it) to prevent a guest-triggerable
host panic?

>  		cmd->prot_table.sgl = cmd->prot_sgl;
>  		ret = sg_alloc_table_chained(&cmd->prot_table, sgl_count,
>  					     cmd->prot_table.sgl,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717142205.103515-1-physicalmtea@gmail.com?part=1