Re: [Xen-devel] [PATCH for-4.5] libxc: don't leak buffer containing the uncompressed PV kernel

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Nov 10, 2014 at 12:08:33PM +0000, Ian Campbell wrote:
> The libxc xc_dom_* infrastructure uses a very simple malloc memory pool which
> is freed by xc_dom_release. However the various xc_try_*_decode routines 
> (other
> than the gzip one) just use plain malloc/realloc and therefore the buffer ends
> up leaked.
> 
> The memory pool currently supports mmap'd buffers as well as a directly
> allocated buffers, however the try decode routines make use of realloc and do
> not fit well into this model. Introduce a concept of an external memory block
> to the memory pool and provide an interface to register such memory.
> 
> The mmap_ptr and mmap_len fields of the memblock tracking struct lose their
> mmap_ prefix since they are now also used for external memory blocks.
> 
> We are only seeing this now because the gzip decoder doesn't leak and it's 
> only
> relatively recently that kernels in the wild have switched to better
> compression.
> 
> This is https://bugs.debian.org/767295
> 
> Reported by: Gedalya <gedalya@xxxxxxxxxxx>
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
> This is a bug fix and should go into 4.5.
> 
> I have a sneaking suspicion this is going to need to backport a very long 
> way...
> ---
>  tools/libxc/include/xc_dom.h        |   10 ++++--
>  tools/libxc/xc_dom_bzimageloader.c  |   18 +++++++++++
>  tools/libxc/xc_dom_core.c           |   61 
> +++++++++++++++++++++++++++--------
>  tools/libxc/xc_dom_decompress_lz4.c |    5 +++
>  4 files changed, 78 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 6ae6a9f..07d7224 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -33,8 +33,13 @@ struct xc_dom_seg {
>  
>  struct xc_dom_mem {
>      struct xc_dom_mem *next;
> -    void *mmap_ptr;
> -    size_t mmap_len;
> +    void *ptr;
> +    enum {
> +        XC_DOM_MEM_TYPE_MALLOC_INTERNAL,
> +        XC_DOM_MEM_TYPE_MALLOC_EXTERNAL,
> +        XC_DOM_MEM_TYPE_MMAP,
> +    } type;
> +    size_t len;
>      unsigned char memory[0];
>  };
>  
> @@ -298,6 +303,7 @@ void xc_dom_log_memory_footprint(struct xc_dom_image 
> *dom);
>  /* --- simple memory pool ------------------------------------------ */
>  
>  void *xc_dom_malloc(struct xc_dom_image *dom, size_t size);
> +int xc_dom_register_external(struct xc_dom_image *dom, void *ptr, size_t 
> size);
>  void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size);
>  void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
>                              const char *filename, size_t * size,
> diff --git a/tools/libxc/xc_dom_bzimageloader.c 
> b/tools/libxc/xc_dom_bzimageloader.c
> index 2225699..991a07b 100644
> --- a/tools/libxc/xc_dom_bzimageloader.c
> +++ b/tools/libxc/xc_dom_bzimageloader.c
> @@ -161,6 +161,13 @@ static int xc_try_bzip2_decode(
>  
>      total = (((uint64_t)stream.total_out_hi32) << 32) | 
> stream.total_out_lo32;
>  
> +    if ( xc_dom_register_external(dom, out_buf, total) )
> +    {
> +        DOMPRINTF("BZIP2: Error registering stream output");
> +        free(out_buf);
> +        goto bzip2_cleanup;
> +    }
> +
>      DOMPRINTF("%s: BZIP2 decompress OK, 0x%zx -> 0x%lx",
>                __FUNCTION__, *size, (long unsigned int) total);
>  
> @@ -305,6 +312,13 @@ static int _xc_try_lzma_decode(
>          }
>      }
>  
> +    if ( xc_dom_register_external(dom, out_buf, stream->total_out) )
> +    {
> +        DOMPRINTF("%s: Error registering stream output", what);
> +        free(out_buf);
> +        goto lzma_cleanup;
> +    }
> +
>      DOMPRINTF("%s: %s decompress OK, 0x%zx -> 0x%zx",
>                __FUNCTION__, what, *size, (size_t)stream->total_out);
>  
> @@ -508,6 +522,10 @@ static int xc_try_lzo1x_decode(
>              if ( out_len != dst_len )
>                  break;
>  
> +            msg = "Error registering stream output";
> +            if ( xc_dom_register_external(dom, out_buf, out_len) )
> +                break;
> +

Is this hunk problematic?

It's called in a loop. Looks like it may register the same ptr multiple
times which leads to freeing same ptr multiple times later.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel