[Spice-commits] 19 commits - server/red_parse_qxl.c server/red_worker.c
Frediano Ziglio
fziglio at kemper.freedesktop.org
Tue Oct 6 03:29:32 PDT 2015
server/red_parse_qxl.c | 218 ++++++++++++++++++++++++++++++++++++++-----------
server/red_worker.c | 42 +++++----
2 files changed, 196 insertions(+), 64 deletions(-)
New commits:
commit 6e3547f8b192f5b01d478ca222bf46736f5c700c
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Thu Sep 17 15:01:05 2015 +0100
Prevent leak if size from red_get_data_chunks don't match in red_get_image
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 3ce4431..dd52602 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -526,6 +526,7 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
&chunks, qxl->bitmap.data);
spice_assert(size == bitmap_size);
if (size != bitmap_size) {
+ red_put_data_chunks(&chunks);
goto error;
}
red->u.bitmap.data = red_get_image_data_chunked(slots, group_id,
@@ -546,6 +547,7 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
&chunks, (QXLDataChunk *)qxl->quic.data);
spice_assert(size == red->u.quic.data_size);
if (size != red->u.quic.data_size) {
+ red_put_data_chunks(&chunks);
goto error;
}
red->u.quic.data = red_get_image_data_chunked(slots, group_id,
commit b3be589ab3b32af3e470a9dec19a61fb086f72fc
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Thu Sep 17 14:28:36 2015 +0100
Prevent data_size to be set independently from data
There was not check for data_size field so one could set data to
a small set of data and data_size much bigger than size of data
leading to buffer overflow.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index c7f8650..3ce4431 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -1388,6 +1388,7 @@ static int red_get_cursor(RedMemSlotInfo *slots, int group_id,
size = red_get_data_chunks_ptr(slots, group_id,
get_memslot_id(slots, addr),
&chunks, &qxl->chunk);
+ red->data_size = MIN(red->data_size, size);
data = red_linearize_chunk(&chunks, size, &free_data);
red_put_data_chunks(&chunks);
if (free_data) {
commit 2b6695f1222f68690ea230e4e37ded7e07188f06
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 15 16:38:23 2015 +0100
Avoid race condition copying segments in red_get_path
The guest can attempt to increase the number of segments while
spice-server is reading them.
Make sure we don't copy more then the allocated segments.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 281faad..c7f8650 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -272,7 +272,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
seg = (SpicePathSeg*)&red->segments[n_segments];
n_segments = 0;
mem_size2 = sizeof(*red);
- while (start+1 < end) {
+ while (start+1 < end && n_segments < red->num_segments) {
red->segments[n_segments++] = seg;
count = start->count;
commit 2693e0497e5626642250cff47a59b3b4b2cd432d
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 15 16:25:17 2015 +0100
Make sure we can read QXLPathSeg structures
start pointer points to a QXLPathSeg structure.
Before reading from the structure, make sure the structure is contained
in the memory range checked.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index f21bfa5..281faad 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -256,7 +256,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
start = (QXLPathSeg*)data;
end = (QXLPathSeg*)(data + size);
- while (start < end) {
+ while (start+1 < end) {
n_segments++;
count = start->count;
segment_size = sizeof(SpicePathSeg) + count * sizeof(SpicePointFix);
@@ -272,7 +272,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
seg = (SpicePathSeg*)&red->segments[n_segments];
n_segments = 0;
mem_size2 = sizeof(*red);
- while (start < end) {
+ while (start+1 < end) {
red->segments[n_segments++] = seg;
count = start->count;
commit a447c4f2ac19a1fa36330ffc90ee70b953b82050
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 13:06:03 2015 +0100
Fix some possible overflows in red_get_string for 32 bit
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 5513e82..f21bfa5 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -892,6 +892,11 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
glyphs++;
glyph_size = start->height * ((start->width * bpp + 7u) / 8u);
red_size += sizeof(SpiceRasterGlyph *) + SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4);
+ /* do the test correctly, we know end - start->data[0] cannot
+ * overflow, don't use start->data[glyph_size] to test for
+ * buffer overflow as this on 32 bit can cause overflow
+ * on the pointer arithmetic */
+ spice_assert(glyph_size <= (char*) end - (char*) &start->data[0]);
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
}
spice_assert(start <= end);
@@ -912,7 +917,8 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
red_get_point_ptr(&glyph->render_pos, &start->render_pos);
red_get_point_ptr(&glyph->glyph_origin, &start->glyph_origin);
glyph_size = glyph->height * ((glyph->width * bpp + 7u) / 8u);
- spice_assert((QXLRasterGlyph*)(&start->data[glyph_size]) <= end);
+ /* see above for similar test */
+ spice_assert(glyph_size <= (char*) end - (char*) &start->data[0]);
memcpy(glyph->data, start->data, glyph_size);
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
glyph = (SpiceRasterGlyph*)
commit 7d69184037d0abb4fcfd5625c765b822aa458808
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 12:28:54 2015 +0100
Prevent DoS from guest trying to allocate too much data on host for chunks
Limit number of chunks to a given amount to avoid guest trying to
allocate too much memory. Using circular or nested chunks lists
guest could try to allocate huge amounts of memory.
Considering the list can be infinite and guest can change data this
also prevents strange security attacks from guest.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index f425869..5513e82 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -37,6 +37,13 @@
G_STATIC_ASSERT(MAX_DATA_CHUNK <= G_MAXINT32);
+/* Limit number of chunks.
+ * The guest can attempt to make host allocate too much memory
+ * just with a large number of small chunks.
+ * Prevent that the chunk list take more memory than the data itself.
+ */
+#define MAX_CHUNKS (MAX_DATA_CHUNK/1024u)
+
#if 0
static void hexdump_qxl(RedMemSlotInfo *slots, int group_id,
QXLPHYSICAL addr, uint8_t bytes)
@@ -100,9 +107,11 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
RedDataChunk *red, QXLDataChunk *qxl)
{
RedDataChunk *red_prev;
- size_t data_size = 0;
+ uint64_t data_size = 0;
+ uint32_t chunk_data_size;
int error;
QXLPHYSICAL next_chunk;
+ unsigned num_chunks = 0;
red->data_size = qxl->data_size;
data_size += red->data_size;
@@ -114,19 +123,43 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
}
while ((next_chunk = qxl->next_chunk) != 0) {
+ /* somebody is trying to use too much memory using a lot of chunks.
+ * Or made a circular list of chunks
+ */
+ if (++num_chunks >= MAX_CHUNKS) {
+ spice_warning("data split in too many chunks, avoiding DoS\n");
+ goto error;
+ }
+
+ memslot_id = get_memslot_id(slots, next_chunk);
+ qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl),
+ group_id, &error);
+ if (error)
+ goto error;
+
+ /* do not waste space for empty chunks.
+ * This could be just a driver issue or an attempt
+ * to allocate too much memory or a circular list.
+ * All above cases are handled by the check for number
+ * of chunks.
+ */
+ chunk_data_size = qxl->data_size;
+ if (chunk_data_size == 0)
+ continue;
+
red_prev = red;
red = spice_new0(RedDataChunk, 1);
+ red->data_size = chunk_data_size;
red->prev_chunk = red_prev;
+ red->data = qxl->data;
red_prev->next_chunk = red;
- memslot_id = get_memslot_id(slots, next_chunk);
- qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id,
- &error);
- if (error)
+ data_size += chunk_data_size;
+ /* this can happen if client is sending nested chunks */
+ if (data_size > MAX_DATA_CHUNK) {
+ spice_warning("too much data inside chunks, avoiding DoS\n");
goto error;
- red->data_size = qxl->data_size;
- data_size += red->data_size;
- red->data = qxl->data;
+ }
if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id))
goto error;
}
commit f3605979ce3b33d60c33b59334b53618e6d8662a
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 12:14:55 2015 +0100
Prevent memory leak if red_get_data_chunks_ptr fails
Free linked list if client tries to do nasty things
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 2863ae2..f425869 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -107,34 +107,43 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
red->data_size = qxl->data_size;
data_size += red->data_size;
red->data = qxl->data;
+ red->prev_chunk = red->next_chunk = NULL;
if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) {
red->data = NULL;
return 0;
}
- red->prev_chunk = NULL;
while ((next_chunk = qxl->next_chunk) != 0) {
red_prev = red;
- red = spice_new(RedDataChunk, 1);
+ red = spice_new0(RedDataChunk, 1);
+ red->prev_chunk = red_prev;
+ red_prev->next_chunk = red;
+
memslot_id = get_memslot_id(slots, next_chunk);
qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id,
&error);
- if (error) {
- return 0;
- }
+ if (error)
+ goto error;
red->data_size = qxl->data_size;
data_size += red->data_size;
red->data = qxl->data;
- if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) {
- red->data = NULL;
- return 0;
- }
- red->prev_chunk = red_prev;
- red_prev->next_chunk = red;
+ if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id))
+ goto error;
}
red->next_chunk = NULL;
return data_size;
+
+error:
+ while (red->prev_chunk) {
+ red_prev = red->prev_chunk;
+ free(red);
+ red = red_prev;
+ }
+ red->data_size = 0;
+ red->next_chunk = NULL;
+ red->data = NULL;
+ return 0;
}
static size_t red_get_data_chunks(RedMemSlotInfo *slots, int group_id,
commit 3738478ed7065fe05f3ee4848f8a7fcdf40aa920
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 12:12:19 2015 +0100
Fix race condition in red_get_data_chunks_ptr
Do not read multiple times data from guest as this can be changed by
other guest vcpus. This causes races and security problems if these
data are used for buffer allocation or checks.
Actually, the 'data' member can't change during read as it is just a
pointer to a fixed array contained in qxl. However, this change will
make it clear that there can be no race condition.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index cfa21f9..2863ae2 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -102,30 +102,33 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
RedDataChunk *red_prev;
size_t data_size = 0;
int error;
+ QXLPHYSICAL next_chunk;
red->data_size = qxl->data_size;
data_size += red->data_size;
- if (!validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id)) {
+ red->data = qxl->data;
+ if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) {
+ red->data = NULL;
return 0;
}
- red->data = qxl->data;
red->prev_chunk = NULL;
- while (qxl->next_chunk) {
+ while ((next_chunk = qxl->next_chunk) != 0) {
red_prev = red;
red = spice_new(RedDataChunk, 1);
- memslot_id = get_memslot_id(slots, qxl->next_chunk);
- qxl = (QXLDataChunk *)get_virt(slots, qxl->next_chunk, sizeof(*qxl), group_id,
+ memslot_id = get_memslot_id(slots, next_chunk);
+ qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id,
&error);
if (error) {
return 0;
}
red->data_size = qxl->data_size;
data_size += red->data_size;
- if (!validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id)) {
+ red->data = qxl->data;
+ if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id)) {
+ red->data = NULL;
return 0;
}
- red->data = qxl->data;
red->prev_chunk = red_prev;
red_prev->next_chunk = red;
}
commit caec52dc77af6ebdac3219a1b10fe2293af21208
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 10:13:24 2015 +0100
Fix integer overflow computing glyph_size in red_get_string
If bpp is int the formula can lead to weird overflows. width and height
are uint16_t so the formula is:
size_t = u16 * (u16 * int + const_int) / const_int;
so it became
size_t = (int) u16 * ((int) u16 * int + const_int) / const_int;
However the (int) u16 * (int) u16 can then became negative to overflow.
Under 64 bit architectures size_t is 64 and int usually 32 so converting
this negative 32 bit number to a unsigned 64 bit lead to a very big
number as the signed is extended and then converted to unsigned.
Using unsigned arithmetic prevent extending the sign.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index d097aa3..cfa21f9 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -804,7 +804,9 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
uint8_t *data;
bool free_data;
size_t chunk_size, qxl_size, red_size, glyph_size;
- int glyphs, bpp = 0, i;
+ int glyphs, i;
+ /* use unsigned to prevent integer overflow in multiplication below */
+ unsigned int bpp = 0;
int error;
uint16_t qxl_flags, qxl_length;
@@ -843,7 +845,7 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
while (start < end) {
spice_assert((QXLRasterGlyph*)(&start->data[0]) <= end);
glyphs++;
- glyph_size = start->height * ((start->width * bpp + 7) / 8);
+ glyph_size = start->height * ((start->width * bpp + 7u) / 8u);
red_size += sizeof(SpiceRasterGlyph *) + SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4);
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
}
@@ -864,7 +866,7 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
glyph->height = start->height;
red_get_point_ptr(&glyph->render_pos, &start->render_pos);
red_get_point_ptr(&glyph->glyph_origin, &start->glyph_origin);
- glyph_size = glyph->height * ((glyph->width * bpp + 7) / 8);
+ glyph_size = glyph->height * ((glyph->width * bpp + 7u) / 8u);
spice_assert((QXLRasterGlyph*)(&start->data[glyph_size]) <= end);
memcpy(glyph->data, start->data, glyph_size);
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
commit dfaedec7890069b35f513e4a8ab4071ca54259ff
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 10:05:20 2015 +0100
Fix race condition in red_get_string
Do not read multiple time an array size that can be changed.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 5656bfb..d097aa3 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -806,6 +806,7 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
size_t chunk_size, qxl_size, red_size, glyph_size;
int glyphs, bpp = 0, i;
int error;
+ uint16_t qxl_flags, qxl_length;
qxl = (QXLString *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
if (error) {
@@ -822,13 +823,15 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
red_put_data_chunks(&chunks);
qxl_size = qxl->data_size;
+ qxl_flags = qxl->flags;
+ qxl_length = qxl->length;
spice_assert(chunk_size == qxl_size);
- if (qxl->flags & SPICE_STRING_FLAGS_RASTER_A1) {
+ if (qxl_flags & SPICE_STRING_FLAGS_RASTER_A1) {
bpp = 1;
- } else if (qxl->flags & SPICE_STRING_FLAGS_RASTER_A4) {
+ } else if (qxl_flags & SPICE_STRING_FLAGS_RASTER_A4) {
bpp = 4;
- } else if (qxl->flags & SPICE_STRING_FLAGS_RASTER_A8) {
+ } else if (qxl_flags & SPICE_STRING_FLAGS_RASTER_A8) {
bpp = 8;
}
spice_assert(bpp != 0);
@@ -845,11 +848,11 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
start = (QXLRasterGlyph*)(&start->data[glyph_size]);
}
spice_assert(start <= end);
- spice_assert(glyphs == qxl->length);
+ spice_assert(glyphs == qxl_length);
red = spice_malloc(red_size);
- red->length = qxl->length;
- red->flags = qxl->flags;
+ red->length = qxl_length;
+ red->flags = qxl_flags;
start = (QXLRasterGlyph*)data;
end = (QXLRasterGlyph*)(data + chunk_size);
commit 9235c84e0fbbf5c19305e82fc1607393b35b74ef
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 10:04:10 2015 +0100
Fix race in red_get_image
Do not read multiple times data from guest as this could be changed
by other vcpu threads.
This causes races and security problems if these data are used for
buffer allocation or checks.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index a9f3ca1..5656bfb 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -393,6 +393,7 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
uint64_t bitmap_size, size;
uint8_t qxl_flags;
int error;
+ QXLPHYSICAL palette;
if (addr == 0) {
return NULL;
@@ -418,12 +419,16 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
switch (red->descriptor.type) {
case SPICE_IMAGE_TYPE_BITMAP:
red->u.bitmap.format = qxl->bitmap.format;
- if (!bitmap_fmt_is_rgb(qxl->bitmap.format) && !qxl->bitmap.palette && !is_mask) {
+ red->u.bitmap.x = qxl->bitmap.x;
+ red->u.bitmap.y = qxl->bitmap.y;
+ red->u.bitmap.stride = qxl->bitmap.stride;
+ palette = qxl->bitmap.palette;
+ if (!bitmap_fmt_is_rgb(red->u.bitmap.format) && !palette && !is_mask) {
spice_warning("guest error: missing palette on bitmap format=%d\n",
red->u.bitmap.format);
goto error;
}
- if (qxl->bitmap.x == 0 || qxl->bitmap.y == 0) {
+ if (red->u.bitmap.x == 0 || red->u.bitmap.y == 0) {
spice_warning("guest error: zero area bitmap\n");
goto error;
}
@@ -431,23 +436,20 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
if (qxl_flags & QXL_BITMAP_TOP_DOWN) {
red->u.bitmap.flags = SPICE_BITMAP_FLAGS_TOP_DOWN;
}
- red->u.bitmap.x = qxl->bitmap.x;
- red->u.bitmap.y = qxl->bitmap.y;
- red->u.bitmap.stride = qxl->bitmap.stride;
if (!bitmap_consistent(&red->u.bitmap)) {
goto error;
}
- if (qxl->bitmap.palette) {
+ if (palette) {
QXLPalette *qp;
int i, num_ents;
- qp = (QXLPalette *)get_virt(slots, qxl->bitmap.palette,
+ qp = (QXLPalette *)get_virt(slots, palette,
sizeof(*qp), group_id, &error);
if (error) {
goto error;
}
num_ents = qp->num_ents;
if (!validate_virt(slots, (intptr_t)qp->ents,
- get_memslot_id(slots, qxl->bitmap.palette),
+ get_memslot_id(slots, palette),
num_ents * sizeof(qp->ents[0]), group_id)) {
goto error;
}
commit 3dfd1a08286d524a742d51952595fcfb6f0c6f1b
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 10:01:51 2015 +0100
Fix race condition on red_get_clip_rects
Do not read multiple time an array size that can be changed.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 40c1c99..a9f3ca1 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -273,6 +273,7 @@ static SpiceClipRects *red_get_clip_rects(RedMemSlotInfo *slots, int group_id,
size_t size;
int i;
int error;
+ uint32_t num_rects;
qxl = (QXLClipRects *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
if (error) {
@@ -284,9 +285,10 @@ static SpiceClipRects *red_get_clip_rects(RedMemSlotInfo *slots, int group_id,
data = red_linearize_chunk(&chunks, size, &free_data);
red_put_data_chunks(&chunks);
- spice_assert(qxl->num_rects * sizeof(QXLRect) == size);
- red = spice_malloc(sizeof(*red) + qxl->num_rects * sizeof(SpiceRect));
- red->num_rects = qxl->num_rects;
+ num_rects = qxl->num_rects;
+ spice_assert(num_rects * sizeof(QXLRect) == size);
+ red = spice_malloc(sizeof(*red) + num_rects * sizeof(SpiceRect));
+ red->num_rects = num_rects;
start = (QXLRect*)data;
for (i = 0; i < red->num_rects; i++) {
commit 0f58e9da56e0cbbe4349eefcbb300b6f285e0423
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 13:09:35 2015 +0100
Prevent 32 bit integer overflow in bitmap_consistent
The overflow may lead to buffer overflow as the row size computed from
width (bitmap->x) can be bigger than the size in bytes (bitmap->stride).
This can make spice-server accept the invalid sizes.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index e2f95e4..40c1c99 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -357,11 +357,12 @@ static const char *bitmap_format_to_string(int format)
return "unknown";
}
-static const int MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[] = {0, 1, 1, 4, 4, 8, 16, 24, 32, 32, 8};
+static const unsigned int MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[] =
+ {0, 1, 1, 4, 4, 8, 16, 24, 32, 32, 8};
static int bitmap_consistent(SpiceBitmap *bitmap)
{
- int bpp;
+ unsigned int bpp;
if (bitmap->format >= SPICE_N_ELEMENTS(MAP_BITMAP_FMT_TO_BITS_PER_PIXEL)) {
spice_warning("wrong format specified for image\n");
@@ -370,7 +371,7 @@ static int bitmap_consistent(SpiceBitmap *bitmap)
bpp = MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[bitmap->format];
- if (bitmap->stride < ((bitmap->x * bpp + 7) / 8)) {
+ if (bitmap->stride < (((uint64_t) bitmap->x * bpp + 7u) / 8u)) {
spice_warning("image stride too small for width: %d < ((%d * %d + 7) / 8) (%s=%d)\n",
bitmap->stride, bitmap->x, bpp,
bitmap_format_to_string(bitmap->format),
commit 68a742aaa8d692940ac15d021799b702412887e5
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 10:00:37 2015 +0100
Fix buffer reading overflow
Not security risk as just for read.
However, this could be used to attempt integer overflows in the
following lines.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index bdd5917..e2f95e4 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -361,7 +361,14 @@ static const int MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[] = {0, 1, 1, 4, 4, 8, 16, 24,
static int bitmap_consistent(SpiceBitmap *bitmap)
{
- int bpp = MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[bitmap->format];
+ int bpp;
+
+ if (bitmap->format >= SPICE_N_ELEMENTS(MAP_BITMAP_FMT_TO_BITS_PER_PIXEL)) {
+ spice_warning("wrong format specified for image\n");
+ return FALSE;
+ }
+
+ bpp = MAP_BITMAP_FMT_TO_BITS_PER_PIXEL[bitmap->format];
if (bitmap->stride < ((bitmap->x * bpp + 7) / 8)) {
spice_warning("image stride too small for width: %d < ((%d * %d + 7) / 8) (%s=%d)\n",
commit 1eb93baa3c594e1214b1c92bbad8a06e9c7e2d12
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 16:02:59 2015 +0100
Check properly surface to be created
Check format is valid.
Check stride is at least the size of required bytes for a row.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index be90068..bdd5917 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -1216,12 +1216,30 @@ void red_put_message(RedMessage *red)
/* nothing yet */
}
+static unsigned int surface_format_to_bpp(uint32_t format)
+{
+ switch (format) {
+ case SPICE_SURFACE_FMT_1_A:
+ return 1;
+ case SPICE_SURFACE_FMT_8_A:
+ return 8;
+ case SPICE_SURFACE_FMT_16_555:
+ case SPICE_SURFACE_FMT_16_565:
+ return 16;
+ case SPICE_SURFACE_FMT_32_xRGB:
+ case SPICE_SURFACE_FMT_32_ARGB:
+ return 32;
+ }
+ return 0;
+}
+
int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
RedSurfaceCmd *red, QXLPHYSICAL addr)
{
QXLSurfaceCmd *qxl;
uint64_t size;
int error;
+ unsigned int bpp;
qxl = (QXLSurfaceCmd *)get_virt(slots, addr, sizeof(*qxl), group_id,
&error);
@@ -1240,9 +1258,24 @@ int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
red->u.surface_create.width = qxl->u.surface_create.width;
red->u.surface_create.height = qxl->u.surface_create.height;
red->u.surface_create.stride = qxl->u.surface_create.stride;
+ bpp = surface_format_to_bpp(red->u.surface_create.format);
+
+ /* check if format is valid */
+ if (!bpp) {
+ return 1;
+ }
+
+ /* check stride is larger than required bytes */
+ size = ((uint64_t) red->u.surface_create.width * bpp + 7u) / 8u;
+ /* the uint32_t conversion is here to avoid problems with -2^31 value */
+ if (red->u.surface_create.stride == G_MININT32
+ || size > (uint32_t) abs(red->u.surface_create.stride)) {
+ return 1;
+ }
+
/* the multiplication can overflow, also abs(-2^31) may return a negative value */
size = (uint64_t) red->u.surface_create.height * abs(red->u.surface_create.stride);
- if (size > MAX_DATA_CHUNK || red->u.surface_create.stride == G_MININT32) {
+ if (size > MAX_DATA_CHUNK) {
return 1;
}
red->u.surface_create.data =
commit ac5f64a80ae637742ed95fd6c98f66281b3e15c6
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Thu Sep 17 15:00:22 2015 +0100
Fix some integer overflow causing large memory allocations
Prevent integer overflow when computing image sizes.
Image index computations are done using 32 bit so this can cause easily
security issues. MAX_DATA_CHUNK is larger than the virtual
card limit, so this is not going to cause change in behaviours.
Comparing size calculation results with MAX_DATA_CHUNK will allow us to
catch overflows.
Prevent guest from allocating large amount of memory.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 3ffa57b..be90068 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -380,7 +380,7 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
QXLImage *qxl;
SpiceImage *red = NULL;
SpicePalette *rp = NULL;
- size_t bitmap_size, size;
+ uint64_t bitmap_size, size;
uint8_t qxl_flags;
int error;
@@ -456,7 +456,10 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
red->u.bitmap.palette = rp;
red->u.bitmap.palette_id = rp->unique;
}
- bitmap_size = red->u.bitmap.y * red->u.bitmap.stride;
+ bitmap_size = (uint64_t) red->u.bitmap.y * red->u.bitmap.stride;
+ if (bitmap_size > MAX_DATA_CHUNK) {
+ goto error;
+ }
if (qxl_flags & QXL_BITMAP_DIRECT) {
red->u.bitmap.data = red_get_image_data_flat(slots, group_id,
qxl->bitmap.data,
@@ -1217,7 +1220,7 @@ int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
RedSurfaceCmd *red, QXLPHYSICAL addr)
{
QXLSurfaceCmd *qxl;
- size_t size;
+ uint64_t size;
int error;
qxl = (QXLSurfaceCmd *)get_virt(slots, addr, sizeof(*qxl), group_id,
@@ -1237,7 +1240,11 @@ int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
red->u.surface_create.width = qxl->u.surface_create.width;
red->u.surface_create.height = qxl->u.surface_create.height;
red->u.surface_create.stride = qxl->u.surface_create.stride;
- size = red->u.surface_create.height * abs(red->u.surface_create.stride);
+ /* the multiplication can overflow, also abs(-2^31) may return a negative value */
+ size = (uint64_t) red->u.surface_create.height * abs(red->u.surface_create.stride);
+ if (size > MAX_DATA_CHUNK || red->u.surface_create.stride == G_MININT32) {
+ return 1;
+ }
red->u.surface_create.data =
(uint8_t*)get_virt(slots, qxl->u.surface_create.data, size, group_id, &error);
if (error) {
commit 0205a6ce63f50af9eda03f14d93b3a2517c42fae
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Tue Sep 8 11:58:11 2015 +0100
Define a constant to limit data from guest.
This limit will prevent guest trying to do nasty things and DoS to host.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 5b1befa..3ffa57b 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -21,11 +21,22 @@
#include <stdbool.h>
#include <inttypes.h>
+#include <glib.h>
#include "common/lz_common.h"
#include "red_common.h"
#include "red_memslots.h"
#include "red_parse_qxl.h"
+/* Max size in bytes for any data field used in a QXL command.
+ * This will for example be useful to prevent the guest from saturating the
+ * host memory if it tries to send overlapping chunks.
+ * This value should be big enough for all requests but limited
+ * to 32 bits. Even better if it fits on 31 bits to detect integer overflows.
+ */
+#define MAX_DATA_CHUNK 0x7ffffffflu
+
+G_STATIC_ASSERT(MAX_DATA_CHUNK <= G_MAXINT32);
+
#if 0
static void hexdump_qxl(RedMemSlotInfo *slots, int group_id,
QXLPHYSICAL addr, uint8_t bytes)
commit 097c638b121e595d9daf79285c447088027a58e2
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Wed Sep 9 12:45:06 2015 +0100
worker: avoid double free or double create of surfaces
A driver can overwrite surface state creating a surface with the same
id of a previous one.
Also can try to destroy surfaces that are not created.
Both requests cause invalid internal states that could lead to crashes
or memory corruptions.
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
diff --git a/server/red_worker.c b/server/red_worker.c
index 7a60cd1..babb597 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -4278,6 +4278,10 @@ static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface
int32_t stride = surface->u.surface_create.stride;
int reloaded_surface = loadvm || (surface->flags & QXL_SURF_FLAG_KEEP_DATA);
+ if (red_surface->refs) {
+ spice_warning("avoiding creating a surface twice");
+ break;
+ }
data = surface->u.surface_create.data;
if (stride < 0) {
data -= (int32_t)(stride * (height - 1));
@@ -4291,7 +4295,10 @@ static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface
break;
}
case QXL_SURFACE_CMD_DESTROY:
- spice_warn_if(!red_surface->context.canvas);
+ if (!red_surface->refs) {
+ spice_warning("avoiding destroying a surface twice");
+ break;
+ }
set_surface_release_info(&red_surface->destroy, surface->release_info, group_id);
red_handle_depends_on_target_surface(worker, surface_id);
/* note that red_handle_depends_on_target_surface must be called before red_current_clear.
commit dd558bb833254fb49069eca052b92ae1abe3e8ff
Author: Frediano Ziglio <fziglio at redhat.com>
Date: Wed Sep 9 12:42:09 2015 +0100
worker: validate correctly surfaces
Do not just give warning and continue to use an invalid index into
an array.
Resolves: CVE-2015-5260
Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
Acked-by: Christophe Fergeau <cfergeau at redhat.com>
diff --git a/server/red_worker.c b/server/red_worker.c
index d8a081e..7a60cd1 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1051,6 +1051,7 @@ typedef struct BitmapData {
SpiceRect lossy_rect;
} BitmapData;
+static inline int validate_surface(RedWorker *worker, uint32_t surface_id);
static void red_draw_qxl_drawable(RedWorker *worker, Drawable *drawable);
static void red_current_flush(RedWorker *worker, int surface_id);
#ifdef DRAW_ALL
@@ -1270,11 +1271,6 @@ static inline int is_primary_surface(RedWorker *worker, uint32_t surface_id)
return FALSE;
}
-static inline void __validate_surface(RedWorker *worker, uint32_t surface_id)
-{
- spice_warn_if(surface_id >= worker->n_surfaces);
-}
-
static int validate_drawable_bbox(RedWorker *worker, RedDrawable *drawable)
{
DrawContext *context;
@@ -1283,7 +1279,7 @@ static int validate_drawable_bbox(RedWorker *worker, RedDrawable *drawable)
/* surface_id must be validated before calling into
* validate_drawable_bbox
*/
- __validate_surface(worker, surface_id);
+ VALIDATE_SURFACE_RETVAL(worker, surface_id, FALSE);
context = &worker->surfaces[surface_id].context;
if (drawable->bbox.top < 0)
@@ -1304,7 +1300,10 @@ static int validate_drawable_bbox(RedWorker *worker, RedDrawable *drawable)
static inline int validate_surface(RedWorker *worker, uint32_t surface_id)
{
- spice_warn_if(surface_id >= worker->n_surfaces);
+ if SPICE_UNLIKELY(surface_id >= worker->n_surfaces) {
+ spice_warning("invalid surface_id %u", surface_id);
+ return 0;
+ }
if (!worker->surfaces[surface_id].context.canvas) {
spice_warning("canvas address is %p for %d (and is NULL)\n",
&(worker->surfaces[surface_id].context.canvas), surface_id);
@@ -4262,12 +4261,14 @@ static inline void red_create_surface(RedWorker *worker, uint32_t surface_id,uin
static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface,
uint32_t group_id, int loadvm)
{
- int surface_id;
+ uint32_t surface_id;
RedSurface *red_surface;
uint8_t *data;
surface_id = surface->surface_id;
- __validate_surface(worker, surface_id);
+ if SPICE_UNLIKELY(surface_id >= worker->n_surfaces) {
+ goto exit;
+ }
red_surface = &worker->surfaces[surface_id];
@@ -4303,6 +4304,7 @@ static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface
default:
spice_error("unknown surface command");
};
+exit:
red_put_surface_cmd(surface);
free(surface);
}
@@ -11036,7 +11038,7 @@ void handle_dev_update(void *opaque, void *payload)
{
RedWorker *worker = opaque;
RedWorkerMessageUpdate *msg = payload;
- SpiceRect *rect = spice_new0(SpiceRect, 1);
+ SpiceRect *rect;
RedSurface *surface;
uint32_t surface_id = msg->surface_id;
const QXLRect *qxl_area = msg->qxl_area;
@@ -11044,17 +11046,16 @@ void handle_dev_update(void *opaque, void *payload)
QXLRect *qxl_dirty_rects = msg->qxl_dirty_rects;
uint32_t clear_dirty_region = msg->clear_dirty_region;
+ VALIDATE_SURFACE_RET(worker, surface_id);
+
+ rect = spice_new0(SpiceRect, 1);
surface = &worker->surfaces[surface_id];
red_get_rect_ptr(rect, qxl_area);
flush_display_commands(worker);
spice_assert(worker->running);
- if (validate_surface(worker, surface_id)) {
- red_update_area(worker, rect, surface_id);
- } else {
- rendering_incorrect(__func__);
- }
+ red_update_area(worker, rect, surface_id);
free(rect);
surface_dirty_region_to_rects(surface, qxl_dirty_rects, num_dirty_rects,
@@ -11093,6 +11094,7 @@ void handle_dev_del_memslot(void *opaque, void *payload)
* surface_id == 0, maybe move the assert upward and merge the two functions? */
static inline void destroy_surface_wait(RedWorker *worker, int surface_id)
{
+ VALIDATE_SURFACE_RET(worker, surface_id);
if (!worker->surfaces[surface_id].context.canvas) {
return;
}
@@ -11362,6 +11364,7 @@ void handle_dev_create_primary_surface(void *opaque, void *payload)
static void dev_destroy_primary_surface(RedWorker *worker, uint32_t surface_id)
{
+ VALIDATE_SURFACE_RET(worker, surface_id);
spice_warn_if(surface_id != 0);
spice_debug(NULL);
More information about the Spice-commits
mailing list