Message ID | 20140416073302.GB14841@suse.de |
---|---|
State | New |
Headers | show |
On Wed, Apr 16, 2014 at 09:33:02AM +0200, Sebastian Krahmer wrote: > > Fix OOB access via malformed incoming_posn parameters > and check that requested memory is actually alloced. > > Signed-off-by: Sebastian Krahmer <krahmer@suse.de> > > --- Please use scripts/checkpatch.pl to check coding style. QEMU always uses {} even for a one-line if body. > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index 8d144ba..f58356e 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,34 @@ 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)); > + if (!s->peers) > + return -1; This is not needed because g_realloc() does not return NULL on out-of-memory: https://developer.gnome.org/glib/unstable/glib-Memory-Allocation.html#g-realloc > > /* 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 +440,19 @@ 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, "could not allocate memory\n"); This error message is wrong because the failure could also indicate out-of-range incoming_posn. > + return; If tmp_fd != -1 this return leaks it.
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index 8d144ba..f58356e 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,34 @@ 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)); + 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 +440,19 @@ 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, "could not allocate memory\n"); + return; + } } if (tmp_fd == -1) {
Fix OOB access via malformed incoming_posn parameters and check that requested memory is actually alloced. Signed-off-by: Sebastian Krahmer <krahmer@suse.de> ---