Re: [U-Boot] [PATCH] gunzip: remove avail_in recalculation

2016-02-03 Thread Kees Cook 
On Fri, Jan 29, 2016 at 12:26 PM, Stephen Warren <swar...@wwwdotorg.org> wrote:
> From: Stephen Warren <swar...@nvidia.com>
>
> Current, the following passes:
> ./u-boot -d arch/sandbox/dts/test.dtb -c 'ut_image_decomp'
> but the following fails:
> ./u-boot -d arch/sandbox/dts/test.dtb -c 'ut dm; ut_image_decomp'
>
> This is because the gunzip code reads input data beyond the end of its
> input buffer. In the first case above, this data just happens to be 0,
> which just happens to trigger gzip to signal the error the decompression
> unit test expects. In the second case above, the "ut dm" test has written
> data to the accidentally-read memory, which causes the gzip code to take a
> different path and so return a different value, which triggers the test
> failure.
>
> The cause of gunzip reading past its input buffer is the re-calculation of
> s.avail_in in zunzip(), since it can underflow. Not only is the formula
> non-sensical (it uses the delta between two output buffer pointers to
> calculate available input buffer size), it also appears to be unnecessary,
> since the gunzip code already maintains this value itself. This patch
> removes this re-calculation to avoid the underflow and redundant work.
>
> The loop exit condition is also adjusted so that if inflate() has consumed
> the entire input buffer, without indicating returning Z_STREAM_END (i.e.
> decompression complete without error), an error is raised. There is still
> opportunity to simplify the code here by splitting up the loop exit
> condition into separate tests. However, this patch makes the minimum
> modifications required to solve the problem at hand, in order to keep the
> diff simple.
>
> I am not entirely convinced that the loop in zunzip() is necessary at all.
> It could only be useful if inflate() can return Z_BUF_ERROR (which
> typically means that it needs more data in the input buffer, or more space
> in the output buffer), even though Z_FINISH is set /and/ the full input is
> available in the input buffer /and/ there is enough space to store the
> decompressed output in the output buffer. The comment in zlib.h after the
> prototype of inflate() implies this is never the case. However, I assume
> there must have been some reason for introducing this loop in the first
> place, as part of commit "Fix gunzip to work for any gziped uImage size".
>
> This patch is similar to the earlier b75650d84d4b "gzip: correctly
> bounds-check output buffer", which corrected a similar issue for
> s.avail_out.
>
> Cc: Catalin Radu <cata...@virtualmetrix.com>
> Cc: Kees Cook <keesc...@chromium.org>
> Fixes: f039ada5c109 ("Fix gunzip to work for any gziped uImage size")
> Signed-off-by: Stephen Warren <swar...@nvidia.com>

Yeah, if s.avail_in is already being updated, the existing code makes
no sense. :)

Acked-by: Kees Cook <keesc...@chromium.org>

-Kees

> ---
> Simon, this patch may be a dependency for any updated version of patch
> "test/py: run C-based unit tests", depending on when/whether we enable
> ut_image_decomp in that test. So, it might make sense to apply this to
> the dm tree directly, or ensure that it's included in an upstream tree
> that the dm tree is rebased upon before applying the test/py patch.
>
>  lib/gunzip.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/lib/gunzip.c b/lib/gunzip.c
> index 80b157f99eb4..da0c76c500d1 100644
> --- a/lib/gunzip.c
> +++ b/lib/gunzip.c
> @@ -286,12 +286,11 @@ int zunzip(void *dst, int dstlen, unsigned char *src, 
> unsigned long *lenp,
> do {
> r = inflate(, Z_FINISH);
> if (stoponerr == 1 && r != Z_STREAM_END &&
> -   (s.avail_out == 0 || r != Z_BUF_ERROR)) {
> +   (s.avail_in == 0 || s.avail_out == 0 || r != 
> Z_BUF_ERROR)) {
> printf("Error: inflate() returned %d\n", r);
> err = -1;
> break;
> }
> -   s.avail_in = *lenp - offset - (int)(s.next_out - (unsigned 
> char*)dst);
> } while (r == Z_BUF_ERROR);
> *lenp = s.next_out - (unsigned char *) dst;
> inflateEnd();
> --
> 2.7.0
>



-- 
Kees Cook
Chrome OS & Brillo Security
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Strange CFI flash problem

2014-04-15 Thread Kees Cook 
On Mon, Apr 14, 2014 at 10:48 PM, Matthias Weißer weiss...@arcor.de wrote:
 Am 14.04.2014 17:38, schrieb Kees Cook:

 On Mon, Apr 14, 2014 at 1:51 AM, Matthias Weißer weiss...@arcor.de
 wrote:

 Am 14.04.2014 08:09, schrieb Matthias Weißer:

 Hi Wolfgang

 Am 11.04.2014 12:43, schrieb Wolfgang Denk:


 Dear Matthias,

 In message 5347bbbc.9000...@arcor.de you wrote:



 we are currently trying to get an out-of-tree board based on 2013.01
 back in sync with current master and observing a strange behavior
 which
 we think is located in the CFI flash system. If we load an image via
 tftp, copy it to flash and then try to run the image via bootm we see
 an
 error while decomressing:


 ...


 Uncompressing Kernel Image ... LZO: uncompress or overwrite error
 -5



 Are you sure your malloc arena is big enough for LZO?  Try if
 increasing it helps...



 We increaded it from 4MB to 8MB and the behavior is still the same.

 We also observed a different behavior when tftping the image to RAM and
 then directly executing it without copying it to flash. It seems that
 the flash device (EN29GL256H) is then in some a mode (maybe auto-select)
 which prevents it from normal read operations which doesn't allow the
 flash driver of the OS come up. We never saw this with our old u-boot.
 If there are no ideas left we will have to bisect the problem.



 Bisecting was successfull. The commit introducing the problem is

 commit ff9d2efdbf1b3b5263f81e843c6724b8bead7f1f
 Author: Kees Cook keesc...@chromium.org
 Date:   Fri Aug 16 07:59:15 2013 -0700

 lzo: correctly bounds-check output buffer

 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 keesc...@chromium.org
 Acked-by: Simon Glass s...@chromium.org

 This commit introduced the usage of the dst_len output parameter as
 additional input parameter containing the maximum output buffer size.
 This
 parameter isn't initialized in cmd_bootm.c:

  454 #ifdef CONFIG_LZO
  455 case IH_COMP_LZO: {
  456 size_t size;
  457
  458 printf(   Uncompressing %s ... , type_name);
  459
  460 ret = lzop_decompress(image_buf, image_len, load_buf,
 size);

 Setting size to some big value (SZE_MAX is not avialable) fixed the
 behavior
 but I am unsure if this is the correct solution. I think its hard to get
 the
 max output buffer size at this point in cmd_bootm.c.


 Does this work?


 Yes. Didn't saw that configuration option. Thanks.


 ---
 From: Kees Cook keesc...@chromium.org
 Subject: [PATCH] bootm: set max output for LZO

 The LZO decompressor wasn't initializing the maximum output size.

 Reported-by: Matthias Weißer weiss...@arcor.de
 Signed-off-by: Kees Cook keesc...@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 9751edc..c243a5b 100644
 --- a/common/cmd_bootm.c
 +++ b/common/cmd_bootm.c
 @@ -453,7 +453,7 @@ static int bootm_load_os(bootm_headers_t *images,
 unsigned long *load_end,
   #endif /* CONFIG_LZMA */
   #ifdef CONFIG_LZO
  case IH_COMP_LZO: {
 -   size_t size;
 +   size_t size = unc_len;

  printf(   Uncompressing %s ... , type_name);


 Tested-by: Matthias Weißer weiss...@arcor.de

Great! Thanks for testing.

Who can commit this patch? LZO is pretty busted at the moment without it. :)

-Kees

-- 
Kees Cook
Chrome OS Security
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] bootm: set max decompression size for LZO

2014-04-15 Thread Kees Cook 
The LZO decompressor wasn't initializing the maximum output size, which
meant it would fail to decompress most of the time.

Reported-by: Matthias Weißer weiss...@arcor.de
Signed-off-by: Kees Cook keesc...@chromium.org
Tested-by: Matthias Weißer weiss...@arcor.de
---
 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 9751edc..c243a5b 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -453,7 +453,7 @@ static int bootm_load_os(bootm_headers_t *images, unsigned 
long *load_end,
 #endif /* CONFIG_LZMA */
 #ifdef CONFIG_LZO
case IH_COMP_LZO: {
-   size_t size;
+   size_t size = unc_len;
 
printf(   Uncompressing %s ... , type_name);
 
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Strange CFI flash problem

2014-04-15 Thread Kees Cook 
On Tue, Apr 15, 2014 at 10:27 AM, Kees Cook keesc...@chromium.org wrote:
 On Mon, Apr 14, 2014 at 10:48 PM, Matthias Weißer weiss...@arcor.de wrote:
 Am 14.04.2014 17:38, schrieb Kees Cook:

 On Mon, Apr 14, 2014 at 1:51 AM, Matthias Weißer weiss...@arcor.de
 wrote:

 Am 14.04.2014 08:09, schrieb Matthias Weißer:

 Hi Wolfgang

 Am 11.04.2014 12:43, schrieb Wolfgang Denk:


 Dear Matthias,

 In message 5347bbbc.9000...@arcor.de you wrote:



 we are currently trying to get an out-of-tree board based on 2013.01
 back in sync with current master and observing a strange behavior
 which
 we think is located in the CFI flash system. If we load an image via
 tftp, copy it to flash and then try to run the image via bootm we see
 an
 error while decomressing:


 ...


 Uncompressing Kernel Image ... LZO: uncompress or overwrite error
 -5



 Are you sure your malloc arena is big enough for LZO?  Try if
 increasing it helps...



 We increaded it from 4MB to 8MB and the behavior is still the same.

 We also observed a different behavior when tftping the image to RAM and
 then directly executing it without copying it to flash. It seems that
 the flash device (EN29GL256H) is then in some a mode (maybe auto-select)
 which prevents it from normal read operations which doesn't allow the
 flash driver of the OS come up. We never saw this with our old u-boot.
 If there are no ideas left we will have to bisect the problem.



 Bisecting was successfull. The commit introducing the problem is

 commit ff9d2efdbf1b3b5263f81e843c6724b8bead7f1f
 Author: Kees Cook keesc...@chromium.org
 Date:   Fri Aug 16 07:59:15 2013 -0700

 lzo: correctly bounds-check output buffer

 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 keesc...@chromium.org
 Acked-by: Simon Glass s...@chromium.org

 This commit introduced the usage of the dst_len output parameter as
 additional input parameter containing the maximum output buffer size.
 This
 parameter isn't initialized in cmd_bootm.c:

  454 #ifdef CONFIG_LZO
  455 case IH_COMP_LZO: {
  456 size_t size;
  457
  458 printf(   Uncompressing %s ... , type_name);
  459
  460 ret = lzop_decompress(image_buf, image_len, load_buf,
 size);

 Setting size to some big value (SZE_MAX is not avialable) fixed the
 behavior
 but I am unsure if this is the correct solution. I think its hard to get
 the
 max output buffer size at this point in cmd_bootm.c.


 Does this work?


 Yes. Didn't saw that configuration option. Thanks.


 ---
 From: Kees Cook keesc...@chromium.org
 Subject: [PATCH] bootm: set max output for LZO

 The LZO decompressor wasn't initializing the maximum output size.

 Reported-by: Matthias Weißer weiss...@arcor.de
 Signed-off-by: Kees Cook keesc...@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 9751edc..c243a5b 100644
 --- a/common/cmd_bootm.c
 +++ b/common/cmd_bootm.c
 @@ -453,7 +453,7 @@ static int bootm_load_os(bootm_headers_t *images,
 unsigned long *load_end,
   #endif /* CONFIG_LZMA */
   #ifdef CONFIG_LZO
  case IH_COMP_LZO: {
 -   size_t size;
 +   size_t size = unc_len;

  printf(   Uncompressing %s ... , type_name);


 Tested-by: Matthias Weißer weiss...@arcor.de

 Great! Thanks for testing.

 Who can commit this patch? LZO is pretty busted at the moment without it. :)

Actually, since this is on -users, I'll resend the patch to the devel list...

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] Strange CFI flash problem

2014-04-14 Thread Kees Cook 
On Mon, Apr 14, 2014 at 1:51 AM, Matthias Weißer weiss...@arcor.de wrote:
 Am 14.04.2014 08:09, schrieb Matthias Weißer:

 Hi Wolfgang

 Am 11.04.2014 12:43, schrieb Wolfgang Denk:

 Dear Matthias,

 In message 5347bbbc.9000...@arcor.de you wrote:


 we are currently trying to get an out-of-tree board based on 2013.01
 back in sync with current master and observing a strange behavior which
 we think is located in the CFI flash system. If we load an image via
 tftp, copy it to flash and then try to run the image via bootm we see an
 error while decomressing:

 ...

 Uncompressing Kernel Image ... LZO: uncompress or overwrite error -5


 Are you sure your malloc arena is big enough for LZO?  Try if
 increasing it helps...


 We increaded it from 4MB to 8MB and the behavior is still the same.

 We also observed a different behavior when tftping the image to RAM and
 then directly executing it without copying it to flash. It seems that
 the flash device (EN29GL256H) is then in some a mode (maybe auto-select)
 which prevents it from normal read operations which doesn't allow the
 flash driver of the OS come up. We never saw this with our old u-boot.
 If there are no ideas left we will have to bisect the problem.


 Bisecting was successfull. The commit introducing the problem is

 commit ff9d2efdbf1b3b5263f81e843c6724b8bead7f1f
 Author: Kees Cook keesc...@chromium.org
 Date:   Fri Aug 16 07:59:15 2013 -0700

 lzo: correctly bounds-check output buffer

 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 keesc...@chromium.org
 Acked-by: Simon Glass s...@chromium.org

 This commit introduced the usage of the dst_len output parameter as
 additional input parameter containing the maximum output buffer size. This
 parameter isn't initialized in cmd_bootm.c:

  454 #ifdef CONFIG_LZO
  455 case IH_COMP_LZO: {
  456 size_t size;
  457
  458 printf(   Uncompressing %s ... , type_name);
  459
  460 ret = lzop_decompress(image_buf, image_len, load_buf, size);

 Setting size to some big value (SZE_MAX is not avialable) fixed the behavior
 but I am unsure if this is the correct solution. I think its hard to get the
 max output buffer size at this point in cmd_bootm.c.

Does this work?

---
From: Kees Cook keesc...@chromium.org
Subject: [PATCH] bootm: set max output for LZO

The LZO decompressor wasn't initializing the maximum output size.

Reported-by: Matthias Weißer weiss...@arcor.de
Signed-off-by: Kees Cook keesc...@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 9751edc..c243a5b 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -453,7 +453,7 @@ static int bootm_load_os(bootm_headers_t *images,
unsigned long *load_end,
 #endif /* CONFIG_LZMA */
 #ifdef CONFIG_LZO
case IH_COMP_LZO: {
-   size_t size;
+   size_t size = unc_len;

printf(   Uncompressing %s ... , type_name);

-- 
1.7.9.5



-- 
Kees Cook
Chrome OS Security
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-11-08 Thread Kees Cook 
On Fri, Nov 8, 2013 at 4:04 AM, Michal Simek mon...@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 keesc...@chromium.org
 Acked-by: Simon Glass s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] sandbox: Correct compiler warnings in cmd_bootm/cmd_ximg

2013-08-30 Thread Kees Cook 
On Fri, Aug 30, 2013 at 10:00 AM, Simon Glass s...@chromium.org wrote:
 Correct the following warnings found with sandbox when compression
 is enabled.

 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/files/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]

 Signed-off-by: Simon Glass s...@chromium.org

Acked-by: Kees Cook keesc...@chromium.org

Thanks! You beat me to it. :)

-Kees

 ---
  common/cmd_bootm.c | 10 ++
  common/cmd_ximg.c  |  5 +++--
  2 files changed, 9 insertions(+), 6 deletions(-)

 diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
 index 1685c14..2dd2642 100644
 --- a/common/cmd_bootm.c
 +++ b/common/cmd_bootm.c
 @@ -436,11 +436,12 @@ static int bootm_load_os(bootm_headers_t *images, 
 unsigned long *load_end,
 }
  #endif /* CONFIG_LZMA */
  #ifdef CONFIG_LZO
 -   case IH_COMP_LZO:
 +   case IH_COMP_LZO: {
 +   size_t size;
 +
 printf(   Uncompressing %s ... , type_name);

 -   ret = lzop_decompress(image_buf, image_len, load_buf,
 - unc_len);
 +   ret = lzop_decompress(image_buf, image_len, load_buf, size);
 if (ret != LZO_E_OK) {
 printf(LZO: uncompress or overwrite error %d 
   - must RESET board to recover\n, ret);
 @@ -449,8 +450,9 @@ static int bootm_load_os(bootm_headers_t *images, 
 unsigned long *load_end,
 return BOOTM_ERR_RESET;
 }

 -   *load_end = load + unc_len;
 +   *load_end = load + size;
 break;
 +   }
  #endif /* CONFIG_LZO */
 default:
 printf(Unimplemented compression type %d\n, comp);
 diff --git a/common/cmd_ximg.c b/common/cmd_ximg.c
 index b439be3..65a8319 100644
 --- a/common/cmd_ximg.c
 +++ b/common/cmd_ximg.c
 @@ -20,6 +20,7 @@
  #include bzlib.h
  #endif
  #include asm/byteorder.h
 +#include asm/io.h

  #ifndef CONFIG_SYS_XIMG_LEN
  /* use 8MByte as default max gunzip size */
 @@ -34,7 +35,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * 
 const argv[])
 ulong   data, len, count;
 int verify;
 int part = 0;
 -   image_header_t  *hdr;
 +   image_header_t  *hdr = NULL;
  #if defined(CONFIG_FIT)
 const char  *uname = NULL;
 const void* fit_hdr;
 @@ -222,7 +223,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char 
 * const argv[])
  * which requires at most 2300 KB of memory.
  */
 i = BZ2_bzBuffToBuffDecompress(
 -   (char *)ntohl(hdr-ih_load),
 +   map_sysmem(ntohl(hdr-ih_load), 0),
 unc_len, (char *)data, len,
 CONFIG_SYS_MALLOC_LEN  (4096 * 1024),
 0);
 --
 1.8.4




-- 
Kees Cook
Chrome OS Security
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-28 Thread 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 keesc...@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@lists.denx.de
 http://lists.denx.de/mailman/listinfo/u-boot



-- 
Kees Cook
Chrome OS Security
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-16 Thread Kees Cook 
On Thu, Aug 15, 2013 at 10:19 AM, Kees Cook keesc...@chromium.org wrote:
 On Wed, Aug 14, 2013 at 10:30 AM, Simon Glass s...@chromium.org wrote:
 On Mon, Aug 12, 2013 at 4:48 PM, Kees Cook keesc...@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 keesc...@chromium.org
 ---
  include/configs/sandbox.h |5 +
  test/Makefile |1 +
  test/compression.c|  384 
 +
  3 files changed, 390 insertions(+)
  create mode 100644 test/compression.c

[snip]
 diff --git a/test/compression.c b/test/compression.c
 new file mode 100644
 index 000..c78c8e4
 --- /dev/null
 +++ b/test/compression.c
[snip[
 +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)

 debug() here?

 Sure, I'll add calls at these failure points.

Ah, I take that back. These tests get hit during normal testing. I'd
rather leave the debug() calls out since they're expected to be
exercised.

-Kees

-- 
Kees Cook
Chrome OS Security
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-16 Thread 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 keesc...@chromium.org
Acked-by: Simon Glass s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-16 Thread 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 keesc...@chromium.org
Acked-by: Simon Glass s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-16 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-16 Thread 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 keesc...@chromium.org
Acked-by: Simon Glass s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-16 Thread Kees Cook 
This adds the missing compression config items to the README.

Signed-off-by: Kees Cook keesc...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-16 Thread 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 keesc...@chromium.org
Acked-by: Simon Glass s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-16 Thread 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 keesc...@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 000..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

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

2013-08-15 Thread Kees Cook 
On Wed, Aug 14, 2013 at 10:30 AM, Simon Glass s...@chromium.org wrote:
 Hi Kees,

 On Mon, Aug 12, 2013 at 4:48 PM, Kees Cook keesc...@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 keesc...@chromium.org
 ---
  include/configs/sandbox.h |5 +
  test/Makefile |1 +
  test/compression.c|  384 
 +
  3 files changed, 390 insertions(+)
  create mode 100644 test/compression.c

 diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
 index 98dd083..b7fe14d 100644
 --- a/include/configs/sandbox.h
 +++ b/include/configs/sandbox.h
 @@ -125,4 +125,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 83594f3..ede113d 100644
 --- a/test/Makefile
 +++ b/test/Makefile
 @@ -25,6 +25,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 000..c78c8e4
 --- /dev/null
 +++ b/test/compression.c
 @@ -0,0 +1,384 @@
 +/*
 + * Copyright (c) 2013, The Chromium Authors
 + *
 + * See file CREDITS for list of people who contributed to this
 + * project.
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 + * MA 02111-1307 USA

 I believe we are moving to a new license structure, although I'm not
 sure how this affects new patches. See Licenses/README.

That file doesn't exist, and I don't see anything named *icense* in
the tree? I must be looking in the wrong place?

 + */
 +
 +#define DEBUG
 +
 +#include common.h
 +#include command.h
 +#include malloc.h
 +
 +#include u-boot/zlib.h
 +#include bzlib.h
 +
 +#ifdef CONFIG_LZMA
 +#include lzma/LzmaTypes.h
 +#include lzma/LzmaDec.h
 +#include lzma/LzmaTools.h
 +#endif /* CONFIG_LZMA */
 +
 +#ifdef CONFIG_LZO
 +#include linux/lzo.h
 +#endif /* CONFIG_LZO */

 You shouldn't need these #ifdefs.

I do, and the other user (common/cmd_bootm.c) uses them too.

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

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

2013-08-15 Thread Kees Cook 
On Thu, Aug 15, 2013 at 12:32 PM, Simon Glass s...@chromium.org wrote:
 Hi Kees,

 On Thu, Aug 15, 2013 at 11:19 AM, Kees Cook keesc...@chromium.org wrote:
 On Wed, Aug 14, 2013 at 10:30 AM, Simon Glass s...@chromium.org wrote:
 Hi Kees,

 On Mon, Aug 12, 2013 at 4:48 PM, Kees Cook keesc...@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 keesc...@chromium.org
 ---
  include/configs/sandbox.h |5 +
  test/Makefile |1 +
  test/compression.c|  384 
 +
  3 files changed, 390 insertions(+)
  create mode 100644 test/compression.c

 diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
 index 98dd083..b7fe14d 100644
 --- a/include/configs/sandbox.h
 +++ b/include/configs/sandbox.h
 @@ -125,4 +125,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 83594f3..ede113d 100644
 --- a/test/Makefile
 +++ b/test/Makefile
 @@ -25,6 +25,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 000..c78c8e4
 --- /dev/null
 +++ b/test/compression.c
 @@ -0,0 +1,384 @@
 +/*
 + * Copyright (c) 2013, The Chromium Authors
 + *
 + * See file CREDITS for list of people who contributed to this
 + * project.
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 + * MA 02111-1307 USA

 I believe we are moving to a new license structure, although I'm not
 sure how this affects new patches. See Licenses/README.

 That file doesn't exist, and I don't see anything named *icense* in
 the tree? I must be looking in the wrong place?

 It updated fairly recently, but the tree is
 http://git.denx.de/u-boot.git, branch master.

Ah, I was behind. :) Fixed now!



 + */
 +
 +#define DEBUG
 +
 +#include common.h
 +#include command.h
 +#include malloc.h
 +
 +#include u-boot/zlib.h
 +#include bzlib.h
 +
 +#ifdef CONFIG_LZMA
 +#include lzma/LzmaTypes.h
 +#include lzma/LzmaDec.h
 +#include lzma/LzmaTools.h
 +#endif /* CONFIG_LZMA */
 +
 +#ifdef CONFIG_LZO
 +#include linux/lzo.h
 +#endif /* CONFIG_LZO */

 You shouldn't need these #ifdefs.

 I do, and the other user (common/cmd_bootm.c) uses them too.

 I tried taking them out and it still builds. Why do you need them?

Well, it would blow up it the #defines were removed from the sandbox
config. But I guess if this is only ever built in the sandbox, then
you're right, it's redundant. I'll drop them all.


 [snip]

 +static int do_test_compression(cmd_tbl_t *cmdtp, int flag, int argc,
 +  char * const argv[])
 +{
 +   int err = 0;
 +#ifdef CONFIG_GZIP_COMPRESSED
 +   err += run_test(gzip, compress_using_gzip, 
 uncompress_using_gzip);
 +#endif

 I notice that this prints a message from the function - is that correct?

  testing gzip ...
 orig_size:350
 compressed_size:206
 uncompressed_size:350
 Deflate need more space to compress left 350 bytes

 ^^^ message here

 compress does not overrun
 Error: inflate() returned -5
 uncompress does not overrun
  gzip: ok

 Both Deflate need more space to compress left 350 bytes and Error:
 inflate() returned -5 come from the respective compression libraries.
 I opted to leave those things unchanged. Should they be eliminated?

 I don't think you need to worry about it - I was only checking that it
 was correct. Sounds like it is.

Okay, cool.

 Thanks for the review!

 You're welcome, and thanks for fixing this and especially for adding tests!


 -Kees

 --
 Kees Cook
 Chrome OS Security

 Regards,
 Simon

v2 on its way... let's see if mailing tries to bounce them all again. :)

-Kees

-- 
Kees Cook
Chrome OS Security
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-12 Thread Kees Cook 
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-12 Thread Kees Cook 
This adds the missing compression config items to the README.

Signed-off-by: Kees Cook keesc...@chromium.org
---
 README |9 +
 1 file changed, 9 insertions(+)

diff --git a/README b/README
index 5c343da..247b8f3 100644
--- a/README
+++ b/README
@@ -1669,6 +1669,10 @@ CBFS (Coreboot Filesystem) support
to compress the specified memory at its best effort.
 
 - Compression support:
+   CONFIG_GZIP
+
+   Enabled by default for gzip compressed images.
+
CONFIG_BZIP2
 
If this option is set, support for bzip2 compressed
@@ -1702,6 +1706,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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-12 Thread 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 keesc...@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 28a8aef..2459fbe 100644
--- a/lib/lzma/LzmaTools.c
+++ b/lib/lzma/LzmaTools.c
@@ -113,15 +113,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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-12 Thread 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 keesc...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-12 Thread 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 keesc...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 6/6] bootm: correctly bounds-check decompression

2013-08-12 Thread Kees Cook 
This passes the actual memory allocation size for the destination to the
decompression routines, avoiding potential memory overflows.

Signed-off-by: Kees Cook keesc...@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 ba0bcd4..ac4fad1 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -362,7 +362,7 @@ static int bootm_load_os(image_info_t os, ulong *load_end, 
int boot_progress)
 
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-08-12 Thread 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 keesc...@chromium.org
---
 include/configs/sandbox.h |5 +
 test/Makefile |1 +
 test/compression.c|  384 +
 3 files changed, 390 insertions(+)
 create mode 100644 test/compression.c

diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 98dd083..b7fe14d 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -125,4 +125,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 83594f3..ede113d 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -25,6 +25,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 000..c78c8e4
--- /dev/null
+++ b/test/compression.c
@@ -0,0 +1,384 @@
+/*
+ * Copyright (c) 2013, The Chromium Authors
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#define DEBUG
+
+#include common.h
+#include command.h
+#include malloc.h
+
+#include u-boot/zlib.h
+#include bzlib.h
+
+#ifdef CONFIG_LZMA
+#include lzma/LzmaTypes.h
+#include lzma/LzmaDec.h
+#include lzma/LzmaTools.h
+#endif /* CONFIG_LZMA */
+
+#ifdef CONFIG_LZO
+#include linux/lzo.h
+#endif /* CONFIG_LZO */
+
+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

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

2013-08-12 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot