Re: lz4: fix system halted at boot kernel x86_64 compressed lz4

[Greg KH <gregkh@linuxfoundation.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