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

[Krzysztof Kolasa <kkolasa@winsoft.pl>]

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:

[PATCH] lz4: fix system halted at boot kernel x86_64 compressed lz4

Decompression process ends with an error when loading 64bit lz4 kernel:

Decoding failed
 
    -- System halted

This condition is not needed for 64bit kernel ( 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 to lz4_uncompress_unknownoutputsize(...)

Signed-off-by: Krzysztof Kolasa <kkolasa@winsoft.pl>
---
 lib/lz4/lz4_decompress.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/lz4/lz4_decompress.c b/lib/lz4/lz4_decompress.c
index f0f5c5c..8a742b1 100644
--- a/lib/lz4/lz4_decompress.c
+++ b/lib/lz4/lz4_decompress.c
@@ -139,8 +139,12 @@ 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 LZ4_ARCH64
+            if ((ref + COPYLENGTH) > oend)
+#else
             if ((ref + COPYLENGTH) > oend ||
                     (op + COPYLENGTH) > oend)
+#endif
                 goto _output_error;
             LZ4_SECURECOPY(ref, op, (oend - COPYLENGTH));
             while (op < cpy)
@@ -270,7 +274,13 @@ 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 LZ4_ARCH64
+            if ((ref + COPYLENGTH) > oend)
+#else
+            if ((ref + COPYLENGTH) > oend ||
+                    (op + COPYLENGTH) > oend)
+#endif
+                goto _output_error;
             LZ4_SECURECOPY(ref, op, (oend - COPYLENGTH));
             while (op < cpy)
                 *op++ = *ref++;
-- 
2.4.0.rc0.dirty