From: Stephen Warren <swarren@nvidia.com>
Date: Fri, 29 Jan 2016 20:26:58 +0000 (-0700)
Subject: gunzip: remove avail_in recalculation
X-Git-Url: http://git.dujemihanovic.xyz/?a=commitdiff_plain;h=1934665742e38c8ced9342a9c3005a3eab0b1a7c;p=u-boot.git

gunzip: remove avail_in recalculation

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
Acked-by: Kees Cook <keescook@chromium.org>

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@VirtualMetrix.com>
Cc: Kees Cook <keescook@chromium.org>
Fixes: f039ada5c109 ("Fix gunzip to work for any gziped uImage size")
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---

diff --git a/lib/gunzip.c b/lib/gunzip.c
index 80b157f99e..da0c76c500 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);