On 6 February 2016 at 13:29, Simon Glass <s...@chromium.org> wrote:
> On 29 January 2016 at 13:26, Stephen Warren <swar...@wwwdotorg.org> wrote:
>> From: Stephen Warren <swar...@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 <cata...@virtualmetrix.com>
>> Cc: Kees Cook <keesc...@chromium.org>
>> Fixes: f039ada5c109 ("Fix gunzip to work for any gziped uImage size")
>> Signed-off-by: Stephen Warren <swar...@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 <s...@chromium.org>
Applied to u-boot-dm, thanks!
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
