Commit 03c519ff authored by hansmi's avatar hansmi
Browse files

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: hansmi's avatarMichael Hanselmann <public@hansmi.ch>
parent 58f198e8
Pipeline #377640 passed with stage
in 2 minutes and 54 seconds
......@@ -21,6 +21,7 @@
#include "config.h"
#include <stdbool.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
......@@ -1656,8 +1657,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;
......@@ -1702,7 +1704,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;
......@@ -1717,7 +1719,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;
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment