| Message ID | 1376665157-31268-4-git-send-email-keescook@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
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 ...
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
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
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
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);