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

Commit Message

Stefan Hajnoczi Feb. 5, 2024, 5:26 p.m. UTC

Hanna Czenczek <[e-mail]> 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.

Suggested-by: Hanna Czenczek <[e-mail]>
Signed-off-by: Stefan Hajnoczi <[e-mail]>
---
 hw/block/virtio-blk.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Manos Pitsidianakis Feb. 6, 2024, 7:20 a.m. UTC | #1

On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi <[e-mail]> wrote:
>Hanna Czenczek <[e-mail]> 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 <[e-mail]>
>Signed-off-by: Stefan Hajnoczi <[e-mail]>
>---
> 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 <[e-mail]>

Hanna Czenczek Feb. 6, 2024, 3:09 p.m. UTC | #2

On 05.02.24 18:26, Stefan Hajnoczi wrote:
> Hanna Czenczek <[e-mail]> 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.
>
> Suggested-by: Hanna Czenczek <[e-mail]>
> Signed-off-by: Stefan Hajnoczi <[e-mail]>
> ---
>   hw/block/virtio-blk.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Hanna Czenczek <[e-mail]>

Patch

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;