Re: [PATCH v4 03/53] btrfs: modify the new_root highest_objectid under a ref count

[Nikolay Borisov <nborisov@suse.com>]

On 4.12.20 г. 10:01 ч., Qu Wenruo wrote:
> 
> 
> On 2020/12/4 上午2:22, Josef Bacik wrote:
>> Qu pointed out a bug in one of my error handling patches, which made me
>> notice that we modify the new_root->highest_objectid _after_ we've
>> dropped the ref to the new_root.  This could lead to a possible UAF, fix
>> this by modifying the ->highest_objectid before we drop our reference to
>> the new_root.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> But found something to cleanup in the future, inlined below.
>> ---
>>  fs/btrfs/ioctl.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 703212ff50a5..f240beed4739 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -717,6 +717,12 @@ static noinline int create_subvol(struct inode *dir,
>>  	btrfs_record_root_in_trans(trans, new_root);
>>  
>>  	ret = btrfs_create_subvol_root(trans, new_root, root, new_dirid);
> 
> Firstly, btrfs_create_subvol_root() is only called here once, and
> new_dirid is always a fixed value, BTRFS_FIRST_FREE_OBJECTID.
> 
> This means, we don't need the parameter at all.
> 
>> +	if (!ret) {
>> +		mutex_lock(&new_root->objectid_mutex);
>> +		new_root->highest_objectid = new_dirid;
>> +		mutex_unlock(&nBut still find something suspicious for the existing naming, inlined below.ew_root->objectid_mutex);
>> +	}
>> +
> 
> Secondly, new_root is a new subvolume root which just get allocated.
> It looks more sane to initialize the highest_objectid inside
> btrfs_get_root_ref() for new root.
> 
> This should reduce the chance to hit such use-after-free bug completely.

Actually btrfs_init_fs_root already does :

    42         ret = btrfs_find_highest_objectid(root,

    43                                         &root->highest_objectid);

Where root would be the newly initialized tree so highest_objectid
should already be correctly initialized. However, looking at the source
of btrfs_find_highest_objectid why do we do BTRFS_FIRST_FREE_OBJECTID -
1 there?

<snip>