From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755062AbbCaPWx (ORCPT <rfc822;w@1wt.eu>);
	Tue, 31 Mar 2015 11:22:53 -0400
Received: from mail.linuxfoundation.org ([140.211.169.12]:48905 "EHLO
	mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1755046AbbCaPWs (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 31 Mar 2015 11:22:48 -0400
Date: Tue, 31 Mar 2015 17:22:45 +0200
From: Greg KH <gregkh@linuxfoundation.org>
To: Krzysztof Kolasa <kkolasa@winsoft.pl>
Cc: dsterba@suse.cz, tom.yeon@windriver.com, linux-kernel@vger.kernel.org
Subject: Re: lz4: fix system halted at boot kernel x86_64 compressed lz4
Message-ID: <20150331152245.GB11455@kroah.com>
References: <55114A1D.7030508@winsoft.pl>
 <20150325004458.GA20767@suse.cz>
 <55125E1B.3090506@winsoft.pl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <55125E1B.3090506@winsoft.pl>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Mar 25, 2015 at 08:04:59AM +0100, Krzysztof Kolasa wrote:
> On 25.03.2015 01:44, David Sterba wrote:
> > On Tue, Mar 24, 2015 at 12:27:25PM +0100, Krzysztof Kolasa wrote:
> >> lz4: fix system halted at boot kernel x86_64 compressed lz4
> >>
> >> Decompression process ends with an error when loading kernel:
> >>
> >> Decoding failed
> >>  -- System halted
> > Serious regression detected ...
> >
> >> This condition is probably not needed ( from the last commit d5e7caf) :
> > The offending patch is on the way to stable trees, so it would be best
> > to postpone it for now.
> >
> >> if( ... ||
> >>     (op + COPYLENGTH) > oend)
> >>     goto _output_error
> >>
> >> macro LZ4_SECURE_COPY() tests op and does not copy any data
> >> when op exceeds the value, decompression process is continued.
> >>
> >> added by analogy security for the ref:
> >>
> >> if ((ref + COPYLENGTH) > oend...
> >>
> >> to lz4_uncompress_unknownoutputsize(...)
> > I did only a quick check, your analysis seems correct. Reviewing the lz4
> > patches is tedious as the kernel implementations do not match the
> > upstream one line-by-line besides that I've missed the side effects of
> > the macro.
> >
> Add patch source for review (send to LKML) :
> ---------------------
> 
> lz4: fix system halted at boot kernel x86_64 compressed lz4
> 
> Decompression process ends with an error when loading kernel:
> 
> Decoding failed
>  -- System halted
> 
> This condition is probably not needed ( from the last commit d5e7caf) :
> 
> if( ... ||
>     (op + COPYLENGTH) > oend)
>     goto _output_error
> 
> macro LZ4_SECURE_COPY() tests op and does not copy any data
> when op exceeds the value, decompression process is continued.
> 
> added by analogy security for the ref:
> 
> if ((ref + COPYLENGTH) > oend...
> 
> to lz4_uncompress_unknownoutputsize(...)
> 
> Signed-off-by: Krzysztof Kolasa <kkolasa@winsoft.pl>
> ---
>  lib/lz4/lz4_decompress.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/lz4/lz4_decompress.c b/lib/lz4/lz4_decompress.c
> index f0f5c5c..e248c4e 100644
> --- a/lib/lz4/lz4_decompress.c
> +++ b/lib/lz4/lz4_decompress.c
> @@ -139,8 +139,7 @@ static int lz4_uncompress(const char *source, char *dest, int osize)
>              /* Error: request to write beyond destination buffer */
>              if (cpy > oend)
>                  goto _output_error;
> -            if ((ref + COPYLENGTH) > oend ||
> -                    (op + COPYLENGTH) > oend)
> +            if ((ref + COPYLENGTH) > oend)
>                  goto _output_error;
>              LZ4_SECURECOPY(ref, op, (oend - COPYLENGTH));
>              while (op < cpy)
> @@ -270,6 +269,8 @@ static int lz4_uncompress_unknownoutputsize(const char *source, char *dest,
>          if (cpy > oend - COPYLENGTH) {
>              if (cpy > oend)
>                  goto _output_error; /* write outside of buf */
> +            if ((ref + COPYLENGTH) > oend)
> +                goto _output_error;
>  
>              LZ4_SECURECOPY(ref, op, (oend - COPYLENGTH));
>              while (op < cpy)
> -- 2.3.3.dirty

I'm confused, what is the problem here?  What went wrong with the
original patch that was posted, which is a mirror of what the lz4 code
looks like "upstream"?

Why make this change?  Does it need to go into 4.0-final, or should I
just revert the original patch?

confused,

greg k-h