[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 at huawei.com> 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;
>>>    		}
>>>
> .
>