From a1ead07b6a0facefafb959542f3088f427c7eb2d Mon Sep 17 00:00:00 2001 From: Frediano Ziglio Date: Tue, 8 Sep 2015 12:12:19 +0100 Subject: [PATCH 57/64] 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 --- server/red_parse_qxl.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c index c4b82be..7cc20e6 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; } -- 2.4.3