debian/patches/0001-memslot-Fix-off-by-one-error-in-group-slot-boundary-.patch

   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