[PATCH 0/3] btrfs-progs: fix btrfs_root_item related memory bugs

[PATCH 0/3] btrfs-progs: fix btrfs_root_item related memory bugs

[Qu Wenruo]

https://github.com/kdave/btrfs-progs/pull/1201

There is a security researcher, that exposeed a bug in btrfs root item
reading code, and that bug itself is valid.

So here we're fixing it mostly by introducing a dedicated helper to
handle the possible different sizes of btrfs_root_item.

Unfortunately we can not benefit from tree-checker, as a lot of tools
are designed to skip tree-checker, e.g. btrfs-check --repair can repair
certain item size related bugs, so it must skipping tree-checker or it
can not even open the fs.

The first patch is a small fix noticed during development.
The second one is the main fix related to btrfs_root_item reading.
The last one is two minor fixes related to btrfs_root_item writing.

Qu Wenruo (3):
  btrfs-progs: return error if btrfs_next_leaf() failed in
    do_list_roots()
  btrfs-progs: fix root_item reads which can cause memory bugs
  btrfs-progs: fix two sites that may write beyond root item size

 check/main.c               | 57 ++++++++++++++++++++++++++++----------
 cmds/inspect-dump-tree.c   | 12 +++++---
 cmds/restore.c             | 18 ++++++++----
 kernel-shared/disk-io.c    | 21 +++++++++-----
 kernel-shared/extent_io.c  | 34 +++++++++++++++++++++++
 kernel-shared/extent_io.h  |  3 ++
 kernel-shared/print-tree.c | 16 +++++++++--
 kernel-shared/root-tree.c  |  8 ++++--
 8 files changed, 133 insertions(+), 36 deletions(-)

--
2.56.0

[PATCH 1/3] btrfs-progs: return error if btrfs_next_leaf() failed in do_list_roots()

[Qu Wenruo]

During the while () loop, if btrfs_next_leaf() returned an error, the
loop is broken, but still return 0.

Which is not the correct return value.

Fix it by properly return the error if @ret is negative.
We need to intentionally ignore @ret > 0 case, as that just means we
have exhausted the root tree.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/restore.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cmds/restore.c b/cmds/restore.c
index 77de65bf1528..ac00faac1d02 100644
--- a/cmds/restore.c
+++ b/cmds/restore.c
@@ -1199,6 +1199,8 @@ static int do_list_roots(struct btrfs_root *root)
 		path.slots[0]++;
 	}
 
+	if (ret < 0)
+		return ret;
 	return 0;
 }
 
-- 
2.56.0

[PATCH 2/3] btrfs-progs: fix root_item reads which can cause memory bugs

[Qu Wenruo]

Currently we read out a root item by the following pattern for most
cases:

	struct btrfs_root_item ri;

	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
			   sizeof(ri));

And in print-tree.c it's the worst case

	struct btrfs_root_item root_item;

	read_extent_buffer(leaf, &root_item,
			   btrfs_item_ptr_offset(leaf, slot),
			   btrfs_item_size(leaf, slot));

These all have their problems:

- Can read beyond the item boundary for very old fses
  The structure btrfs_root_item has gone through one expansion.

  So for very old btrfses, the on-disk root item can be smaller than
  btrfs_root_item.

  Reading using sizeof(struct btrfs_root_item) can cause btrfs to read
  beyond the item size.
  For the worst case, if the root item is at the end of the leaf, such
  read can even read beyond the allocated heap memory.

- Overflow the on-stack memory for print-tree.c
  If the on-disk item is corrupted to have a much larger size than the
  btrfs_root_item, it can easily overflow the on-stack memory.

  This call site also doesn't check if the root item has newer members
  or not.

Fix the problem by introduce read_extent_buffer_root_item(), which will
reject too small items first, then do the read size clamping to handle
older btrfs_root_items.

Also fix the unconditional new member output for print_root_item().

Unfortunately this will bring a minor regression that too small root
items will directly reject btrfs-check.
Unfortunately I do not have any better idea to address this.

Reported-by: Thai Duong <thai@calif.io>
Link: https://lore.kernel.org/linux-btrfs/CADdoe42S7PceeJtXg1zm6Zn0g5GK90JnS0PoCtY2YUF5wp6+sw@mail.gmail.com/
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c               | 46 +++++++++++++++++++++++++++-----------
 cmds/inspect-dump-tree.c   | 12 ++++++----
 cmds/restore.c             | 16 ++++++++-----
 kernel-shared/disk-io.c    | 21 +++++++++++------
 kernel-shared/extent_io.c  | 34 ++++++++++++++++++++++++++++
 kernel-shared/extent_io.h  |  3 +++
 kernel-shared/print-tree.c | 16 ++++++++++---
 kernel-shared/root-tree.c  |  8 +++++--
 8 files changed, 122 insertions(+), 34 deletions(-)

diff --git a/check/main.c b/check/main.c
index 194d04ce017f..baf4c283527f 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9029,7 +9029,6 @@ static int parse_tree_roots(struct list_head *normal_trees,
 	BTRFS_PATH_AUTO_RELEASE(path);
 	struct btrfs_key key;
 	struct btrfs_key found_key;
-	struct btrfs_root_item ri;
 	struct extent_buffer *leaf;
 	int slot;
 	int ret = 0;
@@ -9052,12 +9051,18 @@ static int parse_tree_roots(struct list_head *normal_trees,
 		}
 		btrfs_item_key_to_cpu(leaf, &found_key, path.slots[0]);
 		if (found_key.type == BTRFS_ROOT_ITEM_KEY) {
-			unsigned long offset;
+			struct btrfs_root_item ri;
 			u64 last_snapshot;
 			u8 level;
 
-			offset = btrfs_item_ptr_offset(leaf, path.slots[0]);
-			read_extent_buffer(leaf, &ri, offset, sizeof(ri));
+			ret = read_extent_buffer_root_item(path.nodes[0], &ri,
+							   path.slots[0]);
+			if (ret < 0) {
+				error("bad root item size for key (%llu %u %llu)",
+				      found_key.objectid, found_key.type,
+				      found_key.offset);
+				break;
+			}
 			last_snapshot = btrfs_root_last_snapshot(&ri);
 			level = btrfs_root_level(&ri);
 			if (btrfs_disk_key_objectid(&ri.drop_progress) == 0) {
@@ -9563,11 +9568,13 @@ static struct extent_buffer *btrfs_fsck_clear_root(
 	ret = btrfs_insert_empty_item(trans, gfs_info->tree_root, path, key,
 				      sizeof(ri));
 	if (ret == -EEXIST) {
-		read_extent_buffer(path->nodes[0], &ri,
-				   btrfs_item_ptr_offset(path->nodes[0],
-							 path->slots[0]),
-				   sizeof(ri));
-	} else if (ret) {
+		ret = read_extent_buffer_root_item(path->nodes[0], &ri,
+						   path->slots[0]);
+		if (ret < 0)
+			error("bad root item size for key (%llu %u %llu)",
+			      key->objectid, key->type, key->offset);
+	}
+	if (ret) {
 		btrfs_free_path(path);
 		free_extent_buffer(c);
 		return ERR_PTR(ret);
@@ -10153,9 +10160,16 @@ static int load_log_root(u64 root_id, struct btrfs_path *path, struct btrfs_root
 	btrfs_setup_root(tmp_root, gfs_info, root_id);
 
 	leaf = path->nodes[0];
-	read_extent_buffer(leaf, &tmp_root->root_item,
-			   btrfs_item_ptr_offset(leaf, path->slots[0]),
-			   sizeof(tmp_root->root_item));
+	ret = read_extent_buffer_root_item(leaf, &tmp_root->root_item,
+					   path->slots[0]);
+	if (ret < 0) {
+		struct btrfs_key key;
+
+		btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
+		error("bad root item size for key (%llu %u %llu)",
+		      key.objectid, key.type, key.offset);
+		return ret;
+	}
 
 	tmp_root->root_key.objectid = root_id;
 	tmp_root->root_key.type = BTRFS_ROOT_ITEM_KEY;
@@ -10403,6 +10417,7 @@ static int maybe_repair_root_item(struct btrfs_path *path,
 	struct root_item_info *rii;
 	struct btrfs_root_item ri;
 	unsigned long offset;
+	int ret;
 
 	entry = lookup_cache_extent(roots_info_cache, root_id, 1);
 	if (!entry) {
@@ -10424,7 +10439,12 @@ static int maybe_repair_root_item(struct btrfs_path *path,
 	}
 
 	offset = btrfs_item_ptr_offset(path->nodes[0], path->slots[0]);
-	read_extent_buffer(path->nodes[0], &ri, offset, sizeof(ri));
+	ret = read_extent_buffer_root_item(path->nodes[0], &ri, path->slots[0]);
+	if (ret < 0) {
+		error("bad root item size for key (%llu %u %llu)",
+		      root_key->objectid, root_key->type, root_key->offset);
+		return ret;
+	}
 
 	if (btrfs_root_bytenr(&ri) != rii->bytenr ||
 	    btrfs_root_level(&ri) != rii->level ||
diff --git a/cmds/inspect-dump-tree.c b/cmds/inspect-dump-tree.c
index a9b369b4aba2..3df9ee27b4ab 100644
--- a/cmds/inspect-dump-tree.c
+++ b/cmds/inspect-dump-tree.c
@@ -274,7 +274,6 @@ static int cmd_inspect_dump_tree(const struct cmd_struct *cmd,
 	struct btrfs_fs_info *info;
 	BTRFS_PATH_AUTO_RELEASE(path);
 	struct btrfs_key key;
-	struct btrfs_root_item ri;
 	struct extent_buffer *leaf;
 	struct btrfs_disk_key disk_key;
 	struct btrfs_key found_key;
@@ -539,15 +538,20 @@ again:
 		btrfs_item_key(leaf, &disk_key, path.slots[0]);
 		btrfs_disk_key_to_cpu(&found_key, &disk_key);
 		if (found_key.type == BTRFS_ROOT_ITEM_KEY) {
-			unsigned long offset;
+			struct btrfs_root_item ri;
 			struct extent_buffer *buf;
 			struct btrfs_tree_parent_check check = {
 				.owner_root = key.objectid,
 			};
 			bool skip = (extent_only || device_only || uuid_tree_only);
 
-			offset = btrfs_item_ptr_offset(leaf, slot);
-			read_extent_buffer(leaf, &ri, offset, sizeof(ri));
+			ret = read_extent_buffer_root_item(leaf, &ri, slot);
+			if (ret < 0) {
+				error("bad root item size for key (%llu %u %llu)",
+				      found_key.objectid, found_key.type,
+				      found_key.offset);
+				goto next;
+			}
 			buf = read_tree_block(info, btrfs_root_bytenr(&ri), &check);
 			if (!extent_buffer_uptodate(buf))
 				goto next;
diff --git a/cmds/restore.c b/cmds/restore.c
index ac00faac1d02..f61a1c0847e3 100644
--- a/cmds/restore.c
+++ b/cmds/restore.c
@@ -1156,9 +1156,6 @@ static int do_list_roots(struct btrfs_root *root)
 	struct btrfs_disk_key disk_key;
 	BTRFS_PATH_AUTO_RELEASE(path);
 	struct extent_buffer *leaf;
-	struct btrfs_root_item ri;
-	unsigned long offset;
-	int slot;
 	int ret;
 
 	root = root->fs_info->tree_root;
@@ -1175,6 +1172,9 @@ static int do_list_roots(struct btrfs_root *root)
 	leaf = path.nodes[0];
 
 	while (1) {
+		struct btrfs_root_item ri;
+		int slot;
+
 		slot = path.slots[0];
 		if (slot >= btrfs_header_nritems(leaf)) {
 			ret = btrfs_next_leaf(root, &path);
@@ -1190,8 +1190,14 @@ static int do_list_roots(struct btrfs_root *root)
 			continue;
 		}
 
-		offset = btrfs_item_ptr_offset(leaf, slot);
-		read_extent_buffer(leaf, &ri, offset, sizeof(ri));
+		ret = read_extent_buffer_root_item(leaf, &ri, slot);
+		if (ret < 0) {
+			error("bad root item size for key (%llu %u %llu)",
+			      found_key.objectid, found_key.type,
+			      found_key.offset);
+			break;
+		}
+
 		printf(" tree ");
 		btrfs_print_key(&disk_key);
 		printf(" %llu level %d\n", btrfs_root_bytenr(&ri),
diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 385b74bbac3c..aeda71171ff2 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -644,9 +644,12 @@ struct btrfs_root *btrfs_read_fs_root_no_cache(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 	l = path->nodes[0];
-	read_extent_buffer(l, &root->root_item,
-	       btrfs_item_ptr_offset(l, path->slots[0]),
-	       sizeof(root->root_item));
+	ret = read_extent_buffer_root_item(l, &root->root_item, path->slots[0]);
+	if (ret < 0) {
+		error("bad root item size for key (%llu %u %llu)",
+		      location->objectid, location->type, location->offset);
+		goto out;
+	}
 	memcpy(&root->root_key, location, sizeof(*location));
 	ret = 0;
 out:
@@ -1083,10 +1086,14 @@ static int load_global_roots_objectid(struct btrfs_fs_info *fs_info,
 		}
 
 		btrfs_setup_root(root, fs_info, objectid);
-		read_extent_buffer(path->nodes[0], &root->root_item,
-				   btrfs_item_ptr_offset(path->nodes[0],
-							 path->slots[0]),
-				   sizeof(root->root_item));
+		ret = read_extent_buffer_root_item(path->nodes[0],
+				&root->root_item, path->slots[0]);
+		if (ret < 0) {
+			error("bad root item size for key (%llu %u %llu)",
+			      key.objectid, key.type, key.offset);
+			kfree(root);
+			break;
+		}
 		memcpy(&root->root_key, &key, sizeof(key));
 		ret = read_root_or_create_block(fs_info, root,
 				btrfs_root_bytenr(&root->root_item),
diff --git a/kernel-shared/extent_io.c b/kernel-shared/extent_io.c
index 7f74126327c2..2c090bad94ae 100644
--- a/kernel-shared/extent_io.c
+++ b/kernel-shared/extent_io.c
@@ -624,6 +624,40 @@ void read_extent_buffer(const struct extent_buffer *eb, void *dst,
 	memcpy(dst, eb->data + start, len);
 }
 
+/*
+ * The version with extra helper to read the buffer with a size check.
+ *
+ * @min_size:	The minimal size the structure should have.
+ *		If @len is smaller than this, the read is rejected.
+ * @max_size:	Max size of the structure. Any data beyond that will
+ *		be discarded.
+ */
+static int read_eb_structure(const struct extent_buffer *eb,
+			void *dst, size_t min_size, size_t max_size,
+			unsigned long start, unsigned long len)
+{
+	if (unlikely(len < min_size))
+		return -EIO;
+	/*
+	 * We still allow an item larger than expected, but the read
+	 * will be trucnated to the max size.
+	 *
+	 * It shouldn't cause overflow as the length is truncated, and will
+	 * handle future structure expansion well.
+	 */
+	read_extent_buffer(eb, dst, start, min_t(unsigned long, len, max_size));
+	return 0;
+}
+
+int read_extent_buffer_root_item(const struct extent_buffer *leaf,
+				 struct btrfs_root_item *ri, int slot)
+{
+	memset(ri, 0, sizeof(*ri));
+	return read_eb_structure(leaf, ri, btrfs_legacy_root_item_size(),
+			sizeof(*ri), btrfs_item_ptr_offset(leaf, slot),
+			btrfs_item_size(leaf, slot));
+}
+
 void write_extent_buffer_fsid(struct extent_buffer *eb, const void *src)
 {
 	write_extent_buffer(eb, src, btrfs_header_fsid(), BTRFS_FSID_SIZE);
diff --git a/kernel-shared/extent_io.h b/kernel-shared/extent_io.h
index 9be4ccfc2d7a..776cdba4f293 100644
--- a/kernel-shared/extent_io.h
+++ b/kernel-shared/extent_io.h
@@ -54,6 +54,7 @@ static inline int le_test_bit(int nr, const u8 *addr)
 
 struct btrfs_fs_info;
 struct btrfs_trans_handle;
+struct btrfs_root_item;
 
 struct extent_buffer {
 	struct cache_extent cache_node;
@@ -109,6 +110,8 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv,
 			 unsigned long start, unsigned long len);
 void read_extent_buffer(const struct extent_buffer *eb, void *dst,
 			unsigned long start, unsigned long len);
+int read_extent_buffer_root_item(const struct extent_buffer *eb,
+				 struct btrfs_root_item *ri, int slot);
 void write_extent_buffer_fsid(struct extent_buffer *eb, const void *src);
 void write_extent_buffer_chunk_tree_uuid(struct extent_buffer *eb,
 		const void *src);
diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
index 832ce0678e71..3ce525b6e546 100644
--- a/kernel-shared/print-tree.c
+++ b/kernel-shared/print-tree.c
@@ -657,12 +657,16 @@ static void print_root_item(struct extent_buffer *leaf, int slot)
 	char uuid_str[BTRFS_UUID_UNPARSED_SIZE];
 	char flags_str[32] = {0};
 	struct btrfs_key drop_key;
+	int ret;
+
+	ret = read_extent_buffer_root_item(leaf, &root_item, slot);
+	if (ret < 0) {
+		printf("\t\tbad root item size, skipping\n");
+		return;
+	}
 
 	ri = btrfs_item_ptr(leaf, slot, struct btrfs_root_item);
 	len = btrfs_item_size(leaf, slot);
-
-	memset(&root_item, 0, sizeof(root_item));
-	read_extent_buffer(leaf, &root_item, (unsigned long)ri, len);
 	root_flags_to_str(btrfs_root_flags(&root_item), flags_str);
 
 	printf("\t\tgeneration %llu root_dirid %llu bytenr %llu byte_limit %llu bytes_used %llu\n",
@@ -681,6 +685,12 @@ static void print_root_item(struct extent_buffer *leaf, int slot)
 	btrfs_print_key(&root_item.drop_progress);
 	printf(" drop_level %hhu\n", root_item.drop_level);
 
+	/* Skip newer members for older root item. */
+	if (len < sizeof(root_item)) {
+		printf("\t\tlevel %hhu\n", btrfs_root_level(&root_item));
+		return;
+	}
+
 	printf("\t\tlevel %hhu generation_v2 %llu\n",
 		btrfs_root_level(&root_item), root_item.generation_v2);
 
diff --git a/kernel-shared/root-tree.c b/kernel-shared/root-tree.c
index 39c3f9027b68..03140ceaf097 100644
--- a/kernel-shared/root-tree.c
+++ b/kernel-shared/root-tree.c
@@ -62,8 +62,12 @@ int btrfs_find_last_root(struct btrfs_root *root, u64 objectid,
 		ret = -ENOENT;
 		goto out;
 	}
-	read_extent_buffer(l, item, btrfs_item_ptr_offset(l, slot),
-			   sizeof(*item));
+	ret = read_extent_buffer_root_item(l, item, slot);
+	if (ret < 0) {
+		error("bad root item size for key (%llu %u %llu)",
+		      found_key.objectid, found_key.type, found_key.offset);
+		goto out;
+	}
 	memcpy(key, &found_key, sizeof(found_key));
 	ret = 0;
 out:
-- 
2.56.0

[PATCH 3/3] btrfs-progs: fix two sites that may write beyond root item size

[Qu Wenruo]

The following two call sites are writing a root item without item size
check:

- btrfs_fsck_clear_root()
- maybe_repair_root_item()

Although the root item size is already checked at read time, at write
time we should clamp the write size between item size and sizeof(ri), to
avoid write into the next item or even beyond the heap memory boundary.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/check/main.c b/check/main.c
index baf4c283527f..d66819048c02 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9532,6 +9532,7 @@ static struct extent_buffer *btrfs_fsck_clear_root(
 	struct btrfs_path *path;
 	struct extent_buffer *c;
 	struct btrfs_disk_key disk_key = {};
+	u32 write_size;
 	int ret;
 
 	path = btrfs_alloc_path();
@@ -9579,6 +9580,8 @@ static struct extent_buffer *btrfs_fsck_clear_root(
 		free_extent_buffer(c);
 		return ERR_PTR(ret);
 	}
+	write_size = min_t(u32, sizeof(ri),
+			   btrfs_item_size(path->nodes[0], path->slots[0]));
 	btrfs_set_root_bytenr(&ri, c->start);
 	btrfs_set_root_generation(&ri, trans->transid);
 	btrfs_set_root_refs(&ri, 1);
@@ -9588,7 +9591,7 @@ static struct extent_buffer *btrfs_fsck_clear_root(
 	write_extent_buffer(path->nodes[0], &ri,
 			    btrfs_item_ptr_offset(path->nodes[0],
 						  path->slots[0]),
-			    sizeof(ri));
+			    write_size);
 	btrfs_mark_buffer_dirty(path->nodes[0]);
 	btrfs_free_path(path);
 	return c;
@@ -10476,11 +10479,15 @@ static int maybe_repair_root_item(struct btrfs_path *path,
 		}
 
 		if (!read_only_mode) {
+			const u32 write_size = min_t(u32, sizeof(ri),
+					btrfs_item_size(path->nodes[0],
+							path->slots[0]));
+
 			btrfs_set_root_bytenr(&ri, rii->bytenr);
 			btrfs_set_root_level(&ri, rii->level);
 			btrfs_set_root_generation(&ri, rii->gen);
 			write_extent_buffer(path->nodes[0], &ri,
-					    offset, sizeof(ri));
+					    offset, write_size);
 		}
 
 		return 1;
-- 
2.56.0