| Message ID | ed37cf06762e40be2fcebc9359b1c063b32afef4.1607019557.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup error handling in relocation | expand |
On 2020/12/4 上午2:22, Josef Bacik wrote: > Qu pointed out a bug in one of my error handling patches, which made me > notice that we modify the new_root->highest_objectid _after_ we've > dropped the ref to the new_root. This could lead to a possible UAF, fix > this by modifying the ->highest_objectid before we drop our reference to > the new_root. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> But found something to cleanup in the future, inlined below. > --- > fs/btrfs/ioctl.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 703212ff50a5..f240beed4739 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -717,6 +717,12 @@ static noinline int create_subvol(struct inode *dir, > btrfs_record_root_in_trans(trans, new_root); > > ret = btrfs_create_subvol_root(trans, new_root, root, new_dirid); Firstly, btrfs_create_subvol_root() is only called here once, and new_dirid is always a fixed value, BTRFS_FIRST_FREE_OBJECTID. This means, we don't need the parameter at all. > + if (!ret) { > + mutex_lock(&new_root->objectid_mutex); > + new_root->highest_objectid = new_dirid; > + mutex_unlock(&nBut still find something suspicious for the existing naming, inlined below.ew_root->objectid_mutex); > + } > + Secondly, new_root is a new subvolume root which just get allocated. It looks more sane to initialize the highest_objectid inside btrfs_get_root_ref() for new root. This should reduce the chance to hit such use-after-free bug completely. Thanks, Qu > btrfs_put_root(new_root); > if (ret) { > /* We potentially lose an unused inode item here */ > @@ -724,10 +730,6 @@ static noinline int create_subvol(struct inode *dir, > goto fail; > } > > - mutex_lock(&new_root->objectid_mutex); > - new_root->highest_objectid = new_dirid; > - mutex_unlock(&new_root->objectid_mutex); > - > /* > * insert the directory item > */ >
On 4.12.20 г. 10:01 ч., Qu Wenruo wrote: > > > On 2020/12/4 上午2:22, Josef Bacik wrote: >> Qu pointed out a bug in one of my error handling patches, which made me >> notice that we modify the new_root->highest_objectid _after_ we've >> dropped the ref to the new_root. This could lead to a possible UAF, fix >> this by modifying the ->highest_objectid before we drop our reference to >> the new_root. >> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > But found something to cleanup in the future, inlined below. >> --- >> fs/btrfs/ioctl.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 703212ff50a5..f240beed4739 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -717,6 +717,12 @@ static noinline int create_subvol(struct inode *dir, >> btrfs_record_root_in_trans(trans, new_root); >> >> ret = btrfs_create_subvol_root(trans, new_root, root, new_dirid); > > Firstly, btrfs_create_subvol_root() is only called here once, and > new_dirid is always a fixed value, BTRFS_FIRST_FREE_OBJECTID. > > This means, we don't need the parameter at all. > >> + if (!ret) { >> + mutex_lock(&new_root->objectid_mutex); >> + new_root->highest_objectid = new_dirid; >> + mutex_unlock(&nBut still find something suspicious for the existing naming, inlined below.ew_root->objectid_mutex); >> + } >> + > > Secondly, new_root is a new subvolume root which just get allocated. > It looks more sane to initialize the highest_objectid inside > btrfs_get_root_ref() for new root. > > This should reduce the chance to hit such use-after-free bug completely. Actually btrfs_init_fs_root already does : 42 ret = btrfs_find_highest_objectid(root, 43 &root->highest_objectid); Where root would be the newly initialized tree so highest_objectid should already be correctly initialized. However, looking at the source of btrfs_find_highest_objectid why do we do BTRFS_FIRST_FREE_OBJECTID - 1 there? <snip>
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 703212ff50a5..f240beed4739 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -717,6 +717,12 @@ static noinline int create_subvol(struct inode *dir, btrfs_record_root_in_trans(trans, new_root); ret = btrfs_create_subvol_root(trans, new_root, root, new_dirid); + if (!ret) { + mutex_lock(&new_root->objectid_mutex); + new_root->highest_objectid = new_dirid; + mutex_unlock(&new_root->objectid_mutex); + } + btrfs_put_root(new_root); if (ret) { /* We potentially lose an unused inode item here */ @@ -724,10 +730,6 @@ static noinline int create_subvol(struct inode *dir, goto fail; } - mutex_lock(&new_root->objectid_mutex); - new_root->highest_objectid = new_dirid; - mutex_unlock(&new_root->objectid_mutex); - /* * insert the directory item */
Qu pointed out a bug in one of my error handling patches, which made me notice that we modify the new_root->highest_objectid _after_ we've dropped the ref to the new_root. This could lead to a possible UAF, fix this by modifying the ->highest_objectid before we drop our reference to the new_root. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/ioctl.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)