| Message ID | virtio-snd-fuzz-2427-fix-v1-manos.pitsidianakis@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] virtio-snd: add max size bounds check in input cb | expand |
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
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
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.
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
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 -
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(-) base-commit: b9ee1387e0cf0fba5a73a610d31cb9cead457dc0