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

[Manos Pitsidianakis]

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>