Re: [Xen-devel] [PATCH v2] libxc: add LZ4 decompression support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-09-24 at 17:29 +0100, Jan Beulich wrote:
> Since there's no shared or static library to link against, this simply
> re-uses the hypervisor side code. However, I only audited the code
> added here for possible security issues, not the referenced code in
> the hypervisor tree.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Fold in the addition of a previously missing include in the Mini-OS
>     case - provided by Ian Campbell - to allow stubdom compilation to
>     succeed.
> ---
> I intentionally retained the tab indentation in the code cloned from
> its hypervisor original (which in turn retained it as being a clone
> of the Linux original), to ease diff-ing.
> 
> --- a/tools/libxc/xc_dom_bzimageloader.c
> +++ b/tools/libxc/xc_dom_bzimageloader.c
> @@ -566,6 +566,8 @@ static int xc_try_lzo1x_decode(
>  
>  #else /* __MINIOS__ */
>  
> +#include <mini-os/byteorder.h>
> +
>  int xc_try_bzip2_decode(struct xc_dom_image *dom, void **blob, size_t *size);
>  int xc_try_lzma_decode(struct xc_dom_image *dom, void **blob, size_t *size);
>  int xc_try_lzo1x_decode(struct xc_dom_image *dom, void **blob, size_t *size);
> @@ -573,6 +575,124 @@ int xc_try_xz_decode(struct xc_dom_image
>  
>  #endif /* !__MINIOS__ */
>  
> +#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> +#define STATIC static
> +#define u8 uint8_t
> +#define u16 uint16_t
> +#define u32 uint32_t
> +#define u64 uint64_t
> +#define INIT
> +#define unlikely(x) (x)

I think rather than pollute this file with this sort of thing (which
might have unexpected consequences in the future) this would be better
of placed in a separate file compiled for both regular and stub use.

> +
> +static inline uint_fast16_t le16_to_cpup(const unsigned char *buf)
> +{
> +    return buf[0] | (buf[1] << 8);
> +}
> +
> +static inline uint_fast32_t le32_to_cpup(const unsigned char *buf)
> +{
> +    return le16_to_cpup(buf) | ((uint32_t)le16_to_cpup(buf + 2) << 16);
> +}
> +
> +#include "../../xen/common/lz4/decompress.c"
> +
> +/*
> + * Note: Uncompressed chunk size is used in the compressor side
> + * (userspace side for compression).
> + * It is hardcoded because there is not proper way to extract it
> + * from the binary stream which is generated by the preliminary
> + * version of LZ4 tool so far.
> + */
> +#define LZ4_DEFAULT_UNCOMPRESSED_CHUNK_SIZE (8 << 20)
> +#define ARCHIVE_MAGICNUMBER 0x184C2102
> +
> +static int xc_try_lz4_decode(
> +     struct xc_dom_image *dom, void **blob, size_t *psize)
> +{
> +     int ret = -1;
> +     size_t uncomp_chunksize = LZ4_DEFAULT_UNCOMPRESSED_CHUNK_SIZE;
> +     unsigned char *inp = *blob, *output, *outp;
> +     ssize_t size = *psize - 4;
> +     size_t out_size, out_len, dest_len, chunksize;
> +     const char *msg;
> +
> +     if (size < 4) {
> +             msg = "input too small";
> +             goto exit_0;

The exit_0 loop is after the error print, so these errors don't get
printed.

I don't really like exit_0 and exit_2 as names (what happened to
exit_1?). "err" and "err_free" would be more usual.


> +     }
> +
> +     out_size = out_len = get_unaligned_le32(inp + size);
> +     if (xc_dom_kernel_check_size(dom, out_len)) {
> +             msg = "Decompressed image too large";
> +             goto exit_0;
> +     }
> +
> +     output = malloc(out_len);
> +     if (!output) {
> +             msg = "Could not allocate output buffer";
> +             goto exit_0;
> +     }
> +     outp = output;
> +
> +     chunksize = get_unaligned_le32(inp);
> +     if (chunksize == ARCHIVE_MAGICNUMBER) {
> +             inp += 4;
> +             size -= 4;
> +     } else {
> +             msg = "invalid header";
> +             goto exit_2;
> +     }
> +
> +     for (;;) {
> +             if (size < 4) {
> +                     msg = "missing data";
> +                     goto exit_2;
> +             }
> +             chunksize = get_unaligned_le32(inp);
> +             if (chunksize == ARCHIVE_MAGICNUMBER) {
> +                     inp += 4;
> +                     size -= 4;
> +                     continue;
> +             }
> +             inp += 4;
> +             size -= 4;

We know size is at least 4 from the check at the head of the loop, but
now we subtracted 4 so size could be 0, but lz4_decompress doesn't take
size as an argument so whatever it does it isn't basing the return value
in &chunksize on it, so I assume it reads over the end of the buffer via
inp.

I didn't look to see what the minimum number of bytes which
lz4_decompress will consume is, but I think we need a check of some
description.

> +
> +             if (out_len >= uncomp_chunksize) {
> +                     dest_len = uncomp_chunksize;
> +                     out_len -= dest_len;
> +             } else
> +                     dest_len = out_len;
> +             ret = lz4_decompress(inp, &chunksize, outp, dest_len);
> +             if (ret < 0) {
> +                     msg = "decoding failed";
> +                     goto exit_2;
> +             }
> +
> +             outp += dest_len;
> +             size -= chunksize;
> +
> +             if (size == 0)
> +             {
> +                     *blob = output;
> +                     *psize = out_size;
> +                     return 0;
> +             }
> +
> +             if (size < 0) {
> +                     msg = "data corrupted";
> +                     goto exit_2;
> +             }
> +
> +             inp += chunksize;
> +     }
> +
> +exit_2:
> +     free(output);
> +     DOMPRINTF("LZ4 decompression error: %s\n", msg);
> +exit_0:
> +     return ret;
> +}
> +
>  struct setup_header {
>      uint8_t  _pad0[0x1f1];  /* skip uninteresting stuff */
>      uint8_t  setup_sects;
> @@ -733,6 +853,17 @@ static int xc_dom_probe_bzimage_kernel(s
>              return -EINVAL;
>          }
>      }
> +    else if ( check_magic(dom, "\x02\x21", 2) )
> +    {
> +        ret = xc_try_lz4_decode(dom, &dom->kernel_blob, &dom->kernel_size);
> +        if ( ret < 0 )
> +        {
> +            xc_dom_panic(dom->xch, XC_INVALID_KERNEL,
> +                         "%s unable to LZ4 decompress kernel\n",
> +                         __FUNCTION__);
> +            return -EINVAL;
> +        }
> +    }
>      else
>      {
>          xc_dom_panic(dom->xch, XC_INVALID_KERNEL,
> --- a/xen/common/lz4/decompress.c
> +++ b/xen/common/lz4/decompress.c
> @@ -151,7 +151,7 @@ _output_error:
>       return (int) (-(((unsigned char *)ip) - source));
>  }
>  
> -#ifndef __XEN__
> +#if !defined(__XEN__) && !defined(__XEN_TOOLS__)
>  static int lz4_uncompress_unknownoutputsize(const char *source, char *dest,
>                               int isize, size_t maxoutputsize)
>  {
> @@ -294,7 +294,7 @@ exit_0:
>       return ret;
>  }
>  
> -#ifndef __XEN__
> +#if !defined(__XEN__) && !defined(__XEN_TOOLS__)
>  int lz4_decompress_unknownoutputsize(const char *src, size_t src_len,
>               char *dest, size_t *dest_len)
>  {
> 
> 



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