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

Greg KH
2015-03-31T15:22:53Z

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