[Qemu-devel] [PATCH] ivshmem: fix potential OOB r/w access (#2)

[Sebastian Krahmer]

Fix OOB access via malformed incoming_posn parameters
and check that requested memory is actually alloced.

tmp_fd does not leak on error; see following dup() call.
According to docu g_realloc() may return NULL so we need
to check that. Passes checkpatch.pl, after also fixing wrong
ivshmem.c style itself.


Signed-off-by: Sebastian Krahmer <address@hidden>
---
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 8d144ba..5c0f116 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -28,6 +28,7 @@
 
 #include <sys/mman.h>
 #include <sys/types.h>
+#include <limits.h>
 
 #define PCI_VENDOR_ID_IVSHMEM   PCI_VENDOR_ID_REDHAT_QUMRANET
 #define PCI_DEVICE_ID_IVSHMEM   0x1110
@@ -401,23 +402,41 @@ static void close_guest_eventfds(IVShmemState *s, int 
posn)
 
 /* this function increase the dynamic storage need to store data about other
  * guests */
-static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
+static int increase_dynamic_storage(IVShmemState *s, int new_min_size)
+{
 
     int j, old_nb_alloc;
 
+    /* check for integer overflow */
+    if (new_min_size >= INT_MAX/sizeof(Peer) - 1 || new_min_size <= 0) {
+        return -1;
+    }
+
     old_nb_alloc = s->nb_peers;
 
-    while (new_min_size >= s->nb_peers)
-        s->nb_peers = s->nb_peers * 2;
+    /* heap allocators already have good alloc strategies, no need
+     * to re-implement here. +1 because #new_min_size is used as last array
+     * index */
+    if (new_min_size >= s->nb_peers) {
+        s->nb_peers = new_min_size + 1;
+    } else {
+        return 0;
+    }
 
     IVSHMEM_DPRINTF("bumping storage to %d guests\n", s->nb_peers);
     s->peers = g_realloc(s->peers, s->nb_peers * sizeof(Peer));
 
+    /* g_realloc() may return NULL */
+    if (!s->peers) {
+        return -1;
+    }
+
     /* zero out new pointers */
     for (j = old_nb_alloc; j < s->nb_peers; j++) {
         s->peers[j].eventfds = NULL;
         s->peers[j].nb_eventfds = 0;
     }
+    return 0;
 }
 
 static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
@@ -428,13 +447,20 @@ static void ivshmem_read(void *opaque, const uint8_t * 
buf, int flags)
     long incoming_posn;
 
     memcpy(&incoming_posn, buf, sizeof(long));
+    if (incoming_posn < -1) {
+        fprintf(stderr, "invalid posn index\n");
+        return;
+    }
     /* pick off s->server_chr->msgfd and store it, posn should accompany msg */
     tmp_fd = qemu_chr_fe_get_msgfd(s->server_chr);
     IVSHMEM_DPRINTF("posn is %ld, fd is %d\n", incoming_posn, tmp_fd);
 
     /* make sure we have enough space for this guest */
     if (incoming_posn >= s->nb_peers) {
-        increase_dynamic_storage(s, incoming_posn);
+        if (increase_dynamic_storage(s, incoming_posn) < 0) {
+            fprintf(stderr, "increase_dynamic_storage() failed\n");
+            return;
+        }
     }
 
     if (tmp_fd == -1) {

-- 

~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ address@hidden - SuSE Security Team