Blame SOURCES/0028-memslot-Fix-off-by-one-error-in-group-slot-boundary-.patch

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