diff mbox

[v3,3/4] ivshmem: Fix potential OOB r/w access

Message ID 1410799208-3250-4-git-send-email-afaerber@suse.de
State New
Headers show

Commit Message

Andreas Färber Sept. 15, 2014, 4:40 p.m. UTC 
From: Sebastian Krahmer <krahmer@suse.de>

Fix OOB access via malformed incoming_posn parameters
and check that requested memory is actually alloc'ed.

Signed-off-by: Sebastian Krahmer <krahmer@suse.de>
[AF: Rebased, cleanups, avoid fd leak]
Cc: qemu-stable@nongnu.org
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/misc/ivshmem.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

Comments

Michael S. Tsirkin Sept. 22, 2014, 11:21 a.m. UTC | #1 
On Mon, Sep 15, 2014 at 06:40:07PM +0200, Andreas Färber wrote:
> From: Sebastian Krahmer <krahmer@suse.de>
> 
> Fix OOB access via malformed incoming_posn parameters
> and check that requested memory is actually alloc'ed.
> 
> Signed-off-by: Sebastian Krahmer <krahmer@suse.de>
> [AF: Rebased, cleanups, avoid fd leak]
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/misc/ivshmem.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 24f74f6..ecef82a 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -29,6 +29,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
> @@ -410,14 +411,24 @@ 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;
> +    if (new_min_size >= s->nb_peers) {
> +        /* +1 because #new_min_size is used as last array index */
> +        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));
> @@ -427,6 +438,8 @@ static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
>          s->peers[j].eventfds = NULL;
>          s->peers[j].nb_eventfds = 0;
>      }
> +
> +    return 0;
>  }
>  
>  static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
> @@ -469,7 +482,13 @@ static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
>  
>      /* 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) {
> +            error_report("increase_dynamic_storage() failed");
> +            if (tmp_fd != -1) {
> +                close(tmp_fd);
> +            }
> +            return;
> +        }
>      }
>  
>      if (tmp_fd == -1) {
> -- 
> 1.8.4.5
diff mbox

Patch

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 24f74f6..ecef82a 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -29,6 +29,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
@@ -410,14 +411,24 @@  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;
+    if (new_min_size >= s->nb_peers) {
+        /* +1 because #new_min_size is used as last array index */
+        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));
@@ -427,6 +438,8 @@  static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
         s->peers[j].eventfds = NULL;
         s->peers[j].nb_eventfds = 0;
     }
+
+    return 0;
 }
 
 static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
@@ -469,7 +482,13 @@  static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
 
     /* 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) {
+            error_report("increase_dynamic_storage() failed");
+            if (tmp_fd != -1) {
+                close(tmp_fd);
+            }
+            return;
+        }
     }
 
     if (tmp_fd == -1) {