qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes
@ 2014-09-15 16:40 Andreas Färber
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 1/4] ivshmem: Check ivshmem_read() size argument Andreas Färber
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Andreas Färber @ 2014-09-15 16:40 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Sebastian Krahmer, Michael S. Tsirkin,
	David Marchand, Bruce Rogers, Sebastian Krahmer, Gerd Hoffmann,
	Stefan Hajnoczi, Cam Macdonell, Andreas Färber

Hello,

This series tightens security on incoming data for ivshmem, originally sparked
by SUSE's security team (Sebastian Krahmer). I've combined them and tackled
remaining review feedback.

Regards,
Andreas

Changes from Sebastian's #2:
* Rebased onto Stefan's patches
* Dropped g_realloc() check (Stefan)
* Fixed fd leak and appended a patch fixing another one (Stefan)
* Simplified comment (Stefan)

Changes from Stefan's series:
* Modified to handle partial reads (Peter/Gerd)
* Changed check from > to >= (Peter)

Cc: Cam Macdonell <cam@cs.ualberta.ca>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Sebastian Krahmer <krahmer@suse.de>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: David Marchand <david.marchand@6wind.com>

Andreas Färber (1):
  ivshmem: Fix fd leak on error

Sebastian Krahmer (1):
  ivshmem: Fix potential OOB r/w access

Stefan Hajnoczi (2):
  ivshmem: Check ivshmem_read() size argument
  ivshmem: validate incoming_posn value from server

 hw/misc/ivshmem.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 6 deletions(-)

-- 
1.8.4.5

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] [PATCH v3 1/4] ivshmem: Check ivshmem_read() size argument
  2014-09-15 16:40 [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes Andreas Färber
@ 2014-09-15 16:40 ` Andreas Färber
  2014-09-22 11:21   ` Michael S. Tsirkin
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 2/4] ivshmem: validate incoming_posn value from server Andreas Färber
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Andreas Färber @ 2014-09-15 16:40 UTC (permalink / raw)
  To: qemu-devel
  Cc: Sebastian Krahmer, Michael S. Tsirkin, qemu-stable,
	David Marchand, Bruce Rogers, Stefan Hajnoczi, Cam Macdonell,
	Andreas Färber

From: Stefan Hajnoczi <stefanha@redhat.com>

The third argument to the fd_read() callback implemented by
ivshmem_read() is the number of bytes, not a flags field.  Fix this and
check we received enough bytes before accessing the buffer pointer.

Cc: Cam Macdonell <cam@cs.ualberta.ca>
Reported-by: Sebastian Krahmer <krahmer@suse.de>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
[AF: Handle partial reads via FIFO]
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-stable@nongnu.org
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/misc/ivshmem.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index bd9d718..caeee1e 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -24,6 +24,7 @@
 #include "migration/migration.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/event_notifier.h"
+#include "qemu/fifo8.h"
 #include "sysemu/char.h"
 
 #include <sys/mman.h>
@@ -73,6 +74,7 @@ typedef struct IVShmemState {
 
     CharDriverState **eventfd_chr;
     CharDriverState *server_chr;
+    Fifo8 incoming_fifo;
     MemoryRegion ivshmem_mmio;
 
     /* We might need to register the BAR before we actually have the memory.
@@ -424,14 +426,35 @@ static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
     }
 }
 
-static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
+static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
 {
     IVShmemState *s = opaque;
     int incoming_fd, tmp_fd;
     int guest_max_eventfd;
     long incoming_posn;
 
-    memcpy(&incoming_posn, buf, sizeof(long));
+    if (fifo8_is_empty(&s->incoming_fifo) && size == sizeof(incoming_posn)) {
+        memcpy(&incoming_posn, buf, size);
+    } else {
+        const uint8_t *p;
+        uint32_t num;
+
+        IVSHMEM_DPRINTF("short read of %d bytes\n", size);
+        num = MAX(size, sizeof(long) - fifo8_num_used(&s->incoming_fifo));
+        fifo8_push_all(&s->incoming_fifo, buf, num);
+        if (fifo8_num_used(&s->incoming_fifo) < sizeof(incoming_posn)) {
+            return;
+        }
+        size -= num;
+        buf += num;
+        p = fifo8_pop_buf(&s->incoming_fifo, sizeof(incoming_posn), &num);
+        g_assert(num == sizeof(incoming_posn));
+        memcpy(&incoming_posn, p, sizeof(incoming_posn));
+        if (size > 0) {
+            fifo8_push_all(&s->incoming_fifo, buf, size);
+        }
+    }
+
     /* 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);
@@ -663,6 +686,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
         s->ivshmem_size = ivshmem_get_size(s);
     }
 
+    fifo8_create(&s->incoming_fifo, sizeof(long));
+
     register_savevm(DEVICE(dev), "ivshmem", 0, 0, ivshmem_save, ivshmem_load,
                                                                         dev);
 
@@ -796,6 +821,7 @@ static void pci_ivshmem_uninit(PCIDevice *dev)
     memory_region_del_subregion(&s->bar, &s->ivshmem);
     vmstate_unregister_ram(&s->ivshmem, DEVICE(dev));
     unregister_savevm(DEVICE(dev), "ivshmem", s);
+    fifo8_destroy(&s->incoming_fifo);
 }
 
 static Property ivshmem_properties[] = {
-- 
1.8.4.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Qemu-devel] [PATCH v3 2/4] ivshmem: validate incoming_posn value from server
  2014-09-15 16:40 [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes Andreas Färber
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 1/4] ivshmem: Check ivshmem_read() size argument Andreas Färber
@ 2014-09-15 16:40 ` Andreas Färber
  2014-09-22 11:21   ` Michael S. Tsirkin
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 3/4] ivshmem: Fix potential OOB r/w access Andreas Färber
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Andreas Färber @ 2014-09-15 16:40 UTC (permalink / raw)
  To: qemu-devel
  Cc: Sebastian Krahmer, Michael S. Tsirkin, qemu-stable,
	David Marchand, Bruce Rogers, Stefan Hajnoczi, Cam Macdonell,
	Andreas Färber

From: Stefan Hajnoczi <stefanha@redhat.com>

Check incoming_posn to avoid out-of-bounds array accesses if the ivshmem
server on the host sends invalid values.

Cc: Cam Macdonell <cam@cs.ualberta.ca>
Reported-by: Sebastian Krahmer <krahmer@suse.de>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
[AF: Tighten upper bound check for posn in close_guest_eventfds()]
Cc: qemu-stable@nongnu.org
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/misc/ivshmem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index caeee1e..24f74f6 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -389,6 +389,9 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
     if (!ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
         return;
     }
+    if (posn < 0 || posn >= s->nb_peers) {
+        return;
+    }
 
     guest_curr_max = s->peers[posn].nb_eventfds;
 
@@ -455,6 +458,11 @@ static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
         }
     }
 
+    if (incoming_posn < -1) {
+        IVSHMEM_DPRINTF("invalid incoming_posn %ld\n", incoming_posn);
+        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);
-- 
1.8.4.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Qemu-devel] [PATCH v3 3/4] ivshmem: Fix potential OOB r/w access
  2014-09-15 16:40 [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes Andreas Färber
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 1/4] ivshmem: Check ivshmem_read() size argument Andreas Färber
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 2/4] ivshmem: validate incoming_posn value from server Andreas Färber
@ 2014-09-15 16:40 ` Andreas Färber
  2014-09-22 11:21   ` Michael S. Tsirkin
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 4/4] ivshmem: Fix fd leak on error Andreas Färber
  2014-10-31 16:03 ` [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes Paolo Bonzini
  4 siblings, 1 reply; 10+ messages in thread
From: Andreas Färber @ 2014-09-15 16:40 UTC (permalink / raw)
  To: qemu-devel
  Cc: Sebastian Krahmer, Michael S. Tsirkin, qemu-stable,
	David Marchand, Bruce Rogers, Sebastian Krahmer, Stefan Hajnoczi,
	Cam Macdonell, Andreas Färber

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(-)

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Qemu-devel] [PATCH v3 4/4] ivshmem: Fix fd leak on error
  2014-09-15 16:40 [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes Andreas Färber
                   ` (2 preceding siblings ...)
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 3/4] ivshmem: Fix potential OOB r/w access Andreas Färber
@ 2014-09-15 16:40 ` Andreas Färber
  2014-09-22 11:22   ` Michael S. Tsirkin
  2014-10-31 16:03 ` [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes Paolo Bonzini
  4 siblings, 1 reply; 10+ messages in thread
From: Andreas Färber @ 2014-09-15 16:40 UTC (permalink / raw)
  To: qemu-devel
  Cc: Sebastian Krahmer, Michael S. Tsirkin, qemu-stable,
	David Marchand, Bruce Rogers, Stefan Hajnoczi, Cam Macdonell,
	Andreas Färber

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/misc/ivshmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index ecef82a..bf585b7 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -512,6 +512,7 @@ static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
     if (incoming_fd == -1) {
         fprintf(stderr, "could not allocate file descriptor %s\n",
                                                             strerror(errno));
+        close(tmp_fd);
         return;
     }
 
-- 
1.8.4.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/4] ivshmem: Check ivshmem_read() size argument
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 1/4] ivshmem: Check ivshmem_read() size argument Andreas Färber
@ 2014-09-22 11:21   ` Michael S. Tsirkin
  0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2014-09-22 11:21 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Bruce Rogers, Sebastian Krahmer, qemu-stable, qemu-devel,
	David Marchand, Stefan Hajnoczi, Cam Macdonell

On Mon, Sep 15, 2014 at 06:40:05PM +0200, Andreas Färber wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> The third argument to the fd_read() callback implemented by
> ivshmem_read() is the number of bytes, not a flags field.  Fix this and
> check we received enough bytes before accessing the buffer pointer.
> 
> Cc: Cam Macdonell <cam@cs.ualberta.ca>
> Reported-by: Sebastian Krahmer <krahmer@suse.de>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> [AF: Handle partial reads via FIFO]
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> 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 | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index bd9d718..caeee1e 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -24,6 +24,7 @@
>  #include "migration/migration.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/event_notifier.h"
> +#include "qemu/fifo8.h"
>  #include "sysemu/char.h"
>  
>  #include <sys/mman.h>
> @@ -73,6 +74,7 @@ typedef struct IVShmemState {
>  
>      CharDriverState **eventfd_chr;
>      CharDriverState *server_chr;
> +    Fifo8 incoming_fifo;
>      MemoryRegion ivshmem_mmio;
>  
>      /* We might need to register the BAR before we actually have the memory.
> @@ -424,14 +426,35 @@ static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
>      }
>  }
>  
> -static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
> +static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
>  {
>      IVShmemState *s = opaque;
>      int incoming_fd, tmp_fd;
>      int guest_max_eventfd;
>      long incoming_posn;
>  
> -    memcpy(&incoming_posn, buf, sizeof(long));
> +    if (fifo8_is_empty(&s->incoming_fifo) && size == sizeof(incoming_posn)) {
> +        memcpy(&incoming_posn, buf, size);
> +    } else {
> +        const uint8_t *p;
> +        uint32_t num;
> +
> +        IVSHMEM_DPRINTF("short read of %d bytes\n", size);
> +        num = MAX(size, sizeof(long) - fifo8_num_used(&s->incoming_fifo));
> +        fifo8_push_all(&s->incoming_fifo, buf, num);
> +        if (fifo8_num_used(&s->incoming_fifo) < sizeof(incoming_posn)) {
> +            return;
> +        }
> +        size -= num;
> +        buf += num;
> +        p = fifo8_pop_buf(&s->incoming_fifo, sizeof(incoming_posn), &num);
> +        g_assert(num == sizeof(incoming_posn));
> +        memcpy(&incoming_posn, p, sizeof(incoming_posn));
> +        if (size > 0) {
> +            fifo8_push_all(&s->incoming_fifo, buf, size);
> +        }
> +    }
> +
>      /* 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);
> @@ -663,6 +686,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>          s->ivshmem_size = ivshmem_get_size(s);
>      }
>  
> +    fifo8_create(&s->incoming_fifo, sizeof(long));
> +
>      register_savevm(DEVICE(dev), "ivshmem", 0, 0, ivshmem_save, ivshmem_load,
>                                                                          dev);
>  
> @@ -796,6 +821,7 @@ static void pci_ivshmem_uninit(PCIDevice *dev)
>      memory_region_del_subregion(&s->bar, &s->ivshmem);
>      vmstate_unregister_ram(&s->ivshmem, DEVICE(dev));
>      unregister_savevm(DEVICE(dev), "ivshmem", s);
> +    fifo8_destroy(&s->incoming_fifo);
>  }
>  
>  static Property ivshmem_properties[] = {
> -- 
> 1.8.4.5

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 2/4] ivshmem: validate incoming_posn value from server
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 2/4] ivshmem: validate incoming_posn value from server Andreas Färber
@ 2014-09-22 11:21   ` Michael S. Tsirkin
  0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2014-09-22 11:21 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Bruce Rogers, Sebastian Krahmer, qemu-stable, qemu-devel,
	David Marchand, Stefan Hajnoczi, Cam Macdonell

On Mon, Sep 15, 2014 at 06:40:06PM +0200, Andreas Färber wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Check incoming_posn to avoid out-of-bounds array accesses if the ivshmem
> server on the host sends invalid values.
> 
> Cc: Cam Macdonell <cam@cs.ualberta.ca>
> Reported-by: Sebastian Krahmer <krahmer@suse.de>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> [AF: Tighten upper bound check for posn in close_guest_eventfds()]
> 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 | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index caeee1e..24f74f6 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -389,6 +389,9 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>      if (!ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>          return;
>      }
> +    if (posn < 0 || posn >= s->nb_peers) {
> +        return;
> +    }
>  
>      guest_curr_max = s->peers[posn].nb_eventfds;
>  
> @@ -455,6 +458,11 @@ static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
>          }
>      }
>  
> +    if (incoming_posn < -1) {
> +        IVSHMEM_DPRINTF("invalid incoming_posn %ld\n", incoming_posn);
> +        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);
> -- 
> 1.8.4.5

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 3/4] ivshmem: Fix potential OOB r/w access
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 3/4] ivshmem: Fix potential OOB r/w access Andreas Färber
@ 2014-09-22 11:21   ` Michael S. Tsirkin
  0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2014-09-22 11:21 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Bruce Rogers, Sebastian Krahmer, qemu-stable, qemu-devel,
	David Marchand, Sebastian Krahmer, Stefan Hajnoczi, Cam Macdonell

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Fix fd leak on error
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 4/4] ivshmem: Fix fd leak on error Andreas Färber
@ 2014-09-22 11:22   ` Michael S. Tsirkin
  0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2014-09-22 11:22 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Bruce Rogers, Sebastian Krahmer, qemu-stable, qemu-devel,
	David Marchand, Stefan Hajnoczi, Cam Macdonell

On Mon, Sep 15, 2014 at 06:40:08PM +0200, Andreas Färber wrote:
> Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
> 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index ecef82a..bf585b7 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -512,6 +512,7 @@ static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
>      if (incoming_fd == -1) {
>          fprintf(stderr, "could not allocate file descriptor %s\n",
>                                                              strerror(errno));
> +        close(tmp_fd);
>          return;
>      }
>  
> -- 
> 1.8.4.5

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes
  2014-09-15 16:40 [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes Andreas Färber
                   ` (3 preceding siblings ...)
  2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 4/4] ivshmem: Fix fd leak on error Andreas Färber
@ 2014-10-31 16:03 ` Paolo Bonzini
  4 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2014-10-31 16:03 UTC (permalink / raw)
  To: Andreas Färber, qemu-devel
  Cc: Peter Maydell, Sebastian Krahmer, Michael S. Tsirkin,
	David Marchand, Bruce Rogers, Sebastian Krahmer, Gerd Hoffmann,
	Stefan Hajnoczi, Cam Macdonell

On 15/09/2014 18:40, Andreas Färber wrote:
> Hello,
> 
> This series tightens security on incoming data for ivshmem, originally sparked
> by SUSE's security team (Sebastian Krahmer). I've combined them and tackled
> remaining review feedback.
> 
> Regards,
> Andreas
> 
> Changes from Sebastian's #2:
> * Rebased onto Stefan's patches
> * Dropped g_realloc() check (Stefan)
> * Fixed fd leak and appended a patch fixing another one (Stefan)
> * Simplified comment (Stefan)
> 
> Changes from Stefan's series:
> * Modified to handle partial reads (Peter/Gerd)
> * Changed check from > to >= (Peter)
> 
> Cc: Cam Macdonell <cam@cs.ualberta.ca>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Sebastian Krahmer <krahmer@suse.de>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: David Marchand <david.marchand@6wind.com>
> 
> Andreas Färber (1):
>   ivshmem: Fix fd leak on error
> 
> Sebastian Krahmer (1):
>   ivshmem: Fix potential OOB r/w access
> 
> Stefan Hajnoczi (2):
>   ivshmem: Check ivshmem_read() size argument
>   ivshmem: validate incoming_posn value from server
> 
>  hw/misc/ivshmem.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 60 insertions(+), 6 deletions(-)
> 

These seem to have falled on the floor, and they're a dependency for
Andrew's error_report cleanup, so I picked them up.

Paolo

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2014-10-31 16:03 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-15 16:40 [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes Andreas Färber
2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 1/4] ivshmem: Check ivshmem_read() size argument Andreas Färber
2014-09-22 11:21   ` Michael S. Tsirkin
2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 2/4] ivshmem: validate incoming_posn value from server Andreas Färber
2014-09-22 11:21   ` Michael S. Tsirkin
2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 3/4] ivshmem: Fix potential OOB r/w access Andreas Färber
2014-09-22 11:21   ` Michael S. Tsirkin
2014-09-15 16:40 ` [Qemu-devel] [PATCH v3 4/4] ivshmem: Fix fd leak on error Andreas Färber
2014-09-22 11:22   ` Michael S. Tsirkin
2014-10-31 16:03 ` [Qemu-devel] [PATCH v3 0/4] ivshmem security fixes Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).