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

[Simon Glass]

On 29 January 2016 at 13:26, 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>
> ---
> 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(-)

Acked-by: Simon Glass <sjg at chromium.org>