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

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Apr 03, 2015 at 01:33:54PM +0200, Krzysztof Kolasa wrote:
> On 31.03.2015 17:22, Greg KH wrote:
> > 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
> x86_64 lz4 kernel halted system...
> > 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
> >
> OK, after further tests have modified the previous patch, please check and analyze:

What "previous patch"?  None of my questions were answered here, so I
have no idea what is going on at all.

I'm going to have to just revert the original patch as obviously
something is wrong, but no one will tell me what, so I'll just go back
to the old behavior...

thanks,

greg k-h