[PATCH] btrfs: fix a NULL pointer dereference

[PATCH] btrfs: fix a NULL pointer dereference

[Kangjie Lu]

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;
 
 		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
 			pin_down_extent(fs_info, cache, buf->start,
-- 
2.17.1

Re: [PATCH] btrfs: fix a NULL pointer dereference

[Nikolay Borisov]

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,
> 

Re: [PATCH] btrfs: fix a NULL pointer dereference

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1103 bytes --]



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,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: fix a NULL pointer dereference

[Qu Wenruo]

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,
>>

Re: [PATCH] btrfs: fix a NULL pointer dereference

[Nikolay Borisov]

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,
>>>
> 

[PATCH v2] btrfs: fix a NULL pointer dereference

[Kangjie Lu]

btrfs_lookup_block_group may fail and return NULL. The fix uses
assert to ensure cache is not NULL.

Signed-off-by: Kangjie Lu <kjlu@umn.edu>

---
V2: use assert as suggested by Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 1 +
 1 file changed, 1 insertion(+)

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

Re: [PATCH v2] btrfs: fix a NULL pointer dereference

[Nikolay Borisov]

On 14.03.19 г. 10:13 ч., Kangjie Lu wrote:
> btrfs_lookup_block_group may fail and return NULL. The fix uses
> assert to ensure cache is not NULL.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> 
> ---
> V2: use assert as suggested by Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 994f0cc41799..80d7c272d282 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -7303,6 +7303,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>  
>  		pin = 0;
>  		cache = btrfs_lookup_block_group(fs_info, buf->start);
> +		ASSERT(cache);
>  
>  		if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
>  			pin_down_extent(fs_info, cache, buf->start,
> 

Re: [PATCH] btrfs: fix a NULL pointer dereference

[Qu Wenruo]

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,
>>>>
>>

Re: [PATCH] btrfs: fix a NULL pointer dereference

[Nikolay Borisov]

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.

Re: [PATCH] btrfs: fix a NULL pointer dereference

[Su Yue]

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,
>>>

Re: [PATCH] btrfs: fix a NULL pointer dereference

[Kangjie Lu]

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,
>>>>>

Re: [PATCH] btrfs: fix a NULL pointer dereference

[Josef Bacik]

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

Re: [PATCH] btrfs: fix a NULL pointer dereference

[David Sterba]

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.