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

[Stefan Hajnoczi]

On Wed, Apr 23, 2014 at 03:31:36PM +0200, Sebastian Krahmer wrote:

Please put the patch revision number in the tags section of the commit
message.  This way git-am(1) automatically strips it when applying the
patch.  So:
[PATCH] ivshmem: fix potential OOB r/w access (#2)
should be:
[PATCH v2] ivshmem: fix potential OOB r/w access

The reason for this is to reduce noise in the commit history.  Revision
numbers only matter during code review.

> 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.

These are responses to my review comments.  They should not be part of
the commit description.  Please place changelogs or comments like this
underneath the '---' line.  Again, it's a convention for reducing noise
in the commit history.

Addressing your response:

> tmp_fd does not leak on error; see following dup() call

The existing ivshmem code is broken and leaks the file descriptor.

Please see tcp_get_msgfd().  It transfers ownership of the file
descriptor to the caller.

For more evidence, see the qmp_add_fd() call site.

I just noticed that another caller, qmp_getfd() leaks the fd in an error
code path and am sending a patch to fix that.

Since you are modifying ivshmem, please fix the fd leak in ivshmem and
include a patch in this series.

> According to docu g_realloc() may return NULL so we need to check
> that.

There is only one case where it returns NULL and we cannot reach it.
The documentation says: "n_bytes may be 0, in which case NULL will be
returned".  We never pass n_bytes 0 because of the new_min_size <= 0
check added in your patch.

The entire glib memory allocator is designed to abort with a fatal error
if malloc(3)/realloc(3) fail to allocate memory.  The documentation
states that here:
"If any call to allocate memory fails, the application is terminated.
This also means that there is no need to check if the call succeeded."

Here is the g_realloc() implementation from glib:

  newmem = glib_mem_vtable.realloc (mem, n_bytes);
  TRACE (GLIB_MEM_REALLOC((void*) newmem, (void*)mem, (unsigned int) n_bytes, 
0));
  if (newmem)
    return newmem;

  g_error ("%s: failed to allocate %"G_GSIZE_FORMAT" bytes",
           G_STRLOC, n_bytes);

> 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 */

This comment is confusing since it refers to code that is deleted by
your patch.  People reading the code will not understand why it mentions
re-implementing heap allocation strategies - the loop will be gone.
Please drop this part of the comment.