Xen project Mailing List

Hi,

Just for your information, regarding limit case of size 0 :

lz4 decompression functions are routinely tested to decode a compressed block of null original size, and they handle this use case correctly.

Note that, due to format definition, a block of size 0 is compressed into a compressed-block of size 1 (yeah, I know...). That byte, by the way, will be 0.

Consequently, a compressed-block of size 0 is not supposed to exist (break format). For such a situation, the decompression functions should generate an error (negative function result).

Now, I realize that what I just said apply to the generic version, hosted at http://code.google.com/p/lz4/

Since you are probably using the Linux kernel version, it could be interesting to check if this corner case still holds. Obviously, compressing a null-sized block is a questionable use case, and most lz4 implementations ensure this never happens . But well, there must be a definition for it, just in case it does nonetheless happen.

The linux kernel version, while heavily based on original version, is a slightly modified one, most probably to respect some kernel rule requirement. That work was done by Kyungsik Lee, so you may want to include him in the discussion if you want to ask questions on his version.

Best Regards

Yann

2013/9/24 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)
> {
>
>

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