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

login
register
mail settings
Submitter Kees Cook
Date Aug. 16, 2013, 2:59 p.m.
Message ID <1376665157-31268-4-git-send-email-keescook@chromium.org>
Download mbox | patch
Permalink /patch/267673/
State Accepted
Delegated to: Tom Rini
Headers show

Comments

Kees Cook - Aug. 16, 2013, 2:59 p.m.
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(-)
Michal Simek - Nov. 8, 2013, 12:04 p.m.
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@1' configuration
   Trying 'kernel@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@1' configuration
   Trying 'fdt@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 ...
Kees Cook - Nov. 8, 2013, 3:21 p.m.
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
Michal Simek - Nov. 8, 2013, 3:40 p.m.
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 - Nov. 8, 2013, 3:50 p.m.
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

Patch

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);