From 03c519ff5831ba75120e00ebebbf1d5a1f7220ab Mon Sep 17 00:00:00 2001 From: Michael Hanselmann Date: Sun, 8 Aug 2021 15:35:58 +0200 Subject: [PATCH] Avoid use-after-free in serialization Serializing parsers with large amounts of buffered write data (e.g. in case of a slow or blocked write destination) would cause "serialize_data" to reallocate the state buffer whose default size is 64kB (USBREDIRPARSER_SERIALIZE_BUF_SIZE). The pointer to the position for the write buffer count would then point to a location outside the buffer where the number of write buffers would be written as a 32-bit value. As of QEMU 5.2.0 the serializer is invoked for migrations. Serializations for migrations may happen regularily such as when using the COLO feature[1]. Serialization happens under QEMU's I/O lock. The guest can't control the state while the serialization is happening. The value written is the number of outstanding buffers which would be suceptible to timing and host system system load. The guest would have to continously groom the write buffers. A useful value needs to be allocated in the exact position freed during the buffer size increase, but before the buffer count is written. The author doesn't consider it realistic to exploit this use-after-free reliably. [1] https://wiki.qemu.org/Features/COLO Signed-off-by: Michael Hanselmann --- usbredirparser/usbredirparser.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/usbredirparser/usbredirparser.c b/usbredirparser/usbredirparser.c index d1f9850..dc5f5a4 100644 --- a/usbredirparser/usbredirparser.c +++ b/usbredirparser/usbredirparser.c @@ -20,6 +20,7 @@ */ #include "config.h" +#include #include #include #include @@ -1580,8 +1581,9 @@ int usbredirparser_serialize(struct usbredirparser *parser_pub, struct usbredirparser_priv *parser = (struct usbredirparser_priv *)parser_pub; struct usbredirparser_buf *wbuf; - uint8_t *write_buf_count_pos, *state = NULL, *pos = NULL; + uint8_t *state = NULL, *pos = NULL; uint32_t write_buf_count = 0, len, remain = 0; + ptrdiff_t write_buf_count_pos; *state_dest = NULL; *state_len = 0; @@ -1626,7 +1628,7 @@ int usbredirparser_serialize(struct usbredirparser *parser_pub, parser->data, parser->data_read, "packet-data")) return -1; - write_buf_count_pos = pos; + write_buf_count_pos = pos - state; /* To be replaced with write_buf_count later */ if (serialize_int(parser, &state, &pos, &remain, 0, "write_buf_count")) return -1; @@ -1641,7 +1643,7 @@ int usbredirparser_serialize(struct usbredirparser *parser_pub, wbuf = wbuf->next; } /* Patch in write_buf_count */ - memcpy(write_buf_count_pos, &write_buf_count, sizeof(int32_t)); + memcpy(state + write_buf_count_pos, &write_buf_count, sizeof(int32_t)); /* Patch in length */ len = pos - state; -- 2.23.0