101 lines
3.8 KiB
Diff
101 lines
3.8 KiB
Diff
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
|
|
From: Christophe Fergeau <cfergeau@redhat.com>
|
|
Date: Thu, 29 Nov 2018 14:18:39 +0100
|
|
Subject: [PATCH] memslot: Fix off-by-one error in group/slot boundary check
|
|
|
|
RedMemSlotInfo keeps an array of groups, and each group contains an
|
|
array of slots. Unfortunately, these checks are off by 1, they check
|
|
that the index is greater or equal to the number of elements in the
|
|
array, while these arrays are 0 based. The check should only check for
|
|
strictly greater than the number of elements.
|
|
|
|
For the group array, this is not a big issue, as these memslot groups
|
|
are created by spice-server users (eg QEMU), and the group ids used to
|
|
index that array are also generated by the spice-server user, so it
|
|
should not be possible for the guest to set them to arbitrary values.
|
|
|
|
The slot id is more problematic, as it's calculated from a QXLPHYSICAL
|
|
address, and such addresses are usually set by the guest QXL driver, so
|
|
the guest can set these to arbitrary values, including malicious values,
|
|
which are probably easy to build from the guest PCI configuration.
|
|
|
|
This patch fixes the arrays bound check, and adds a test case for this.
|
|
|
|
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
|
|
---
|
|
server/memslot.c | 4 ++--
|
|
server/tests/test-qxl-parsing.c | 32 ++++++++++++++++++++++++++++++++
|
|
2 files changed, 34 insertions(+), 2 deletions(-)
|
|
|
|
diff --git a/server/memslot.c b/server/memslot.c
|
|
index 7074b43..8c59c38 100644
|
|
--- a/server/memslot.c
|
|
+++ b/server/memslot.c
|
|
@@ -99,14 +99,14 @@ unsigned long memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t
|
|
MemSlot *slot;
|
|
|
|
*error = 0;
|
|
- if (group_id > info->num_memslots_groups) {
|
|
+ if (group_id >= info->num_memslots_groups) {
|
|
spice_critical("group_id too big");
|
|
*error = 1;
|
|
return 0;
|
|
}
|
|
|
|
slot_id = memslot_get_id(info, addr);
|
|
- if (slot_id > info->num_memslots) {
|
|
+ if (slot_id >= info->num_memslots) {
|
|
print_memslots(info);
|
|
spice_critical("slot_id %d too big, addr=%" PRIx64, slot_id, addr);
|
|
*error = 1;
|
|
diff --git a/server/tests/test-qxl-parsing.c b/server/tests/test-qxl-parsing.c
|
|
index 9c0c3b1..83f2083 100644
|
|
--- a/server/tests/test-qxl-parsing.c
|
|
+++ b/server/tests/test-qxl-parsing.c
|
|
@@ -85,6 +85,33 @@ static void deinit_qxl_surface(QXLSurfaceCmd *qxl)
|
|
free(from_physical(qxl->u.surface_create.data));
|
|
}
|
|
|
|
+static void test_memslot_invalid_group_id(void)
|
|
+{
|
|
+ RedMemSlotInfo mem_info;
|
|
+ int error;
|
|
+ init_meminfo(&mem_info);
|
|
+
|
|
+ memslot_get_virt(&mem_info, 0, 16, 1, &error);
|
|
+}
|
|
+
|
|
+static void test_memslot_invalid_slot_id(void)
|
|
+{
|
|
+ RedMemSlotInfo mem_info;
|
|
+ int error;
|
|
+ init_meminfo(&mem_info);
|
|
+
|
|
+ memslot_get_virt(&mem_info, 1 << mem_info.memslot_id_shift, 16, 0, &error);
|
|
+}
|
|
+
|
|
+static void test_memslot_invalid_addresses(void)
|
|
+{
|
|
+ g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/group_id", 0, 0);
|
|
+ g_test_trap_assert_stderr("*group_id too big*");
|
|
+
|
|
+ g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/slot_id", 0, 0);
|
|
+ g_test_trap_assert_stderr("*slot_id 1 too big*");
|
|
+}
|
|
+
|
|
static void test_no_issues(void)
|
|
{
|
|
RedMemSlotInfo mem_info;
|
|
@@ -262,6 +289,11 @@ int main(int argc, char *argv[])
|
|
{
|
|
g_test_init(&argc, &argv, NULL);
|
|
|
|
+ /* try to use invalid memslot group/slot */
|
|
+ g_test_add_func("/server/memslot-invalid-addresses", test_memslot_invalid_addresses);
|
|
+ g_test_add_func("/server/memslot-invalid-addresses/subprocess/group_id", test_memslot_invalid_group_id);
|
|
+ g_test_add_func("/server/memslot-invalid-addresses/subprocess/slot_id", test_memslot_invalid_slot_id);
|
|
+
|
|
/* try to create a surface with no issues, should succeed */
|
|
g_test_add_func("/server/qxl-parsing-no-issues", test_no_issues);
|
|
|