Re: [PATCh v2 9/9] btrfs: inode: Verify inode mode to avoid NULL pointer dereference

[Qu Wenruo <wqu@suse.de>]

On 2019/3/28 下午9:53, David Sterba wrote:
> On Wed, Mar 20, 2019 at 02:37:17PM +0800, Qu Wenruo wrote:
>> @@ -5726,18 +5734,29 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
>>  	struct btrfs_root *root = BTRFS_I(dir)->root;
>>  	struct btrfs_root *sub_root = root;
>>  	struct btrfs_key location;
>> +	u8 di_type = 0;
>>  	int index;
>>  	int ret = 0;
>>  
>>  	if (dentry->d_name.len > BTRFS_NAME_LEN)
>>  		return ERR_PTR(-ENAMETOOLONG);
>>  
>> -	ret = btrfs_inode_by_name(dir, dentry, &location);
>> +	ret = btrfs_inode_by_name(dir, dentry, &location, &di_type);
>>  	if (ret < 0)
>>  		return ERR_PTR(ret);
>>  
>>  	if (location.type == BTRFS_INODE_ITEM_KEY) {
>>  		inode = btrfs_iget(dir->i_sb, &location, root, NULL);
>> +
>> +		/* Do extra check against inode mode with di_type */
>> +		if (btrfs_inode_type(inode) != di_type) {
>> +			btrfs_crit(fs_info,
>> +"inode mode mismatch with dir: inode mode=0%o btrfs type=%u dir type=%u",
>> +				  inode->i_mode, btrfs_inode_type(inode),
>> +				  di_type);
>> +			iput(inode);
> 
> The iput here seems suspicious.

How? this pairs to that inode = btrfs_iget() line.

And as long as we hit this branch, we will return either a valid inode
or an error pointer.
> 
>> +			return ERR_PTR(-EUCLEAN);
>> +		}
>>  		return inode;
>>  	}
> 
> 5792         index = srcu_read_lock(&fs_info->subvol_srcu);
> 5793         ret = fixup_tree_root_location(fs_info, dir, dentry,
> 5794                                        &location, &sub_root);
> 5795         if (ret < 0) {
> 5796                 if (ret != -ENOENT)
> 5797                         inode = ERR_PTR(ret);
> 5798                 else
> 5799                         inode = new_simple_dir(dir->i_sb, &location, sub_root);
> 5800         } else {
> 5801                 inode = btrfs_iget(dir->i_sb, &location, sub_root, NULL);
> 5802         }
> 
> The iget happens after this block so either there's another one in the caller
> to pair, or the above iput is wrong.

This block is unrelated to above (location.type == BTRFS_INODE_ITEM_KEY)
branch.

Or did I miss something?

Thanks,
Qu

>