[U-Boot] [PATCH v2 0/6] handle compression buffer overflows

[U-Boot] [PATCH v2 0/6] handle compression buffer overflows

[Kees Cook]

v2: added acks, various suggested cleanups

This series fixes gzip, lzma, and lzo to not overflow when writing
to output buffers. Without this, it might be possible for untrusted
compressed input to overflow the buffers used to hold the decompressed
image.

To catch these conditions, I added a series of compression tests available
in the sandbox build. Without the fixes in patches 3, 4, and 5, the
overflows are visible.

Thanks,

-Kees

Kees Cook (6):
      sandbox: add compression tests
      documentation: add more compression configs
      gzip: correctly bounds-check output buffer
      lzma: correctly bounds-check output buffer
      lzo: correctly bounds-check output buffer
      bootm: allow correct bounds-check of destination

 README                     |    9 ++
 common/cmd_bootm.c         |    2 +-
 include/configs/sandbox.h  |    5 +
 lib/gunzip.c               |    4 +-
 lib/lzma/LzmaTools.c       |    8 +-
 lib/lzo/lzo1x_decompress.c |    8 +-
 test/Makefile              |    1 +
 test/compression.c         |  335 ++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 366 insertions(+), 6 deletions(-)
 create mode 100644 test/compression.c

[U-Boot] [PATCH 1/6] sandbox: add compression tests

[Kees Cook]

This adds the "test_compression" command when building the sandbox. This
tests the existing compression and decompression routines for simple
sanity and for buffer overflow conditions.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2:
 - updates, suggested by Simon Glass:
   - replace license text with correct stub
   - drop redundant #ifdefs
---
 include/configs/sandbox.h |    5 +
 test/Makefile             |    1 +
 test/compression.c        |  335 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 341 insertions(+)
 create mode 100644 test/compression.c

diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index af3d6ad..4027030 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -109,4 +109,9 @@
 					"stdout=serial\0" \
 					"stderr=serial\0"
 
+#define CONFIG_GZIP_COMPRESSED
+#define CONFIG_BZIP2
+#define CONFIG_LZO
+#define CONFIG_LZMA
+
 #endif
diff --git a/test/Makefile b/test/Makefile
index 99ce890..a68613d 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -9,6 +9,7 @@ include $(TOPDIR)/config.mk
 LIB	= $(obj)libtest.o
 
 COBJS-$(CONFIG_SANDBOX) += command_ut.o
+COBJS-$(CONFIG_SANDBOX) += compression.o
 
 COBJS	:= $(sort $(COBJS-y))
 SRCS	:= $(COBJS:.o=.c)
diff --git a/test/compression.c b/test/compression.c
new file mode 100644
index 0000000..8834d5e
--- /dev/null
+++ b/test/compression.c
@@ -0,0 +1,335 @@
+/*
+ * Copyright (c) 2013, The Chromium Authors
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#define DEBUG
+
+#include <common.h>
+#include <command.h>
+#include <malloc.h>
+
+#include <u-boot/zlib.h>
+#include <bzlib.h>
+
+#include <lzma/LzmaTypes.h>
+#include <lzma/LzmaDec.h>
+#include <lzma/LzmaTools.h>
+
+#include <linux/lzo.h>
+
+static const char plain[] =
+	"I am a highly compressable bit of text.\n"
+	"I am a highly compressable bit of text.\n"
+	"I am a highly compressable bit of text.\n"
+	"There are many like me, but this one is mine.\n"
+	"If I were any shorter, there wouldn't be much sense in\n"
+	"compressing me in the first place. At least with lzo, anyway,\n"
+	"which appears to behave poorly in the face of short text\n"
+	"messages.\n";
+
+/* bzip2 -c /tmp/plain.txt > /tmp/plain.bz2 */
+static const char bzip2_compressed[] =
+	"\x42\x5a\x68\x39\x31\x41\x59\x26\x53\x59\xe5\x63\xdd\x09\x00\x00"
+	"\x28\x57\x80\x00\x10\x40\x85\x20\x20\x04\x00\x3f\xef\xdf\xf0\x30"
+	"\x00\xd6\xd0\x34\x91\x89\xa6\xf5\x4d\x19\x1a\x19\x0d\x02\x34\xd4"
+	"\xc9\x00\x34\x34\x00\x02\x48\x41\x35\x4f\xd4\xc6\x88\xd3\x50\x3d"
+	"\x4f\x51\x82\x4f\x88\xc3\x0d\x05\x62\x4f\x91\xa3\x52\x1b\xd0\x52"
+	"\x41\x4a\xa3\x98\xc2\x6b\xca\xa3\x82\xa5\xac\x8b\x15\x99\x68\xad"
+	"\xdf\x29\xd6\xf1\xf7\x5a\x10\xcd\x8c\x26\x61\x94\x95\xfe\x9e\x16"
+	"\x18\x28\x69\xd4\x23\x64\xcc\x2b\xe5\xe8\x5f\x00\xa4\x70\x26\x2c"
+	"\xee\xbd\x59\x6d\x6a\xec\xfc\x31\xda\x59\x0a\x14\x2a\x60\x1c\xf0"
+	"\x04\x86\x73\x9a\xc5\x5b\x87\x3f\x5b\x4c\x93\xe6\xb5\x35\x0d\xa6"
+	"\xb1\x2e\x62\x7b\xab\x67\xe7\x99\x2a\x14\x5e\x9f\x64\xcb\x96\xf4"
+	"\x0d\x65\xd4\x39\xe6\x8b\x7e\xea\x1c\x03\x69\x97\x83\x58\x91\x96"
+	"\xe1\xf0\x9d\xa4\x15\x8b\xb8\xc6\x93\xdc\x3d\xd9\x3c\x22\x55\xef"
+	"\xfb\xbb\x2a\xd3\x87\xa2\x8b\x04\xd9\x19\xf8\xe2\xfd\x4f\xdb\x1a"
+	"\x07\xc8\x60\xa3\x3f\xf8\xbb\x92\x29\xc2\x84\x87\x2b\x1e\xe8\x48";
+static const unsigned long bzip2_compressed_size = 240;
+
+/* lzma -z -c /tmp/plain.txt > /tmp/plain.lzma */
+static const char lzma_compressed[] =
+	"\x5d\x00\x00\x80\x00\xff\xff\xff\xff\xff\xff\xff\xff\x00\x24\x88"
+	"\x08\x26\xd8\x41\xff\x99\xc8\xcf\x66\x3d\x80\xac\xba\x17\xf1\xc8"
+	"\xb9\xdf\x49\x37\xb1\x68\xa0\x2a\xdd\x63\xd1\xa7\xa3\x66\xf8\x15"
+	"\xef\xa6\x67\x8a\x14\x18\x80\xcb\xc7\xb1\xcb\x84\x6a\xb2\x51\x16"
+	"\xa1\x45\xa0\xd6\x3e\x55\x44\x8a\x5c\xa0\x7c\xe5\xa8\xbd\x04\x57"
+	"\x8f\x24\xfd\xb9\x34\x50\x83\x2f\xf3\x46\x3e\xb9\xb0\x00\x1a\xf5"
+	"\xd3\x86\x7e\x8f\x77\xd1\x5d\x0e\x7c\xe1\xac\xde\xf8\x65\x1f\x4d"
+	"\xce\x7f\xa7\x3d\xaa\xcf\x26\xa7\x58\x69\x1e\x4c\xea\x68\x8a\xe5"
+	"\x89\xd1\xdc\x4d\xc7\xe0\x07\x42\xbf\x0c\x9d\x06\xd7\x51\xa2\x0b"
+	"\x7c\x83\x35\xe1\x85\xdf\xee\xfb\xa3\xee\x2f\x47\x5f\x8b\x70\x2b"
+	"\xe1\x37\xf3\x16\xf6\x27\x54\x8a\x33\x72\x49\xea\x53\x7d\x60\x0b"
+	"\x21\x90\x66\xe7\x9e\x56\x61\x5d\xd8\xdc\x59\xf0\xac\x2f\xd6\x49"
+	"\x6b\x85\x40\x08\x1f\xdf\x26\x25\x3b\x72\x44\xb0\xb8\x21\x2f\xb3"
+	"\xd7\x9b\x24\x30\x78\x26\x44\x07\xc3\x33\xd1\x4d\x03\x1b\xe1\xff"
+	"\xfd\xf5\x50\x8d\xca";
+static const unsigned long lzma_compressed_size = 229;
+
+/* lzop -c /tmp/plain.txt > /tmp/plain.lzo */
+static const char lzo_compressed[] =
+	"\x89\x4c\x5a\x4f\x00\x0d\x0a\x1a\x0a\x10\x30\x20\x60\x09\x40\x01"
+	"\x05\x03\x00\x00\x09\x00\x00\x81\xb4\x52\x09\x54\xf1\x00\x00\x00"
+	"\x00\x09\x70\x6c\x61\x69\x6e\x2e\x74\x78\x74\x65\xb1\x07\x9c\x00"
+	"\x00\x01\x5e\x00\x00\x01\x0f\xc3\xc7\x7a\xe0\x00\x16\x49\x20\x61"
+	"\x6d\x20\x61\x20\x68\x69\x67\x68\x6c\x79\x20\x63\x6f\x6d\x70\x72"
+	"\x65\x73\x73\x61\x62\x6c\x65\x20\x62\x69\x74\x20\x6f\x66\x20\x74"
+	"\x65\x78\x74\x2e\x0a\x20\x2f\x9c\x00\x00\x22\x54\x68\x65\x72\x65"
+	"\x20\x61\x72\x65\x20\x6d\x61\x6e\x79\x20\x6c\x69\x6b\x65\x20\x6d"
+	"\x65\x2c\x20\x62\x75\x74\x20\x74\x68\x69\x73\x20\x6f\x6e\x65\x20"
+	"\x69\x73\x20\x6d\x69\x6e\x65\x2e\x0a\x49\x66\x20\x49\x20\x77\x84"
+	"\x06\x0a\x6e\x79\x20\x73\x68\x6f\x72\x74\x65\x72\x2c\x20\x74\x90"
+	"\x08\x00\x08\x77\x6f\x75\x6c\x64\x6e\x27\x74\x20\x62\x65\x20\x6d"
+	"\x75\x63\x68\x20\x73\x65\x6e\x73\x65\x20\x69\x6e\x0a\xf8\x19\x02"
+	"\x69\x6e\x67\x20\x6d\x64\x02\x64\x06\x00\x5a\x20\x66\x69\x72\x73"
+	"\x74\x20\x70\x6c\x61\x63\x65\x2e\x20\x41\x74\x20\x6c\x65\x61\x73"
+	"\x74\x20\x77\x69\x74\x68\x20\x6c\x7a\x6f\x2c\x20\x61\x6e\x79\x77"
+	"\x61\x79\x2c\x0a\x77\x68\x69\x63\x68\x20\x61\x70\x70\x65\x61\x72"
+	"\x73\x20\x74\x6f\x20\x62\x65\x68\x61\x76\x65\x20\x70\x6f\x6f\x72"
+	"\x6c\x79\x20\x69\x6e\x20\x74\x68\x65\x20\x66\x61\x63\x65\x20\x6f"
+	"\x66\x20\x73\x68\x6f\x72\x74\x20\x74\x65\x78\x74\x0a\x6d\x65\x73"
+	"\x73\x61\x67\x65\x73\x2e\x0a\x11\x00\x00\x00\x00\x00\x00";
+static const unsigned long lzo_compressed_size = 334;
+
+
+#define TEST_BUFFER_SIZE	512
+
+typedef int (*mutate_func)(void *, unsigned long, void *, unsigned long,
+			   unsigned long *);
+
+static int compress_using_gzip(void *in, unsigned long in_size,
+			       void *out, unsigned long out_max,
+			       unsigned long *out_size)
+{
+	int ret;
+	unsigned long inout_size = out_max;
+
+	ret = gzip(out, &inout_size, in, in_size);
+	if (out_size)
+		*out_size = inout_size;
+
+	return ret;
+}
+
+static int uncompress_using_gzip(void *in, unsigned long in_size,
+				 void *out, unsigned long out_max,
+				 unsigned long *out_size)
+{
+	int ret;
+	unsigned long inout_size = in_size;
+
+	ret = gunzip(out, out_max, in, &inout_size);
+	if (out_size)
+		*out_size = inout_size;
+
+	return ret;
+}
+
+static int compress_using_bzip2(void *in, unsigned long in_size,
+			        void *out, unsigned long out_max,
+			        unsigned long *out_size)
+{
+	/* There is no bzip2 compression in u-boot, so fake it. */
+	assert(in_size == strlen(plain));
+	assert(memcmp(plain, in, in_size) == 0);
+
+	if (bzip2_compressed_size > out_max)
+		return -1;
+
+	memcpy(out, bzip2_compressed, bzip2_compressed_size);
+	if (out_size)
+		*out_size = bzip2_compressed_size;
+
+	return 0;
+}
+
+static int uncompress_using_bzip2(void *in, unsigned long in_size,
+				  void *out, unsigned long out_max,
+				  unsigned long *out_size)
+{
+	int ret;
+	unsigned int inout_size = out_max;
+
+	ret = BZ2_bzBuffToBuffDecompress(out, &inout_size, in, in_size,
+			CONFIG_SYS_MALLOC_LEN < (4096 * 1024), 0);
+	if (out_size)
+		*out_size = inout_size;
+
+	return (ret != BZ_OK);
+}
+
+static int compress_using_lzma(void *in, unsigned long in_size,
+			       void *out, unsigned long out_max,
+			       unsigned long *out_size)
+{
+	/* There is no lzma compression in u-boot, so fake it. */
+	assert(in_size == strlen(plain));
+	assert(memcmp(plain, in, in_size) == 0);
+
+	if (lzma_compressed_size > out_max)
+		return -1;
+
+	memcpy(out, lzma_compressed, lzma_compressed_size);
+	if (out_size)
+		*out_size = lzma_compressed_size;
+
+	return 0;
+}
+
+static int uncompress_using_lzma(void *in, unsigned long in_size,
+				 void *out, unsigned long out_max,
+				 unsigned long *out_size)
+{
+	int ret;
+	SizeT inout_size = out_max;
+
+	ret = lzmaBuffToBuffDecompress(out, &inout_size, in, in_size);
+	if (out_size)
+		*out_size = inout_size;
+
+	return (ret != SZ_OK);
+}
+
+static int compress_using_lzo(void *in, unsigned long in_size,
+			      void *out, unsigned long out_max,
+			      unsigned long *out_size)
+{
+	/* There is no lzo compression in u-boot, so fake it. */
+	assert(in_size == strlen(plain));
+	assert(memcmp(plain, in, in_size) == 0);
+
+	if (lzo_compressed_size > out_max)
+		return -1;
+
+	memcpy(out, lzo_compressed, lzo_compressed_size);
+	if (out_size)
+		*out_size = lzo_compressed_size;
+
+	return 0;
+}
+
+static int uncompress_using_lzo(void *in, unsigned long in_size,
+				void *out, unsigned long out_max,
+				unsigned long *out_size)
+{
+	int ret;
+	size_t input_size = in_size;
+	size_t output_size = out_max;
+
+	ret = lzop_decompress(in, input_size, out, &output_size);
+	if (out_size)
+		*out_size = output_size;
+
+	return (ret != LZO_E_OK);
+}
+
+#define errcheck(statement) if (!(statement)) { \
+	fprintf(stderr, "\tFailed: %s\n", #statement); \
+	ret = 1; \
+	goto out; \
+}
+
+static int run_test(char *name, mutate_func compress, mutate_func uncompress)
+{
+	ulong orig_size, compressed_size, uncompressed_size;
+	void *orig_buf;
+	void *compressed_buf = NULL;
+	void *uncompressed_buf = NULL;
+	void *compare_buf = NULL;
+	int ret;
+
+	printf(" testing %s ...\n", name);
+
+	orig_buf = (void *)plain;
+	orig_size = strlen(orig_buf); /* Trailing NULL not included. */
+	errcheck(orig_size > 0);
+
+	compressed_size = uncompressed_size = TEST_BUFFER_SIZE;
+	compressed_buf = malloc(compressed_size);
+	errcheck(compressed_buf != NULL);
+	uncompressed_buf = malloc(uncompressed_size);
+	errcheck(uncompressed_buf != NULL);
+	compare_buf = malloc(uncompressed_size);
+	errcheck(compare_buf != NULL);
+
+	/* Compress works as expected. */
+	printf("\torig_size:%lu\n", orig_size);
+	memset(compressed_buf, 'A', TEST_BUFFER_SIZE);
+	errcheck(compress(orig_buf, orig_size,
+			compressed_buf, compressed_size,
+			&compressed_size) == 0);
+	printf("\tcompressed_size:%lu\n", compressed_size);
+	errcheck(compressed_size > 0);
+	errcheck(compressed_size < orig_size);
+	errcheck(((char *)compressed_buf)[compressed_size-1] != 'A');
+	errcheck(((char *)compressed_buf)[compressed_size] == 'A');
+
+	/* Uncompresses with space remaining. */
+	errcheck(uncompress(compressed_buf, compressed_size,
+			  uncompressed_buf, uncompressed_size,
+			  &uncompressed_size) == 0);
+	printf("\tuncompressed_size:%lu\n", uncompressed_size);
+	errcheck(uncompressed_size == orig_size);
+	errcheck(memcmp(orig_buf, uncompressed_buf, orig_size) == 0);
+
+	/* Uncompresses with exactly the right size output buffer. */
+	memset(uncompressed_buf, 'A', TEST_BUFFER_SIZE);
+	errcheck(uncompress(compressed_buf, compressed_size,
+			  uncompressed_buf, orig_size,
+			  &uncompressed_size) == 0);
+	errcheck(uncompressed_size == orig_size);
+	errcheck(memcmp(orig_buf, uncompressed_buf, orig_size) == 0);
+	errcheck(((char *)uncompressed_buf)[orig_size] == 'A');
+
+	/* Make sure compression does not over-run. */
+	memset(compare_buf, 'A', TEST_BUFFER_SIZE);
+	ret = compress(orig_buf, orig_size,
+		       compare_buf, compressed_size - 1,
+		       NULL);
+	errcheck(((char *)compare_buf)[compressed_size] == 'A');
+	errcheck(ret != 0);
+	printf("\tcompress does not overrun\n");
+
+	/* Make sure decompression does not over-run. */
+	memset(compare_buf, 'A', TEST_BUFFER_SIZE);
+	ret = uncompress(compressed_buf, compressed_size,
+			 compare_buf, uncompressed_size - 1,
+			 NULL);
+	errcheck(((char *)compare_buf)[uncompressed_size - 1] == 'A');
+	errcheck(ret != 0);
+	printf("\tuncompress does not overrun\n");
+
+	/* Got here, everything is fine. */
+	ret = 0;
+
+out:
+	printf(" %s: %s\n", name, ret == 0 ? "ok" : "FAILED");
+
+	free(compare_buf);
+	free(uncompressed_buf);
+	free(compressed_buf);
+
+	return ret;
+}
+
+
+static int do_test_compression(cmd_tbl_t *cmdtp, int flag, int argc,
+			       char * const argv[])
+{
+	int err = 0;
+
+	err += run_test("gzip", compress_using_gzip, uncompress_using_gzip);
+	err += run_test("bzip2", compress_using_bzip2, uncompress_using_bzip2);
+	err += run_test("lzma", compress_using_lzma, uncompress_using_lzma);
+	err += run_test("lzo", compress_using_lzo, uncompress_using_lzo);
+
+	printf("test_compression %s\n", err == 0 ? "ok" : "FAILED");
+
+	return err;
+}
+
+U_BOOT_CMD(
+	test_compression,	5,	1,	do_test_compression,
+	"Basic test of compressors: gzip bzip2 lzma lzo", ""
+);
-- 
1.7.9.5

[U-Boot] [PATCH 1/6] sandbox: add compression tests

[Simon Glass]

On Fri, Aug 16, 2013 at 8:59 AM, Kees Cook <keescook@chromium.org> wrote:
> This adds the "test_compression" command when building the sandbox. This
> tests the existing compression and decompression routines for simple
> sanity and for buffer overflow conditions.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/6] documentation: add more compression configs

[Kees Cook]

This adds the missing compression config items to the README.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2:
 - adjusted language slightly, thanks to Simon Glass
---
 README |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/README b/README
index 3918807..6485350 100644
--- a/README
+++ b/README
@@ -1680,6 +1680,10 @@ CBFS (Coreboot Filesystem) support
 		to compress the specified memory at its best effort.
 
 - Compression support:
+		CONFIG_GZIP
+
+		Enabled by default to support gzip compressed images.
+
 		CONFIG_BZIP2
 
 		If this option is set, support for bzip2 compressed
@@ -1713,6 +1717,11 @@ CBFS (Coreboot Filesystem) support
 		then calculate the amount of needed dynamic memory (ensuring
 		the appropriate CONFIG_SYS_MALLOC_LEN value).
 
+		CONFIG_LZO
+
+		If this option is set, support for LZO compressed images
+		is included.
+
 - MII/PHY support:
 		CONFIG_PHY_ADDR
 
-- 
1.7.9.5

[U-Boot] [PATCH 2/6] documentation: add more compression configs

[Simon Glass]

On Fri, Aug 16, 2013 at 8:59 AM, Kees Cook <keescook@chromium.org> wrote:
> This adds the missing compression config items to the README.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 3/6] gzip: correctly bounds-check output buffer

[Kees Cook]

The output buffer size must not be reset by the gzip decoder or there
is a risk of overflowing memory during decompression.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Simon Glass <sjg@chromium.org>
---
 lib/gunzip.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/gunzip.c b/lib/gunzip.c
index 9959781..35abfb3 100644
--- a/lib/gunzip.c
+++ b/lib/gunzip.c
@@ -89,13 +89,13 @@ int zunzip(void *dst, int dstlen, unsigned char *src, unsigned long *lenp,
 	s.avail_out = dstlen;
 	do {
 		r = inflate(&s, Z_FINISH);
-		if (r != Z_STREAM_END && r != Z_BUF_ERROR && stoponerr == 1) {
+		if (stoponerr == 1 && r != Z_STREAM_END &&
+		    (s.avail_out == 0 || r != Z_BUF_ERROR)) {
 			printf("Error: inflate() returned %d\n", r);
 			inflateEnd(&s);
 			return -1;
 		}
 		s.avail_in = *lenp - offset - (int)(s.next_out - (unsigned char*)dst);
-		s.avail_out = dstlen;
 	} while (r == Z_BUF_ERROR);
 	*lenp = s.next_out - (unsigned char *) dst;
 	inflateEnd(&s);
-- 
1.7.9.5

[U-Boot] [PATCH 3/6] gzip: correctly bounds-check output buffer

[Michal Simek]

Hi Kees,

On 08/16/2013 04:59 PM, Kees Cook wrote:
> The output buffer size must not be reset by the gzip decoder or there
> is a risk of overflowing memory during decompression.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Acked-by: Simon Glass <sjg@chromium.org>
> ---
>  lib/gunzip.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/gunzip.c b/lib/gunzip.c
> index 9959781..35abfb3 100644
> --- a/lib/gunzip.c
> +++ b/lib/gunzip.c
> @@ -89,13 +89,13 @@ int zunzip(void *dst, int dstlen, unsigned char *src, unsigned long *lenp,
>  	s.avail_out = dstlen;
>  	do {
>  		r = inflate(&s, Z_FINISH);
> -		if (r != Z_STREAM_END && r != Z_BUF_ERROR && stoponerr == 1) {
> +		if (stoponerr == 1 && r != Z_STREAM_END &&
> +		    (s.avail_out == 0 || r != Z_BUF_ERROR)) {
>  			printf("Error: inflate() returned %d\n", r);
>  			inflateEnd(&s);
>  			return -1;
>  		}
>  		s.avail_in = *lenp - offset - (int)(s.next_out - (unsigned char*)dst);
> -		s.avail_out = dstlen;
>  	} while (r == Z_BUF_ERROR);
>  	*lenp = s.next_out - (unsigned char *) dst;
>  	inflateEnd(&s);
> 

I have done u-boot upgrade to v2013.10 version and I see the problem with this patch
when I am trying to boot my zynq image.

After reverting this patch everything works as expected.

Here is the image I am using.
http://www.monstr.eu/20131108-image.ub

Below is the bootlog.

Do you have any idea what can be wrong?

Thanks,
Michal

U-Boot 2013.10 (Nov 08 2013 - 13:02:26)

Memory: ECC disabled
DRAM:  1 GiB
WARNING: Caches not enabled
MMC:   zynq_sdhci: 0
SF: Detected N25Q128A with page size 256 Bytes, erase size 4 KiB, total 16 MiB
*** Warning - bad CRC, using default environment

In:    serial
Out:   serial
Err:   serial
Net:   Gem.e000b000
U-BOOT for zynq-zc702

Gem.e000b000 Waiting for PHY auto negotiation to complete.... done
BOOTP broadcast 1
DHCP client bound to address 192.168.0.90
Hit any key to stop autoboot:  0
U-Boot-PetaLinux> run netboot
Gem.e000b000:7 is connected to Gem.e000b000.  Reconnecting to Gem.e000b000
Gem.e000b000 Waiting for PHY auto negotiation to complete.... done
Using Gem.e000b000 device
TFTP from server 192.168.0.100; our IP address is 192.168.0.90
Filename 'image.ub'.
Load address: 0x1000000
Loading: #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #################################################################
	 #######################################
	 2 MiB/s
done
Bytes transferred = 12964752 (c5d390 hex)
## Loading kernel from FIT Image at 01000000 ...
   Using 'conf at 1' configuration
   Trying 'kernel at 1' kernel subimage
     Description:  PetaLinux Kernel
     Type:         Kernel Image
     Compression:  gzip compressed
     Data Start:   0x010000f0
     Data Size:    12949283 Bytes = 12.3 MiB
     Architecture: ARM
     OS:           Linux
     Load Address: 0x10008000
     Entry Point:  0x10008000
     Hash algo:    crc32
     Hash value:   39564940
   Verifying Hash Integrity ... crc32+ OK
## Loading fdt from FIT Image at 01000000 ...
   Using 'conf at 1' configuration
   Trying 'fdt at 1' fdt subimage
     Description:  Flattened Device Tree blob
     Type:         Flat Device Tree
     Compression:  uncompressed
     Data Start:   0x01c598f8
     Data Size:    14133 Bytes = 13.8 KiB
     Architecture: ARM
     Hash algo:    crc32
     Hash value:   be457cb0
     Hash algo:    sha1
     Hash value:   206ffdb413e297d4a143a47fa8598cee4527a63a
   Verifying Hash Integrity ... crc32+ sha1+ OK
   Booting using the fdt blob at 0x1c598f8
   Uncompressing Kernel Image ... Error: inflate() returned -5
GUNZIP: uncompress, out-of-mem or overwrite error - must RESET board to recover
resetting ...


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131108/6b21f45d/attachment.pgp>

[U-Boot] [PATCH 3/6] gzip: correctly bounds-check output buffer

[Kees Cook]

On Fri, Nov 8, 2013 at 4:04 AM, Michal Simek <monstr@monstr.eu> wrote:
> Hi Kees,
>
> On 08/16/2013 04:59 PM, Kees Cook wrote:
>> The output buffer size must not be reset by the gzip decoder or there
>> is a risk of overflowing memory during decompression.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Acked-by: Simon Glass <sjg@chromium.org>
>> ---
>>  lib/gunzip.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/gunzip.c b/lib/gunzip.c
>> index 9959781..35abfb3 100644
>> --- a/lib/gunzip.c
>> +++ b/lib/gunzip.c
>> @@ -89,13 +89,13 @@ int zunzip(void *dst, int dstlen, unsigned char *src, unsigned long *lenp,
>>       s.avail_out = dstlen;
>>       do {
>>               r = inflate(&s, Z_FINISH);
>> -             if (r != Z_STREAM_END && r != Z_BUF_ERROR && stoponerr == 1) {
>> +             if (stoponerr == 1 && r != Z_STREAM_END &&
>> +                 (s.avail_out == 0 || r != Z_BUF_ERROR)) {
>>                       printf("Error: inflate() returned %d\n", r);
>>                       inflateEnd(&s);
>>                       return -1;
>>               }
>>               s.avail_in = *lenp - offset - (int)(s.next_out - (unsigned char*)dst);
>> -             s.avail_out = dstlen;
>>       } while (r == Z_BUF_ERROR);
>>       *lenp = s.next_out - (unsigned char *) dst;
>>       inflateEnd(&s);
>>
>
> I have done u-boot upgrade to v2013.10 version and I see the problem with this patch
> when I am trying to boot my zynq image.
>
> After reverting this patch everything works as expected.

Eek, sorry this is causing you trouble!

> Here is the image I am using.
> http://www.monstr.eu/20131108-image.ub

Is there any way you can extract just the gzipped kernel from this
image? I'm not sure how to get at it from this .ub file.

> Below is the bootlog.
>
> Do you have any idea what can be wrong?
> [...]
> Uncompressing Kernel Image ... Error: inflate() returned -5
> GUNZIP: uncompress, out-of-mem or overwrite error - must RESET board to recover
> resetting ...

Either my change is failing to detect end-of-buffer correctly, or it
_is_, in which case this has uncovered an unsafe caller of gunzip.
This is after the "Uncompressing" message, so it's this caller:

        case IH_COMP_GZIP:
                printf("   Uncompressing %s ... ", type_name);
                if (gunzip(load_buf, unc_len, image_buf, &image_len) != 0) {
                        puts("GUNZIP: uncompress, out-of-mem or overwrite "
                                "error - must RESET board to recover\n");
                        if (boot_progress)
                                bootstage_error(BOOTSTAGE_ID_DECOMP_IMAGE);
                        return BOOTM_ERR_RESET;
                }

                *load_end = load + image_len;
                break;

If the uncompressed length of the kernel image is larger than
"unc_len", then this is catching a legitimate memory overflow. This is
entirely controlled by CONFIG_SYS_BOOTM_LEN. Is it possible this is
set too low for your build?

-Kees

-- 
Kees Cook
Chrome OS Security

[U-Boot] [PATCH 3/6] gzip: correctly bounds-check output buffer

[Michal Simek]

On 11/08/2013 04:21 PM, Kees Cook wrote:
> On Fri, Nov 8, 2013 at 4:04 AM, Michal Simek <monstr@monstr.eu> wrote:
>> Hi Kees,
>>
>> On 08/16/2013 04:59 PM, Kees Cook wrote:
>>> The output buffer size must not be reset by the gzip decoder or there
>>> is a risk of overflowing memory during decompression.
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> Acked-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>  lib/gunzip.c |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/gunzip.c b/lib/gunzip.c
>>> index 9959781..35abfb3 100644
>>> --- a/lib/gunzip.c
>>> +++ b/lib/gunzip.c
>>> @@ -89,13 +89,13 @@ int zunzip(void *dst, int dstlen, unsigned char *src, unsigned long *lenp,
>>>       s.avail_out = dstlen;
>>>       do {
>>>               r = inflate(&s, Z_FINISH);
>>> -             if (r != Z_STREAM_END && r != Z_BUF_ERROR && stoponerr == 1) {
>>> +             if (stoponerr == 1 && r != Z_STREAM_END &&
>>> +                 (s.avail_out == 0 || r != Z_BUF_ERROR)) {
>>>                       printf("Error: inflate() returned %d\n", r);
>>>                       inflateEnd(&s);
>>>                       return -1;
>>>               }
>>>               s.avail_in = *lenp - offset - (int)(s.next_out - (unsigned char*)dst);
>>> -             s.avail_out = dstlen;
>>>       } while (r == Z_BUF_ERROR);
>>>       *lenp = s.next_out - (unsigned char *) dst;
>>>       inflateEnd(&s);
>>>
>>
>> I have done u-boot upgrade to v2013.10 version and I see the problem with this patch
>> when I am trying to boot my zynq image.
>>
>> After reverting this patch everything works as expected.
> 
> Eek, sorry this is causing you trouble!

no worries. Problem is on my side. Look below.

>> Here is the image I am using.
>> http://www.monstr.eu/20131108-image.ub
> 
> Is there any way you can extract just the gzipped kernel from this
> image? I'm not sure how to get at it from this .ub file.

Sure just run imi. Then you will get data start address and length.
And you can use unzip command.

>> Below is the bootlog.
>>
>> Do you have any idea what can be wrong?
>> [...]
>> Uncompressing Kernel Image ... Error: inflate() returned -5
>> GUNZIP: uncompress, out-of-mem or overwrite error - must RESET board to recover
>> resetting ...
> 
> Either my change is failing to detect end-of-buffer correctly, or it
> _is_, in which case this has uncovered an unsafe caller of gunzip.
> This is after the "Uncompressing" message, so it's this caller:
> 
>         case IH_COMP_GZIP:
>                 printf("   Uncompressing %s ... ", type_name);
>                 if (gunzip(load_buf, unc_len, image_buf, &image_len) != 0) {
>                         puts("GUNZIP: uncompress, out-of-mem or overwrite "
>                                 "error - must RESET board to recover\n");
>                         if (boot_progress)
>                                 bootstage_error(BOOTSTAGE_ID_DECOMP_IMAGE);
>                         return BOOTM_ERR_RESET;
>                 }
> 
>                 *load_end = load + image_len;
>                 break;
> 
> If the uncompressed length of the kernel image is larger than
> "unc_len", then this is catching a legitimate memory overflow. This is
> entirely controlled by CONFIG_SYS_BOOTM_LEN. Is it possible this is
> set too low for your build?

Ah yes, that's the issue. My image is 14MB and have just 16MB BOOTM_LEN.

Thanks for pointing to this.
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131108/f50fc63a/attachment.pgp>

[U-Boot] [PATCH 3/6] gzip: correctly bounds-check output buffer

[Michal Simek]

On 11/08/2013 04:40 PM, Michal Simek wrote:
> On 11/08/2013 04:21 PM, Kees Cook wrote:
>> On Fri, Nov 8, 2013 at 4:04 AM, Michal Simek <monstr@monstr.eu> wrote:
>>> Hi Kees,
>>>
>>> On 08/16/2013 04:59 PM, Kees Cook wrote:
>>>> The output buffer size must not be reset by the gzip decoder or there
>>>> is a risk of overflowing memory during decompression.
>>>>
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> Acked-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>  lib/gunzip.c |    4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/gunzip.c b/lib/gunzip.c
>>>> index 9959781..35abfb3 100644
>>>> --- a/lib/gunzip.c
>>>> +++ b/lib/gunzip.c
>>>> @@ -89,13 +89,13 @@ int zunzip(void *dst, int dstlen, unsigned char *src, unsigned long *lenp,
>>>>       s.avail_out = dstlen;
>>>>       do {
>>>>               r = inflate(&s, Z_FINISH);
>>>> -             if (r != Z_STREAM_END && r != Z_BUF_ERROR && stoponerr == 1) {
>>>> +             if (stoponerr == 1 && r != Z_STREAM_END &&
>>>> +                 (s.avail_out == 0 || r != Z_BUF_ERROR)) {
>>>>                       printf("Error: inflate() returned %d\n", r);
>>>>                       inflateEnd(&s);
>>>>                       return -1;
>>>>               }
>>>>               s.avail_in = *lenp - offset - (int)(s.next_out - (unsigned char*)dst);
>>>> -             s.avail_out = dstlen;
>>>>       } while (r == Z_BUF_ERROR);
>>>>       *lenp = s.next_out - (unsigned char *) dst;
>>>>       inflateEnd(&s);
>>>>
>>>
>>> I have done u-boot upgrade to v2013.10 version and I see the problem with this patch
>>> when I am trying to boot my zynq image.
>>>
>>> After reverting this patch everything works as expected.
>>
>> Eek, sorry this is causing you trouble!
> 
> no worries. Problem is on my side. Look below.
> 
>>> Here is the image I am using.
>>> http://www.monstr.eu/20131108-image.ub
>>
>> Is there any way you can extract just the gzipped kernel from this
>> image? I'm not sure how to get at it from this .ub file.
> 
> Sure just run imi. Then you will get data start address and length.
> And you can use unzip command.
> 
>>> Below is the bootlog.
>>>
>>> Do you have any idea what can be wrong?
>>> [...]
>>> Uncompressing Kernel Image ... Error: inflate() returned -5
>>> GUNZIP: uncompress, out-of-mem or overwrite error - must RESET board to recover
>>> resetting ...
>>
>> Either my change is failing to detect end-of-buffer correctly, or it
>> _is_, in which case this has uncovered an unsafe caller of gunzip.
>> This is after the "Uncompressing" message, so it's this caller:
>>
>>         case IH_COMP_GZIP:
>>                 printf("   Uncompressing %s ... ", type_name);
>>                 if (gunzip(load_buf, unc_len, image_buf, &image_len) != 0) {
>>                         puts("GUNZIP: uncompress, out-of-mem or overwrite "
>>                                 "error - must RESET board to recover\n");
>>                         if (boot_progress)
>>                                 bootstage_error(BOOTSTAGE_ID_DECOMP_IMAGE);
>>                         return BOOTM_ERR_RESET;
>>                 }
>>
>>                 *load_end = load + image_len;
>>                 break;
>>
>> If the uncompressed length of the kernel image is larger than
>> "unc_len", then this is catching a legitimate memory overflow. This is
>> entirely controlled by CONFIG_SYS_BOOTM_LEN. Is it possible this is
>> set too low for your build?
> 
> Ah yes, that's the issue. My image is 14MB and have just 16MB BOOTM_LEN.
> 

I have read README about BOOTM_LEN and it cares just about compressed images
but macro is generic enough to also handle uncompressed images and this checking
should be probably done too.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131108/cbda11c7/attachment.pgp>

[U-Boot] [PATCH 4/6] lzma: correctly bounds-check output buffer

[Kees Cook]

The output buffer size must be correctly passed to the lzma decoder or
there is a risk of overflowing memory during decompression. Switching
to the LZMA_FINISH_END mode means nothing is left in an unknown state
once the buffer becomes full.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Simon Glass <sjg@chromium.org>
---
 lib/lzma/LzmaTools.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/lzma/LzmaTools.c b/lib/lzma/LzmaTools.c
index 8d1165e11b..0aec2f9 100644
--- a/lib/lzma/LzmaTools.c
+++ b/lib/lzma/LzmaTools.c
@@ -97,15 +97,19 @@ int lzmaBuffToBuffDecompress (unsigned char *outStream, SizeT *uncompressedSize,
     g_Alloc.Alloc = SzAlloc;
     g_Alloc.Free = SzFree;
 
+    /* Short-circuit early if we know the buffer can't hold the results. */
+    if (outSizeFull != (SizeT)-1 && *uncompressedSize < outSizeFull)
+        return SZ_ERROR_OUTPUT_EOF;
+
     /* Decompress */
-    outProcessed = outSizeFull;
+    outProcessed = *uncompressedSize;
 
     WATCHDOG_RESET();
 
     res = LzmaDecode(
         outStream, &outProcessed,
         inStream + LZMA_DATA_OFFSET, &compressedSize,
-        inStream, LZMA_PROPS_SIZE, LZMA_FINISH_ANY, &state, &g_Alloc);
+        inStream, LZMA_PROPS_SIZE, LZMA_FINISH_END, &state, &g_Alloc);
     *uncompressedSize = outProcessed;
     if (res != SZ_OK)  {
         return res;
-- 
1.7.9.5

[U-Boot] [PATCH 5/6] lzo: correctly bounds-check output buffer

[Kees Cook]

This checks the size of the output buffer and fails if it was going to
overflow the buffer during lzo decompression.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Simon Glass <sjg@chromium.org>
---
 lib/lzo/lzo1x_decompress.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/lzo/lzo1x_decompress.c b/lib/lzo/lzo1x_decompress.c
index e6ff708..35f3793 100644
--- a/lib/lzo/lzo1x_decompress.c
+++ b/lib/lzo/lzo1x_decompress.c
@@ -68,13 +68,14 @@ int lzop_decompress(const unsigned char *src, size_t src_len,
 	unsigned char *start = dst;
 	const unsigned char *send = src + src_len;
 	u32 slen, dlen;
-	size_t tmp;
+	size_t tmp, remaining;
 	int r;
 
 	src = parse_header(src);
 	if (!src)
 		return LZO_E_ERROR;
 
+	remaining = *dst_len;
 	while (src < send) {
 		/* read uncompressed block size */
 		dlen = get_unaligned_be32(src);
@@ -93,6 +94,10 @@ int lzop_decompress(const unsigned char *src, size_t src_len,
 		if (slen <= 0 || slen > dlen)
 			return LZO_E_ERROR;
 
+		/* abort if buffer ran out of room */
+		if (dlen > remaining)
+			return LZO_E_OUTPUT_OVERRUN;
+
 		/* decompress */
 		tmp = dlen;
 		r = lzo1x_decompress_safe((u8 *) src, slen, dst, &tmp);
@@ -105,6 +110,7 @@ int lzop_decompress(const unsigned char *src, size_t src_len,
 
 		src += slen;
 		dst += dlen;
+		remaining -= dlen;
 	}
 
 	return LZO_E_INPUT_OVERRUN;
-- 
1.7.9.5

[U-Boot] [PATCH 6/6] bootm: allow correct bounds-check of destination

[Kees Cook]

While nothing presently examines the destination size, it should at
least be correct so that future users of sys_mapmem() will not be
surprised. Without this, it might be possible to overflow memory.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Simon Glass <sjg@chromium.org>
---
 common/cmd_bootm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 046e22f..0f67112 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -368,7 +368,7 @@ static int bootm_load_os(bootm_headers_t *images, unsigned long *load_end,
 
 	const char *type_name = genimg_get_type_name(os.type);
 
-	load_buf = map_sysmem(load, image_len);
+	load_buf = map_sysmem(load, unc_len);
 	image_buf = map_sysmem(image_start, image_len);
 	switch (comp) {
 	case IH_COMP_NONE:
-- 
1.7.9.5

[U-Boot] [PATCH v2 0/6] handle compression buffer overflows

[Kees Cook]

Hi,

Can someone commit this series? It's been fully acked now...

Thanks,

-Kees

On Fri, Aug 16, 2013 at 7:59 AM, Kees Cook <keescook@chromium.org> wrote:
> v2: added acks, various suggested cleanups
>
> This series fixes gzip, lzma, and lzo to not overflow when writing
> to output buffers. Without this, it might be possible for untrusted
> compressed input to overflow the buffers used to hold the decompressed
> image.
>
> To catch these conditions, I added a series of compression tests available
> in the sandbox build. Without the fixes in patches 3, 4, and 5, the
> overflows are visible.
>
> Thanks,
>
> -Kees
>
> Kees Cook (6):
>       sandbox: add compression tests
>       documentation: add more compression configs
>       gzip: correctly bounds-check output buffer
>       lzma: correctly bounds-check output buffer
>       lzo: correctly bounds-check output buffer
>       bootm: allow correct bounds-check of destination
>
>  README                     |    9 ++
>  common/cmd_bootm.c         |    2 +-
>  include/configs/sandbox.h  |    5 +
>  lib/gunzip.c               |    4 +-
>  lib/lzma/LzmaTools.c       |    8 +-
>  lib/lzo/lzo1x_decompress.c |    8 +-
>  test/Makefile              |    1 +
>  test/compression.c         |  335 ++++++++++++++++++++++++++++++++++++++++++++
>  8 files changed, 366 insertions(+), 6 deletions(-)
>  create mode 100644 test/compression.c
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Kees Cook
Chrome OS Security

[U-Boot] [PATCH v2 0/6] handle compression buffer overflows

[Simon Glass]

Hi Kees,

On Wed, Aug 28, 2013 at 12:13 PM, Kees Cook <keescook@chromium.org> wrote:
> Hi,
>
> Can someone commit this series? It's been fully acked now...

I'm happy to pull these in for Tom.

I see a few warnings when I run buildman:

$ ./tools/buildman/buildman -b us-kees sandbox -se
Summary of 7 commits for 1 boards (1 thread, 32 jobs per thread)
01: omap5: Correct include order, drop CONFIG_SYS_PROMPT define
02: sandbox: add compression tests
   sandbox: +   sandbox
+cmd_bootm.c: In function ?bootm_load_os?:
+cmd_bootm.c:443:11: warning: passing argument 4 of ?lzop_decompress?
from incompatible pointer type [enabled by default]
+/usr/local/google/c/cosarm/src/third_party/u-boot/us-kees/.bm-work/00/include/linux/lzo.h:31:5:
note: expected ?size_t *? but argument is of type ?uint *?
+cmd_ximg.c: In function ?do_imgextract?:
+cmd_ximg.c:225:6: warning: cast to pointer from integer of different
size [-Wint-to-pointer-cast]
+cmd_ximg.c:225:14: warning: ?hdr? may be used uninitialized in this
function [-Wuninitialized]
03: documentation: add more compression configs
04: gzip: correctly bounds-check output buffer
05: lzma: correctly bounds-check output buffer
06: lzo: correctly bounds-check output buffer
07: bootm: allow correct bounds-check of destination

I believe these are pre-existing, but didn't happen for sandbox since
it was not enabling these options, but could you please create a patch
to fix these that we can apply first?

To build for sandbox: 'make sandbox_config; make'

Regards,
Simon

>
> Thanks,
>
> -Kees
>
> On Fri, Aug 16, 2013 at 7:59 AM, Kees Cook <keescook@chromium.org> wrote:
>> v2: added acks, various suggested cleanups
>>
>> This series fixes gzip, lzma, and lzo to not overflow when writing
>> to output buffers. Without this, it might be possible for untrusted
>> compressed input to overflow the buffers used to hold the decompressed
>> image.
>>
>> To catch these conditions, I added a series of compression tests available
>> in the sandbox build. Without the fixes in patches 3, 4, and 5, the
>> overflows are visible.
>>
>> Thanks,
>>
>> -Kees
>>
>> Kees Cook (6):
>>       sandbox: add compression tests
>>       documentation: add more compression configs
>>       gzip: correctly bounds-check output buffer
>>       lzma: correctly bounds-check output buffer
>>       lzo: correctly bounds-check output buffer
>>       bootm: allow correct bounds-check of destination
>>
>>  README                     |    9 ++
>>  common/cmd_bootm.c         |    2 +-
>>  include/configs/sandbox.h  |    5 +
>>  lib/gunzip.c               |    4 +-
>>  lib/lzma/LzmaTools.c       |    8 +-
>>  lib/lzo/lzo1x_decompress.c |    8 +-
>>  test/Makefile              |    1 +
>>  test/compression.c         |  335 ++++++++++++++++++++++++++++++++++++++++++++
>>  8 files changed, 366 insertions(+), 6 deletions(-)
>>  create mode 100644 test/compression.c
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
>
>
> --
> Kees Cook
> Chrome OS Security

[U-Boot] [PATCH 0/6] handle compression buffer overflows

[Kees Cook]

[sending, now subscribed so mailman won't yell at me]

This series fixes gzip, lzma, and lzo to not overflow when writing
to output buffers. Without this, it might be possible for untrusted
compressed input to overflow the buffers used to hold the decompressed
image.

To catch these conditions, I added a series of compression tests available
in the sandbox build. Without the fixes in patches 3, 4, and 5, the
overflows are visible.

Thanks,

-Kees

[U-Boot] [PATCH 3/6] gzip: correctly bounds-check output buffer

[Kees Cook]

The output buffer size not be reset by the gzip decoder or there is a
risk of overflowing memory during decompression.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/gunzip.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/gunzip.c b/lib/gunzip.c
index 99a8ab0..682a05f 100644
--- a/lib/gunzip.c
+++ b/lib/gunzip.c
@@ -105,13 +105,13 @@ int zunzip(void *dst, int dstlen, unsigned char *src, unsigned long *lenp,
 	s.avail_out = dstlen;
 	do {
 		r = inflate(&s, Z_FINISH);
-		if (r != Z_STREAM_END && r != Z_BUF_ERROR && stoponerr == 1) {
+		if (stoponerr == 1 && r != Z_STREAM_END &&
+		    (s.avail_out == 0 || r != Z_BUF_ERROR)) {
 			printf("Error: inflate() returned %d\n", r);
 			inflateEnd(&s);
 			return -1;
 		}
 		s.avail_in = *lenp - offset - (int)(s.next_out - (unsigned char*)dst);
-		s.avail_out = dstlen;
 	} while (r == Z_BUF_ERROR);
 	*lenp = s.next_out - (unsigned char *) dst;
 	inflateEnd(&s);
-- 
1.7.9.5

[U-Boot] [PATCH 3/6] gzip: correctly bounds-check output buffer

[Simon Glass]

On Mon, Aug 12, 2013 at 5:02 PM, Kees Cook <keescook@chromium.org> wrote:
> The output buffer size not be reset by the gzip decoder or there is a
> risk of overflowing memory during decompression.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Looks right to me.

Acked-by: Simon Glass <sjg@chromium.org>