On Fri, Jan 29, 2016 at 12:26 PM, 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>
Yeah, if s.avail_in is already being updated, the existing code makes no sense. :) Acked-by: Kees Cook <keesc...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot