Re: [PATCH v2] ubifs: Fix use-after-free in ubifs_tnc_end_commit

[Zhihao Cheng]

在 2024/11/11 5:48, Waqar Hameed 写道:
> On Sat, Nov 09, 2024 at 10:34 +0800 Zhihao Cheng <chengzhihao1@xxxxxxxxxx> wrote:
>
> > 在 2024/10/9 22:46, Waqar Hameed 写道:
> > 3 nits below.
> >
> > 1. Make the title as 'ubifs: authentication: Fix use-after-free in
> > ubifs_tnc_end_commit'
> >
> > 2. At the begining of the commit msg, describe the problem:
> > After TNC tree inserting(which may trigger a znode split and change the
> > znode->parent) and deleting(which may trigger znode freeing), the
> > znode->cparent(which still points to a freed znode) may not be updated at the
> > begining of commit, which could trigger an UAF problem while accessing
> > znode->cparent in write_index().
>
> Alright, will rephrase the commit message a bit.
>
> > > Running
> > >     rm -f /etc/test-file.bin
> > >     dd if=/dev/urandom of=/etc/test-file.bin bs=1M count=60 conv=fsync
> > > in a loop, with `CONFIG_UBIFS_FS_AUTHENTICATION`, KASAN reports:
> > >     BUG: KASAN: use-after-free in ubifs_tnc_end_commit+0xa5c/0x1950
> > >     Write of size 32 at addr ffffff800a3af86c by task ubifs_bgt0_20/153
> > >     Call trace:
> >
> > [...]
> > > diff --git a/fs/ubifs/tnc_commit.c b/fs/ubifs/tnc_commit.c
> > > index a55e04822d16..a464eb557585 100644
> > > --- a/fs/ubifs/tnc_commit.c
> > > +++ b/fs/ubifs/tnc_commit.c
> > > @@ -657,6 +657,7 @@ static int get_znodes_to_commit(struct ubifs_info *c)
> > >    		znode->alt = 0;
> > >    		cnext = find_next_dirty(znode);
> > >    		if (!cnext) {
> >
> > 3. I'd like to add the the assertion 'ubifs_assert(c, !znode->parent);'
>
> Wouldn't the assert always be true? Since the root node wouldn't have a
> parent. Or are we afraid of some other edge cases (bugs?) that might
> have been missed and want to be defensive here? Either way, I'll add the
> assert.

A defensive code here is to prevent some unknown bugs, in which the last 
returned znode has a parent, although it looks impossible for current 
realization.
> > > +			znode->cparent = NULL;
> > >    			znode->cnext = c->cnext;
> > >    			break;
> > >    		}
> .