[PATCH 0/5] virtio-snd OOB fixes

[PATCH 0/5] virtio-snd OOB fixes

[Manos Pitsidianakis]

This series adds guards against two potential OOB memory accesses by
checking for integer overflow, and by checking for the correct size of
input buffers.

Also, at the request of mst I added myself as a maintainer.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
Manos Pitsidianakis (5):
      MAINTAINERS: add me as maintainer to virtio-snd
      virtio-snd: remove TODO comments
      virtio-snd: handle 5.14.6.2 for PCM_INFO properly
      virtio-snd: fix max_size bounds check in input cb
      virtio-snd: tighten read amount in in_cb

 MAINTAINERS           |  2 +-
 hw/audio/virtio-snd.c | 68 +++++++++++++++++++++++++++++----------------------
 2 files changed, 40 insertions(+), 30 deletions(-)
---
base-commit: 07f97d5da04a9f97e273de85c76f5017d8135a6e
change-id: 20260220-virtio-snd-series-d9e6aa95466a

--
γαῖα πυρί μιχθήτω

[PATCH 1/5] MAINTAINERS: add me as maintainer to virtio-snd

[Manos Pitsidianakis]

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index e0c481e2125d81298a52caf7f7b51c3e4f026d85..63231747b0692f764b51bf36797cff3e9849e1a2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2601,7 +2601,7 @@ F: include/hw/virtio/virtio-mem.h
 
 virtio-snd
 M: Gerd Hoffmann <kraxel@redhat.com>
-R: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+M: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 S: Supported
 F: hw/audio/virtio-snd.c
 F: hw/audio/virtio-snd-pci.c

-- 
2.47.3

[PATCH 2/5] virtio-snd: remove TODO comments

[Manos Pitsidianakis]

Replying with a VIRTIO_SND_S_BAD_MSG error does not warrant a device
reset. Instead, a device reset happens when the driver requests it from the
transport.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/audio/virtio-snd.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 9101560f3879761d99b2dc97e48864958edfb3bf..fd03efc12043b67bc6f3a16315d2bcc46ddbe990 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -168,9 +168,6 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s,
                                sizeof(virtio_snd_query_info));
 
     if (msg_sz != sizeof(virtio_snd_query_info)) {
-        /*
-         * TODO: do we need to set DEVICE_NEEDS_RESET?
-         */
         qemu_log_mask(LOG_GUEST_ERROR,
                 "%s: virtio-snd command size incorrect %zu vs \
                 %zu\n", __func__, msg_sz, sizeof(virtio_snd_query_info));
@@ -184,9 +181,6 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s,
 
     if (iov_size(cmd->elem->in_sg, cmd->elem->in_num) <
         sizeof(virtio_snd_hdr) + size * count) {
-        /*
-         * TODO: do we need to set DEVICE_NEEDS_RESET?
-         */
         error_report("pcm info: buffer too small, got: %zu, needed: %zu",
                 iov_size(cmd->elem->in_sg, cmd->elem->in_num),
                 sizeof(virtio_snd_pcm_info));
@@ -244,9 +238,6 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
     virtio_snd_pcm_set_params *st_params;
 
     if (stream_id >= s->snd_conf.streams || s->pcm->pcm_params == NULL) {
-        /*
-         * TODO: do we need to set DEVICE_NEEDS_RESET?
-         */
         virtio_error(VIRTIO_DEVICE(s), "Streams have not been initialized.\n");
         return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
     }
@@ -297,9 +288,6 @@ static void virtio_snd_handle_pcm_set_params(VirtIOSound *s,
                                sizeof(virtio_snd_pcm_set_params));
 
     if (msg_sz != sizeof(virtio_snd_pcm_set_params)) {
-        /*
-         * TODO: do we need to set DEVICE_NEEDS_RESET?
-         */
         qemu_log_mask(LOG_GUEST_ERROR,
                 "%s: virtio-snd command size incorrect %zu vs \
                 %zu\n", __func__, msg_sz, sizeof(virtio_snd_pcm_set_params));
@@ -609,9 +597,6 @@ static void virtio_snd_handle_pcm_release(VirtIOSound *s,
                                sizeof(stream_id));
 
     if (msg_sz != sizeof(stream_id)) {
-        /*
-         * TODO: do we need to set DEVICE_NEEDS_RESET?
-         */
         qemu_log_mask(LOG_GUEST_ERROR,
                 "%s: virtio-snd command size incorrect %zu vs \
                 %zu\n", __func__, msg_sz, sizeof(stream_id));
@@ -623,9 +608,6 @@ static void virtio_snd_handle_pcm_release(VirtIOSound *s,
     trace_virtio_snd_handle_pcm_release(stream_id);
     stream = virtio_snd_pcm_get_stream(s, stream_id);
     if (stream == NULL) {
-        /*
-         * TODO: do we need to set DEVICE_NEEDS_RESET?
-         */
         error_report("already released stream %"PRIu32, stream_id);
         virtio_error(VIRTIO_DEVICE(s),
                      "already released stream %"PRIu32,
@@ -668,9 +650,6 @@ process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
                                sizeof(virtio_snd_hdr));
 
     if (msg_sz != sizeof(virtio_snd_hdr)) {
-        /*
-         * TODO: do we need to set DEVICE_NEEDS_RESET?
-         */
         qemu_log_mask(LOG_GUEST_ERROR,
                 "%s: virtio-snd command size incorrect %zu vs \
                 %zu\n", __func__, msg_sz, sizeof(virtio_snd_hdr));

-- 
2.47.3

[PATCH 3/5] virtio-snd: handle 5.14.6.2 for PCM_INFO properly

[Manos Pitsidianakis]

The section 5.14.6.2 of the VIRTIO spec says:

  5.14.6.2 Driver Requirements: Item Information Request

  - The driver MUST NOT set start_id and count such that start_id +
    count is greater than the total number of particular items that is
    indicated in the device configuration space.

  - The driver MUST provide a buffer of sizeof(struct virtio_snd_hdr) +
    count * size bytes for the response.

While we performed some check for the second requirement, it failed to
check for integer overflow.

Add also a check for the first requirement, which should limit exposure
to any overflow, since realistically the number of streams will be low
enough in value such that overflow is improbable.

Cc: qemu-stable@nongnu.org
Reported-by: "罗铭源" <myluo24@m.fudan.edu.cn>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/audio/virtio-snd.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index fd03efc12043b67bc6f3a16315d2bcc46ddbe990..e9c24d679538f0d375c7da05a836833b0897f698 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -156,7 +156,7 @@ static virtio_snd_pcm_set_params *virtio_snd_pcm_get_params(VirtIOSound *s,
 static void virtio_snd_handle_pcm_info(VirtIOSound *s,
                                        virtio_snd_ctrl_command *cmd)
 {
-    uint32_t stream_id, start_id, count, size;
+    uint32_t stream_id, start_id, count, size, tmp;
     virtio_snd_pcm_info val;
     virtio_snd_query_info req;
     VirtIOSoundPCMStream *stream = NULL;
@@ -179,11 +179,34 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s,
     count = le32_to_cpu(req.count);
     size = le32_to_cpu(req.size);
 
-    if (iov_size(cmd->elem->in_sg, cmd->elem->in_num) <
-        sizeof(virtio_snd_hdr) + size * count) {
+    /*
+     * 5.14.6.2 Driver Requirements: Item Information Request
+     * "The driver MUST NOT set start_id and count such that start_id + count
+     * is greater than the total number of particular items that is indicated
+     * in the device configuration space."
+     */
+    if (start_id > s->snd_conf.streams
+        || !g_uint_checked_add(&tmp, start_id, count)
+        || start_id + count > s->snd_conf.streams) {
+        error_report("pcm info: start_id + count is greater than the total "
+                     "number of streams, got: start_id = %u, count = %u",
+                     start_id, count);
+        cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+        return;
+    }
+
+    /*
+     * 5.14.6.2 Driver Requirements: Item Information Request
+     * "The driver MUST provide a buffer of sizeof(struct virtio_snd_hdr) +
+     * count * size bytes for the response."
+     */
+    if (!g_uint_checked_mul(&tmp, size, count)
+        || !g_uint_checked_add(&tmp, tmp, sizeof(virtio_snd_hdr))
+        || iov_size(cmd->elem->in_sg, cmd->elem->in_num) <
+           sizeof(virtio_snd_hdr) + size * count) {
         error_report("pcm info: buffer too small, got: %zu, needed: %zu",
                 iov_size(cmd->elem->in_sg, cmd->elem->in_num),
-                sizeof(virtio_snd_pcm_info));
+                sizeof(virtio_snd_pcm_info) * count);
         cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
         return;
     }

-- 
2.47.3

[PATCH 4/5] virtio-snd: fix max_size bounds check in input cb

[Manos Pitsidianakis]

In 98e77e3d we calculated the max size and checked that each buffer is smaller than it.

We neglected to subtract the size of the virtio_snd_pcm_status header
from the max size, and max_size was thus larger than the correct value,
leading to potential OOB writes.

If the buffer cannot fit the header or can fit only the header, return
the buffer immediately.

Cc: qemu-stable@nongnu.org
Fixes: 98e77e3dd8dd6e7aa9a7dffa60f49c8c8a49d4e3 ("virtio-snd: add max size bounds check in input cb")
Reported-by: DARKNAVY <vr@darknavy.com>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/audio/virtio-snd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index e9c24d679538f0d375c7da05a836833b0897f698..3437211f7904ac77265d8ace8c1a5a582c0be96d 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1255,6 +1255,12 @@ static void virtio_snd_pcm_in_cb(void *data, int available)
             }
 
             max_size = iov_size(buffer->elem->in_sg, buffer->elem->in_num);
+            if (max_size <= sizeof(virtio_snd_pcm_status)) {
+                return_rx_buffer(stream, buffer);
+                continue;
+            }
+            max_size -= sizeof(virtio_snd_pcm_status);
+
             for (;;) {
                 if (buffer->size >= max_size) {
                     return_rx_buffer(stream, buffer);

-- 
2.47.3

[PATCH 5/5] virtio-snd: tighten read amount in in_cb

[Manos Pitsidianakis]

The amount of bytes to read passed to AUD_read() should never surpass
the maximum available buffer length. Tighten the current amount by
MIN(<amount>, max_size - <existing size>).

Cc: qemu-stable@nongnu.org
Fixes: 98e77e3dd8dd6e7aa9a7dffa60f49c8c8a49d4e3 ("virtio-snd: add max size bounds check in input cb")
Reported-by: DARKNAVY <vr@darknavy.com>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/audio/virtio-snd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 3437211f7904ac77265d8ace8c1a5a582c0be96d..fc0781ae9a3564f547e0295a95d8f71fb5426aa9 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1240,7 +1240,7 @@ static void virtio_snd_pcm_in_cb(void *data, int available)
 {
     VirtIOSoundPCMStream *stream = data;
     VirtIOSoundPCMBuffer *buffer;
-    size_t size, max_size;
+    size_t size, max_size, to_read;
 
     WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
         while (!QSIMPLEQ_EMPTY(&stream->queue)) {
@@ -1266,10 +1266,12 @@ static void virtio_snd_pcm_in_cb(void *data, int available)
                     return_rx_buffer(stream, buffer);
                     break;
                 }
+                to_read = stream->params.period_bytes - buffer->size;
+                to_read = MIN(to_read, available);
+                to_read = MIN(to_read, max_size - buffer->size);
                 size = AUD_read(stream->voice.in,
-                        buffer->data + buffer->size,
-                        MIN(available, (stream->params.period_bytes -
-                                        buffer->size)));
+                                buffer->data + buffer->size,
+                                to_read);
                 if (!size) {
                     available = 0;
                     break;

-- 
2.47.3

Re: [PATCH 1/5] MAINTAINERS: add me as maintainer to virtio-snd

[Alex Bennée]

Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:

> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e0c481e2125d81298a52caf7f7b51c3e4f026d85..63231747b0692f764b51bf36797cff3e9849e1a2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2601,7 +2601,7 @@ F: include/hw/virtio/virtio-mem.h
>  
>  virtio-snd
>  M: Gerd Hoffmann <kraxel@redhat.com>
> -R: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> +M: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>  S: Supported
>  F: hw/audio/virtio-snd.c
>  F: hw/audio/virtio-snd-pci.c

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro