[PATCH 0/2] btrfs-progs: fix a heap-overflow bug in raid56 reconstruction

[PATCH 0/2] btrfs-progs: fix a heap-overflow bug in raid56 reconstruction

[Qu Wenruo]

During my local tests on a kernel patchset of falling back to
RAID1/RAID1C3 for single-data-RAID56, it turns out that "btrfs check
--check-data-csum" can report a lot of false csum mismatch.

It turns out that there is heap-overflow bug where we can get garbage
into the IO buffer for RAID56 reconstruction read.

Fix it and add a regression test case.

Qu Wenruo (2):
  btrfs-progs: fix raid56 reconstruction read
  btrfs-progs: fsck-tests: add a new test case for the raid56 read fix

 Makefile                                 |  2 +-
 kernel-shared/extent_io.c                | 14 +++++++++++-
 tests/fsck-tests/074-raid56-read/test.sh | 27 ++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100755 tests/fsck-tests/074-raid56-read/test.sh

--
2.54.0

[PATCH 1/2] btrfs-progs: fix raid56 reconstruction read

[Qu Wenruo]

[BUG]
The following script can easily lead to false alerts from
"btrfs check --check-data-csum":

	mkfs.btrfs -f -d raid5 $dev1 $dev2 > /dev/null
	mount $dev1 $mnt
	$fsstress -w -n 64 -d $mnt
	umount $mnt
	btrfs check --check-data-csum $dev1

Which will normally report a lot of csum mismatch on mirror 2:

 Opening filesystem to check...
 Checking filesystem on /dev/test/scratch1
 UUID: e742b7b6-2562-48e5-b411-d50975633045
 [1/8] checking log skipped (none written)
 [2/8] checking root items
 [3/8] checking extents
 [4/8] checking free space tree
 [5/8] checking fs roots
 [6/8] checking csums against data
 mirror 2 bytenr 298909696 csum 0xd800f135 expected csum 0x7f6b1842
 mirror 2 bytenr 298913792 csum 0x1f49b645 expected csum 0x7f6b1842
 ...
 mirror 2 bytenr 1372917760 csum 0x16f78e6b expected csum 0x9462d728
 mirror 2 bytenr 1372921856 csum 0xa972667e expected csum 0x3bea937f
 ERROR: errors found in csum tree
 [7/8] checking root refs
 [8/8] checking quota groups skipped (not enabled on this FS)
 found 2736128 bytes used, error(s) found

And if asan is enabled, it will detect a heap-over-flow:

[6/8] checking csums against data
=================================================================
==6512==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7e982fa2c800 at pc 0x7f883111e4da bp 0x7ffc69e8c9a0 sp 0x7ffc69e8c148
READ of size 65536 at 0x7e982fa2c800 thread T0
    #0 0x7f883111e4d9 in memcpy /usr/src/debug/gcc/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors_memintrinsics.inc:115
    #1 0x560b9f250c4a in read_raid56 kernel-shared/extent_io.c:413
    #2 0x560b9f25100a in read_data_from_disk kernel-shared/extent_io.c:443
    #3 0x560b9f3faa12 in check_extent_csums check/main.c:5964
    #4 0x560b9f3fcaee in check_csum_root check/main.c:6235
    #5 0x560b9f3fcef6 in check_csums check/main.c:6276
    #6 0x560b9f41d168 in cmd_check check/main.c:11082
    #7 0x560b9f1f04fb in cmd_execute cmds/commands.h:126
    #8 0x560b9f1f1cc8 in main /home/adam/btrfs-progs/btrfs.c:474
    #9 0x7f8830c27634  (/usr/lib/libc.so.6+0x27634) (BuildId: 2f722da304c0a508c891285e6840199c35019c8d)
    #10 0x7f8830c276e8 in __libc_start_main (/usr/lib/libc.so.6+0x276e8) (BuildId: 2f722da304c0a508c891285e6840199c35019c8d)
    #11 0x560b9f1f03d4 in _start (/home/adam/btrfs-progs/btrfs+0x883d4) (BuildId: 2fde8cac91b9fec1b45e1b5145ee594466b76396)

0x7e982fa2c800 is located 0 bytes after 65536-byte region [0x7e982fa1c800,0x7e982fa2c800)
allocated by thread T0 here:
    #0 0x7f8831120cb5 in malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:67
    #1 0x560b9f250874 in read_raid56 kernel-shared/extent_io.c:348
    #2 0x560b9f25100a in read_data_from_disk kernel-shared/extent_io.c:443
    #3 0x560b9f3faa12 in check_extent_csums check/main.c:5964
    #4 0x560b9f3fcaee in check_csum_root check/main.c:6235
    #5 0x560b9f3fcef6 in check_csums check/main.c:6276
    #6 0x560b9f41d168 in cmd_check check/main.c:11082
    #7 0x560b9f1f04fb in cmd_execute cmds/commands.h:126
    #8 0x560b9f1f1cc8 in main /home/adam/btrfs-progs/btrfs.c:474
    #9 0x7f8830c27634  (/usr/lib/libc.so.6+0x27634) (BuildId: 2f722da304c0a508c891285e6840199c35019c8d)
    #10 0x7f8830c276e8 in __libc_start_main (/usr/lib/libc.so.6+0x276e8) (BuildId: 2f722da304c0a508c891285e6840199c35019c8d)
    #11 0x560b9f1f03d4 in _start (/home/adam/btrfs-progs/btrfs+0x883d4) (BuildId: 2fde8cac91b9fec1b45e1b5145ee594466b76396)

SUMMARY: AddressSanitizer: heap-buffer-overflow kernel-shared/extent_io.c:413 in read_raid56
Shadow bytes around the buggy address:
  0x7e982fa2c580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7e982fa2c600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7e982fa2c680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7e982fa2c700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7e982fa2c780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x7e982fa2c800:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x7e982fa2c880: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x7e982fa2c900: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x7e982fa2c980: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x7e982fa2ca00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x7e982fa2ca80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):

[CAUSE]
During read of mirrors other than mirror 0/1 from RAID56 profiles,
btrfs-progs go through the following call chain:

 read_data_from_disk()
 |- btrfs_map_block()
 |  Note that @read_len returned by btrfs_map_block() is always
 |  BTRFS_STRIPE_LEN.
 |
 \- read_raid56()
    | Which does read out all stripes, but there is no extra checks
    | against if the logical range crosses stripe boundaries.
    |
    |- raid56_recov()
    |  Which rebuilds the target stripe.
    |
    |- memcpy(buf, pointers[failed_a] + (logical - full_stripe_start) %
              BTRFS_STRIPE_LEN, len);

If @logical is not at the begining of a stripe, then @logical + @len
will go beyond the stripe buffer size, causing gargage into the target
buffer.

[FIX]
Calculate the correct read length inside the data stripe for RAID56
inside read_data_from_disk().

And add extra ASSERT()s inside read_raid56().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/extent_io.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel-shared/extent_io.c b/kernel-shared/extent_io.c
index cb48699c2e46..ea26d9e6c94b 100644
--- a/kernel-shared/extent_io.c
+++ b/kernel-shared/extent_io.c
@@ -324,6 +324,8 @@ static int read_raid56(struct btrfs_fs_info *fs_info, void *buf, u64 logical,
 	const int tolerance = (multi->type & BTRFS_RAID_RAID6 ? 2 : 1);
 	const int num_stripes = multi->num_stripes;
 	const u64 full_stripe_start = raid_map[0];
+	const u64 stripe_start = full_stripe_start +
+				 round_down(logical - full_stripe_start, BTRFS_STRIPE_LEN);
 	void **pointers = NULL;
 	unsigned long *failed_stripe_bitmap = NULL;
 	int failed_a = -1;
@@ -336,7 +338,8 @@ static int read_raid56(struct btrfs_fs_info *fs_info, void *buf, u64 logical,
 	ASSERT(raid_map);
 
 	/* The read length should be inside one stripe */
-	ASSERT(len <= BTRFS_STRIPE_LEN);
+	ASSERT(logical >= stripe_start &&
+	       logical + len <= stripe_start + BTRFS_STRIPE_LEN);
 
 	pointers = calloc(num_stripes, sizeof(void *));
 	if (!pointers)
@@ -439,6 +442,15 @@ int read_data_from_disk(struct btrfs_fs_info *info, void *buf, u64 logical,
 
 	/* We need to rebuild from P/Q */
 	if (mirror > 1 && multi->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
+		u64 stripe_start;
+
+		/*
+		 * The returned @read_len is always BTRFS_STRIPE_LEN. We have to calcualte
+		 * the proper length to the stripe boundary.
+		 */
+		stripe_start = raid_map[0] + round_down(logical - raid_map[0], BTRFS_STRIPE_LEN);
+		read_len = min((stripe_start + BTRFS_STRIPE_LEN) - logical, read_len);
+
 		ret = read_raid56(info, buf, logical, read_len, mirror, multi,
 				  raid_map);
 		kfree(multi);
-- 
2.54.0

[PATCH 2/2] btrfs-progs: fsck-tests: add a new test case for the raid56 read fix

[Qu Wenruo]

The new test case is a regression test, which does:

- Create a btrfs with RAID5 data
- Fsstress it with 256 write operations
- Unmount
- Make sure "btrfs check --check-data-csum" reports no error

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Makefile                                 |  2 +-
 tests/fsck-tests/074-raid56-read/test.sh | 27 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100755 tests/fsck-tests/074-raid56-read/test.sh

diff --git a/Makefile b/Makefile
index 4f471ff4a439..934d6dfb9c37 100644
--- a/Makefile
+++ b/Makefile
@@ -536,7 +536,7 @@ test-convert: btrfs btrfs-convert
 
 test-check: test-fsck
 test-check-lowmem: test-fsck
-test-fsck: btrfs btrfs-image btrfs-corrupt-block mkfs.btrfs btrfstune
+test-fsck: btrfs btrfs-image btrfs-corrupt-block mkfs.btrfs btrfstune fsstress
 ifneq ($(MAKECMDGOALS),test-check-lowmem)
 	@echo "  TEST     fsck-tests.sh"
 	$(Q)bash tests/fsck-tests.sh
diff --git a/tests/fsck-tests/074-raid56-read/test.sh b/tests/fsck-tests/074-raid56-read/test.sh
new file mode 100755
index 000000000000..6caae9837c35
--- /dev/null
+++ b/tests/fsck-tests/074-raid56-read/test.sh
@@ -0,0 +1,27 @@
+#!/bin/bash
+#
+# Make sure "btrfs check" can properly handle RAID5 data reconstruction.
+
+source "$TEST_TOP/common" || exit
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_prereq fsstress
+check_global_prereq fallocate
+
+setup_root_helper
+setup_loopdevs 3
+prepare_loopdevs
+dev1=${loopdevs[1]}
+TEST_DEV=$dev1
+fsstress_prog="$INTERNAL_BIN/fsstress"
+
+run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f -m raid1 -d raid5 "${loopdevs[@]}"
+run_check_mount_test_dev
+run_check $SUDO_HELPER $fsstress_prog -w -n 256 -d "$TEST_MNT"
+run_check_umount_test_dev
+
+# Check data csum should not report false alerts
+run_check $SUDO_HELPER "$TOP/btrfs" check --check-data-csum "$dev1"
+
+cleanup_loopdevs
-- 
2.54.0