[PATCH] AM335x: MLO image issue

[PATCH] AM335x: MLO image issue

[Teresa Gámez]

Hello,

we have encountered an issue in the AM335x MLO, which seems to depend on the 
image size itself. The bootloader hangs up in the decompress_unlzo() function.
The decompress_unlzo() function fails with "dest len longer than block size"
and ends up in it's error function in an infinite loop.

As the error seems to depend in the image size, adding or removing random code
makes the image working again.

We have seen this issue so far only with a MLO size of 98823 bytes and
barebox.z size: 61003 bytes. But could reproduce it using different toolchains.

So we suspect this is an issue in the lzo code. 
Syncing the lzo code with the kernel code, seems to help.
For this we have attached a patch.
But we are unsure if this is the solution to the bug we have.

Any ideas on this?

Regards,
Teresa

Stefan Müller-Klieser (1):
  lib/lzo: port lzo fix from upstream kernel

 lib/lzo/lzo1x_decompress_safe.c | 105 ++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 47 deletions(-)

-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] lib/lzo: port lzo fix from upstream kernel

[Teresa Gámez]

From: Stefan Müller-Klieser <s.mueller-klieser@phytec.de>

This ports commit 72cf90124e87d975d0b from mainline kernel to the
barebox. We were experiencing reproducible boot hang ups on one am335x
board which depended on the barebox.z size. The MLO was trapped in the
decompression code. We found some bug reports around the lzo
decompressor in kernel and u-boot. With this version we cannot reproduce
the hang up any more.

Signed-off-by: Stefan Müller-Klieser <s.mueller-klieser@phytec.de>
---
 lib/lzo/lzo1x_decompress_safe.c | 105 ++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 47 deletions(-)

diff --git a/lib/lzo/lzo1x_decompress_safe.c b/lib/lzo/lzo1x_decompress_safe.c
index 48bedba..2c2ded1 100644
--- a/lib/lzo/lzo1x_decompress_safe.c
+++ b/lib/lzo/lzo1x_decompress_safe.c
@@ -16,31 +16,21 @@
 #include <lzo.h>
 #include "lzodefs.h"
 
-#define HAVE_IP(t, x)					\
-	(((size_t)(ip_end - ip) >= (size_t)(t + x)) &&	\
-	 (((t + x) >= t) && ((t + x) >= x)))
-
-#define HAVE_OP(t, x)					\
-	(((size_t)(op_end - op) >= (size_t)(t + x)) &&	\
-	 (((t + x) >= t) && ((t + x) >= x)))
-
-#define NEED_IP(t, x)					\
-	do {						\
-		if (!HAVE_IP(t, x))			\
-			goto input_overrun;		\
-	} while (0)
-
-#define NEED_OP(t, x)					\
-	do {						\
-		if (!HAVE_OP(t, x))			\
-			goto output_overrun;		\
-	} while (0)
-
-#define TEST_LB(m_pos)					\
-	do {						\
-		if ((m_pos) < out)			\
-			goto lookbehind_overrun;	\
-	} while (0)
+#define HAVE_IP(x)      ((size_t)(ip_end - ip) >= (size_t)(x))
+#define HAVE_OP(x)      ((size_t)(op_end - op) >= (size_t)(x))
+#define NEED_IP(x)      if (!HAVE_IP(x)) goto input_overrun
+#define NEED_OP(x)      if (!HAVE_OP(x)) goto output_overrun
+#define TEST_LB(m_pos)  if ((m_pos) < out) goto lookbehind_overrun
+
+/* This MAX_255_COUNT is the maximum number of times we can add 255 to a base
+ * count without overflowing an integer. The multiply will overflow when
+ * multiplying 255 by more than MAXINT/255. The sum will overflow earlier
+ * depending on the base count. Since the base count is taken from a u8
+ * and a few bits, it is safe to assume that it will always be lower than
+ * or equal to 2*255, thus we can always prevent any overflow by accepting
+ * two less 255 steps. See Documentation/lzo.txt for more information.
+ */
+#define MAX_255_COUNT      ((((size_t)~0) / 255) - 2)
 
 int lzo1x_decompress_safe(const unsigned char *in, size_t in_len,
 			  unsigned char *out, size_t *out_len)
@@ -72,17 +62,24 @@ int lzo1x_decompress_safe(const unsigned char *in, size_t in_len,
 		if (t < 16) {
 			if (likely(state == 0)) {
 				if (unlikely(t == 0)) {
+					size_t offset;
+					const unsigned char *ip_last = ip;
+
 					while (unlikely(*ip == 0)) {
-						t += 255;
 						ip++;
-						NEED_IP(1, 0);
+						NEED_IP(1);
 					}
-					t += 15 + *ip++;
+					offset = ip - ip_last;
+					if (unlikely(offset > MAX_255_COUNT))
+						return LZO_E_ERROR;
+
+					offset = (offset << 8) - offset;
+					t += offset + 15 + *ip++;
 				}
 				t += 3;
 copy_literal_run:
 #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
-				if (likely(HAVE_IP(t, 15) && HAVE_OP(t, 15))) {
+				if (likely(HAVE_IP(t + 15) && HAVE_OP(t + 15))) {
 					const unsigned char *ie = ip + t;
 					unsigned char *oe = op + t;
 					do {
@@ -98,8 +95,8 @@ copy_literal_run:
 				} else
 #endif
 				{
-					NEED_OP(t, 0);
-					NEED_IP(t, 3);
+					NEED_OP(t);
+					NEED_IP(t + 3);
 					do {
 						*op++ = *ip++;
 					} while (--t > 0);
@@ -112,7 +109,7 @@ copy_literal_run:
 				m_pos -= t >> 2;
 				m_pos -= *ip++ << 2;
 				TEST_LB(m_pos);
-				NEED_OP(2, 0);
+				NEED_OP(2);
 				op[0] = m_pos[0];
 				op[1] = m_pos[1];
 				op += 2;
@@ -133,13 +130,20 @@ copy_literal_run:
 		} else if (t >= 32) {
 			t = (t & 31) + (3 - 1);
 			if (unlikely(t == 2)) {
+				size_t offset;
+				const unsigned char *ip_last = ip;
+
 				while (unlikely(*ip == 0)) {
-					t += 255;
 					ip++;
-					NEED_IP(1, 0);
+					NEED_IP(1);
 				}
-				t += 31 + *ip++;
-				NEED_IP(2, 0);
+				offset = ip - ip_last;
+				if (unlikely(offset > MAX_255_COUNT))
+					return LZO_E_ERROR;
+
+				offset = (offset << 8) - offset;
+				t += offset + 31 + *ip++;
+				NEED_IP(2);
 			}
 			m_pos = op - 1;
 			next = get_unaligned_le16(ip);
@@ -151,13 +155,20 @@ copy_literal_run:
 			m_pos -= (t & 8) << 11;
 			t = (t & 7) + (3 - 1);
 			if (unlikely(t == 2)) {
+				size_t offset;
+				const unsigned char *ip_last = ip;
+
 				while (unlikely(*ip == 0)) {
-					t += 255;
 					ip++;
-					NEED_IP(1, 0);
+					NEED_IP(1);
 				}
-				t += 7 + *ip++;
-				NEED_IP(2, 0);
+				offset = ip - ip_last;
+				if (unlikely(offset > MAX_255_COUNT))
+					return LZO_E_ERROR;
+
+				offset = (offset << 8) - offset;
+				t += offset + 7 + *ip++;
+				NEED_IP(2);
 			}
 			next = get_unaligned_le16(ip);
 			ip += 2;
@@ -171,7 +182,7 @@ copy_literal_run:
 #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
 		if (op - m_pos >= 8) {
 			unsigned char *oe = op + t;
-			if (likely(HAVE_OP(t, 15))) {
+			if (likely(HAVE_OP(t + 15))) {
 				do {
 					COPY8(op, m_pos);
 					op += 8;
@@ -181,7 +192,7 @@ copy_literal_run:
 					m_pos += 8;
 				} while (op < oe);
 				op = oe;
-				if (HAVE_IP(6, 0)) {
+				if (HAVE_IP(6)) {
 					state = next;
 					COPY4(op, ip);
 					op += next;
@@ -189,7 +200,7 @@ copy_literal_run:
 					continue;
 				}
 			} else {
-				NEED_OP(t, 0);
+				NEED_OP(t);
 				do {
 					*op++ = *m_pos++;
 				} while (op < oe);
@@ -198,7 +209,7 @@ copy_literal_run:
 #endif
 		{
 			unsigned char *oe = op + t;
-			NEED_OP(t, 0);
+			NEED_OP(t);
 			op[0] = m_pos[0];
 			op[1] = m_pos[1];
 			op += 2;
@@ -211,15 +222,15 @@ match_next:
 		state = next;
 		t = next;
 #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
-		if (likely(HAVE_IP(6, 0) && HAVE_OP(4, 0))) {
+		if (likely(HAVE_IP(6) && HAVE_OP(4))) {
 			COPY4(op, ip);
 			op += t;
 			ip += t;
 		} else
 #endif
 		{
-			NEED_IP(t, 3);
-			NEED_OP(t, 0);
+			NEED_IP(t + 3);
+			NEED_OP(t);
 			while (t > 0) {
 				*op++ = *ip++;
 				t--;
-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] AM335x: MLO image issue

[Sascha Hauer]

On Mon, Mar 23, 2015 at 04:17:32PM +0100, Teresa Gámez wrote:
> Hello,
> 
> we have encountered an issue in the AM335x MLO, which seems to depend on the 
> image size itself. The bootloader hangs up in the decompress_unlzo() function.
> The decompress_unlzo() function fails with "dest len longer than block size"
> and ends up in it's error function in an infinite loop.
> 
> As the error seems to depend in the image size, adding or removing random code
> makes the image working again.
> 
> We have seen this issue so far only with a MLO size of 98823 bytes and
> barebox.z size: 61003 bytes. But could reproduce it using different toolchains.
> 
> So we suspect this is an issue in the lzo code. 
> Syncing the lzo code with the kernel code, seems to help.
> For this we have attached a patch.
> But we are unsure if this is the solution to the bug we have.
> 
> Any ideas on this?

I have no idea how lzo works. I could apply the patch though, as it only
contains an update to the latest kernel code. Of course it would feel
better if we knew what we are doing.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib/lzo: port lzo fix from upstream kernel

[Holger Schurig]

Teresa,

your commit text doesn't seem to be accurate:


> This ports commit 72cf90124e87d975d0b from mainline kernel to the

... and ...

> -#define HAVE_IP(t, x)                                  \
> -       (((size_t)(ip_end - ip) >= (size_t)(t + x)) &&  \
> -        (((t + x) >= t) && ((t + x) >= x)))

... doesn't match. Because the removal of HAVE_IP isn't in the patch
you mentioned, it's in af958a38a60c7ca3d8a39c918c1baa2ff7b6b233

I guess you combined those two kernel patches?

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] AM335x: MLO image issue

[Holger Schurig]

Sascha,

AFAIK he implemented two patches from Torvald's linux kernel git tree:

* af958a38a60c7ca3d8a39c918c1baa2ff7b6b233 removes a broken
check-for-integer-overflow method
* 72cf90124e87d975d0b2114d930808c58b4c05e4 add's a new
make-sure-integers-dont-overflow logic, which is also marked as "put
it into stable kernels"

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox