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

[Kees Cook]

On Fri, Jan 29, 2016 at 12:26 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> From: Stephen Warren <swarren at 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 <Catalin at VirtualMetrix.com>
> Cc: Kees Cook <keescook at chromium.org>
> Fixes: f039ada5c109 ("Fix gunzip to work for any gziped uImage size")
> Signed-off-by: Stephen Warren <swarren at nvidia.com>

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

Acked-by: Kees Cook <keescook at 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(&s, 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(&s);
> --
> 2.7.0
>



-- 
Kees Cook
Chrome OS & Brillo Security