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