linux-kernel - Re: [PATCH RFC] ubifs: Fix use-after-free in ubifs_tnc_end

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <pndses42uam.fsf@axis.com>
Date: Wed, 6 Nov 2024 17:36:49 +0100
From: Waqar Hameed <waqar.hameed@...s.com>
To: Zhihao Cheng <chengzhihao1@...wei.com>
CC: Richard Weinberger <richard@....at>, Sascha Hauer
	<s.hauer@...gutronix.de>, <kernel@...s.com>, <linux-mtd@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] ubifs: Fix use-after-free in ubifs_tnc_end_commit

Sorry for the late response Zhihao! I've been quite busy these days...

On Fri, Oct 18, 2024 at 09:40 +0800 Zhihao Cheng <chengzhihao1@...wei.com> wrote:

> 在 2024/10/18 2:36, Waqar Hameed 写道:
>> On Wed, Oct 16, 2024 at 10:11 +0800 Zhihao Cheng <chengzhihao1@...wei.com> wrote:
>> [...]
>> 
>>> BTW, what is the configuration of your flash?(eg. erase size, page size)?
>> $ mtdinfo /dev/mtd2
>>    mtd2
>>    Name:                           firmware
>>    Type:                           nand
>>    Eraseblock size:                131072 bytes, 128.0 KiB
>>    Amount of eraseblocks:          1832 (240123904 bytes, 229.0 MiB)
>>    Minimum input/output unit size: 2048 bytes
>>    Sub-page size:                  2048 bytes
>>    OOB size:                       64 bytes
>>    Character device major/minor:   90:4
>>    Bad blocks are allowed:         true
>>    Device is writable:             true
>> $ ubinfo /dev/ubi0_0
>>    Volume ID:   0 (on ubi0)
>>    Type:        dynamic
>>    Alignment:   1
>>    Size:        661 LEBs (83931136 bytes, 80.0 MiB)
>>    State:       OK
>>    Name:        test-vol
>>    Character device major/minor: 244:1
>> [...]
>
> Thanks, I will change my nandsim configurations to generate a mtd device the
> same model.

Did you manage to reproduce the issue with this?

>> 
>>> Well, let's do a preliminary analysis.
>>> The znode->cparent[znode->ciip] is a freed address in write_index(), which
>>> means:
>>> 1. 'znode->ciip' is valid, znode->cparent is freed by tnc_delete, however znode
>>> cannot be freed if znode->cnext is not NULL, which means:
>>>    a) 'znode->cparent' is not dirty, we should add an assertion like
>>>    ubifs_assert(c, ubifs_zn_dirty(znode->cparent)) in get_znodes_to_commit().
>>>    Note, please check that 'znode->cparent' is not NULL before the assertion.
>>>    b) 'znode->cparent' is dirty, but it is not added into list 'c->cnext', we
>>>    should traverse the entire TNC in get_znodes_to_commit() to make sure that all
>>>    dirty znodes are collected into list 'c->cnext', so another assertion is
>>>   needed.

I'm a little worried that traversing the whole TNC could change the
timing behavior, and thus might not trigger the race. Let's do that in
steps? Start with the other asserts (see diff below) and later just do
this assert. Does that sound reasonable?

I could modify `dbg_check_tnc()` so that it also checks that each dirty
`znode` is present in `c->cnext` list. We then call this at the end of
`get_znodes_to_commit()`.

>>> 2. 'znode->ciip' is invalid, and the value beyonds the memory area of
>>> znode->cparent. All znodes are allocated with size of 'c->max_znode_sz', which
>>> means that 'znode->ciip' exceeds the 'c->fantout', so we can add an assertion
>>> like ubifs_assert(c, znode->ciip < c->fantout) in get_znodes_to_commit().
>>>
>>> That's what I can think of, are there any other possibilities?
>> I looked a little more at `get_znodes_to_commit()` when adding the
>> asserts you suggest, and I have a question: what happens when
>> `find_next_dirty()` returns `NULL`? In that case
>> ```
>> znode->cnext = c->cnext;
>> ```
>> but `znode->cparent` and `znode->ciip` are not updated. Shouldn't they?
>
> Good thinking.
> According to the implementation of find_next_dirty(), the order of dirty znodes
> collection is bottom-up, which means that the last dirty znode is the root
> znode, so it doesn't have a parent. You can verify that by adding assertion to
> check whether the last dirty znode is the root.

[...]

To summarize, I'll start a run with the following asserts:

diff --git a/fs/ubifs/tnc_commit.c b/fs/ubifs/tnc_commit.c
index a55e04822d16..4eef82e02afe 100644
--- a/fs/ubifs/tnc_commit.c
+++ b/fs/ubifs/tnc_commit.c
@@ -652,11 +652,17 @@ static int get_znodes_to_commit(struct ubifs_info *c)
 	}
 	cnt += 1;
 	while (1) {
+		ubifs_assert(c, znode->ciip < c->fantout);
+		if (znode->cparent) {
+			ubifs_assert(c, ubifs_zn_dirty(znode->cparent));
+		}
+
 		ubifs_assert(c, !ubifs_zn_cow(znode));
 		__set_bit(COW_ZNODE, &znode->flags);
 		znode->alt = 0;
 		cnext = find_next_dirty(znode);
 		if (!cnext) {
+			ubifs_assert(c, znode == c->zroot.znode);
 			znode->cnext = c->cnext;
 			break;
 		}

Then later, another run with a modified `dbg_check_tnc()` to check that
all dirty `znode`s are indeed present in the list `c->cnext`.