| Message ID | 20220324134454.15192-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: btrfs: fix possible use-after-free bug in error handling code of btrfs_get_root_ref() | expand |
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.
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
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.
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?
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")
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;
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(-)