btrfs: lzo: Harden decompression callers to avoid

Since compression.h is using SZ_* macros, and if some user only includes compression.h without linux/sizes.h, it will cause compile error.

One example is lzo.c, if it uses BTRFS_MAX_COMPRESSED, it would cause compile error.

Fix it by adding linux/sizes.h in compression.h

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/compression.h | 1 +
 1 file changed, 1 insertion(+)

fs/btrfs/compression.h +1 -0

index cc605f7b23fb..317703d9b073 100644
@@ -6,6 +6,7 @@
 #ifndef BTRFS_COMPRESSION_H
 #define BTRFS_COMPRESSION_H
 
+#include <linux/sizes.h>
 /*
  * 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

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/lzo.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

fs/btrfs/lzo.c +0 -0

index 0667ea07f766..3d2ae4c08876 100644

fs/btrfs/lzo.c +12 -0

+ *
+ * 2.1 Segment header
+ *     Fixed size. LZO_LEN (4) bytes long, LE16.
+ *     Records the total size of the segment (*excludes* the header).
+ *
+ * 2.2 Data Payload
+ *     Variable size. Size up limit should be lzo1x_worst_compress(PAGE_SIZE).
+ *
+ * While for inlined LZO compressed data extent, it doesn't have Header, just
+ * one Segment.
 
 
 
+ */
+

struct workspace {

fs/btrfs/lzo.c +0 -0

 	void *mem;
 	void *buf;	/* where decompressed data goes */

Quoted 1 line · Qu Wenruo

Signed-off-by Qu Wenruo <[email protected]>

Overall it looks good and useful just a couple of nits below.

Quoted 9 lines

---
 fs/btrfs/lzo.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

fs/btrfs/lzo.c +0 -0

index 0667ea07f766..3d2ae4c08876 100644

fs/btrfs/lzo.c line 23

Quoted 7 lines

fs/btrfs/lzo.c +4 -0

@@ -17,6 +17,29 @@
 
 #define LZO_LEN	4
 
+/*
+ * Btrfs LZO compression format
+ *
+ * Regular LZO compressed data extent is consist of:

nit: s/is consist/consists

fs/btrfs/lzo.c line 30

Quoted 7 lines

fs/btrfs/lzo.c +7 -0

+ * 1.  Header
+ *     Fixed size. LZO_LEN (4) bytes long, LE16.
+ *     Records the total size (*includes* the header) of real compressed data.
+ *
+ * 2.  Segment(s)
+ *     Variable size. Includes one segment header, and then data payload.
+ *     One btrfs compressed data can have one or more segments.

So "one btrfs compressed data" should really mean "one btrfs compressed extent" I guess?

Quoted 18 lines

fs/btrfs/lzo.c +12 -0

+ *
+ * 2.1 Segment header
+ *     Fixed size. LZO_LEN (4) bytes long, LE16.
+ *     Records the total size of the segment (*excludes* the header).
+ *
+ * 2.2 Data Payload
+ *     Variable size. Size up limit should be lzo1x_worst_compress(PAGE_SIZE).
+ *
+ * While for inlined LZO compressed data extent, it doesn't have Header, just
+ * one Segment.
 
 
 
+ */
+

struct workspace {

fs/btrfs/lzo.c line 45

Quoted 2 lines

fs/btrfs/lzo.c +0 -0

 	void *mem;
 	void *buf;	/* where decompressed data goes */

--
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

James Harvey reported that some corrupted compressed extent data can lead to various kernel memory corruption.

Such corrupted extent data belongs to inode with NODATASUM flags, thus data csum won't help us detecting such bug.

If lucky enough, kasan could catch it like:
==================================================================

BUG: KASAN: slab-out-of-bounds in lzo_decompress_bio+0x384/0x7a0 [btrfs]
Write of size 4096 at addr ffff8800606cb0f8 by task kworker/u16:0/2338

CPU: 3 PID: 2338 Comm: kworker/u16:0 Tainted: G           O      4.17.0-rc5-custom+ #50
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
Workqueue: btrfs-endio btrfs_endio_helper [btrfs]
Call Trace:
 dump_stack+0xc2/0x16b
 print_address_description+0x6a/0x270
 kasan_report+0x260/0x380
 memcpy+0x34/0x50
 lzo_decompress_bio+0x384/0x7a0 [btrfs]
 end_compressed_bio_read+0x99f/0x10b0 [btrfs]
 bio_endio+0x32e/0x640
 normal_work_helper+0x15a/0xea0 [btrfs]
 process_one_work+0x7e3/0x1470
 worker_thread+0x1b0/0x1170
 kthread+0x2db/0x390
 ret_from_fork+0x22/0x40

...
==================================================================

The offending compressed data has the following info:

Header: length 32768 (Looks completely valid)
Segment 0 Header: length 3472882419 (Obvious out of bounds)

Then when handling segment 0, since it's over the current page, we need the compressed data to workspace, then such large size would trigger out-of-bounds memory access, screwing up the whole kernel.

Fix it by adding extra checks on header and segment headers to ensure we won't access out-of-bounds, and even checks the decompressed data won't be out-of-bounds.

Reported-by James Harvey <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/lzo.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

fs/btrfs/lzo.c +3 -0

index 3d2ae4c08876..78ebc809072f 100644
@@ -281,6 +281,7 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 	unsigned long working_bytes;
 	size_t in_len;
 	size_t out_len;
+	size_t max_segment_len = lzo1x_worst_compress(PAGE_SIZE);
 	unsigned long in_offset;
 	unsigned long in_page_bytes_left;
 	unsigned long tot_in;

@@ -294,6 +295,18 @@ 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);
+	/*
+	 * Compressed data header check.

fs/btrfs/lzo.c +21 -1

 	tot_in = LZO_LEN;
 	in_offset = LZO_LEN;

@@ -308,6 +321,17 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 		in_offset += LZO_LEN;
 		tot_in += LZO_LEN;
 
+		/*
+		 * Segment header check.
+		 *
+		 * The segment length must not exceed max lzo compression
+		 * size, nor the total compressed size
+		 */
+		if (in_len > max_segment_len || tot_in + in_len > tot_len) {
+			ret = -EUCLEAN;
+			goto done;
+		}
+
 		tot_in += in_len;
 		working_bytes = in_len;
 		may_late_unmap = need_unmap = false;

@@ -358,7 +382,7 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 			}
 		}
 
-		out_len = lzo1x_worst_compress(PAGE_SIZE);
+		out_len = max_segment_len;
 		ret = lzo1x_decompress_safe(buf, in_len, workspace->buf,
 					    &out_len);
 		if (need_unmap)

@@ -368,6 +392,15 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 			ret = -EIO;
 			break;
 		}
+		/*
+		 * Decompressed data length check.
+		 * The uncompressed data should not exceed uncompressed extent
+		 * size.
+		 */
+		if (tot_out + out_len > cb->len) {
+			ret = -EUCLEAN;
+			break;
+		}
 
 		buf_start = tot_out;
 		tot_out += out_len;

Quoted 74 lines · Qu Wenruo

James Harvey reported that some corrupted compressed extent data can lead to various kernel memory corruption.

Such corrupted extent data belongs to inode with NODATASUM flags, thus data csum won't help us detecting such bug.

If lucky enough, kasan could catch it like:
==================================================================

BUG: KASAN: slab-out-of-bounds in lzo_decompress_bio+0x384/0x7a0 [btrfs]
Write of size 4096 at addr ffff8800606cb0f8 by task kworker/u16:0/2338

CPU: 3 PID: 2338 Comm: kworker/u16:0 Tainted: G           O      4.17.0-rc5-custom+ #50
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
Workqueue: btrfs-endio btrfs_endio_helper [btrfs]
Call Trace:
 dump_stack+0xc2/0x16b
 print_address_description+0x6a/0x270
 kasan_report+0x260/0x380
 memcpy+0x34/0x50
 lzo_decompress_bio+0x384/0x7a0 [btrfs]
 end_compressed_bio_read+0x99f/0x10b0 [btrfs]
 bio_endio+0x32e/0x640
 normal_work_helper+0x15a/0xea0 [btrfs]
 process_one_work+0x7e3/0x1470
 worker_thread+0x1b0/0x1170
 kthread+0x2db/0x390
 ret_from_fork+0x22/0x40

...
==================================================================

The offending compressed data has the following info:

Header: length 32768 (Looks completely valid)
Segment 0 Header: length 3472882419 (Obvious out of bounds)

Then when handling segment 0, since it's over the current page, we need the compressed data to workspace, then such large size would trigger out-of-bounds memory access, screwing up the whole kernel.

Fix it by adding extra checks on header and segment headers to ensure we won't access out-of-bounds, and even checks the decompressed data won't be out-of-bounds.

Reported-by James Harvey <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/lzo.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

fs/btrfs/lzo.c +10 -0

index 3d2ae4c08876..78ebc809072f 100644
@@ -281,6 +281,7 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 	unsigned long working_bytes;
 	size_t in_len;
 	size_t out_len;
+	size_t max_segment_len = lzo1x_worst_compress(PAGE_SIZE);
 	unsigned long in_offset;
 	unsigned long in_page_bytes_left;
 	unsigned long tot_in;

@@ -294,6 +295,18 @@ 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);
+	/*
+	 * Compressed data header check.
+	 *
+	 * The real compressed size can't exceed extent length, and all pages
+	 * should be used (a full pending page is not possible).
+	 * If this happens it means the compressed extent is corrupted.
+	 */
+	if (tot_len > min_t(size_t, BTRFS_MAX_COMPRESSED, srclen) ||
+	    tot_len < srclen - PAGE_SIZE) {

fs/btrfs/lzo.c static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb) · line 309

Quoted 3 lines

fs/btrfs/lzo.c +3 -0

+		ret = -EUCLEAN;
+		goto done;
+	}

So tot_len is the compressed size as written in the compressed stream, whereas srclen is the number of bytes on-disk this compressed extent take up (as derived from submit_compressed_extents). Shouldn't those two always be equal, i.e perhaps an assert is in order?

srclen comes from the async_extent struct, which in turns is initialized in compress_file_range with the value of "total_compressed", and the value there is actually initialized by btrfs_compress_pages->lzo_compress_pages (that code makes me wanna sing "You spin me right round, baby Right round like a record, baby").

Quoted 47 lines

fs/btrfs/lzo.c +21 -1

 	tot_in = LZO_LEN;
 	in_offset = LZO_LEN;

@@ -308,6 +321,17 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 		in_offset += LZO_LEN;
 		tot_in += LZO_LEN;
 
+		/*
+		 * Segment header check.
+		 *
+		 * The segment length must not exceed max lzo compression
+		 * size, nor the total compressed size
+		 */
+		if (in_len > max_segment_len || tot_in + in_len > tot_len) {
+			ret = -EUCLEAN;
+			goto done;
+		}
+
 		tot_in += in_len;
 		working_bytes = in_len;
 		may_late_unmap = need_unmap = false;

@@ -358,7 +382,7 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 			}
 		}
 
-		out_len = lzo1x_worst_compress(PAGE_SIZE);
+		out_len = max_segment_len;
 		ret = lzo1x_decompress_safe(buf, in_len, workspace->buf,
 					    &out_len);
 		if (need_unmap)

@@ -368,6 +392,15 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 			ret = -EIO;
 			break;
 		}
+		/*
+		 * Decompressed data length check.
+		 * The uncompressed data should not exceed uncompressed extent
+		 * size.
+		 */
+		if (tot_out + out_len > cb->len) {
+			ret = -EUCLEAN;
+			break;
+		}
 
 		buf_start = tot_out;
 		tot_out += out_len;

Quoted 77 lines · Nikolay Borisov

Quoted 74 lines · Qu Wenruo

James Harvey reported that some corrupted compressed extent data can lead to various kernel memory corruption.

Such corrupted extent data belongs to inode with NODATASUM flags, thus data csum won't help us detecting such bug.

If lucky enough, kasan could catch it like:
==================================================================

BUG: KASAN: slab-out-of-bounds in lzo_decompress_bio+0x384/0x7a0 [btrfs]
Write of size 4096 at addr ffff8800606cb0f8 by task kworker/u16:0/2338

CPU: 3 PID: 2338 Comm: kworker/u16:0 Tainted: G           O      4.17.0-rc5-custom+ #50
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
Workqueue: btrfs-endio btrfs_endio_helper [btrfs]
Call Trace:
 dump_stack+0xc2/0x16b
 print_address_description+0x6a/0x270
 kasan_report+0x260/0x380
 memcpy+0x34/0x50
 lzo_decompress_bio+0x384/0x7a0 [btrfs]
 end_compressed_bio_read+0x99f/0x10b0 [btrfs]
 bio_endio+0x32e/0x640
 normal_work_helper+0x15a/0xea0 [btrfs]
 process_one_work+0x7e3/0x1470
 worker_thread+0x1b0/0x1170
 kthread+0x2db/0x390
 ret_from_fork+0x22/0x40

...
==================================================================

The offending compressed data has the following info:

Header: length 32768 (Looks completely valid)
Segment 0 Header: length 3472882419 (Obvious out of bounds)

Then when handling segment 0, since it's over the current page, we need the compressed data to workspace, then such large size would trigger out-of-bounds memory access, screwing up the whole kernel.

Fix it by adding extra checks on header and segment headers to ensure we won't access out-of-bounds, and even checks the decompressed data won't be out-of-bounds.

Reported-by James Harvey <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/lzo.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

fs/btrfs/lzo.c +10 -0

index 3d2ae4c08876..78ebc809072f 100644
@@ -281,6 +281,7 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 	unsigned long working_bytes;
 	size_t in_len;
 	size_t out_len;
+	size_t max_segment_len = lzo1x_worst_compress(PAGE_SIZE);
 	unsigned long in_offset;
 	unsigned long in_page_bytes_left;
 	unsigned long tot_in;

@@ -294,6 +295,18 @@ 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);
+	/*
+	 * Compressed data header check.
+	 *
+	 * The real compressed size can't exceed extent length, and all pages
+	 * should be used (a full pending page is not possible).
+	 * If this happens it means the compressed extent is corrupted.
+	 */
+	if (tot_len > min_t(size_t, BTRFS_MAX_COMPRESSED, srclen) ||
+	    tot_len < srclen - PAGE_SIZE) {

Quoted 8 lines

fs/btrfs/lzo.c static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb) · line 309

Quoted 3 lines

fs/btrfs/lzo.c +3 -0

+		ret = -EUCLEAN;
+		goto done;
+	}

So tot_len is the compressed size as written in the compressed stream, whereas srclen is the number of bytes on-disk this compressed extent take up (as derived from submit_compressed_extents). Shouldn't those two always be equal, i.e perhaps an assert is in order?

Nope, and in most case, tot_len is smaller than srclen (extent size).

The compressed data can have pending zeros, and in fact, for 8K all "0xcd" lzo compressed data, tot_len is 100 bytes, and the remaining bytes are all pending zeros.
While compressed extent size is always rounded up to sector size, and in that 8K all "0xcd" case, @srclen will be 4K.

Thanks,
Qu

Quoted 6 lines

srclen comes from the async_extent struct, which in turns is initialized in compress_file_range with the value of "total_compressed", and the value there is actually initialized by btrfs_compress_pages->lzo_compress_pages (that code makes me wanna sing "You spin me right round, baby Right round like a record, baby").

Quoted 46 lines

Quoted 46 lines

fs/btrfs/lzo.c +21 -1

 	tot_in = LZO_LEN;
 	in_offset = LZO_LEN;

@@ -308,6 +321,17 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 		in_offset += LZO_LEN;
 		tot_in += LZO_LEN;
 
+		/*
+		 * Segment header check.
+		 *
+		 * The segment length must not exceed max lzo compression
+		 * size, nor the total compressed size
+		 */
+		if (in_len > max_segment_len || tot_in + in_len > tot_len) {
+			ret = -EUCLEAN;
+			goto done;
+		}
+
 		tot_in += in_len;
 		working_bytes = in_len;
 		may_late_unmap = need_unmap = false;

@@ -358,7 +382,7 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 			}
 		}
 
-		out_len = lzo1x_worst_compress(PAGE_SIZE);
+		out_len = max_segment_len;
 		ret = lzo1x_decompress_safe(buf, in_len, workspace->buf,
 					    &out_len);
 		if (need_unmap)

@@ -368,6 +392,15 @@ static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 			ret = -EIO;
 			break;
 		}
+		/*
+		 * Decompressed data length check.
+		 * The uncompressed data should not exceed uncompressed extent
+		 * size.
+		 */
+		if (tot_out + out_len > cb->len) {
+			ret = -EUCLEAN;
+			break;
+		}
 

Quoted 3 lines

fs/btrfs/lzo.c static int lzo_decompress_bio(struct list_head *ws, struct compressed_bio *cb) · line 406

Quoted 3 lines

fs/btrfs/lzo.c +0 -0

 		buf_start = tot_out;
 		tot_out += out_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

srclen comes from the async_extent struct, which in turns is initialized in compress_file_range with the value of "total_compressed", and the value there is actually initialized by btrfs_compress_pages->lzo_compress_pages (that code makes me wanna sing "You spin me right round, baby Right round like a record, baby").