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

[Michal Simek <monstr@monstr.eu>]

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>