[PATCH] lib: zlib: Use post-increment only in inffast.c.

From: Chin Liang See chin.liang.see@intel.com
An old inffast.c optimization turns out to not be optimal anymore with modern compilers, and furthermore was not compliant with the C standard, for which decrementing a pointer before its allocated memory is undefined. Per the recommendation of a security audit of the zlib code by Trail of Bits and TrustInSoft, in support of the Mozilla Foundation, this "optimization" was removed, in order to avoid the possibility of undefined behavior.
Signed-off-by: Mark Adler madler@alumni.caltech.edu Signed-off-by: Chin Liang See chin.liang.see@intel.com Signed-off-by: Jit Loon Lim jit.loon.lim@intel.com --- lib/zlib/inffast.c | 87 ++++++++++++++++++---------------------------- 1 file changed, 34 insertions(+), 53 deletions(-)
diff --git a/lib/zlib/inffast.c b/lib/zlib/inffast.c index e3c7f3b892..a1d617a325 100644 --- a/lib/zlib/inffast.c +++ b/lib/zlib/inffast.c @@ -12,25 +12,6 @@
#ifndef ASMINF
-/* Allow machine dependent optimization for post-increment or pre-increment. - Based on testing to date, - Pre-increment preferred for: - - PowerPC G3 (Adler) - - MIPS R5000 (Randers-Pehrson) - Post-increment preferred for: - - none - No measurable difference: - - Pentium III (Anderson) - - M68060 (Nikl) - */ -#ifdef POSTINC -# define OFF 0 -# define PUP(a) *(a)++ -#else -# define OFF 1 -# define PUP(a) *++(a) -#endif - /* Decode literal, length, and distance codes and write out the resulting literal and match bytes until either not enough input or output is @@ -97,7 +78,7 @@ void inflate_fast(z_streamp strm, unsigned start)
/* copy state to local variables */ state = (struct inflate_state FAR *)strm->state; - in = strm->next_in - OFF; + in = strm->next_in; last = in + (strm->avail_in - 5); if (in > last && strm->avail_in > 5) { /* @@ -107,7 +88,7 @@ void inflate_fast(z_streamp strm, unsigned start) strm->avail_in = 0xffffffff - (uintptr_t)in; last = in + (strm->avail_in - 5); } - out = strm->next_out - OFF; + out = strm->next_out; beg = out - (start - strm->avail_out); end = out + (strm->avail_out - 257); #ifdef INFLATE_STRICT @@ -128,9 +109,9 @@ void inflate_fast(z_streamp strm, unsigned start) input data or output space */ do { if (bits < 15) { - hold += (unsigned long)(PUP(in)) << bits; + hold += (unsigned long)(*in++) << bits; bits += 8; - hold += (unsigned long)(PUP(in)) << bits; + hold += (unsigned long)(*in++) << bits; bits += 8; } this = lcode[hold & lmask]; @@ -143,14 +124,14 @@ void inflate_fast(z_streamp strm, unsigned start) Tracevv((stderr, this.val >= 0x20 && this.val < 0x7f ? "inflate: literal '%c'\n" : "inflate: literal 0x%02x\n", this.val)); - PUP(out) = (unsigned char)(this.val); + *out++ = (unsigned char)(this.val); } else if (op & 16) { /* length base */ len = (unsigned)(this.val); op &= 15; /* number of extra bits */ if (op) { if (bits < op) { - hold += (unsigned long)(PUP(in)) << bits; + hold += (unsigned long)(*in++) << bits; bits += 8; } len += (unsigned)hold & ((1U << op) - 1); @@ -159,9 +140,9 @@ void inflate_fast(z_streamp strm, unsigned start) } Tracevv((stderr, "inflate: length %u\n", len)); if (bits < 15) { - hold += (unsigned long)(PUP(in)) << bits; + hold += (unsigned long)(*in++) << bits; bits += 8; - hold += (unsigned long)(PUP(in)) << bits; + hold += (unsigned long)(*in++) << bits; bits += 8; } this = dcode[hold & dmask]; @@ -174,10 +155,10 @@ void inflate_fast(z_streamp strm, unsigned start) dist = (unsigned)(this.val); op &= 15; /* number of extra bits */ if (bits < op) { - hold += (unsigned long)(PUP(in)) << bits; + hold += (unsigned long)(*in++) << bits; bits += 8; if (bits < op) { - hold += (unsigned long)(PUP(in)) << bits; + hold += (unsigned long)(*in++) << bits; bits += 8; } } @@ -200,13 +181,13 @@ void inflate_fast(z_streamp strm, unsigned start) state->mode = BAD; break; } - from = window - OFF; + from = window; if (write == 0) { /* very common case */ from += wsize - op; if (op < len) { /* some from window */ len -= op; do { - PUP(out) = PUP(from); + *out++ = *from++; } while (--op); from = out - dist; /* rest from output */ } @@ -217,14 +198,14 @@ void inflate_fast(z_streamp strm, unsigned start) if (op < len) { /* some from end of window */ len -= op; do { - PUP(out) = PUP(from); + *out++ = *from++; } while (--op); - from = window - OFF; + from = window; if (write < len) { /* some from start of window */ op = write; len -= op; do { - PUP(out) = PUP(from); + *out++ = *from++; } while (--op); from = out - dist; /* rest from output */ } @@ -235,21 +216,21 @@ void inflate_fast(z_streamp strm, unsigned start) if (op < len) { /* some from window */ len -= op; do { - PUP(out) = PUP(from); + *out++ = *from++; } while (--op); from = out - dist; /* rest from output */ } } while (len > 2) { - PUP(out) = PUP(from); - PUP(out) = PUP(from); - PUP(out) = PUP(from); + *out++ = *from++; + *out++ = *from++; + *out++ = *from++; len -= 3; } if (len) { - PUP(out) = PUP(from); + *out++ = *from++; if (len > 1) - PUP(out) = PUP(from); + *out++ = *from++; } } else { @@ -259,25 +240,25 @@ void inflate_fast(z_streamp strm, unsigned start) from = out - dist; /* copy direct from output */ /* minimum length is three */ /* Align out addr */ - if (!((long)(out - 1 + OFF) & 1)) { - PUP(out) = PUP(from); + if (!((long)(out - 1) & 1)) { + *out++ = *from++; len--; } - sout = (unsigned short *)(out - OFF); + sout = (unsigned short *)(out); if (dist > 2 ) { unsigned short *sfrom;
- sfrom = (unsigned short *)(from - OFF); + sfrom = (unsigned short *)(from); loops = len >> 1; do - PUP(sout) = get_unaligned(++sfrom); + *sout++ = get_unaligned(++sfrom); while (--loops); - out = (unsigned char *)sout + OFF; - from = (unsigned char *)sfrom + OFF; + out = (unsigned char *)sout; + from = (unsigned char *)sfrom; } else { /* dist == 1 or dist == 2 */ unsigned short pat16;
- pat16 = *(sout-2+2*OFF); + pat16 = *(sout-2); if (dist == 1) #if defined(__BIG_ENDIAN) pat16 = (pat16 & 0xff) | ((pat16 & 0xff ) << 8); @@ -288,12 +269,12 @@ void inflate_fast(z_streamp strm, unsigned start) #endif loops = len >> 1; do - PUP(sout) = pat16; + *sout++ = pat16; while (--loops); - out = (unsigned char *)sout + OFF; + out = (unsigned char *)sout; } if (len & 1) - PUP(out) = PUP(from); + *out++ = *from++; } } else if ((op & 64) == 0) { /* 2nd level distance code */ @@ -329,8 +310,8 @@ void inflate_fast(z_streamp strm, unsigned start) hold &= (1U << bits) - 1;
/* update state and return */ - strm->next_in = in + OFF; - strm->next_out = out + OFF; + strm->next_in = in; + strm->next_out = out; strm->avail_in = (unsigned)(in < last ? 5 + (last - in) : 5 - (in - last)); strm->avail_out = (unsigned)(out < end ? 257 + (end - out) : 257 - (out - end));

On Sun, 11 Sept 2022 at 11:47, Jit Loon Lim jit.loon.lim@intel.com wrote:
From: Chin Liang See chin.liang.see@intel.com
An old inffast.c optimization turns out to not be optimal anymore with modern compilers, and furthermore was not compliant with the C standard, for which decrementing a pointer before its allocated memory is undefined. Per the recommendation of a security audit of the zlib code by Trail of Bits and TrustInSoft, in support of the Mozilla Foundation, this "optimization" was removed, in order to avoid the possibility of undefined behavior.
A similar change was merged into an official zlib in 2016: https://github.com/madler/zlib/commit/9aaec95e82117 It makes me wonder can zlib be used as an external project in U-Boot? To be up to date with zlib development.

On Sun, Sep 11, 2022 at 04:16:12PM +0300, Sergei Antonov wrote:
On Sun, 11 Sept 2022 at 11:47, Jit Loon Lim jit.loon.lim@intel.com wrote:
From: Chin Liang See chin.liang.see@intel.com
An old inffast.c optimization turns out to not be optimal anymore with modern compilers, and furthermore was not compliant with the C standard, for which decrementing a pointer before its allocated memory is undefined. Per the recommendation of a security audit of the zlib code by Trail of Bits and TrustInSoft, in support of the Mozilla Foundation, this "optimization" was removed, in order to avoid the possibility of undefined behavior.
A similar change was merged into an official zlib in 2016: https://github.com/madler/zlib/commit/9aaec95e82117
As this commit message is copy/pasted from zlib, we need to better reflect that this is a port of the above mentioned commit to U-Boot. We should also mention that this is a fix for CVE-2016-9841 which I assume is why someone within Intel found and made this change. Given 2e2e784de060 ("zlib: Port fix for CVE-2018-25032 to U-Boot") I wonder if there are any other fixes that need to be posted / addressed?
It makes me wonder can zlib be used as an external project in U-Boot? To be up to date with zlib development.
Given the work to port the code to our codebase, no, not directly. It's an infrequently changing enough code base that it would make more sense I think to either re-sync (and compare our changes vs 1.2.5, which is when we last did this) or check over the commit log between then and now for any relevant changes to pick up.

An old inffast.c optimization turns out to not be optimal anymore with modern compilers, and furthermore was not compliant with the C standard, for which decrementing a pointer before its allocated memory is undefined. Per the recommendation of a security audit of the zlib code by Trail of Bits and TrustInSoft, in support of the Mozilla Foundation, this "optimization" was removed, in order to avoid the possibility of undefined behavior.
A similar change was merged into an official zlib in 2016: https://github.com/madler/zlib/commit/9aaec95e82117
As this commit message is copy/pasted from zlib, we need to better reflect that this is a port of the above mentioned commit to U-Boot. We should also mention that this is a fix for CVE-2016-9841 which I assume is why someone within Intel found and made this change. Given 2e2e784de060 ("zlib: Port fix for CVE-2018-25032 to U-Boot") I wonder if there are any other fixes that need to be posted / addressed?
It makes me wonder can zlib be used as an external project in U-Boot? To be up to date with zlib development.
Given the work to port the code to our codebase, no, not directly. It's an infrequently changing enough code base that it would make more sense I think to either re-sync (and compare our changes vs 1.2.5, which is when we last did this) or check over the commit log between then and now for any relevant changes to pick up.
Probably makes some sense to rebase to the latest every so often as there's been some optimisations for architectures, which make be useful for speed, and bug fixes that have landed since then plus things like fixes for CVE-2022-37434 which I know we back port explicitly at times.
Peter

On Sun, Sep 11, 2022 at 04:46:37PM +0800, Jit Loon Lim wrote:
From: Chin Liang See chin.liang.see@intel.com
An old inffast.c optimization turns out to not be optimal anymore with modern compilers, and furthermore was not compliant with the C standard, for which decrementing a pointer before its allocated memory is undefined. Per the recommendation of a security audit of the zlib code by Trail of Bits and TrustInSoft, in support of the Mozilla Foundation, this "optimization" was removed, in order to avoid the possibility of undefined behavior.
This is a backport of commit 9aaec95e82117 from the upstream zlib project.
Signed-off-by: Mark Adler madler@alumni.caltech.edu Signed-off-by: Chin Liang See chin.liang.see@intel.com Signed-off-by: Jit Loon Lim jit.loon.lim@intel.com
I was about to apply this with a slightly updated commit message and then I discovered that this causes at least the following tests to fail in CI on sandbox: FAILED test/py/tests/test_fit.py::test_fit - AssertionError: Kernel not loaded FAILED test/py/tests/test_ut.py::test_ut[ut_dm_dm_test_video_bmp16] - ValueError: U-Boot exi... FAILED test/py/tests/test_ut.py::test_ut[ut_dm_dm_test_video_bmp24] - ValueError: U-Boot exi... FAILED test/py/tests/test_ut.py::test_ut[ut_dm_dm_test_video_bmp24_32] - ValueError: U-Boot ... FAILED test/py/tests/test_fs/test_squashfs/test_sqfs_load.py::test_sqfs_load - AssertionError FAILED test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py::test_sqfs_ls - AssertionError Given that the sqfs ones are saying the data is corrupt, I'm unsure if the problem is the code or the patch here.
participants (4)
-
Jit Loon Lim
-
Peter Robinson
-
Sergei Antonov
-
Tom Rini