[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] ivshmem: fix potential OOB r/w access
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH] ivshmem: fix potential OOB r/w access |
Date: |
Wed, 23 Apr 2014 10:59:25 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
>
> ---
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.