btrfs: fix a NULL pointer dereference
diff mbox series

Message ID 20190314075041.28966-1-kjlu@umn.edu
State New
Headers show
Series
  • btrfs: fix a NULL pointer dereference
Related show

Commit Message

Kangjie Lu March 14, 2019, 7:50 a.m. UTC
btrfs_lookup_block_group may fail and return NULL. The fix goes
to out when it fails to avoid NULL pointer dereference.

Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
 fs/btrfs/extent-tree.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Nikolay Borisov March 14, 2019, 7:54 a.m. UTC | #1
On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
> btrfs_lookup_block_group may fail and return NULL. The fix goes
> to out when it fails to avoid NULL pointer dereference.

Actually no, in this case btrfs_lookup_block_group must never fail
because if we have an allocated eb then it must have been allocated from
a bg.

> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>  fs/btrfs/extent-tree.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 994f0cc41799..b1e7985bcb9d 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>  
>  		pin = 0;
>  		cache = btrfs_lookup_block_group(fs_info, buf->start);
> +		if (!cache)
> +			goto out;
>  
>  		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>  			pin_down_extent(fs_info, cache, buf->start,
>
Qu Wenruo March 14, 2019, 7:59 a.m. UTC | #2
On 2019/3/14 下午3:50, Kangjie Lu wrote:
> btrfs_lookup_block_group may fail and return NULL. The fix goes
> to out when it fails to avoid NULL pointer dereference.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>  fs/btrfs/extent-tree.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 994f0cc41799..b1e7985bcb9d 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>  
>  		pin = 0;
>  		cache = btrfs_lookup_block_group(fs_info, buf->start);
> +		if (!cache)
> +			goto out;

The check itself is OK.

Reviewed-by: Qu Wenruo <wqu@suse.com>

The problem is, here we're freeing a tree block, if there is no block
group for it, we shouldn't be able to read the extent buffer out.

So it's near impossible to hit. (Unless there is some other things wrong)

Thanks,
Qu

>  
>  		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>  			pin_down_extent(fs_info, cache, buf->start,
>
Qu Wenruo March 14, 2019, 8:02 a.m. UTC | #3
On 2019/3/14 下午3:54, Nikolay Borisov wrote:
> 
> 
> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>> to out when it fails to avoid NULL pointer dereference.
> 
> Actually no, in this case btrfs_lookup_block_group must never fail
> because if we have an allocated eb then it must have been allocated from
> a bg.

Yep, that's the normal case.

However I'm wondering if it's possible to get a bad eb which is cached.

Then we could hit such situation.

So I still believe being safe here still makes sense, especially who
knows future fuzzed image will be.

Thanks,
Qu

> 
>>
>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>> ---
>>  fs/btrfs/extent-tree.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 994f0cc41799..b1e7985bcb9d 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>  
>>  		pin = 0;
>>  		cache = btrfs_lookup_block_group(fs_info, buf->start);
>> +		if (!cache)
>> +			goto out;
>>  
>>  		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>>  			pin_down_extent(fs_info, cache, buf->start,
>>
Nikolay Borisov March 14, 2019, 8:03 a.m. UTC | #4
On 14.03.19 г. 10:02 ч., Qu Wenruo wrote:
> 
> 
> On 2019/3/14 下午3:54, Nikolay Borisov wrote:
>>
>>
>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>>> to out when it fails to avoid NULL pointer dereference.
>>
>> Actually no, in this case btrfs_lookup_block_group must never fail
>> because if we have an allocated eb then it must have been allocated from
>> a bg.
> 
> Yep, that's the normal case.
> 
> However I'm wondering if it's possible to get a bad eb which is cached.
> 
> Then we could hit such situation.
> 
> So I still believe being safe here still makes sense, especially who
> knows future fuzzed image will be.

Then I'd rather have ASSERT(cache)

> 
> Thanks,
> Qu
> 
>>
>>>
>>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>>> ---
>>>  fs/btrfs/extent-tree.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 994f0cc41799..b1e7985bcb9d 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>>  
>>>  		pin = 0;
>>>  		cache = btrfs_lookup_block_group(fs_info, buf->start);
>>> +		if (!cache)
>>> +			goto out;
>>>  
>>>  		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>>>  			pin_down_extent(fs_info, cache, buf->start,
>>>
>
Qu Wenruo March 14, 2019, 9:15 a.m. UTC | #5
On 2019/3/14 下午4:03, Nikolay Borisov wrote:
> 
> 
> On 14.03.19 г. 10:02 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/3/14 下午3:54, Nikolay Borisov wrote:
>>>
>>>
>>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>>>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>>>> to out when it fails to avoid NULL pointer dereference.
>>>
>>> Actually no, in this case btrfs_lookup_block_group must never fail
>>> because if we have an allocated eb then it must have been allocated from
>>> a bg.
>>
>> Yep, that's the normal case.
>>
>> However I'm wondering if it's possible to get a bad eb which is cached.
>>
>> Then we could hit such situation.
>>
>> So I still believe being safe here still makes sense, especially who
>> knows future fuzzed image will be.
> 
> Then I'd rather have ASSERT(cache)

Isn't assert() a bad idea for production build without assert() support?

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>>>> ---
>>>>  fs/btrfs/extent-tree.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>> index 994f0cc41799..b1e7985bcb9d 100644
>>>> --- a/fs/btrfs/extent-tree.c
>>>> +++ b/fs/btrfs/extent-tree.c
>>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>>>  
>>>>  		pin = 0;
>>>>  		cache = btrfs_lookup_block_group(fs_info, buf->start);
>>>> +		if (!cache)
>>>> +			goto out;
>>>>  
>>>>  		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>>>>  			pin_down_extent(fs_info, cache, buf->start,
>>>>
>>
Nikolay Borisov March 14, 2019, 9:18 a.m. UTC | #6
On 14.03.19 г. 11:15 ч., Qu Wenruo wrote:
> Isn't assert() a bad idea for production build without assert() support?

As already discussed this is invariant of the code, if this invariant is
broken then in production builds we'd likely crash (which is good since
we want to understand why and not silently handle it). In debug builds
the assert will be triggered.
Su Yue March 14, 2019, 10:23 a.m. UTC | #7
On 2019/3/14 4:02 PM, Qu Wenruo wrote:
> 
> 
> On 2019/3/14 下午3:54, Nikolay Borisov wrote:
>>
>>
>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>>> to out when it fails to avoid NULL pointer dereference.
>>
>> Actually no, in this case btrfs_lookup_block_group must never fail
>> because if we have an allocated eb then it must have been allocated from
>> a bg.
> 
> Yep, that's the normal case.
> 
> However I'm wondering if it's possible to get a bad eb which is cached.
> 
> Then we could hit such situation.
> 
> So I still believe being safe here still makes sense, especially who
> knows future fuzzed image will be.

Plus one.
Personally, I'd rather like the version 1.

Thanks,
Su
> 
> Thanks,
> Qu
> 
>>
>>>
>>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>>> ---
>>>   fs/btrfs/extent-tree.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 994f0cc41799..b1e7985bcb9d 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>>   
>>>   		pin = 0;
>>>   		cache = btrfs_lookup_block_group(fs_info, buf->start);
>>> +		if (!cache)
>>> +			goto out;
>>>   
>>>   		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>>>   			pin_down_extent(fs_info, cache, buf->start,
>>>
Kangjie Lu March 14, 2019, 3:26 p.m. UTC | #8
On 3/14/19 4:15 AM, Qu Wenruo wrote:
>
> On 2019/3/14 下午4:03, Nikolay Borisov wrote:
>>
>> On 14.03.19 г. 10:02 ч., Qu Wenruo wrote:
>>>
>>> On 2019/3/14 下午3:54, Nikolay Borisov wrote:
>>>>
>>>> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
>>>>> btrfs_lookup_block_group may fail and return NULL. The fix goes
>>>>> to out when it fails to avoid NULL pointer dereference.
>>>> Actually no, in this case btrfs_lookup_block_group must never fail
>>>> because if we have an allocated eb then it must have been allocated from
>>>> a bg.
>>> Yep, that's the normal case.
>>>
>>> However I'm wondering if it's possible to get a bad eb which is cached.
>>>
>>> Then we could hit such situation.
>>>
>>> So I still believe being safe here still makes sense, especially who
>>> knows future fuzzed image will be.
>> Then I'd rather have ASSERT(cache)
> Isn't assert() a bad idea for production build without assert() support?

I also agree with that, in general, assert should not be used in

production runs. The first patch might be better.


>
> Thanks,
> Qu
>
>>> Thanks,
>>> Qu
>>>
>>>>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>>>>> ---
>>>>>   fs/btrfs/extent-tree.c | 2 ++
>>>>>   1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>>> index 994f0cc41799..b1e7985bcb9d 100644
>>>>> --- a/fs/btrfs/extent-tree.c
>>>>> +++ b/fs/btrfs/extent-tree.c
>>>>> @@ -7303,6 +7303,8 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>>>>>   
>>>>>   		pin = 0;
>>>>>   		cache = btrfs_lookup_block_group(fs_info, buf->start);
>>>>> +		if (!cache)
>>>>> +			goto out;
>>>>>   
>>>>>   		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>>>>>   			pin_down_extent(fs_info, cache, buf->start,
>>>>>
Josef Bacik March 14, 2019, 3:41 p.m. UTC | #9
On Thu, Mar 14, 2019 at 09:54:07AM +0200, Nikolay Borisov wrote:
> 
> 
> On 14.03.19 г. 9:50 ч., Kangjie Lu wrote:
> > btrfs_lookup_block_group may fail and return NULL. The fix goes
> > to out when it fails to avoid NULL pointer dereference.
> 
> Actually no, in this case btrfs_lookup_block_group must never fail
> because if we have an allocated eb then it must have been allocated from
> a bg.
> 

Agreed, we only get to btrfs_free_tree_block() if we are actually deleting the
extent buffer.  We would have had to read in the extent buffer first to get
here, which would have failed if there was no block group.  We can't get into
this situation with a specifically crafted file system to exploit this as we'd
bail out well before we could get to btrfs_free_tree_block().  Adding an
ASSERT() makes sure developers aren't doing anything stupid, but again we'd have
to be doing something _super_ stupid to hit it.  Thanks,

Josef
David Sterba March 25, 2019, 4:35 p.m. UTC | #10
On Thu, Mar 14, 2019 at 02:50:40AM -0500, Kangjie Lu wrote:
> btrfs_lookup_block_group may fail and return NULL. The fix goes
> to out when it fails to avoid NULL pointer dereference.

The subject, changelog and code change are not valid anymore after the
discussion. Please update them accordingly and resend. Thanks.

Patch
diff mbox series

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 994f0cc41799..b1e7985bcb9d 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -7303,6 +7303,8 @@  void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
 
 		pin = 0;
 		cache = btrfs_lookup_block_group(fs_info, buf->start);
+		if (!cache)
+			goto out;
 
 		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
 			pin_down_extent(fs_info, cache, buf->start,