btrfs: Part 2 of enhanced defence against fuzzed images

There is one report of fuzzed image which leads to BUG_ON() in btrfs_delete_delayed_dir_index().

Although that fuzzed image can already be addressed by enhanced extent-tree error handler, it's still better to hunt down more BUG_ON().

This patch will hunt down two BUG_ON()s in btrfs_delete_delayed_dir_index():
- One for error from btrfs_delayed_item_reserve_metadata() Instead of BUG_ON(), we output an error message and free the item. And return the error.
All callers of this function handles the error by aborting current trasaction.

- One for possible EEXIST from __btrfs_add_delayed_deletion_item() That function can return -EEXIST.
We already have a good enough error message for that, only need to clean up the reserved metadata space and allocated item.

To help above cleanup, also modifiy __btrfs_remove_delayed_item() called in btrfs_release_delayed_item(), to skip unassociated item.

Link https://bugzilla.kernel.org/show_bug.cgi?id=203253

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

---
 fs/btrfs/delayed-inode.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

fs/btrfs/delayed-inode.c +3 -0

index 43fdb2992956..c4946d58f49b 100644
@@ -474,6 +474,9 @@ static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item)
 	struct rb_root_cached *root;
 	struct btrfs_delayed_root *delayed_root;
 
+	/* Not associated with any delayed_node */
+	if (!delayed_item->delayed_node)
+		return;
 	delayed_root = delayed_item->delayed_node->root->fs_info->delayed_root;
 
 	BUG_ON(!delayed_root);

This patch will introduce ROOT_ITEM check, which includes:
- Key->objectid and key->offset check
Currently only some easy check, e.g. 0 as rootid is invalid.

- Item size check
Root item size is fixed.

- Generation checks
Generation, v2_generaetion and last_snapshot should not pass super generation + 1

- Level and alignment check
Level should be in [0, 7], and bytenr must be aligned to sector size.

- Flags check

Link https://bugzilla.kernel.org/show_bug.cgi?id=203261

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

---
 fs/btrfs/tree-checker.c | 92 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

fs/btrfs/tree-checker.c +0 -0

index a4c7f7ed8490..71bbbce3076d 100644

fs/btrfs/tree-checker.c +11 -0

+
+	/* No Such Tree id*/
+	if (key->objectid == 0) {
+		generic_err(leaf, slot, "invalid root id 0");
+		return -EUCLEAN;
+	}
+
+	/*
+	 * Some older kernel may create ROOT_ITEM with non-zero offset,
+	 * so here we only check offset for reloc tree whose key->offset
+	 * must be a valid tree.

fs/btrfs/tree-checker.c +27 -0

+			    btrfs_root_generation_v2(&ri),
+			    btrfs_super_generation(fs_info->super_copy) + 1);
+		return -EUCLEAN;
+	}
+	if (btrfs_root_last_snapshot(&ri) >
+	    btrfs_super_generation(fs_info->super_copy) + 1) {
+		generic_err(leaf, slot,
+		"invalid root last_snapshot, have %llu expect (0, %llu]",
+			    btrfs_root_last_snapshot(&ri),
+			    btrfs_super_generation(fs_info->super_copy) + 1);
+		return -EUCLEAN;
+	}
+
+	/* Alignment and level check */
+	if (!IS_ALIGNED(btrfs_root_bytenr(&ri), fs_info->sectorsize)) {
+		generic_err(leaf, slot,
+			"invalid root bytenr, have %llu expect to be aligned to %u",
+			    btrfs_root_bytenr(&ri), fs_info->sectorsize);
+		return -EUCLEAN;
+	}
+	if (btrfs_root_level(&ri) >= BTRFS_MAX_LEVEL) {
+		generic_err(leaf, slot,
+			"invalid root level, have %u expect [0, %u]",
+			    btrfs_root_level(&ri), BTRFS_MAX_LEVEL - 1);
+		return -EUCLEAN;
+	}
+	if (ri.drop_level >= BTRFS_MAX_LEVEL) {

fs/btrfs/tree-checker.c +9 -0

+			    btrfs_root_flags(&ri), valid_root_flags);
+		return -EUCLEAN;
+	}
+	return 0;
+}
+
 /*
  * Common point to switch the item-specific validation.
  */

@@ -845,6 +934,9 @@ static int check_leaf_item(struct extent_buffer *leaf,
 	case BTRFS_INODE_ITEM_KEY:
 		ret = check_inode_item(leaf, key, slot);
 		break;
+	case BTRFS_ROOT_ITEM_KEY:
+		ret = check_root_item(leaf, key, slot);
+		break;
 	}
 	return ret;
 }

Signature · 1 line

2.22.0

fs/btrfs/tree-checker.c static int check_inode_item(struct extent_buffer *leaf, · line 846

Quoted 7 lines

fs/btrfs/tree-checker.c +6 -0

+	}
+
+	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
+			   sizeof(ri));
+
+	/* Generateion related */

typo here and a few more times below

fs/btrfs/tree-checker.c static int check_inode_item(struct extent_buffer *leaf, · line 858

Quoted 12 lines

fs/btrfs/tree-checker.c +12 -0

+	if (btrfs_root_generation(&ri) >
+	    btrfs_super_generation(fs_info->super_copy) + 1) {
+		generic_err(leaf, slot,
+			"invalid root generaetion, have %llu expect (0, %llu]",
+			    btrfs_root_generation(&ri),
+			    btrfs_super_generation(fs_info->super_copy) + 1);
+		return -EUCLEAN;
+	}
+	if (btrfs_root_generation_v2(&ri) >
+	    btrfs_super_generation(fs_info->super_copy) + 1) {
+		generic_err(leaf, slot,
+		"invalid root v2 generaetion, have %llu expect (0, %llu]",

So (0, %llu] here means that it must be greater than zero, right? I'm not sure that everyone uses the same notation for open/closed notation.

Quoted 28 lines

fs/btrfs/tree-checker.c +27 -0

+			    btrfs_root_generation_v2(&ri),
+			    btrfs_super_generation(fs_info->super_copy) + 1);
+		return -EUCLEAN;
+	}
+	if (btrfs_root_last_snapshot(&ri) >
+	    btrfs_super_generation(fs_info->super_copy) + 1) {
+		generic_err(leaf, slot,
+		"invalid root last_snapshot, have %llu expect (0, %llu]",
+			    btrfs_root_last_snapshot(&ri),
+			    btrfs_super_generation(fs_info->super_copy) + 1);
+		return -EUCLEAN;
+	}
+
+	/* Alignment and level check */
+	if (!IS_ALIGNED(btrfs_root_bytenr(&ri), fs_info->sectorsize)) {
+		generic_err(leaf, slot,
+			"invalid root bytenr, have %llu expect to be aligned to %u",
+			    btrfs_root_bytenr(&ri), fs_info->sectorsize);
+		return -EUCLEAN;
+	}
+	if (btrfs_root_level(&ri) >= BTRFS_MAX_LEVEL) {
+		generic_err(leaf, slot,
+			"invalid root level, have %u expect [0, %u]",
+			    btrfs_root_level(&ri), BTRFS_MAX_LEVEL - 1);
+		return -EUCLEAN;
+	}
+	if (ri.drop_level >= BTRFS_MAX_LEVEL) {

fs/btrfs/tree-checker.c static int check_inode_item(struct extent_buffer *leaf, · line 895

Quoted 10 lines

fs/btrfs/tree-checker.c +10 -0

+		generic_err(leaf, slot,
+			"invalid root level, have %u expect [0, %u]",
+			    ri.drop_level, BTRFS_MAX_LEVEL - 1);
+		return -EUCLEAN;
+	}
+
+	/* Flags check */
+	if (btrfs_root_flags(&ri) & ~valid_root_flags) {
+		generic_err(leaf, slot,
+			"invalid root flags, have 0x%llx expect mask 0x%llu",

                                                                     0x%llx

Quoted 21 lines

fs/btrfs/tree-checker.c +9 -0

+			    btrfs_root_flags(&ri), valid_root_flags);
+		return -EUCLEAN;
+	}
+	return 0;
+}
+
 /*
  * Common point to switch the item-specific validation.
  */

@@ -845,6 +934,9 @@ static int check_leaf_item(struct extent_buffer *leaf,
 	case BTRFS_INODE_ITEM_KEY:
 		ret = check_inode_item(leaf, key, slot);
 		break;
+	case BTRFS_ROOT_ITEM_KEY:
+		ret = check_root_item(leaf, key, slot);
+		break;
 	}
 	return ret;
 }

Signature · 1 line

2.22.0

[snip]

Quoted 10 lines

fs/btrfs/tree-checker.c static int check_inode_item(struct extent_buffer *leaf, · line 846

Quoted 7 lines

fs/btrfs/tree-checker.c +6 -0

+	}
+
+	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
+			   sizeof(ri));
+
+	/* Generateion related */

typo here and a few more times below

Quoted 15 lines

fs/btrfs/tree-checker.c static int check_inode_item(struct extent_buffer *leaf, · line 858

Quoted 12 lines

fs/btrfs/tree-checker.c +12 -0

+	if (btrfs_root_generation(&ri) >
+	    btrfs_super_generation(fs_info->super_copy) + 1) {
+		generic_err(leaf, slot,
+			"invalid root generaetion, have %llu expect (0, %llu]",
+			    btrfs_root_generation(&ri),
+			    btrfs_super_generation(fs_info->super_copy) + 1);
+		return -EUCLEAN;
+	}
+	if (btrfs_root_generation_v2(&ri) >
+	    btrfs_super_generation(fs_info->super_copy) + 1) {
+		generic_err(leaf, slot,
+		"invalid root v2 generaetion, have %llu expect (0, %llu]",

So (0, %llu] here means that it must be greater than zero, right? I'm not sure that everyone uses the same notation for open/closed notation.

AFAIK in tree checker it's all the same notation.

Or any better solution for that?

This wave has the following features:
- Hunt down BUG_ON() in btrfs_delete_delayed_dir_index() EEXIST can cause BUG_ON(). And all callers of this function has already handled error by aborting transacation.

- Only allocate extents from the same block group type
This is a very tricky bug, needs MIXED_GROUP super flag with regular block groups (separate META and DATA) and corrupted extent tree.

- ROOT_ITEM check for tree checker
This kills the unaligned bytenr, invalid level and incorrect reloc tree.

Reported-by Jungyeon Yoon <[email protected]>

Qu Wenruo (3):
btrfs: delayed-inode: Kill the BUG_ON() in
btrfs_delete_delayed_dir_index()
btrfs: extent-tree: Make sure we only allocate extents from block
groups with the same type
btrfs: tree-checker: Add ROOT_ITEM check