[PATCH v1 1/1] virtio-snd: add max size bounds check in input cb

[PATCH v1 1/1] virtio-snd: add max size bounds check in input cb

[Manos Pitsidianakis]

When reading input audio in the virtio-snd input callback,
virtio_snd_pcm_in_cb(), we do not check whether the iov can actually fit
the data buffer. This is because we use the buffer->size field as a
total-so-far accumulator instead of byte-size-left like in TX buffers.

This triggers an out of bounds write if the size of the virtio queue
element is equal to virtio_snd_pcm_status, which makes the available
space for audio data zero. This commit adds a check for reaching the
maximum buffer size before attempting any writes.

Reported-by: Zheyu Ma <zheyuma97@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2427
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/audio/virtio-snd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 5993f4f040..e6432ac959 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1261,7 +1261,7 @@ static void virtio_snd_pcm_in_cb(void *data, int available)
 {
     VirtIOSoundPCMStream *stream = data;
     VirtIOSoundPCMBuffer *buffer;
-    size_t size;
+    size_t size, max_size;
 
     WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
         while (!QSIMPLEQ_EMPTY(&stream->queue)) {
@@ -1275,7 +1275,12 @@ static void virtio_snd_pcm_in_cb(void *data, int available)
                 continue;
             }
 
+            max_size = iov_size(buffer->elem->in_sg, buffer->elem->in_num);
             for (;;) {
+                if (buffer->size >= max_size) {
+                    return_rx_buffer(stream, buffer);
+                    break;
+                }
                 size = AUD_read(stream->voice.in,
                         buffer->data + buffer->size,
                         MIN(available, (stream->params.period_bytes -

base-commit: b9ee1387e0cf0fba5a73a610d31cb9cead457dc0
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH v1 1/1] virtio-snd: add max size bounds check in input cb

[Philippe Mathieu-Daudé]

On 8/7/24 09:09, Manos Pitsidianakis wrote:
> When reading input audio in the virtio-snd input callback,
> virtio_snd_pcm_in_cb(), we do not check whether the iov can actually fit
> the data buffer. This is because we use the buffer->size field as a
> total-so-far accumulator instead of byte-size-left like in TX buffers.
> 
> This triggers an out of bounds write if the size of the virtio queue
> element is equal to virtio_snd_pcm_status, which makes the available
> space for audio data zero. This commit adds a check for reaching the
> maximum buffer size before attempting any writes.
> 
> Reported-by: Zheyu Ma <zheyuma97@gmail.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2427
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   hw/audio/virtio-snd.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index 5993f4f040..e6432ac959 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -1261,7 +1261,7 @@ static void virtio_snd_pcm_in_cb(void *data, int available)
>   {
>       VirtIOSoundPCMStream *stream = data;
>       VirtIOSoundPCMBuffer *buffer;
> -    size_t size;
> +    size_t size, max_size;
>   
>       WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
>           while (!QSIMPLEQ_EMPTY(&stream->queue)) {
> @@ -1275,7 +1275,12 @@ static void virtio_snd_pcm_in_cb(void *data, int available)
>                   continue;
>               }
>   
> +            max_size = iov_size(buffer->elem->in_sg, buffer->elem->in_num);
>               for (;;) {
> +                if (buffer->size >= max_size) {
> +                    return_rx_buffer(stream, buffer);

return_rx_buffer() could be renamed
flush_input_stream_to_buffer() for clarity.

I suspect virtio_snd_pcm_in_cb() has a high complexity. Anyhow,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +                    break;
> +                }
>                   size = AUD_read(stream->voice.in,
>                           buffer->data + buffer->size,
>                           MIN(available, (stream->params.period_bytes -
> 
> base-commit: b9ee1387e0cf0fba5a73a610d31cb9cead457dc0

Re: [PATCH v1 1/1] virtio-snd: add max size bounds check in input cb

[Manos Pitsidianakis]

Thanks for the review Philippe,

On Mon, 08 Jul 2024 11:28, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> +            max_size = iov_size(buffer->elem->in_sg, 
>>               buffer->elem->in_num);
>>               for (;;) {
>> +                if (buffer->size >= max_size) {
>> +                    return_rx_buffer(stream, buffer);
>
>return_rx_buffer() could be renamed
>flush_input_stream_to_buffer() for clarity.
>

return_rx_buffer() is vague but I think flush_input_stream_to_buffer() 
is more vague 🤔. I tried to use short names as much as possible and 
document the purpose in doc comments. Unfortunately the device's state 
machine is complex so the code complexity is necessary :/

Manos

Re: [PATCH v1 1/1] virtio-snd: add max size bounds check in input cb

[Matias Ezequiel Vara Larsen]

Thanks Manos for sending this,

On Mon, Jul 08, 2024 at 10:09:49AM +0300, Manos Pitsidianakis wrote:
> When reading input audio in the virtio-snd input callback,
> virtio_snd_pcm_in_cb(), we do not check whether the iov can actually fit
> the data buffer. This is because we use the buffer->size field as a
> total-so-far accumulator instead of byte-size-left like in TX buffers.
> 
> This triggers an out of bounds write if the size of the virtio queue
> element is equal to virtio_snd_pcm_status, which makes the available
> space for audio data zero.

Do you mean that the guest driver has set up a request in the rx queue
in which the writable chain of descriptors only contains the status? Is
this correct? Is `available` indicating the available space in the
virtqueue?

Thanks, Matias.

Re: [PATCH v1 1/1] virtio-snd: add max size bounds check in input cb

[Manos Pitsidianakis]

On Tue, 09 Jul 2024 17:16, Matias Ezequiel Vara Larsen <mvaralar@redhat.com> wrote:
>Thanks Manos for sending this,
>
>On Mon, Jul 08, 2024 at 10:09:49AM +0300, Manos Pitsidianakis wrote:
>> When reading input audio in the virtio-snd input callback,
>> virtio_snd_pcm_in_cb(), we do not check whether the iov can actually fit
>> the data buffer. This is because we use the buffer->size field as a
>> total-so-far accumulator instead of byte-size-left like in TX buffers.
>> 
>> This triggers an out of bounds write if the size of the virtio queue
>> element is equal to virtio_snd_pcm_status, which makes the available
>> space for audio data zero.
>
>Do you mean that the guest driver has set up a request in the rx queue
>in which the writable chain of descriptors only contains the status? Is
>this correct? Is `available` indicating the available space in the
>virtqueue?
>
>Thanks, Matias.


Hi Matias,

See mentioned bug report, this was found by fuzzing, it's not behavior 
from existing drivers.

Manos