]>
Commit | Line | Data |
---|---|---|
925955e9 DC |
1 | From 6eff47e72cb2f23d168be58bab8bdd60df49afd0 Mon Sep 17 00:00:00 2001 |
2 | From: Christophe Fergeau <cfergeau@redhat.com> | |
3 | Date: Thu, 29 Nov 2018 14:18:39 +0100 | |
4 | Subject: [spice-server] memslot: Fix off-by-one error in group/slot boundary | |
5 | check | |
6 | ||
7 | RedMemSlotInfo keeps an array of groups, and each group contains an | |
8 | array of slots. Unfortunately, these checks are off by 1, they check | |
9 | that the index is greater or equal to the number of elements in the | |
10 | array, while these arrays are 0 based. The check should only check for | |
11 | strictly greater than the number of elements. | |
12 | ||
13 | For the group array, this is not a big issue, as these memslot groups | |
14 | are created by spice-server users (eg QEMU), and the group ids used to | |
15 | index that array are also generated by the spice-server user, so it | |
16 | should not be possible for the guest to set them to arbitrary values. | |
17 | ||
18 | The slot id is more problematic, as it's calculated from a QXLPHYSICAL | |
19 | address, and such addresses are usually set by the guest QXL driver, so | |
20 | the guest can set these to arbitrary values, including malicious values, | |
21 | which are probably easy to build from the guest PCI configuration. | |
22 | ||
23 | This patch fixes the arrays bound check, and adds a test case for this. | |
24 | ||
25 | Signed-off-by: Christophe Fergeau <cfergeau@redhat.com> | |
26 | Signed-off-by: Dominik Csapak <d.csapak@proxmox.com> | |
27 | --- | |
28 | server/memslot.c | 4 ++-- | |
29 | server/tests/test-qxl-parsing.c | 30 ++++++++++++++++++++++++++++++ | |
30 | 2 files changed, 32 insertions(+), 2 deletions(-) | |
31 | ||
32 | diff --git a/server/memslot.c b/server/memslot.c | |
33 | index b27324efb..fb3d5cfd5 100644 | |
34 | --- a/server/memslot.c | |
35 | +++ b/server/memslot.c | |
36 | @@ -97,13 +97,13 @@ void *memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size | |
37 | ||
38 | MemSlot *slot; | |
39 | ||
40 | - if (group_id > info->num_memslots_groups) { | |
41 | + if (group_id >= info->num_memslots_groups) { | |
42 | spice_critical("group_id too big"); | |
43 | return NULL; | |
44 | } | |
45 | ||
46 | slot_id = memslot_get_id(info, addr); | |
47 | - if (slot_id > info->num_memslots) { | |
48 | + if (slot_id >= info->num_memslots) { | |
49 | print_memslots(info); | |
50 | spice_critical("slot_id %d too big, addr=%" PRIx64, slot_id, addr); | |
51 | return NULL; | |
52 | diff --git a/server/tests/test-qxl-parsing.c b/server/tests/test-qxl-parsing.c | |
53 | index 8565239f0..447425984 100644 | |
54 | --- a/server/tests/test-qxl-parsing.c | |
55 | +++ b/server/tests/test-qxl-parsing.c | |
56 | @@ -98,6 +98,31 @@ static void deinit_qxl_surface(QXLSurfaceCmd *qxl) | |
57 | g_free(from_physical(qxl->u.surface_create.data)); | |
58 | } | |
59 | ||
60 | +static void test_memslot_invalid_group_id(void) | |
61 | +{ | |
62 | + RedMemSlotInfo mem_info; | |
63 | + init_meminfo(&mem_info); | |
64 | + | |
65 | + memslot_get_virt(&mem_info, 0, 16, 1); | |
66 | +} | |
67 | + | |
68 | +static void test_memslot_invalid_slot_id(void) | |
69 | +{ | |
70 | + RedMemSlotInfo mem_info; | |
71 | + init_meminfo(&mem_info); | |
72 | + | |
73 | + memslot_get_virt(&mem_info, 1 << mem_info.memslot_id_shift, 16, 0); | |
74 | +} | |
75 | + | |
76 | +static void test_memslot_invalid_addresses(void) | |
77 | +{ | |
78 | + g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/group_id", 0, 0); | |
79 | + g_test_trap_assert_stderr("*group_id too big*"); | |
80 | + | |
81 | + g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/slot_id", 0, 0); | |
82 | + g_test_trap_assert_stderr("*slot_id 1 too big*"); | |
83 | +} | |
84 | + | |
85 | static void test_no_issues(void) | |
86 | { | |
87 | RedMemSlotInfo mem_info; | |
88 | @@ -317,6 +342,11 @@ int main(int argc, char *argv[]) | |
89 | { | |
90 | g_test_init(&argc, &argv, NULL); | |
91 | ||
92 | + /* try to use invalid memslot group/slot */ | |
93 | + g_test_add_func("/server/memslot-invalid-addresses", test_memslot_invalid_addresses); | |
94 | + g_test_add_func("/server/memslot-invalid-addresses/subprocess/group_id", test_memslot_invalid_group_id); | |
95 | + g_test_add_func("/server/memslot-invalid-addresses/subprocess/slot_id", test_memslot_invalid_slot_id); | |
96 | + | |
97 | /* try to create a surface with no issues, should succeed */ | |
98 | g_test_add_func("/server/qxl-parsing-no-issues", test_no_issues); | |
99 | ||
100 | -- | |
101 | 2.19.2 | |
102 |