[PATCH 0/4] Enhance btrfs-progs for hostile image (FPE error part)

[PATCH 0/4] Enhance btrfs-progs for hostile image (FPE error part)

[Qu Wenruo]

This patchset fixes 3 FPE error reported by Lukas Lueg<lukas.lueg@gmail.com>.
Corresponding kernel BZ#96971, BZ#97031, BZ#97041.

What a pity that I didn't realize it's a good chance to submit more patches. :)

There are still 3 BZ, but that's about memory corruption, and some even
corrupt the backtrace. So it will take a little longer to investigate it.

Qu Wenruo (4):
  btrfs-progs: Remove non-exist csum size.
  btrfs-progs: Read the whole superblock instead of struct    
    btrfs_super_block.
  btrfs-progs: Introduce better superblock check.
  btrfs-progs: Add extra chunk item check to avoid btrfs-progs crash.

 ctree.h   |   2 +-
 disk-io.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 volumes.c |  60 +++++++++++++++++++--
 3 files changed, 216 insertions(+), 21 deletions(-)

-- 
2.4.0

[PATCH 1/4] btrfs-progs: Remove non-exist csum size.

[Qu Wenruo]

Current btrfs only support CRC32 as checksum algorithm.
But in btrfs_csum_sizes array, we have an extra 0 at tail, causing
csum_type 1 can still be considered as supported csum type.

Fix it by removing the tailing 0.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 ctree.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ctree.h b/ctree.h
index 10dc838..af70c6f 100644
--- a/ctree.h
+++ b/ctree.h
@@ -149,7 +149,7 @@ struct btrfs_free_space_ctl;
 /* csum types */
 #define BTRFS_CSUM_TYPE_CRC32	0
 
-static int btrfs_csum_sizes[] = { 4, 0 };
+static int btrfs_csum_sizes[] = { 4 };
 
 /* four bytes for CRC32 */
 #define BTRFS_CRC32_SIZE 4
-- 
2.4.0

[PATCH 2/4] btrfs-progs: Read the whole superblock instead of struct btrfs_super_block.

[Qu Wenruo]

Before the patch, btrfs-progs will only read
sizeof(struct btrfs_super_block) and restore it into super_copy.

This makes checksum check for superblock impossible.
Change it to read the whole superblock.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 disk-io.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index c1cf146..e6ac3e9 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1264,7 +1264,8 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 {
 	u8 fsid[BTRFS_FSID_SIZE];
 	int fsid_is_initialized = 0;
-	struct btrfs_super_block buf;
+	char tmp[BTRFS_SUPER_INFO_SIZE];
+	struct btrfs_super_block *buf = (struct btrfs_super_block *)tmp;
 	int i;
 	int ret;
 	int max_super = super_recover ? BTRFS_SUPER_MIRROR_MAX : 1;
@@ -1272,15 +1273,15 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 	u64 bytenr;
 
 	if (sb_bytenr != BTRFS_SUPER_INFO_OFFSET) {
-		ret = pread64(fd, &buf, sizeof(buf), sb_bytenr);
-		if (ret < sizeof(buf))
+		ret = pread64(fd, buf, BTRFS_SUPER_INFO_SIZE, sb_bytenr);
+		if (ret < BTRFS_SUPER_INFO_SIZE)
 			return -1;
 
-		if (btrfs_super_bytenr(&buf) != sb_bytenr ||
-		    btrfs_super_magic(&buf) != BTRFS_MAGIC)
+		if (btrfs_super_bytenr(buf) != sb_bytenr ||
+		    btrfs_super_magic(buf) != BTRFS_MAGIC)
 			return -1;
 
-		memcpy(sb, &buf, sizeof(*sb));
+		memcpy(sb, &buf, BTRFS_SUPER_INFO_SIZE);
 		return 0;
 	}
 
@@ -1293,22 +1294,22 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 
 	for (i = 0; i < max_super; i++) {
 		bytenr = btrfs_sb_offset(i);
-		ret = pread64(fd, &buf, sizeof(buf), bytenr);
-		if (ret < sizeof(buf))
+		ret = pread64(fd, buf, BTRFS_SUPER_INFO_SIZE, bytenr);
+		if (ret < BTRFS_SUPER_INFO_SIZE)
 			break;
 
-		if (btrfs_super_bytenr(&buf) != bytenr )
+		if (btrfs_super_bytenr(buf) != bytenr )
 			continue;
 		/* if magic is NULL, the device was removed */
-		if (btrfs_super_magic(&buf) == 0 && i == 0)
+		if (btrfs_super_magic(buf) == 0 && i == 0)
 			return -1;
-		if (btrfs_super_magic(&buf) != BTRFS_MAGIC)
+		if (btrfs_super_magic(buf) != BTRFS_MAGIC)
 			continue;
 
 		if (!fsid_is_initialized) {
-			memcpy(fsid, buf.fsid, sizeof(fsid));
+			memcpy(fsid, buf->fsid, sizeof(fsid));
 			fsid_is_initialized = 1;
-		} else if (memcmp(fsid, buf.fsid, sizeof(fsid))) {
+		} else if (memcmp(fsid, buf->fsid, sizeof(fsid))) {
 			/*
 			 * the superblocks (the original one and
 			 * its backups) contain data of different
@@ -1317,9 +1318,9 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 			continue;
 		}
 
-		if (btrfs_super_generation(&buf) > transid) {
-			memcpy(sb, &buf, sizeof(*sb));
-			transid = btrfs_super_generation(&buf);
+		if (btrfs_super_generation(buf) > transid) {
+			memcpy(sb, buf, BTRFS_SUPER_INFO_SIZE);
+			transid = btrfs_super_generation(buf);
 		}
 	}
 
-- 
2.4.0

[PATCH 3/4] btrfs-progs: Introduce better superblock check.

[Qu Wenruo]

Now btrfs-progs will have much more restrict superblock check based on
kernel superblock check.

This should at least provide some hostile crafted image to crash command
like btrfsck.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 disk-io.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 144 insertions(+), 4 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index e6ac3e9..bf796c6 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -38,6 +38,7 @@
 #define BTRFS_BAD_BYTENR		(-1)
 #define BTRFS_BAD_FSID			(-2)
 
+#define IS_ALIGNED(x, a)                (((x) & ((typeof(x))(a) - 1)) == 0)
 static int check_tree_block(struct btrfs_root *root, struct extent_buffer *buf)
 {
 
@@ -1259,6 +1260,144 @@ struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
 	return info->fs_root;
 }
 
+/* Just to save some space */
+#define pr_err(fmt, args...)	(fprintf(stderr, fmt, ##args))
+
+/*
+ * Check if the super is validate.
+ *
+ * Unlike kernel one, we don't have extra check, so it's quite restrict
+ * in btrfs-progs.
+ */
+static int check_super(struct btrfs_super_block *sb)
+{
+	char result[BTRFS_CSUM_SIZE];
+	u32 crc;
+	u16 csum_type;
+	int csum_size;
+
+	if (btrfs_super_magic(sb) != BTRFS_MAGIC) {
+		pr_err("ERROR: superblock magic doesn't match\n");
+		return -EIO;
+	}
+
+	csum_type = btrfs_super_csum_type(sb);
+	if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) {
+		pr_err("ERROR: unsupported checksum algorithm %u\n",
+			csum_type);
+		return -EIO;
+	}
+	csum_size = btrfs_csum_sizes[csum_type];
+
+	crc = ~(u32)0;
+	crc = btrfs_csum_data(NULL, (char *)sb + BTRFS_CSUM_SIZE, crc,
+			      BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
+	btrfs_csum_final(crc, result);
+
+	if (memcmp(result, sb->csum, csum_size)) {
+		pr_err("ERROR: superblock checksum mismatch\n");
+		return -EIO;
+	}
+	if (btrfs_super_root_level(sb) >= BTRFS_MAX_LEVEL) {
+		pr_err("ERROR: tree_root level too big: %d>=%d\n",
+			btrfs_super_root_level(sb), BTRFS_MAX_LEVEL);
+		return -EIO;
+	}
+	if (btrfs_super_chunk_root_level(sb) >= BTRFS_MAX_LEVEL) {
+		pr_err("ERROR: chunk_root level too big: %d>=%d\n",
+			btrfs_super_chunk_root_level(sb), BTRFS_MAX_LEVEL);
+		return -EIO;
+	}
+	if (btrfs_super_log_root_level(sb) >= BTRFS_MAX_LEVEL) {
+		pr_err("ERROR: log_root level too big: %d>=%d\n",
+			btrfs_super_log_root_level(sb), BTRFS_MAX_LEVEL);
+		return -EIO;
+	}
+
+	if (!IS_ALIGNED(btrfs_super_root(sb), 4096)) {
+		pr_err("ERROR: tree_root block unaligned: %llu\n",
+			btrfs_super_root(sb));
+		return -EIO;
+	}
+	if (!IS_ALIGNED(btrfs_super_chunk_root(sb), 4096)) {
+		pr_err("ERROR: chunk_root block unaligned: %llu\n",
+			btrfs_super_chunk_root(sb));
+		return -EIO;
+	}
+	if (!IS_ALIGNED(btrfs_super_log_root(sb), 4096)) {
+		pr_err("ERROR: log_root block unaligned: %llu\n",
+			btrfs_super_log_root(sb));
+		return -EIO;
+	}
+	if (btrfs_super_nodesize(sb) < 4096) {
+		pr_err("ERROR: nodesize too small: %u < 4096\n",
+			btrfs_super_nodesize(sb));
+		return -EIO;
+	}
+	if (!IS_ALIGNED(btrfs_super_nodesize(sb), 4096)) {
+		pr_err("ERROR: nodesize unaligned: %u\n",
+			btrfs_super_nodesize(sb));
+		return -EIO;
+	}
+	if (btrfs_super_sectorsize(sb) < 4096) {
+		pr_err("ERROR: sectorsize too small: %u < 4096\n",
+			btrfs_super_sectorsize(sb));
+		return -EIO;
+	}
+	if (!IS_ALIGNED(btrfs_super_sectorsize(sb), 4096)) {
+		pr_err("ERROR: sectorsize unaligned: %u\n",
+			btrfs_super_sectorsize(sb));
+		return -EIO;
+	}
+
+	if (memcmp(sb->fsid, sb->dev_item.fsid, BTRFS_UUID_SIZE) != 0) {
+		char fsid[BTRFS_UUID_UNPARSED_SIZE];
+		char dev_fsid[BTRFS_UUID_UNPARSED_SIZE];
+
+		uuid_unparse(sb->fsid, fsid);
+		uuid_unparse(sb->dev_item.fsid, dev_fsid);
+		printk(KERN_ERR
+			"ERROR: dev_item UUID does not match fsid: %s != %s\n",
+			dev_fsid, fsid);
+		return -EIO;
+	}
+
+	/*
+	 * Hint to catch really bogus numbers, bitflips or so
+	 */
+	if (btrfs_super_num_devices(sb) > (1UL << 31)) {
+		pr_err("ERROR: suspicious number of devices: %llu\n",
+			btrfs_super_num_devices(sb));
+		return -EIO;
+	}
+
+	if (btrfs_super_num_devices(sb) == 0) {
+		pr_err("ERROR: number of devices is 0\n");
+		return -EIO;
+	}
+
+	/*
+	 * Obvious sys_chunk_array corruptions, it must hold at least one key
+	 * and one chunk
+	 */
+	if (btrfs_super_sys_array_size(sb) > BTRFS_SYSTEM_CHUNK_ARRAY_SIZE) {
+		pr_err("BTRFS: system chunk array too big %u > %u\n",
+			btrfs_super_sys_array_size(sb),
+			BTRFS_SYSTEM_CHUNK_ARRAY_SIZE);
+		return -EIO;
+	}
+	if (btrfs_super_sys_array_size(sb) < sizeof(struct btrfs_disk_key)
+			+ sizeof(struct btrfs_chunk)) {
+		pr_err("BTRFS: system chunk array too small %u < %lu\n",
+			btrfs_super_sys_array_size(sb),
+			sizeof(struct btrfs_disk_key) +
+			sizeof(struct btrfs_chunk));
+		return -EIO;
+	}
+
+	return 0;
+}
+
 int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 			 int super_recover)
 {
@@ -1277,10 +1416,11 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 		if (ret < BTRFS_SUPER_INFO_SIZE)
 			return -1;
 
-		if (btrfs_super_bytenr(buf) != sb_bytenr ||
-		    btrfs_super_magic(buf) != BTRFS_MAGIC)
+		if (btrfs_super_bytenr(buf) != sb_bytenr)
 			return -1;
 
+		if (check_super(buf))
+			return -1;
 		memcpy(sb, &buf, BTRFS_SUPER_INFO_SIZE);
 		return 0;
 	}
@@ -1302,8 +1442,8 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 			continue;
 		/* if magic is NULL, the device was removed */
 		if (btrfs_super_magic(buf) == 0 && i == 0)
-			return -1;
-		if (btrfs_super_magic(buf) != BTRFS_MAGIC)
+			break;
+		if (check_super(buf))
 			continue;
 
 		if (!fsid_is_initialized) {
-- 
2.4.0

[PATCH 4/4] btrfs-progs: Add extra chunk item check to avoid btrfs-progs crash.

[Qu Wenruo]

Adds extra check when reading a chunk item:
1) Check chunk type.
Don't allow any unsupported type/profile bit.

2) Check num_stripes
Any chunk item should contain at least one stripe.
For system chunk, the chunk item size(calculated by btrfs_stripe size *
(num_stripes - 1) + btrfs_chunk size) should not exceed
BTRFS_SYSTEM_CHUNK_SIZE(2048).
For normal chunk, the chunk item size(calculated) should match the chunk
item size.

3) Check num_stripes/sub_stripes against chunk profile.
Num_stripes/sub_stripes must meet its lower limit for its chunk profile.

Reported-by: Lukas Lueg <lukas.lueg@gmail.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 volumes.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/volumes.c b/volumes.c
index 16dbf64..77cc305 100644
--- a/volumes.c
+++ b/volumes.c
@@ -1575,9 +1575,14 @@ static struct btrfs_device *fill_missing_device(u64 devid)
 	return device;
 }
 
+/*
+ * Slot is used to verfy the chunk item is valid
+ *
+ * For sys chunk in superblock, pass -1 to indicate sys chunk.
+ */
 static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 			  struct extent_buffer *leaf,
-			  struct btrfs_chunk *chunk)
+			  struct btrfs_chunk *chunk, int slot)
 {
 	struct btrfs_mapping_tree *map_tree = &root->fs_info->mapping_tree;
 	struct map_lookup *map;
@@ -1615,6 +1620,51 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 	map->type = btrfs_chunk_type(leaf, chunk);
 	map->sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
 
+	/* Check on chunk item type */
+	if (map->type & ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
+			  BTRFS_BLOCK_GROUP_PROFILE_MASK)) {
+		fprintf(stderr, "Unknown chunk type bits: %llu\n",
+			map->type & ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
+				      BTRFS_BLOCK_GROUP_PROFILE_MASK));
+		ret = -EIO;
+		goto out;
+	}
+
+	/*
+	 * Btrfs_chunk contains at least one stripe, and for sys_chunk
+	 * it can't exceed the system chunk array size
+	 * For normal chunk, it should match its chunk item size.
+	 */
+	if (num_stripes < 1 ||
+	    (slot == -1 && sizeof(struct btrfs_stripe) * num_stripes >
+	     BTRFS_SYSTEM_CHUNK_ARRAY_SIZE) ||
+	    (slot >= 0 && sizeof(struct btrfs_stripe) * (num_stripes - 1) >
+	     btrfs_item_size_nr(leaf, slot))) {
+		fprintf(stderr, "Invalid num_stripes: %u\n",
+		        num_stripes);
+		ret = -EIO;
+		goto out;
+	}
+
+	/*
+	 * Device number check against profile
+	 */
+	if ((map->type & BTRFS_BLOCK_GROUP_RAID10 && num_stripes < 4 &&
+	     map->sub_stripes < 2) ||
+	    (map->type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 2) ||
+	    (map->type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 3) ||
+	    (map->type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 4) ||
+	    (map->type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 1) ||
+	    ((map->type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
+	     num_stripes != 1)) {
+		fprintf(stderr,
+			"Invalid num_stripes:sub_stripes %u:%u for profile %llu\n",
+		        num_stripes, map->sub_stripes,
+			map->type & BTRFS_BLOCK_GROUP_PROFILE_MASK);
+		ret = -EIO;
+		goto out;
+	}
+
 	for (i = 0; i < num_stripes; i++) {
 		map->stripes[i].physical =
 			btrfs_stripe_offset_nr(leaf, chunk, i);
@@ -1635,6 +1685,9 @@ static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 	BUG_ON(ret);
 
 	return 0;
+out:
+	free(map);
+	return ret;
 }
 
 static int fill_device_from_item(struct extent_buffer *leaf,
@@ -1773,7 +1826,7 @@ int btrfs_read_sys_array(struct btrfs_root *root)
 
 		if (key.type == BTRFS_CHUNK_ITEM_KEY) {
 			chunk = (struct btrfs_chunk *)(ptr - (u8 *)super_copy);
-			ret = read_one_chunk(root, &key, sb, chunk);
+			ret = read_one_chunk(root, &key, sb, chunk, -1);
 			if (ret)
 				break;
 			num_stripes = btrfs_chunk_num_stripes(sb, chunk);
@@ -1835,7 +1888,8 @@ int btrfs_read_chunk_tree(struct btrfs_root *root)
 		} else if (found_key.type == BTRFS_CHUNK_ITEM_KEY) {
 			struct btrfs_chunk *chunk;
 			chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk);
-			ret = read_one_chunk(root, &found_key, leaf, chunk);
+			ret = read_one_chunk(root, &found_key, leaf, chunk,
+					     slot);
 			BUG_ON(ret);
 		}
 		path->slots[0]++;
-- 
2.4.0

Re: [PATCH 2/4] btrfs-progs: Read the whole superblock instead of struct btrfs_super_block.

[David Sterba]

On Wed, May 13, 2015 at 05:15:34PM +0800, Qu Wenruo wrote:
> Before the patch, btrfs-progs will only read
> sizeof(struct btrfs_super_block) and restore it into super_copy.
> 
> This makes checksum check for superblock impossible.
> Change it to read the whole superblock.

We've been there already:
http://thread.gmane.org/gmane.comp.file-systems.btrfs/36563

IIRC it caused a bug on PPC64 machine but I don't have it at hand right
now.

Re: [PATCH 1/4] btrfs-progs: Remove non-exist csum size.

[David Sterba]

On Wed, May 13, 2015 at 05:15:33PM +0800, Qu Wenruo wrote:
> Current btrfs only support CRC32 as checksum algorithm.
> But in btrfs_csum_sizes array, we have an extra 0 at tail, causing
> csum_type 1 can still be considered as supported csum type.
> 
> Fix it by removing the tailing 0.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied, thanks.

Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to avoid btrfs-progs crash.

[David Sterba]

On Wed, May 13, 2015 at 05:15:36PM +0800, Qu Wenruo wrote:
> Adds extra check when reading a chunk item:
> 1) Check chunk type.
> Don't allow any unsupported type/profile bit.
> 
> 2) Check num_stripes
> Any chunk item should contain at least one stripe.
> For system chunk, the chunk item size(calculated by btrfs_stripe size *
> (num_stripes - 1) + btrfs_chunk size) should not exceed
> BTRFS_SYSTEM_CHUNK_SIZE(2048).
> For normal chunk, the chunk item size(calculated) should match the chunk
> item size.
> 
> 3) Check num_stripes/sub_stripes against chunk profile.
> Num_stripes/sub_stripes must meet its lower limit for its chunk profile.
> 
> Reported-by: Lukas Lueg <lukas.lueg@gmail.com>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied, thanks.

Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to avoid btrfs-progs crash.

[WorMzy Tykashi]

(resending to list because of gmail's daft HTML headers getting the
original message rejected)

Hi guys,

Following a bisect, it appears that this patch breaks fsck test 006:

$ git checkout f146c40c65e0142b52418a0a1cbaf2
808e658d76
HEAD is now at f146c40... btrfs-progs: Add extra chunk item check to
avoid btrfs-progs crash.
...autogen, configure, make..
$ make test-fsck
    [TEST]   fsck-tests.sh
    [TEST]   001-bad-file-extent-bytenr
    [TEST]   002-bad-transid
parent transid verify failed on 29360128 wanted 9 found 755944791
parent transid verify failed on 29360128 wanted 9 found 755944791
Ignoring transid failure
    [TEST]   003-shift-offsets
    [TEST]   004-no-dir-index
    [TEST]   005-bad-item-offset
    [TEST]   006-bad-root-items
failed: /home/wormzy/btrfs-progs-unstable/btrfs check --repair test.img
test failed for case 006-bad-root-items
Makefile:169: recipe for target 'test-fsck' failed
make: *** [test-fsck] Error 1

Does this test just need updating?

Cheers,


WorMzy

On 13 May 2015 at 17:18, David Sterba <dsterba@suse.cz> wrote:
> On Wed, May 13, 2015 at 05:15:36PM +0800, Qu Wenruo wrote:
>> Adds extra check when reading a chunk item:
>> 1) Check chunk type.
>> Don't allow any unsupported type/profile bit.
>>
>> 2) Check num_stripes
>> Any chunk item should contain at least one stripe.
>> For system chunk, the chunk item size(calculated by btrfs_stripe size *
>> (num_stripes - 1) + btrfs_chunk size) should not exceed
>> BTRFS_SYSTEM_CHUNK_SIZE(2048).
>> For normal chunk, the chunk item size(calculated) should match the chunk
>> item size.
>>
>> 3) Check num_stripes/sub_stripes against chunk profile.
>> Num_stripes/sub_stripes must meet its lower limit for its chunk profile.
>>
>> Reported-by: Lukas Lueg <lukas.lueg@gmail.com>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>
> Applied, thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to avoid btrfs-progs crash.

[Qu Wenruo]

Yes, I also find the problem and already sent the fixing patch:
https://patchwork.kernel.org/patch/6411191/

The problem is that my previous check patch is too restrict, making DUP 
chunk with only 1 stripe invalid.

Fixing patch will allow degraded chunk to exist and fix the bug.

Thanks,
Qu

-------- Original Message  --------
Subject: Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to 
avoid btrfs-progs crash.
From: WorMzy Tykashi <wormzy.tykashi@gmail.com>
To: David Sterba <dsterba@suse.cz>, Qu Wenruo <quwenruo@cn.fujitsu.com>, 
linux-btrfs@vger.kernel.org <linux-btrfs@vger.kernel.org>, 
<lukas.lueg@gmail.com>
Date: 2015年05月20日 00:30

> Hi guys,
>
> Following a bisect, it appears that this patch breaks fsck test 006:
>
> $ git checkout f146c40c65e0142b52418a0a1cbaf2808e658d76
> HEAD is now at f146c40... btrfs-progs: Add extra chunk item check to
> avoid btrfs-progs crash.
> ...autogen, configure, make..
> $ make test-fsck
>      [TEST]   fsck-tests.sh
>      [TEST]   001-bad-file-extent-bytenr
>      [TEST]   002-bad-transid
> parent transid verify failed on 29360128 wanted 9 found 755944791
> parent transid verify failed on 29360128 wanted 9 found 755944791
> Ignoring transid failure
>      [TEST]   003-shift-offsets
>      [TEST]   004-no-dir-index
>      [TEST]   005-bad-item-offset
>      [TEST]   006-bad-root-items
> failed: /home/wormzy/btrfs-progs-unstable/btrfs check --repair test.img
> test failed for case 006-bad-root-items
> Makefile:169: recipe for target 'test-fsck' failed
> make: *** [test-fsck] Error 1
>
> Does this test just need updating?
>
> Cheers,
>
>
> WorMzy
>
> On 13 May 2015 at 17:18, David Sterba <dsterba@suse.cz
> <mailto:dsterba@suse.cz>> wrote:
>
>     On Wed, May 13, 2015 at 05:15:36PM +0800, Qu Wenruo wrote:
>     > Adds extra check when reading a chunk item:
>     > 1) Check chunk type.
>     > Don't allow any unsupported type/profile bit.
>     >
>     > 2) Check num_stripes
>     > Any chunk item should contain at least one stripe.
>     > For system chunk, the chunk item size(calculated by btrfs_stripe size *
>     > (num_stripes - 1) + btrfs_chunk size) should not exceed
>     > BTRFS_SYSTEM_CHUNK_SIZE(2048).
>     > For normal chunk, the chunk item size(calculated) should match the chunk
>     > item size.
>     >
>     > 3) Check num_stripes/sub_stripes against chunk profile.
>     > Num_stripes/sub_stripes must meet its lower limit for its chunk profile.
>     >
>     > Reported-by: Lukas Lueg <lukas.lueg@gmail.com <mailto:lukas.lueg@gmail.com>>
>     > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com <mailto:quwenruo@cn.fujitsu.com>>
>
>     Applied, thanks.
>     --
>     To unsubscribe from this list: send the line "unsubscribe
>     linux-btrfs" in
>     the body of a message to majordomo@vger.kernel.org
>     <mailto:majordomo@vger.kernel.org>
>     More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to avoid btrfs-progs crash.

[WorMzy Tykashi]

On 20 May 2015 01:25:03 BST, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>Yes, I also find the problem and already sent the fixing patch:
>https://patchwork.kernel.org/patch/6411191/
>
>The problem is that my previous check patch is too restrict, making DUP
>
>chunk with only 1 stripe invalid.
>
>Fixing patch will allow degraded chunk to exist and fix the bug.
>
>Thanks,
>Qu
>
>-------- Original Message  --------
>Subject: Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to 
>avoid btrfs-progs crash.
>From: WorMzy Tykashi <wormzy.tykashi@gmail.com>
>To: David Sterba <dsterba@suse.cz>, Qu Wenruo
><quwenruo@cn.fujitsu.com>, 
>linux-btrfs@vger.kernel.org <linux-btrfs@vger.kernel.org>, 
><lukas.lueg@gmail.com>
>Date: 2015年05月20日 00:30
>
>> Hi guys,
>>
>> Following a bisect, it appears that this patch breaks fsck test 006:
>>
>> $ git checkout f146c40c65e0142b52418a0a1cbaf2808e658d76
>> HEAD is now at f146c40... btrfs-progs: Add extra chunk item check to
>> avoid btrfs-progs crash.
>> ...autogen, configure, make..
>> $ make test-fsck
>>      [TEST]   fsck-tests.sh
>>      [TEST]   001-bad-file-extent-bytenr
>>      [TEST]   002-bad-transid
>> parent transid verify failed on 29360128 wanted 9 found 755944791
>> parent transid verify failed on 29360128 wanted 9 found 755944791
>> Ignoring transid failure
>>      [TEST]   003-shift-offsets
>>      [TEST]   004-no-dir-index
>>      [TEST]   005-bad-item-offset
>>      [TEST]   006-bad-root-items
>> failed: /home/wormzy/btrfs-progs-unstable/btrfs check --repair
>test.img
>> test failed for case 006-bad-root-items
>> Makefile:169: recipe for target 'test-fsck' failed
>> make: *** [test-fsck] Error 1
>>
>> Does this test just need updating?
>>
>> Cheers,
>>
>>
>> WorMzy
>>
>> On 13 May 2015 at 17:18, David Sterba <dsterba@suse.cz
>> <mailto:dsterba@suse.cz>> wrote:
>>
>>     On Wed, May 13, 2015 at 05:15:36PM +0800, Qu Wenruo wrote:
>>     > Adds extra check when reading a chunk item:
>>     > 1) Check chunk type.
>>     > Don't allow any unsupported type/profile bit.
>>     >
>>     > 2) Check num_stripes
>>     > Any chunk item should contain at least one stripe.
>>     > For system chunk, the chunk item size(calculated by
>btrfs_stripe size *
>>     > (num_stripes - 1) + btrfs_chunk size) should not exceed
>>     > BTRFS_SYSTEM_CHUNK_SIZE(2048).
>>     > For normal chunk, the chunk item size(calculated) should match
>the chunk
>>     > item size.
>>     >
>>     > 3) Check num_stripes/sub_stripes against chunk profile.
>>     > Num_stripes/sub_stripes must meet its lower limit for its chunk
>profile.
>>     >
>>     > Reported-by: Lukas Lueg <lukas.lueg@gmail.com
><mailto:lukas.lueg@gmail.com>>
>>     > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com
><mailto:quwenruo@cn.fujitsu.com>>
>>
>>     Applied, thanks.
>>     --
>>     To unsubscribe from this list: send the line "unsubscribe
>>     linux-btrfs" in
>>     the body of a message to majordomo@vger.kernel.org
>>     <mailto:majordomo@vger.kernel.org>
>>     More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>

Thanks Qu, I missed that patch. 

Cheers, 


WorMzy 

Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to avoid btrfs-progs crash.

[David Sterba]

On Wed, May 20, 2015 at 08:25:03AM +0800, Qu Wenruo wrote:
> Yes, I also find the problem and already sent the fixing patch:
> https://patchwork.kernel.org/patch/6411191/
> 
> The problem is that my previous check patch is too restrict, making DUP 
> chunk with only 1 stripe invalid.
> 
> Fixing patch will allow degraded chunk to exist and fix the bug.

Next time please mention that in the changelog or at least as a side
note that it fixes the broken test or that fixes some previous commit.

Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to avoid btrfs-progs crash.

[Qu Wenruo]

-------- Original Message  --------
Subject: Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to 
avoid btrfs-progs crash.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年05月20日 20:20

> On Wed, May 20, 2015 at 08:25:03AM +0800, Qu Wenruo wrote:
>> Yes, I also find the problem and already sent the fixing patch:
>> https://patchwork.kernel.org/patch/6411191/
>>
>> The problem is that my previous check patch is too restrict, making DUP
>> chunk with only 1 stripe invalid.
>>
>> Fixing patch will allow degraded chunk to exist and fix the bug.
>
> Next time please mention that in the changelog or at least as a side
> note that it fixes the broken test or that fixes some previous commit.
>
Sorry, my fault.

I'll pay more attention on such patch.

Thanks for pointing it out.
Qu

Re: [PATCH 2/4] btrfs-progs: Read the whole superblock instead of struct btrfs_super_block.

[David Sterba]

On Wed, May 13, 2015 at 05:15:34PM +0800, Qu Wenruo wrote:
> Before the patch, btrfs-progs will only read
> sizeof(struct btrfs_super_block) and restore it into super_copy.
> 
> This makes checksum check for superblock impossible.
> Change it to read the whole superblock.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

I've applied this despite there are the issues on ppc64. This patch is
required for the extended superblock checks in the next patch.

Re: [PATCH 3/4] btrfs-progs: Introduce better superblock check.

[David Sterba]

On Wed, May 13, 2015 at 05:15:35PM +0800, Qu Wenruo wrote:
> Now btrfs-progs will have much more restrict superblock check based on
> kernel superblock check.
> 
> This should at least provide some hostile crafted image to crash command
> like btrfsck.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied with some changes.

> +/* Just to save some space */
> +#define pr_err(fmt, args...)	(fprintf(stderr, fmt, ##args))

fprintf(stderr, ...)

> +	/*
> +	 * Hint to catch really bogus numbers, bitflips or so
> +	 */
> +	if (btrfs_super_num_devices(sb) > (1UL << 31)) {
> +		pr_err("ERROR: suspicious number of devices: %llu\n",
> +			btrfs_super_num_devices(sb));
> +		return -EIO;

This is supposed to be only a warning.

> +	}
> +