Re: [PATCH 3/5] virtio-blk: add vq_rq[] bounds check in virtio_blk_dma_restart_cb()

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>Hanna Czenczek <hreitz@redhat.com> noted that the array index in
>virtio_blk_dma_restart_cb() is not bounds-checked:
>
>  g_autofree VirtIOBlockReq **vq_rq = g_new0(VirtIOBlockReq *, num_queues);
>  ...
>  while (rq) {
>      VirtIOBlockReq *next = rq->next;
>      uint16_t idx = virtio_get_queue_index(rq->vq);
>
>      rq->next = vq_rq[idx];
>                 ^^^^^^^^^^
>
>The code is correct because both rq->vq and vq_rq[] depend on
>num_queues, but this is indirect and not 100% obvious. Add an assertion.

This sentence could be useful as an inline comment too.

>
>Suggested-by: Hanna Czenczek <hreitz@redhat.com>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
> hw/block/virtio-blk.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>index a0735a9bca..f3193f4b75 100644
>--- a/hw/block/virtio-blk.c
>+++ b/hw/block/virtio-blk.c
>@@ -1209,6 +1209,7 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
>         VirtIOBlockReq *next = rq->next;
>         uint16_t idx = virtio_get_queue_index(rq->vq);
> 
>+        assert(idx < num_queues);
>         rq->next = vq_rq[idx];
>         vq_rq[idx] = rq;
>         rq = next;
>-- 
>2.43.0
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>