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 <[email protected]> >>> --- >>> 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 <[email protected]> >> >>> /* >>> * 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 [email protected] >> 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 [email protected] > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
