Re: [PATCH] virtio: Fix packed virtqueue used_idx mask

[Hanna Czenczek <hreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2893 bytes --]

On 25.07.23 16:04, German Maglione wrote:
>
>
> On Fri, Jul 21, 2023 at 3:51 PM Hanna Czenczek <hreitz@redhat.com> wrote:
>
>     virtio_queue_packed_set_last_avail_idx() is used by vhost devices
>     to set
>     the internal queue indices to what has been reported by the vhost
>     back-end through GET_VRING_BASE.  For packed virtqueues, this
>     32-bit value is expected to contain both the device's internal
>     avail and
>     used indices, as well as their respective wrap counters.
>
>     To get the used index, we shift the 32-bit value right by 16, and then
>     apply a mask of 0x7ffff.  That seems to be a typo, because it
>     should be
>     0x7fff; first of all, the virtio specification says that the maximum
>     queue size for packed virt queues is 2^15, so the indices cannot
>     exceed
>     2^15 - 1 anyway, making 0x7fff the correct mask.  Second, the mask
>     clearly is wrong from context, too, given that (A) `idx & 0x70000`
>     must
>     be 0 at this point (`idx` is 32 bit and was shifted to the right by 16
>     already), (B) `idx & 0x8000` is the used_wrap_counter, so should
>     not be
>     part of the used index, and (C) `vq->used_idx` is a `uint16_t`, so
>     cannot fit the 0x70000 part of the mask anyway.
>
>     This most likely never produced any guest-visible bugs, though,
>     because
>     for a vhost device, qemu will probably not evaluate the used index
>     outside of virtio_queue_packed_get_last_avail_idx(), where we
>     reconstruct the 32-bit value from avail and used indices and their
>     wrap
>     counters again.  There, it does not matter whether the highest bit of
>     the used_idx is the used index wrap counter, because we put the wrap
>     counter exactly in that position anyway.
>
>     Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>     ---
>      hw/virtio/virtio.c | 2 +-
>      1 file changed, 1 insertion(+), 1 deletion(-)
>
>     diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>     index 295a603e58..309038fd46 100644
>     --- a/hw/virtio/virtio.c
>     +++ b/hw/virtio/virtio.c
>     @@ -3321,7 +3321,7 @@ static void
>     virtio_queue_packed_set_last_avail_idx(VirtIODevice *vdev,
>          vq->last_avail_wrap_counter =
>              vq->shadow_avail_wrap_counter = !!(idx & 0x8000);
>          idx >>= 16;
>     -    vq->used_idx = idx & 0x7ffff;
>     +    vq->used_idx = idx & 0x7fff;
>
>
> isn't there a macro with this value?
> or a macro that convert a number of bits in a mask?, something like:
> #define BIT_MASK(n) (~(~0 << n))

((1 << n) - 1) would be what I’d come up with; in any case, there is 
MAKE_64BIT_MASK in qemu/bitops.h, but I don’t know whether I really like 
MAKE_64BIT_MASK(0, 15) more than 0x7fff.  In addition, that would need 
to be done throughout that function and I don’t think that’s worth it 
right now.

Hanna

[-- Attachment #2: Type: text/html, Size: 4770 bytes --]