[PATCH] virtio-snd: Skip invalid message sizes and null streams

[PATCH] virtio-snd: Skip invalid message sizes and null streams

[Zheyu Ma]

This update changes how virtio_snd_handle_tx_xfer handles message size
discrepancies and null streams. Instead of using error handling paths
which led to unnecessary processing and potential null pointer dereferences,
the function now continues to the next loop iteration.

ASAN log illustrating the issue addressed:

ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000b4 (pc 0x57cea39967b8 bp 0x7ffce84d51b0 sp 0x7ffce84d5160 T0)
    #0 0x57cea39967b8 in qemu_mutex_lock_impl qemu/util/qemu-thread-posix.c:92:5
    #1 0x57cea128c462 in qemu_mutex_lock qemu/include/qemu/thread.h:122:5
    #2 0x57cea128d72f in qemu_lockable_lock qemu/include/qemu/lockable.h:95:5
    #3 0x57cea128c294 in qemu_lockable_auto_lock qemu/include/qemu/lockable.h:105:5
    #4 0x57cea1285eb2 in virtio_snd_handle_rx_xfer qemu/hw/audio/virtio-snd.c:1026:9
    #5 0x57cea2caebbc in virtio_queue_notify_vq qemu/hw/virtio/virtio.c:2268:9
    #6 0x57cea2cae412 in virtio_queue_host_notifier_read qemu/hw/virtio/virtio.c:3671:9
    #7 0x57cea39822f1 in aio_dispatch_handler qemu/util/aio-posix.c:372:9
    #8 0x57cea3979385 in aio_dispatch_handlers qemu/util/aio-posix.c:414:20
    #9 0x57cea3978eb1 in aio_dispatch qemu/util/aio-posix.c:424:5
    #10 0x57cea3a1eede in aio_ctx_dispatch qemu/util/async.c:360:5

Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 hw/audio/virtio-snd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index e604d8f30c..d9e9f980f7 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -913,13 +913,13 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
                             &hdr,
                             sizeof(virtio_snd_pcm_xfer));
         if (msg_sz != sizeof(virtio_snd_pcm_xfer)) {
-            goto tx_err;
+            continue;
         }
         stream_id = le32_to_cpu(hdr.stream_id);
 
         if (stream_id >= s->snd_conf.streams
             || s->pcm->streams[stream_id] == NULL) {
-            goto tx_err;
+            continue;
         }
 
         stream = s->pcm->streams[stream_id];
-- 
2.34.1

Re: [PATCH] virtio-snd: Skip invalid message sizes and null streams

[Manos Pitsidianakis]

Hello Ma,

On Thu, 21 Mar 2024 23:42, Zheyu Ma <zheyuma97@gmail.com> wrote:
>This update changes how virtio_snd_handle_tx_xfer handles message size
>discrepancies and null streams. Instead of using error handling paths
>which led to unnecessary processing and potential null pointer dereferences,
>the function now continues to the next loop iteration.
>
>ASAN log illustrating the issue addressed:
>
>ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000b4 (pc 0x57cea39967b8 bp 0x7ffce84d51b0 sp 0x7ffce84d5160 T0)
>    #0 0x57cea39967b8 in qemu_mutex_lock_impl qemu/util/qemu-thread-posix.c:92:5
>    #1 0x57cea128c462 in qemu_mutex_lock qemu/include/qemu/thread.h:122:5
>    #2 0x57cea128d72f in qemu_lockable_lock qemu/include/qemu/lockable.h:95:5
>    #3 0x57cea128c294 in qemu_lockable_auto_lock qemu/include/qemu/lockable.h:105:5
>    #4 0x57cea1285eb2 in virtio_snd_handle_rx_xfer qemu/hw/audio/virtio-snd.c:1026:9
>    #5 0x57cea2caebbc in virtio_queue_notify_vq qemu/hw/virtio/virtio.c:2268:9
>    #6 0x57cea2cae412 in virtio_queue_host_notifier_read qemu/hw/virtio/virtio.c:3671:9
>    #7 0x57cea39822f1 in aio_dispatch_handler qemu/util/aio-posix.c:372:9
>    #8 0x57cea3979385 in aio_dispatch_handlers qemu/util/aio-posix.c:414:20
>    #9 0x57cea3978eb1 in aio_dispatch qemu/util/aio-posix.c:424:5
>    #10 0x57cea3a1eede in aio_ctx_dispatch qemu/util/async.c:360:5
>
>Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
>---
> hw/audio/virtio-snd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
>index e604d8f30c..d9e9f980f7 100644
>--- a/hw/audio/virtio-snd.c
>+++ b/hw/audio/virtio-snd.c
>@@ -913,13 +913,13 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
>                             &hdr,
>                             sizeof(virtio_snd_pcm_xfer));
>         if (msg_sz != sizeof(virtio_snd_pcm_xfer)) {
>-            goto tx_err;
>+            continue;
>         }
>         stream_id = le32_to_cpu(hdr.stream_id);
> 
>         if (stream_id >= s->snd_conf.streams
>             || s->pcm->streams[stream_id] == NULL) {
>-            goto tx_err;
>+            continue;
>         }
> 
>         stream = s->pcm->streams[stream_id];
>-- 
>2.34.1
>

While the bug is valid I think the fix is insufficient, but not because 
it is wrong. The invalid elements are leaked and the guest never gets a 
BAD_MSG response. The problem is in the error handling logic; I think 
the invalid queue should be moved to the device struct since it's not 
stream specific.

Cc'ing qemu-stable because this bug is present in current versions.

Please make the same changes to virtio_snd_handle_rx_xfer() as well and 
send a v2, cc'ing qemu-stable. With those changes you can add:

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

I will prepare a patch fixing the invalid element handling logic for 
when this fix is accepted.

Thanks,
Manos