| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 3 Apr 2015 13:33:54 +0200
From: Krzysztof Kolasa <kkolasa@...soft.pl>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: dsterba@...e.cz, tom.yeon@...driver.com,
linux-kernel@...r.kernel.org
Subject: Re: lz4: fix system halted at boot kernel x86_64 compressed lz4
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@...soft.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@...soft.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists