Re: [PATCH] vhost: fix used ring index wrap in vhost_add_used_n_in_order

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] vhost: wrong start index for log_used() in vhost_add_used_n_in_order()
--

commit dd320e3245fc3d77e9d6856274ea3ebedcc56808
Author: Fang Xieyan <fangxy@xiaopeng.com>

vhost: fix used ring index wrap in vhost_add_used_n_in_order

This commit masks start with (vq->num - 1) inside the loop in
vhost_add_used_n_in_order() so that advancing start by nheads[i] cannot
leave start >= vq->num when logging used-ring updates.

> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 4d5d32e68d35f..b1a9f8f05cf65 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -3188,8 +3188,7 @@ static int vhost_add_used_n_in_order(struct vhost_virtqueue *vq,
>  		}
>  		start += nheads[i];
>  		new += nheads[i];
> -		if (start >= vq->num)
> -			start -= vq->num;
> +		start &= vq->num - 1;
>  	}
>  
>  	if (unlikely(vq->log_used)) {

[Severity: Critical]
Does masking start inside the loop still leave the log_used() calculations
in drivers/vhost/vhost.c:vhost_add_used_n_in_order() broken, allowing
out-of-bounds writes to the dirty-log bitmap as well as missed dirty-page
logging?

Before the loop in vhost_add_used_n_in_order(), used is anchored to the
initial start index, and then the loop advances and masks start on each
iteration:

	start = vq->last_used_idx & (vq->num - 1);
	used = vq->used->ring + start;

	for (i = 0; i < count; i++) {
		if (vhost_put_used(vq, &heads[i], start, 1)) {
			vq_err(vq, "Failed to write used");
			return -EFAULT;
		}
		start += nheads[i];
		new += nheads[i];
		start &= vq->num - 1;
	}

	if (unlikely(vq->log_used)) {
		/* Make sure data is seen before log. */
		smp_wmb();
		/* Log used ring entry write. */
		log_used(vq, ((void __user *)used - (void __user *)vq->used),
			 (vq->num - start) * sizeof *used);
		if (start + count > vq->num)
			log_used(vq, 0,
				 (start + count - vq->num) * sizeof *used);
	}

When vq->log_used is enabled, the first log_used() call pairs the pre-loop
used pointer with the post-loop start index. If start wraps around the ring
during the loop so that the post-loop start is smaller than the initial
start (for example, initial start = vq->num - 1, count = 1, nheads[0] = 1,
ending with start = 0), then (vq->num - start) * sizeof *used logs
vq->num elements starting from vq->used->ring[vq->num - 1], extending
(vq->num - 1) elements past the end of vq->used->ring.

Similarly, in the second log_used() call, on the vhost-net receive path in
handle_rx(), count can reach 65 while vq->num is 32. If the post-loop start
is 31, (start + count - vq->num) * sizeof *used logs 64 elements from the
start of vq->used, again extending past the end of the 32-entry ring.

Because drivers/vhost/vhost.c:vq_log_used_access_ok() only validates the
bitmap range covering vhost_get_used_size(vq, vq->num):

	return !log_used || log_access_ok(log_base, log_addr,
					  vhost_get_used_size(vq, vq->num));

can logging past the end of vq->used->ring cross a page boundary and cause
log_used() -> log_write() to set bits beyond the end of the userspace
dirty-log bitmap?

Conversely, because each iteration writes a single element at the strided
index start before advancing start by nheads[i], can this also fail to log
dirty pages that were actually written (for instance, when start wraps
during the loop with post-loop start + count <= vq->num so the second
log_used() call is skipped, or when nheads[i] > 1 causes the total span of
written ring entries to be much larger than count)? Would it make more sense
to log each written entry inside the loop, or compute the logged range from
the initial start index and total advance (new - old)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020232956.99970-1-fangxy@xiaopeng.com?part=1