Re: [Xen-devel] [PATCH] lz4: check for underruns

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Mon, 2014-07-28 at 10:32 +0100, Jan Beulich wrote:
> While overruns are already being taken care of, underruns (resulting
> from overflows in the respective "op + length" (or similar) operations
> weren't.
> 
> This is CVE-2014-4611.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Note that while a similar patch was proposed for Linux as replacement
> for the three ones not really fixing the issue, there was no response
> to them within over three weeks. Rather than continuing to wait in
> order to not have our code needlessly diverge from the Linux original,
> aim at fixing the issue in our tree now with the option of replacing
> this fix with a Linux upstream blessed one later on.
> 
> --- a/xen/common/lz4/decompress.c
> +++ b/xen/common/lz4/decompress.c
> @@ -84,6 +84,8 @@ static int INIT lz4_uncompress(const uns
>                       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;
> @@ -142,6 +144,8 @@ static int INIT lz4_uncompress(const uns
>                               goto _output_error;
>                       continue;
>               }
> +             if (unlikely((unsigned long)cpy < (unsigned long)op))
> +                     goto _output_error;
>               LZ4_SECURECOPY(ref, op, cpy);

SECURE_COPY already includes the check but without the error handling
bit, just continuing under those circumstances is a bit odd, but your
change isn't incorrect because of it.

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel