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

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

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