fs/ubifs/tnc_commit.c | 1 + 1 file changed, 1 insertion(+)
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:
dump_backtrace+0x0/0x340
show_stack+0x18/0x24
dump_stack_lvl+0x9c/0xbc
print_address_description.constprop.0+0x74/0x2b0
kasan_report+0x1d8/0x1f0
kasan_check_range+0xf8/0x1a0
memcpy+0x84/0xf4
ubifs_tnc_end_commit+0xa5c/0x1950
do_commit+0x4e0/0x1340
ubifs_bg_thread+0x234/0x2e0
kthread+0x36c/0x410
ret_from_fork+0x10/0x20
Allocated by task 401:
kasan_save_stack+0x38/0x70
__kasan_kmalloc+0x8c/0xd0
__kmalloc+0x34c/0x5bc
tnc_insert+0x140/0x16a4
ubifs_tnc_add+0x370/0x52c
ubifs_jnl_write_data+0x5d8/0x870
do_writepage+0x36c/0x510
ubifs_writepage+0x190/0x4dc
__writepage+0x58/0x154
write_cache_pages+0x394/0x830
do_writepages+0x1f0/0x5b0
filemap_fdatawrite_wbc+0x170/0x25c
file_write_and_wait_range+0x140/0x190
ubifs_fsync+0xe8/0x290
vfs_fsync_range+0xc0/0x1e4
do_fsync+0x40/0x90
__arm64_sys_fsync+0x34/0x50
invoke_syscall.constprop.0+0xa8/0x260
do_el0_svc+0xc8/0x1f0
el0_svc+0x34/0x70
el0t_64_sync_handler+0x108/0x114
el0t_64_sync+0x1a4/0x1a8
Freed by task 403:
kasan_save_stack+0x38/0x70
kasan_set_track+0x28/0x40
kasan_set_free_info+0x28/0x4c
__kasan_slab_free+0xd4/0x13c
kfree+0xc4/0x3a0
tnc_delete+0x3f4/0xe40
ubifs_tnc_remove_range+0x368/0x73c
ubifs_tnc_remove_ino+0x29c/0x2e0
ubifs_jnl_delete_inode+0x150/0x260
ubifs_evict_inode+0x1d4/0x2e4
evict+0x1c8/0x450
iput+0x2a0/0x3c4
do_unlinkat+0x2cc/0x490
__arm64_sys_unlinkat+0x90/0x100
invoke_syscall.constprop.0+0xa8/0x260
do_el0_svc+0xc8/0x1f0
el0_svc+0x34/0x70
el0t_64_sync_handler+0x108/0x114
el0t_64_sync+0x1a4/0x1a8
The offending `memcpy()` in `ubifs_copy_hash()` has a use-after-free
when a node becomes root in TNC but still has a `cparent` to an already
freed node. More specifically, consider the following TNC:
zroot
/
/
zp1
/
/
zn
Inserting a new node `zn_new` with a key smaller then `zn` will trigger
a split in `tnc_insert()` if `zp1` is full:
zroot
/ \
/ \
zp1 zp2
/ \
/ \
zn_new zn
`zn->parent` has now been moved to `zp2`, *but* `zn->cparent` still
points to `zp1`.
Now, consider a removal of all the nodes _except_ `zn`. Just when
`tnc_delete()` is about to delete `zroot` and `zp2`:
zroot
\
\
zp2
\
\
zn
`zroot` and `zp2` get freed and the tree collapses:
zn
`zn` now becomes the new `zroot`.
`get_znodes_to_commit()` will now only find `zn`, the new `zroot`, and
`write_index()` will check its `znode->cparent` that wrongly points to
the already freed `zp1`. `ubifs_copy_hash()` thus gets wrongly called
with `znode->cparent->zbranch[znode->iip].hash` that triggers the
use-after-free!
Fix this by explicitly setting `znode->cparent` to `NULL` in
`get_znodes_to_commit()` for the root node. The search for the dirty
nodes is bottom-up in the tree. Thus, when `find_next_dirty(znode)`
returns NULL, the current `znode` _is_ the root node.
Fixes: 16a26b20d2af ("ubifs: authentication: Add hashes to index nodes")
Tested-by: Waqar Hameed <waqar.hameed@axis.com>
Co-developed-by: Zhihao Cheng <chengzhihao1@huawei.com>
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
Changes in v2:
* Implement the actual fix from discussions in RFC patch.
* Link to first RFC version: https://lore.kernel.org/lkml/1225b9b5bbf5278e5ae512177712915f1bc0aebf.1728570925.git.waqar.hameed@axis.com/
fs/ubifs/tnc_commit.c | 1 +
1 file changed, 1 insertion(+)
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) {
+ znode->cparent = NULL;
znode->cnext = c->cnext;
break;
}
--
2.39.5在 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().
> 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);'
> + znode->cparent = NULL;
> znode->cnext = c->cnext;
> break;
> }
>
On Sat, Nov 09, 2024 at 10:34 +0800 Zhihao Cheng <chengzhihao1@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.
>> + znode->cparent = NULL;
>> znode->cnext = c->cnext;
>> break;
>> }
>>
在 2024/11/11 5:48, Waqar Hameed 写道:
> On Sat, Nov 09, 2024 at 10:34 +0800 Zhihao Cheng <chengzhihao1@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;
>>> }
>>>
> .
>
© 2016 - 2024 Red Hat, Inc.