Re: [PATCH] Revert "zlib: Port fix for CVE-2016-9841 to U-Boot"
On 6/27/24 17:49, Tom Rini wrote:
In commit 340fdf1303dc ("zlib: Port fix for CVE-2016-9841 to U-Boot")
Michal brings in (correctly) the upstream fix for CVE-2016-9841.
However, when upstream was fixing this issue they also removed a
necessary optimization for some CPU classes as part of simplifying the
code. This in turn leads to boot failures on the platforms as they now
take too long to decompress images and so the watchdog sees the system
as stuck.
The long term fix here is as Christophe has posted, which is to restore
the optimization. Given the nearness of the release, what I do here is
very similar, result wise, but less so, code wise. This is a revert of
Michal's commit _except_ we only allow for post-increment in the code,
thus keeping the CVE resolved. For the next release this commit shall be
reverted and then Christophe's patch applied.
Sorry I was out and sorry for problems. Good to see this patch.
I pretty much think that long term goal should be to use upstream zlib
and sync it up regularly.
Thanks,
Michal
Re: [PATCH] Revert "zlib: Port fix for CVE-2016-9841 to U-Boot"
Le 27/06/2024 à 17:49, Tom Rini a écrit :
In commit 340fdf1303dc ("zlib: Port fix for CVE-2016-9841 to U-Boot")
Michal brings in (correctly) the upstream fix for CVE-2016-9841.
However, when upstream was fixing this issue they also removed a
necessary optimization for some CPU classes as part of simplifying the
code. This in turn leads to boot failures on the platforms as they now
take too long to decompress images and so the watchdog sees the system
as stuck.
The long term fix here is as Christophe has posted, which is to restore
the optimization. Given the nearness of the release, what I do here is
very similar, result wise, but less so, code wise. This is a revert of
Michal's commit _except_ we only allow for post-increment in the code,
thus keeping the CVE resolved. For the next release this commit shall be
reverted and then Christophe's patch applied.
This largely reverts commit 340fdf1303dce7e5f53ddd981471836058ff23ef.
Reported-by: Christophe Leroy
Signed-off-by: Tom Rini
Tested-by: Christophe Leroy
--
Cc: Christophe Leroy
Cc: Michal Simek
---
lib/zlib/inffast.c | 117 +
1 file changed, 75 insertions(+), 42 deletions(-)
diff --git a/lib/zlib/inffast.c b/lib/zlib/inffast.c
index 5e2a65ad4d27..c271d85ea191 100644
--- a/lib/zlib/inffast.c
+++ b/lib/zlib/inffast.c
@@ -1,5 +1,5 @@
/* inffast.c -- fast decoding
- * Copyright (C) 1995-2008, 2010, 2013 Mark Adler
+ * Copyright (C) 1995-2004 Mark Adler
* For conditions of distribution and use, see copyright notice in zlib.h
*/
@@ -12,6 +12,12 @@
#ifndef ASMINF
+/*
+ * Only allow post-increment in order to resolve CVE-2016-9841
+ */
+# define OFF 0
+# define PUP(a) *(a)++
+
/*
Decode literal, length, and distance codes and write out the resulting
literal and match bytes until either not enough input or output is
@@ -47,13 +53,12 @@
requires strm->avail_out >= 258 for each loop to avoid checking for
output space.
*/
-void ZLIB_INTERNAL inflate_fast(strm, start)
-z_streamp strm;
-unsigned start; /* inflate()'s starting value for strm->avail_out */
+void inflate_fast(z_streamp strm, unsigned start)
+/* start: inflate()'s starting value for strm->avail_out */
{
struct inflate_state FAR *state;
-z_const unsigned char FAR *in; /* local strm->next_in */
-z_const unsigned char FAR *last;/* have enough input while in < last */
+unsigned char FAR *in; /* local strm->next_in */
+unsigned char FAR *last;/* while in < last, enough input available */
unsigned char FAR *out; /* local strm->next_out */
unsigned char FAR *beg; /* inflate()'s initial strm->next_out */
unsigned char FAR *end; /* while out < end, enough space available */
@@ -79,7 +84,7 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
/* copy state to local variables */
state = (struct inflate_state FAR *)strm->state;
-in = strm->next_in;
+in = strm->next_in - OFF;
last = in + (strm->avail_in - 5);
if (in > last && strm->avail_in > 5) {
/*
@@ -89,7 +94,7 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
strm->avail_in = 0x - (uintptr_t)in;
last = in + (strm->avail_in - 5);
}
-out = strm->next_out;
+out = strm->next_out - OFF;
beg = out - (start - strm->avail_out);
end = out + (strm->avail_out - 257);
#ifdef INFLATE_STRICT
@@ -110,9 +115,9 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
input data or output space */
do {
if (bits < 15) {
-hold += (unsigned long)(*in++) << bits;
+hold += (unsigned long)(PUP(in)) << bits;
bits += 8;
-hold += (unsigned long)(*in++) << bits;
+hold += (unsigned long)(PUP(in)) << bits;
bits += 8;
}
here = lcode[hold & lmask];
@@ -125,14 +130,14 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
Tracevv((stderr, here.val >= 0x20 && here.val < 0x7f ?
"inflate: literal '%c'\n" :
"inflate: literal 0x%02x\n", here.val));
-*out++ = (unsigned char)(here.val);
+PUP(out) = (unsigned char)(here.val);
}
else if (op & 16) { /* length base */
len = (unsigned)(here.val);
op &= 15; /* number of extra bits */
if (op) {
if (bits < op) {
-hold += (unsigned long)(*in++) << bits;
+hold += (unsigned long)(PUP(in)) << bits;
bits += 8;
}
len += (unsigned)hold & ((1U << op) - 1);
@@ -141,9 +146,9 @@ unsigned start; /* inflate()'s starting value for
[PATCH] Revert "zlib: Port fix for CVE-2016-9841 to U-Boot"
In commit 340fdf1303dc ("zlib: Port fix for CVE-2016-9841 to U-Boot")
Michal brings in (correctly) the upstream fix for CVE-2016-9841.
However, when upstream was fixing this issue they also removed a
necessary optimization for some CPU classes as part of simplifying the
code. This in turn leads to boot failures on the platforms as they now
take too long to decompress images and so the watchdog sees the system
as stuck.
The long term fix here is as Christophe has posted, which is to restore
the optimization. Given the nearness of the release, what I do here is
very similar, result wise, but less so, code wise. This is a revert of
Michal's commit _except_ we only allow for post-increment in the code,
thus keeping the CVE resolved. For the next release this commit shall be
reverted and then Christophe's patch applied.
This largely reverts commit 340fdf1303dce7e5f53ddd981471836058ff23ef.
Reported-by: Christophe Leroy
Signed-off-by: Tom Rini
--
Cc: Christophe Leroy
Cc: Michal Simek
---
lib/zlib/inffast.c | 117 +
1 file changed, 75 insertions(+), 42 deletions(-)
diff --git a/lib/zlib/inffast.c b/lib/zlib/inffast.c
index 5e2a65ad4d27..c271d85ea191 100644
--- a/lib/zlib/inffast.c
+++ b/lib/zlib/inffast.c
@@ -1,5 +1,5 @@
/* inffast.c -- fast decoding
- * Copyright (C) 1995-2008, 2010, 2013 Mark Adler
+ * Copyright (C) 1995-2004 Mark Adler
* For conditions of distribution and use, see copyright notice in zlib.h
*/
@@ -12,6 +12,12 @@
#ifndef ASMINF
+/*
+ * Only allow post-increment in order to resolve CVE-2016-9841
+ */
+# define OFF 0
+# define PUP(a) *(a)++
+
/*
Decode literal, length, and distance codes and write out the resulting
literal and match bytes until either not enough input or output is
@@ -47,13 +53,12 @@
requires strm->avail_out >= 258 for each loop to avoid checking for
output space.
*/
-void ZLIB_INTERNAL inflate_fast(strm, start)
-z_streamp strm;
-unsigned start; /* inflate()'s starting value for strm->avail_out */
+void inflate_fast(z_streamp strm, unsigned start)
+/* start: inflate()'s starting value for strm->avail_out */
{
struct inflate_state FAR *state;
-z_const unsigned char FAR *in; /* local strm->next_in */
-z_const unsigned char FAR *last;/* have enough input while in < last */
+unsigned char FAR *in; /* local strm->next_in */
+unsigned char FAR *last;/* while in < last, enough input available */
unsigned char FAR *out; /* local strm->next_out */
unsigned char FAR *beg; /* inflate()'s initial strm->next_out */
unsigned char FAR *end; /* while out < end, enough space available */
@@ -79,7 +84,7 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
/* copy state to local variables */
state = (struct inflate_state FAR *)strm->state;
-in = strm->next_in;
+in = strm->next_in - OFF;
last = in + (strm->avail_in - 5);
if (in > last && strm->avail_in > 5) {
/*
@@ -89,7 +94,7 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
strm->avail_in = 0x - (uintptr_t)in;
last = in + (strm->avail_in - 5);
}
-out = strm->next_out;
+out = strm->next_out - OFF;
beg = out - (start - strm->avail_out);
end = out + (strm->avail_out - 257);
#ifdef INFLATE_STRICT
@@ -110,9 +115,9 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
input data or output space */
do {
if (bits < 15) {
-hold += (unsigned long)(*in++) << bits;
+hold += (unsigned long)(PUP(in)) << bits;
bits += 8;
-hold += (unsigned long)(*in++) << bits;
+hold += (unsigned long)(PUP(in)) << bits;
bits += 8;
}
here = lcode[hold & lmask];
@@ -125,14 +130,14 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
Tracevv((stderr, here.val >= 0x20 && here.val < 0x7f ?
"inflate: literal '%c'\n" :
"inflate: literal 0x%02x\n", here.val));
-*out++ = (unsigned char)(here.val);
+PUP(out) = (unsigned char)(here.val);
}
else if (op & 16) { /* length base */
len = (unsigned)(here.val);
op &= 15; /* number of extra bits */
if (op) {
if (bits < op) {
-hold += (unsigned long)(*in++) << bits;
+hold += (unsigned long)(PUP(in)) << bits;
bits += 8;
}
len += (unsigned)hold & ((1U << op) - 1);
@@ -141,9 +146,9 @@ unsigned start; /* inflate()'s starting value for
strm->avail_out */
}
Tracevv((stderr, "inflate: length %u\n", len));
if (bits < 15) {
