fs: btrfs: fix possible use-after-free bug in error handling code of btrfs_get_root_ref()

[PATCH] fs: btrfs: fix possible use-after-free bug in error handling code of btrfs_get_root_ref()

Posted by Jia-Ju Bai 4 years, 2 months ago

In btrfs_get_root_ref(), when btrfs_insert_fs_root() fails,
btrfs_put_root() will be called to possibly free the memory area of
the variable root. However, this variable is then used again in error
handling code after "goto fail", when ret is not -EEXIST.

To fix this possible bug, btrfs_put_root() is only called when ret is 
-EEXIST for "goto again".

Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 fs/btrfs/disk-io.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b30309f187cf..126f244cdf88 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1850,9 +1850,10 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
 
 	ret = btrfs_insert_fs_root(fs_info, root);
 	if (ret) {
-		btrfs_put_root(root);
-		if (ret == -EEXIST)
+		if (ret == -EEXIST) {
+			btrfs_put_root(root);
 			goto again;
+		}
 		goto fail;
 	}
 	return root;
-- 
2.17.1

Re: [PATCH] fs: btrfs: fix possible use-after-free bug in error handling code of btrfs_get_root_ref()

Posted by David Sterba 4 years, 2 months ago

On Thu, Mar 24, 2022 at 06:44:54AM -0700, Jia-Ju Bai wrote:
> In btrfs_get_root_ref(), when btrfs_insert_fs_root() fails,
> btrfs_put_root() will be called to possibly free the memory area of
> the variable root. However, this variable is then used again in error
> handling code after "goto fail", when ret is not -EEXIST.
> 
> To fix this possible bug, btrfs_put_root() is only called when ret is 
> -EEXIST for "goto again".
> 
> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  fs/btrfs/disk-io.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index b30309f187cf..126f244cdf88 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1850,9 +1850,10 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
>  
>  	ret = btrfs_insert_fs_root(fs_info, root);
>  	if (ret) {
> -		btrfs_put_root(root);
> -		if (ret == -EEXIST)
> +		if (ret == -EEXIST) {
> +			btrfs_put_root(root);

I think this fix is correct, though it's not that clear. If you look how
the code changed, there was the unconditional put and then followed by a
free:

8c38938c7bb0 ("btrfs: move the root freeing stuff into btrfs_put_root")

Here it's putting twice where one will be the final free.

And then the whole refcounting gets updated in

4785e24fa5d2 ("btrfs: don't take an extra root ref at allocation time")

which could be removing the wrong put, I'm not yet sure.

Re: [PATCH] fs: btrfs: fix possible use-after-free bug in error handling code of btrfs_get_root_ref()

Posted by Jia-Ju Bai 4 years, 2 months ago


On 2022/3/25 2:19, David Sterba wrote:
> On Thu, Mar 24, 2022 at 06:44:54AM -0700, Jia-Ju Bai wrote:
>> In btrfs_get_root_ref(), when btrfs_insert_fs_root() fails,
>> btrfs_put_root() will be called to possibly free the memory area of
>> the variable root. However, this variable is then used again in error
>> handling code after "goto fail", when ret is not -EEXIST.
>>
>> To fix this possible bug, btrfs_put_root() is only called when ret is
>> -EEXIST for "goto again".
>>
>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>> ---
>>   fs/btrfs/disk-io.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index b30309f187cf..126f244cdf88 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -1850,9 +1850,10 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
>>   
>>   	ret = btrfs_insert_fs_root(fs_info, root);
>>   	if (ret) {
>> -		btrfs_put_root(root);
>> -		if (ret == -EEXIST)
>> +		if (ret == -EEXIST) {
>> +			btrfs_put_root(root);
> I think this fix is correct, though it's not that clear. If you look how
> the code changed, there was the unconditional put and then followed by a
> free:
>
> 8c38938c7bb0 ("btrfs: move the root freeing stuff into btrfs_put_root")
>
> Here it's putting twice where one will be the final free.
>
> And then the whole refcounting gets updated in
>
> 4785e24fa5d2 ("btrfs: don't take an extra root ref at allocation time")
>
> which could be removing the wrong put, I'm not yet sure.

Thanks for the reply!

I think the bug should be introduced by this commit:
bc44d7c4b2b1 ("btrfs: push btrfs_grab_fs_root into btrfs_get_fs_root")

This commit has a change:
      ret = btrfs_insert_fs_root(fs_info, root);
      if (ret) {
+      btrfs_put_fs_root(root);
          if (ret == -EEXIST) {
              btrfs_free_fs_root(root);
              goto again;
          }

I could add a Fixes tag of this commit in my V2 patch.
Is it okay?


Best wishes,
Jia-Ju Bai

Re: [PATCH] fs: btrfs: fix possible use-after-free bug in error handling code of btrfs_get_root_ref()

Posted by David Sterba 4 years, 2 months ago

On Fri, Mar 25, 2022 at 04:04:17PM +0800, Jia-Ju Bai wrote:
> >> @@ -1850,9 +1850,10 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
> >>   
> >>   	ret = btrfs_insert_fs_root(fs_info, root);
> >>   	if (ret) {
> >> -		btrfs_put_root(root);
> >> -		if (ret == -EEXIST)
> >> +		if (ret == -EEXIST) {
> >> +			btrfs_put_root(root);
> > I think this fix is correct, though it's not that clear. If you look how
> > the code changed, there was the unconditional put and then followed by a
> > free:
> >
> > 8c38938c7bb0 ("btrfs: move the root freeing stuff into btrfs_put_root")
> >
> > Here it's putting twice where one will be the final free.
> >
> > And then the whole refcounting gets updated in
> >
> > 4785e24fa5d2 ("btrfs: don't take an extra root ref at allocation time")
> >
> > which could be removing the wrong put, I'm not yet sure.
> 
> Thanks for the reply!
> 
> I think the bug should be introduced by this commit:
> bc44d7c4b2b1 ("btrfs: push btrfs_grab_fs_root into btrfs_get_fs_root")
> 
> This commit has a change:
>       ret = btrfs_insert_fs_root(fs_info, root);
>       if (ret) {
> +      btrfs_put_fs_root(root);
>           if (ret == -EEXIST) {
>               btrfs_free_fs_root(root);
>               goto again;
>           }
> 
> I could add a Fixes tag of this commit in my V2 patch.
> Is it okay?

I can add it myself, that's a minor thing. The fix is correct, I've
rewritten the changelog a bit, patch now added to misc-next, thanks.

Re: [PATCH] fs: btrfs: fix possible use-after-free bug in error handling code of btrfs_get_root_ref()

Posted by Lee Jones 2 years, 10 months ago

On Fri, 01 Apr 2022, David Sterba wrote:

> On Fri, Mar 25, 2022 at 04:04:17PM +0800, Jia-Ju Bai wrote:
> > >> @@ -1850,9 +1850,10 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
> > >>   
> > >>   	ret = btrfs_insert_fs_root(fs_info, root);
> > >>   	if (ret) {
> > >> -		btrfs_put_root(root);
> > >> -		if (ret == -EEXIST)
> > >> +		if (ret == -EEXIST) {
> > >> +			btrfs_put_root(root);
> > > I think this fix is correct, though it's not that clear. If you look how
> > > the code changed, there was the unconditional put and then followed by a
> > > free:
> > >
> > > 8c38938c7bb0 ("btrfs: move the root freeing stuff into btrfs_put_root")
> > >
> > > Here it's putting twice where one will be the final free.
> > >
> > > And then the whole refcounting gets updated in
> > >
> > > 4785e24fa5d2 ("btrfs: don't take an extra root ref at allocation time")
> > >
> > > which could be removing the wrong put, I'm not yet sure.
> > 
> > Thanks for the reply!
> > 
> > I think the bug should be introduced by this commit:
> > bc44d7c4b2b1 ("btrfs: push btrfs_grab_fs_root into btrfs_get_fs_root")
> > 
> > This commit has a change:
> >       ret = btrfs_insert_fs_root(fs_info, root);
> >       if (ret) {
> > +      btrfs_put_fs_root(root);
> >           if (ret == -EEXIST) {
> >               btrfs_free_fs_root(root);
> >               goto again;
> >           }
> > 
> > I could add a Fixes tag of this commit in my V2 patch.
> > Is it okay?
> 
> I can add it myself, that's a minor thing. The fix is correct, I've
> rewritten the changelog a bit, patch now added to misc-next, thanks.

Where is 'misc-next' please?

-- 
Lee Jones [李琼斯]

Re: [PATCH] fs: btrfs: fix possible use-after-free bug in error handling code of btrfs_get_root_ref()

Posted by Lee Jones 2 years, 10 months ago

On Wed, 23 Aug 2023, Lee Jones wrote:

> On Fri, 01 Apr 2022, David Sterba wrote:
> 
> > On Fri, Mar 25, 2022 at 04:04:17PM +0800, Jia-Ju Bai wrote:
> > > >> @@ -1850,9 +1850,10 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
> > > >>   
> > > >>   	ret = btrfs_insert_fs_root(fs_info, root);
> > > >>   	if (ret) {
> > > >> -		btrfs_put_root(root);
> > > >> -		if (ret == -EEXIST)
> > > >> +		if (ret == -EEXIST) {
> > > >> +			btrfs_put_root(root);
> > > > I think this fix is correct, though it's not that clear. If you look how
> > > > the code changed, there was the unconditional put and then followed by a
> > > > free:
> > > >
> > > > 8c38938c7bb0 ("btrfs: move the root freeing stuff into btrfs_put_root")
> > > >
> > > > Here it's putting twice where one will be the final free.
> > > >
> > > > And then the whole refcounting gets updated in
> > > >
> > > > 4785e24fa5d2 ("btrfs: don't take an extra root ref at allocation time")
> > > >
> > > > which could be removing the wrong put, I'm not yet sure.
> > > 
> > > Thanks for the reply!
> > > 
> > > I think the bug should be introduced by this commit:
> > > bc44d7c4b2b1 ("btrfs: push btrfs_grab_fs_root into btrfs_get_fs_root")
> > > 
> > > This commit has a change:
> > >       ret = btrfs_insert_fs_root(fs_info, root);
> > >       if (ret) {
> > > +      btrfs_put_fs_root(root);
> > >           if (ret == -EEXIST) {
> > >               btrfs_free_fs_root(root);
> > >               goto again;
> > >           }
> > > 
> > > I could add a Fixes tag of this commit in my V2 patch.
> > > Is it okay?
> > 
> > I can add it myself, that's a minor thing. The fix is correct, I've
> > rewritten the changelog a bit, patch now added to misc-next, thanks.
> 
> Where is 'misc-next' please?

Nevermind.  I've just seen a) how old this thread is and b) the commit
subject was changed and subsequently committed as:

168a2f776b976 ("btrfs: fix root ref counts in error handling in btrfs_get_root_ref")

-- 
Lee Jones [李琼斯]