[PATCH] vringh: reject empty / undersized indirect descriptor tables

[PATCH] vringh: reject empty / undersized indirect descriptor tables

[Fang Xieyan]

move_to_indirect() rejects an indirect descriptor table only when its
length is not an exact multiple of sizeof(struct vring_desc).  A guest
descriptor with VRING_DESC_F_INDIRECT and len == 0 passes that check, so
*desc_max becomes 0, yet __vringh_iov() keeps walking the (empty) table
and aborts with -ELOOP only after reading one full descriptor past its
end -- leaking 16 bytes of memory adjacent to the table into a kernel
stack variable.

Reject any len smaller than one descriptor, before the existing stride
check, so no descriptor is ever fetched from an empty table.

Fixes: f87d0fbb5798 ("vringh: host-side implementation of virtio rings.")
Cc: stable@vger.kernel.org
Assisted-by: Hawkeye:GLM-5.3-flash
Assisted-by: Qoder:Qwen3.8-Max
Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
---
 drivers/vhost/vringh.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Leak path: once move_to_indirect() sets *desc_max = 0 and points *descs
at the (empty) table, the next __vringh_iov() iteration first runs

	err = copy(vrh, &desc, &descs[i], sizeof(desc));

i.e. a 16-byte read from descs[0] -- one full struct vring_desc past the
end of the table -- *before* the "indirect_count > desc_max" test fires.
When the guest page backing the table sits just before a sensitive host
page, those 16 bytes are attacker-influenced adjacent memory.

The multiple-of-16 stride check is kept for defense in depth.

Userspace reproducer (move_to_indirect()/__vringh_iov() extracted verbatim,
2048-byte region followed by a guarded red zone):

    [VULNERABLE] return=-62 (-ELOOP)  OOB-read=YES  bytes-past-region=16
    [PATCHED   ] return=-22 (-EINVAL) OOB-read=no   bytes-past-region=0

diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index 9066f9f..0767748 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -197,8 +197,9 @@ static int move_to_indirect(const struct vringh *vrh,
 	}
 
 	len = vringh32_to_cpu(vrh, desc->len);
-	if (unlikely(len % sizeof(struct vring_desc))) {
-		vringh_bad("Strange indirect len %u", desc->len);
+	if (unlikely(len < sizeof(struct vring_desc) ||
+		     len % sizeof(struct vring_desc))) {
+		vringh_bad("Invalid indirect len %u", desc->len);
 		return -EINVAL;
 	}
 
-- 
2.50.1

Re: [PATCH] vringh: reject empty / undersized indirect descriptor tables

[Michael S. Tsirkin]

On Thu, Sep 24, 2026 at 11:06:27AM +0800, Fang Xieyan wrote:
> move_to_indirect() rejects an indirect descriptor table only when its
> length is not an exact multiple of sizeof(struct vring_desc).  A guest
> descriptor with VRING_DESC_F_INDIRECT and len == 0 passes that check, so
> *desc_max becomes 0, yet __vringh_iov() keeps walking the (empty) table
> and aborts with -ELOOP only after reading one full descriptor past its
> end -- leaking 16 bytes of memory adjacent to the table into a kernel
> stack variable.
> 
> Reject any len smaller than one descriptor, before the existing stride
> check, so no descriptor is ever fetched from an empty table.
> 
> Fixes: f87d0fbb5798 ("vringh: host-side implementation of virtio rings.")
> Cc: stable@vger.kernel.org
> Assisted-by: Hawkeye:GLM-5.3-flash
> Assisted-by: Qoder:Qwen3.8-Max
> Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
> ---
>  drivers/vhost/vringh.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Leak path: once move_to_indirect() sets *desc_max = 0 and points *descs
> at the (empty) table, the next __vringh_iov() iteration first runs
> 
> 	err = copy(vrh, &desc, &descs[i], sizeof(desc));
> 
> i.e. a 16-byte read from descs[0] -- one full struct vring_desc past the
> end of the table -- *before* the "indirect_count > desc_max" test fires.
> When the guest page backing the table sits just before a sensitive host
> page, those 16 bytes are attacker-influenced adjacent memory.


> The multiple-of-16 stride check is kept for defense in depth.
> 
> Userspace reproducer (move_to_indirect()/__vringh_iov() extracted verbatim,
> 2048-byte region followed by a guarded red zone):
> 
>     [VULNERABLE] return=-62 (-ELOOP)  OOB-read=YES  bytes-past-region=16
>     [PATCHED   ] return=-22 (-EINVAL) OOB-read=no   bytes-past-region=0

it seems nicer not to fail with EINVAL not with ELOOP as we currently
do, so the patch is fine. but the commit log if weird, looks like
an ai hallucination.


"an attacker" "vulnerable" and "leak" - is there an implication that this is a
security problem somehow? Because I do not see how this data gets
anywhere.

> 
> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> index 9066f9f..0767748 100644
> --- a/drivers/vhost/vringh.c
> +++ b/drivers/vhost/vringh.c
> @@ -197,8 +197,9 @@ static int move_to_indirect(const struct vringh *vrh,
>  	}
>  
>  	len = vringh32_to_cpu(vrh, desc->len);
> -	if (unlikely(len % sizeof(struct vring_desc))) {
> -		vringh_bad("Strange indirect len %u", desc->len);
> +	if (unlikely(len < sizeof(struct vring_desc) ||
> +		     len % sizeof(struct vring_desc))) {
> +		vringh_bad("Invalid indirect len %u", desc->len);
>  		return -EINVAL;
>  	}
>  
> -- 
> 2.50.1

[PATCH v2] vringh: reject empty / undersized indirect descriptor tables

[Fang Xieyan]

move_to_indirect() validates an indirect descriptor table only with
"len % sizeof(struct vring_desc)".  A descriptor flagged
VRING_DESC_F_INDIRECT with len == 0 passes that test, so *desc_max is
set to 0 while *descs points at the empty table.  __vringh_iov() then
copies one struct vring_desc from descs[0] -- 16 bytes past the end of
the table -- before the "indirect_count > desc_max" loop detection
aborts the walk.  The over-read value is discarded when the walk aborts
and is never used to map anything, but the access itself is out of
bounds.

Reject any len smaller than one descriptor, alongside the existing
stride check, so an empty table is refused with -EINVAL and no
descriptor is ever fetched from it.

Fixes: f87d0fbb5798 ("vringh: host-side implementation of virtio rings.")
Cc: stable@vger.kernel.org
Assisted-by: Hawkeye:GLM-5.3-flash
Assisted-by: Qoder:Qwen3.8-Max
Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
---
 drivers/vhost/vringh.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Changes in v2:
  - Commit message rewritten per Michael's review: the over-read value
    is copied into a local struct vring_desc and never reaches the
    caller, so the security framing ("leak" etc.) is dropped and this
    is presented as the plain out-of-bounds read fix it is.
  - Code is unchanged; the diff is identical to v1.

Testing:
  Userspace reproducer carrying the move_to_indirect()/__vringh_iov()
  logic with an instrumented copy() (len == 0 indirect table, 2048-byte
  mapped region followed by 64 guard bytes):

    without the fix:  reads 16 bytes past the table end, returns -ELOOP
    with the fix:     returns -EINVAL, no read past the table end

diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index 9066f9f..0767748 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -197,8 +197,9 @@ static int move_to_indirect(const struct vringh *vrh,
 	}
 
 	len = vringh32_to_cpu(vrh, desc->len);
-	if (unlikely(len % sizeof(struct vring_desc))) {
-		vringh_bad("Strange indirect len %u", desc->len);
+	if (unlikely(len < sizeof(struct vring_desc) ||
+		     len % sizeof(struct vring_desc))) {
+		vringh_bad("Invalid indirect len %u", desc->len);
 		return -EINVAL;
 	}
 
-- 
2.50.1