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 <[email protected]> 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/[email protected]?part=1
