virtio: Fix packed virtqueue used_idx mask

[PATCH] virtio: Fix packed virtqueue used_idx mask

Posted by Hanna Czenczek 9 months, 2 weeks ago

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;
     vq->used_wrap_counter = !!(idx & 0x8000);
 }
 
-- 
2.41.0

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

Posted by German Maglione 9 months, 1 week ago

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;
>      vq->used_wrap_counter = !!(idx & 0x8000);
>  }
>
> --
> 2.41.0
>
>

Reviewed-by: German Maglione <gmaglione@redhat.com>

-- 
German

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

Posted by German Maglione 9 months, 1 week ago

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))



>      vq->used_wrap_counter = !!(idx & 0x8000);
>  }
>
> --
> 2.41.0
>
>
>


-- 
German

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

Posted by Hanna Czenczek 9 months, 1 week ago

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