On 15.05.2018 11:32, Qu Wenruo wrote:
>
>
> On 2018年05月15日 16:05, Nikolay Borisov wrote:
>>
>>
>> On 15.05.2018 10:36, Qu Wenruo wrote:
>>> Unlike zlib decompression, lzo decompression doesn't need any
>>> initialization, thus we can't detect early corruption from
>>> initialization.
>>>
>>> However for lzo compressed extent, its first 4bytes records the real
>>> unaligned compressed data size.
>>> We could use this as a clue, since any compressed extent should not
>>> exceed 128K, thus if we find such compressed data length, we are sure
>>> it's corrupted, then no need to continue decompression.
>>>
>>> Normally, such problem won't really bother anyone, as compression relies
>>> on dataCoW and data csum, which means normally such corruption should be
>>> detect by data csum before going into compression.
>>> However due to a bug in compression condition, it's possible to create
>>> compressed extent without csum.
>>>
>>> So we still need to do extra check for lzo just in case the compressed
>>> data is corrupted.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>>> ---
>>> lease note that, even with the binary dump of corrupted extent provided
>>> by the original reporter, James Harvey, I can only reproduce the "decompress
>>> failed" error message, but not the serious memory corruption followed.
>>> So there must be something missing, maybe we need to double check both
>>> btrfs lzo caller and kernel lzo lib.
>>>
>>> But anyway, making btrfs lzo compression a little more robust is never a
>>> bad thing.
>>> ---
>>> fs/btrfs/compression.h | 1 +
>>> fs/btrfs/lzo.c | 4 ++++
>>> 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
>>> index cc605f7b23fb..317703d9b073 100644
>>> --- a/fs/btrfs/compression.h
>>> +++ b/fs/btrfs/compression.h
>>> @@ -6,6 +6,7 @@
>>> #ifndef BTRFS_COMPRESSION_H
>>> #define BTRFS_COMPRESSION_H
>>>
>>> +#include <linux/sizes.h>
>>
>> Stray include otherwise:
>
> Surprisingly that's really needed.
>
> As in compression.h we uses SZ_*, while we didn't include that header.
> It's other *.c files get that header included first so no compiler error.
>
> However in this case, lzo.c is only including compression.h, no other
> headers including sizes.h, so it will cause compiler error.
>
> That's to fix it.
That's a separate change with a separate changelog
>
> Thanks,
> Qu
>
>>
>> Reviewed-by: Nikolay Borisov <nborisov@xxxxxxxx>
>>
>>> /*
>>> * We want to make sure that amount of RAM required to uncompress an extent is
>>> * reasonable, so we limit the total size in ram of a compressed extent to
>>> diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
>>> index 0667ea07f766..7ae2c0925770 100644
>>> --- a/fs/btrfs/lzo.c
>>> +++ b/fs/btrfs/lzo.c
>>> @@ -271,6 +271,10 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
>>>
>>> data_in = kmap(pages_in[0]);
>>> tot_len = read_compress_length(data_in);
>>> + if (tot_len > BTRFS_MAX_COMPRESSED) {
>>> + ret = -EIO;
>>> + goto done;
>>> + }
>>>
>>> tot_in = LZO_LEN;
>>> in_offset = LZO_LEN;
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
