lz4: add overrun checks to lz4_uncompress_unknownoutputsize()

PATCH In review Greg KH 2 participants 4 messages Jul 3, 2014 – Jul 25, 2014 Stable branch

lore

Share & tools

lore

Greg KH

author maintainer

2014-07-03 23:06 UTC #0 lore raw

PATCH 1/1 lz4: add overrun checks to lz4_uncompress_unknownoutputsize()

From: Greg Kroah-Hartman <[email protected]>

Jan points out that I forgot to make the needed fixes to the lz4_uncompress_unknownoutputsize() function to mirror the changes done in lz4_decompress() with regards to potential pointer overflows.

The only in-kernel user of this function is the zram code, which only takes data from a valid compressed buffer that it made itself, so it's not a big issue. But due to external kernel modules using this function, it's better to be safe here.

Reported-by Jan Beulich <[email protected]>

Cc "Don A. Bailey" <[email protected]>

Cc stable <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

lib/lz4/lz4_decompress.c +5 -1

index b74da447e81e..7a85967060a5 100644
@@ -192,6 +192,8 @@ static int lz4_uncompress_unknownoutputsize(const char *source, char *dest,
 			int s = 255;
 			while ((ip < iend) && (s == 255)) {
 				s = *ip++;
+				if (unlikely(length > (size_t)(length + s)))
+					goto _output_error;
 				length += s;
 			}
 		}

@@ -232,6 +234,8 @@ static int lz4_uncompress_unknownoutputsize(const char *source, char *dest,
 		if (length == ML_MASK) {
 			while (ip < iend) {
 				int s = *ip++;
+				if (unlikely(length > (size_t)(length + s)))
+					goto _output_error;
 				length += s;
 				if (s == 255)
 					continue;

@@ -284,7 +288,7 @@ static int lz4_uncompress_unknownoutputsize(const char *source, char *dest,
 
 	/* write overflow error detected */
 _output_error:
-	return (int) (-(((char *) ip) - source));
+	return -1;
 }
 
 int lz4_decompress(const unsigned char *src, size_t *src_len,

Jan Beulich

2014-07-04 06:12 UTC #1 lore raw

Quoted 6 lines

Quoted 1 line
Quoted 1 line · On 04.07.14 at 01:11, <[email protected]> wrote:
From: Greg Kroah-Hartman <[email protected]>
Jan points out that I forgot to make the needed fixes to the lz4_uncompress_unknownoutputsize() function to mirror the changes done in lz4_decompress() with regards to potential pointer overflows.

Except that meanwhile Don agreed with my statement that neither this nor the two earlier patches really fix the issue. So rather than pushing this into 3.16 and stable trees, I wonder whether the two earlier ones shouldn't be reverted and then a clean and correct fix be applied.

Jan

Jan Beulich

2014-07-04 13:01 UTC #2 lore raw

In reply to Greg KH

Quoted 8 lines

Quoted 7 lines
Quoted 2 lines · Jan Beulich
Quoted 1 line · On 04.07.14 at 01:11, <[email protected]> wrote:
From: Greg Kroah-Hartman <[email protected]>
Jan points out that I forgot to make the needed fixes to the lz4_uncompress_unknownoutputsize() function to mirror the changes done in lz4_decompress() with regards to potential pointer overflows.

Quoted 5 lines

Except that meanwhile Don agreed with my statement that neither this nor the two earlier patches really fix the issue. So rather than pushing this into 3.16 and stable trees, I wonder whether the two earlier ones shouldn't be reverted and then a clean and correct fix be applied.

So here's a patch which I think adds the missing pieces.

Jan

lz4: check for underruns

While overruns are already being taken care of, underruns (resulting from overflows in the respective "op + length" (or similar) operations weren't. Fix this, allowing commits 4a3a990451, 4148c1f67a, and 206204a116 to be reverted (perhaps apart from the return value adjustments two of the three do).

Signed-off-by Jan Beulich <[email protected]>

---
The patch applies correctly with or without said reverts carried out.

lib/lz4/lz4_decompress.c +8 -0

@@ -89,6 +89,8 @@ static int lz4_uncompress(const char *so
 			ip += length;
 			break; /* EOF */
 		}
+		if (unlikely((unsigned long)cpy < (unsigned long)op))
+			goto _output_error;
 		LZ4_WILDCOPY(ip, op, cpy);
 		ip -= (op - cpy);
 		op = cpy;

@@ -147,6 +149,8 @@ static int lz4_uncompress(const char *so
 				goto _output_error;
 			continue;
 		}
+		if (unlikely((unsigned long)cpy < (unsigned long)op))
+			goto _output_error;
 		LZ4_SECURECOPY(ref, op, cpy);
 		op = cpy; /* correction */
 	}

@@ -209,6 +213,8 @@ static int lz4_uncompress_unknownoutputs
 			op += length;
 			break;/* Necessarily EOF, due to parsing restrictions */
 		}
+		if (unlikely((unsigned long)cpy < (unsigned long)op))
+			goto _output_error;
 		LZ4_WILDCOPY(ip, op, cpy);
 		ip -= (op - cpy);
 		op = cpy;

@@ -272,6 +278,8 @@ static int lz4_uncompress_unknownoutputs
 				goto _output_error;
 			continue;
 		}
+		if (unlikely((unsigned long)cpy < (unsigned long)op))
+			goto _output_error;
 		LZ4_SECURECOPY(ref, op, cpy);
 		op = cpy; /* correction */
 	}

Jan Beulich

2014-07-25 06:00 UTC #3 lore raw

In reply to Greg KH

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines · On 04.07.14 at 15:01, wrote:
Quoted 2 lines · Jan Beulich
Quoted 1 line · On 04.07.14 at 01:11, <[email protected]> wrote:
From: Greg Kroah-Hartman <[email protected]>
Jan points out that I forgot to make the needed fixes to the lz4_uncompress_unknownoutputsize() function to mirror the changes done in lz4_decompress() with regards to potential pointer overflows.

Quoted 7 lines

Quoted 5 lines
Except that meanwhile Don agreed with my statement that neither this nor the two earlier patches really fix the issue. So rather than pushing this into 3.16 and stable trees, I wonder whether the two earlier ones shouldn't be reverted and then a clean and correct fix be applied.
So here's a patch which I think adds the missing pieces.

There wasn't any feedback at all so far, which I'm irritated by. Did I do anything obviously wrong without noticing myself?

Jan

Quoted 53 lines

lz4: check for underruns

Signed-off-by Jan Beulich <[email protected]>

---
The patch applies correctly with or without said reverts carried out.

lib/lz4/lz4_decompress.c +8 -0

@@ -89,6 +89,8 @@ static int lz4_uncompress(const char *so
 			ip += length;
 			break; /* EOF */
 		}
+		if (unlikely((unsigned long)cpy < (unsigned long)op))
+			goto _output_error;
 		LZ4_WILDCOPY(ip, op, cpy);
 		ip -= (op - cpy);
 		op = cpy;

@@ -147,6 +149,8 @@ static int lz4_uncompress(const char *so
 				goto _output_error;
 			continue;
 		}
+		if (unlikely((unsigned long)cpy < (unsigned long)op))
+			goto _output_error;
 		LZ4_SECURECOPY(ref, op, cpy);
 		op = cpy; /* correction */
 	}

@@ -209,6 +213,8 @@ static int lz4_uncompress_unknownoutputs
 			op += length;
 			break;/* Necessarily EOF, due to parsing restrictions */
 		}
+		if (unlikely((unsigned long)cpy < (unsigned long)op))
+			goto _output_error;
 		LZ4_WILDCOPY(ip, op, cpy);
 		ip -= (op - cpy);
 		op = cpy;

@@ -272,6 +278,8 @@ static int lz4_uncompress_unknownoutputs
 				goto _output_error;
 			continue;
 		}
+		if (unlikely((unsigned long)cpy < (unsigned long)op))
+			goto _output_error;
 		LZ4_SECURECOPY(ref, op, cpy);
 		op = cpy; /* correction */
 	}