[PATCH] lz4: add overrun checks to lz4_uncompress_unknownoutputsize()

[PATCH] lz4: add overrun checks to lz4_uncompress_unknownoutputsize()

[Greg KH]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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 <JBeulich@suse.com>
Cc: "Don A. Bailey" <donb@securitymouse.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/lib/lz4/lz4_decompress.c b/lib/lz4/lz4_decompress.c
index b74da447e81e..7a85967060a5 100644
--- a/lib/lz4/lz4_decompress.c
+++ b/lib/lz4/lz4_decompress.c
@@ -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,

Re: [PATCH] lz4: add overrun checks to lz4_uncompress_unknownoutputsize()

[Jan Beulich]

>>> On 04.07.14 at 01:11, <gregkh@linuxfoundation.org> wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> 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

Re: [PATCH] lz4: add overrun checks to lz4_uncompress_unknownoutputsize()

[Jan Beulich]

>>> On 04.07.14 at 08:12, Jan Beulich wrote:
>>>> On 04.07.14 at 01:11, <gregkh@linuxfoundation.org> wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > 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.

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 <jbeulich@suse.com>
---
The patch applies correctly with or without said reverts carried out.

--- a/lib/lz4/lz4_decompress.c
+++ b/lib/lz4/lz4_decompress.c
@@ -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 */
 	}

Re: [PATCH] lz4: add overrun checks to lz4_uncompress_unknownoutputsize()

[Jan Beulich]

>>> On 04.07.14 at 15:01,  wrote:
>>>> On 04.07.14 at 08:12, Jan Beulich wrote:
> >>>> On 04.07.14 at 01:11, <gregkh@linuxfoundation.org> wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > 
> > > 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.
> 
> 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

> 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 <jbeulich@suse.com>
> ---
> The patch applies correctly with or without said reverts carried out.
> 
> --- a/lib/lz4/lz4_decompress.c
> +++ b/lib/lz4/lz4_decompress.c
> @@ -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 */
>  	}
> 
> 
>