Re: [PATCH v3] btrfs: validate root ref names in tree-checker

[Qu Wenruo <wqu@suse.com>]

在 2026/5/11 00:33, Zhang Cen 写道:
> ROOT_REF and ROOT_BACKREF items contain a struct btrfs_root_ref followed
> by the subvolume name. Several readers assume that this layout is already
> valid and then use the on-disk name length directly. A corrupted item can
> therefore make those readers address bytes outside the item, and
> BTRFS_IOC_GET_SUBVOL_INFO can copy too many bytes into its fixed-size UAPI
> name buffer.
> 
> Validate ROOT_REF and ROOT_BACKREF items in tree-checker before any reader
> uses them. Reject items smaller than struct btrfs_root_ref, reject records
> whose name_len does not exactly describe the remaining item payload, and
> reject names longer than BTRFS_SUBVOL_NAME_MAX.
> 
> For BTRFS_IOC_GET_SUBVOL_INFO, copy only the validated on-disk name_len and
> keep a local BTRFS_VOL_NAME_MAX guard for the fixed ioctl output field.
> Terminate the copied name explicitly.
> 
> Sanitizer validation reported:
> BUG: KASAN: slab-out-of-bounds in read_extent_buffer()
> Write of size 505 at addr ffff88810936d608
> Call trace:
>    read_extent_buffer()
>    btrfs_ioctl_get_subvol_info()
> 
> Signed-off-by: Zhang Cen <rollkingzzc@gmail.com>
> ---
> Changes since v2:
> - Regenerate the mail-ready patch without the nested mbox wrapper.
> - No code changes beyond the v2 fix.
> 
>   fs/btrfs/ioctl.c       | 17 +++++++++++++----
>   fs/btrfs/tree-checker.c | 39 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 52 insertions(+), 4 deletions(-)
> diff --git aa/fs/btrfs/ioctl.c bb/fs/btrfs/ioctl.c
> index a39460bf68a7..b43d33eaea07 100644
> --- aa/fs/btrfs/ioctl.c
> +++ bb/fs/btrfs/ioctl.c
> @@ -1956,7 +1956,8 @@ static int btrfs_ioctl_get_subvol_info(struct inode *inode, void __user *argp)
>   	struct btrfs_root_ref *rref;
>   	struct extent_buffer *leaf;
>   	unsigned long item_off;
> -	unsigned long item_len;
> +	u32 item_size;

Move the declaration where it's really used, aka, the if () branch where 
it's first utilized.

> +	u16 name_len;
>   	int slot;
>   	int ret = 0;
>   
> @@ -2034,14 +2035,21 @@ static int btrfs_ioctl_get_subvol_info(struct inode *inode, void __user *argp)
>   			subvol_info->parent_id = key.offset;
>   
>   			rref = btrfs_item_ptr(leaf, slot, struct btrfs_root_ref);
> +			item_size = btrfs_item_size(leaf, slot);
> +			name_len = btrfs_root_ref_name_len(leaf, rref);
> +			if (name_len > item_size - sizeof(*rref) ||
> +			    name_len > BTRFS_VOL_NAME_MAX) {

Those conditions are already checked in tree-checker.
And you return -EUCLEAN without any error message.

An ASSERT() will be more than enough, and I even prefer not to have an 
ASSERT().

And that's the idea of tree-checker, we validate everything (that we can 
validate) in tree-checker, so that callers won't need to bother 
re-checking them at random location again and again.

> +				ret = -EUCLEAN;
> +				goto out;
> +			}
> +
>   			subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
>   
> -			item_off = btrfs_item_ptr_offset(leaf, slot)
> -					+ sizeof(struct btrfs_root_ref);
> -			item_len = btrfs_item_size(leaf, slot)
> -					- sizeof(struct btrfs_root_ref);
> +			item_off = btrfs_item_ptr_offset(leaf, slot) +
> +				   sizeof(*rref);
>   			read_extent_buffer(leaf, subvol_info->name,
> -					   item_off, item_len);
> +					   item_off, name_len);
> +			subvol_info->name[name_len] = '\0';

No need to manually add terminating \0.

subvol_info is allocated by kzalloc_obj(), all members in 
subvol_info->name[] should be zero, and the BTRFS_VOL_NAME_MAX check 
ensured there is at least one byte left in name[] that is not touched.

>   		} else {
>   			ret = -ENOENT;
>   			goto out;
> diff --git aa/fs/btrfs/tree-checker.c bb/fs/btrfs/tree-checker.c
> index 1f15d0793a9c..41417b2df32d 100644
> --- aa/fs/btrfs/tree-checker.c
> +++ bb/fs/btrfs/tree-checker.c
> @@ -1371,6 +1371,38 @@ static int check_root_item(struct extent_buffer *leaf, struct btrfs_key *key,
>   	return 0;
>   }
>   
> +static int check_root_ref(struct extent_buffer *leaf, struct btrfs_key *key,
> +				  int slot)
> +{
> +	struct btrfs_root_ref *rref;
> +	u32 item_size = btrfs_item_size(leaf, slot);
> +	u32 name_len;
> +
> +	if (unlikely(item_size < sizeof(*rref))) {

There should be a name_len that is not zero.
So the check should be (item_size <= sizeof(*ref)).

Thanks,
Qu