[PATCH 0/2] ubifs: tnc_delete: Fix two uaf bug when mem failure occurs

[PATCH 0/2] ubifs: tnc_delete: Fix two uaf bug when mem failure occurs

[Zhihao Cheng]

Fix two TNC traversing uaf problems after failure happens in
tnc_delete().

Zhihao Cheng (2):
  ubifs: tnc_delete: Don't free zero-branch znodes if memory allocation
    fails
  ubifs: tnc_delete: Don't update collapsed znodes if memory allocation
    fails

 fs/ubifs/tnc.c   | 44 +++++++++++++++++++++++++++++++++-----------
 fs/ubifs/ubifs.h |  2 ++
 2 files changed, 35 insertions(+), 11 deletions(-)

-- 
2.52.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/2] ubifs: tnc_delete: Don't free zero-branch znodes if memory allocation fails

[Zhihao Cheng]

The freed zero-branch znodes won't be removed from TNC if a failure
(eg. mem alloc in insert_old_idx_znode) happens in tnc_delete, then
TNC traversing could access the freed znodes, the details shown as:

 file: consists of many data nodes, TNC is:
                       znode_A (lv 2)
		 /                       \
         znode (lv 1)                 znode_B (lv 1)
	  /        \                      |
    znode(lv 0)...znode(lv 0)         znode_C (lv 0, child_cnt = 1)
    |    |    |    |	  |               |
 ino_1 ino_f dent dn0...dn_i-1		dn_i

    P1
 truncate(file, dn_i-1)
  ubifs_setattr
   ubifs_jnl_truncate
    ubifs_tnc_remove_range
     tnc_delete // dn_i is removed, znode_C->child_cnt becomes 0
      do-while
       kfree(znode_C)
       ENOMEM = insert_old_idx_znode(c, znode_B)
       return
      znode->zbranch[i] = znode->zbranch[i + 1] // skip,
      // znode_C is still in TNC

 the TNC looks like:
                       znode_A (lv 2)
		 /                       \
         znode (lv 1)                 znode_B (lv 1)
	  /        \                      |
    znode(lv 0)...znode(lv 0)         znode_C (lv 0, child_cnt = 0, freed)
    |    |    |    |	  |
 ino_1 ino_f dent dn0...dn_i-1

     P2
 drop_caches_sysctl_handler
  shrink_slab
   ubifs_shrink_scan
    shrink_tnc_trees
     shrink_tnc
      ubifs_tnc_levelorder_next
       access znode_C // UAF

The root cause is that zero-branch znodes freeing & znode->zbranch is not
atomic, which could be interrupted by the insert_old_idx_znode() failure,
fix it by freeing znodes after all insert_old_idx_znode() callers success.
The znodes collapsing won't meet the problem if insert_old_idx() fails,
because znode is freed after updating c->zroot, the TNC traversing won't
access a freed znode.
Notice: we add new field 'fnext' in struct ubifs_znode to trace to be
freed znodes. Because tnc_delete and do_commit could race, the members
parent/cnext/cparent can not be reused. Importing a stack array to record
znodes is also not recommended, becase the UBIFS_MAX_LEVELS*sizeof(ptr)
becomes 4096, which may cause the compiler to complain.

Reproducer is https://bugzilla.kernel.org/show_bug.cgi?id=222129.

Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/ubifs/tnc.c   | 40 +++++++++++++++++++++++++++++++---------
 fs/ubifs/ubifs.h |  2 ++
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index 776e6bc6a00d..679afddadf82 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -2514,8 +2514,9 @@ int ubifs_tnc_add_nm(struct ubifs_info *c, const union ubifs_key *key,
 static int tnc_delete(struct ubifs_info *c, struct ubifs_znode *znode, int n)
 {
 	struct ubifs_zbranch *zbr;
-	struct ubifs_znode *zp;
+	struct ubifs_znode *zp, *zn;
 	int i, err;
+	struct ubifs_znode free_head = { .fnext = NULL };
 
 	/* Delete without merge for now */
 	ubifs_assert(c, znode->level == 0);
@@ -2543,7 +2544,7 @@ static int tnc_delete(struct ubifs_info *c, struct ubifs_znode *znode, int n)
 	 * This was the last zbranch, we have to delete this znode from the
 	 * parent.
 	 */
-
+	zn = &free_head;
 	do {
 		ubifs_assert(c, !ubifs_zn_obsolete(znode));
 		ubifs_assert(c, ubifs_zn_dirty(znode));
@@ -2554,15 +2555,20 @@ static int tnc_delete(struct ubifs_info *c, struct ubifs_znode *znode, int n)
 		atomic_long_dec(&c->dirty_zn_cnt);
 
 		err = insert_old_idx_znode(c, znode);
-		if (err)
+		if (err) {
+			zn = free_head.fnext;
+			while (zn) {
+				struct ubifs_znode *next = zn->fnext;
+
+				zn->fnext = NULL;
+				zn = next;
+			}
 			return err;
+		}
+
+		zn->fnext = znode;
+		zn = znode;
 
-		if (znode->cnext) {
-			__set_bit(OBSOLETE_ZNODE, &znode->flags);
-			atomic_long_inc(&c->clean_zn_cnt);
-			atomic_long_inc(&ubifs_clean_zn_cnt);
-		} else
-			kfree(znode);
 		znode = zp;
 	} while (znode->child_cnt == 1); /* while removing last child */
 
@@ -2575,6 +2581,22 @@ static int tnc_delete(struct ubifs_info *c, struct ubifs_znode *znode, int n)
 			znode->zbranch[i].znode->iip = i;
 	}
 
+	/* Free zero-branchs znodes. */
+	zn = free_head.fnext;
+	while (zn) {
+		struct ubifs_znode *next = zn->fnext;
+
+		zn->fnext = NULL;
+		if (zn->cnext) {
+			__set_bit(OBSOLETE_ZNODE, &zn->flags);
+			atomic_long_inc(&c->clean_zn_cnt);
+			atomic_long_inc(&ubifs_clean_zn_cnt);
+		} else {
+			kfree(zn);
+		}
+		zn = next;
+	}
+
 	/*
 	 * If this is the root and it has only 1 child then
 	 * collapse the tree.
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 00db0d19a85e..c3ae7c2625a4 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -761,6 +761,7 @@ struct ubifs_zbranch {
  * @parent: parent znode or NULL if it is the root
  * @cnext: next znode to commit
  * @cparent: parent node for this commit
+ * @fnext: next znode to free
  * @ciip: index in cparent's zbranch array
  * @flags: znode flags (%DIRTY_ZNODE, %COW_ZNODE or %OBSOLETE_ZNODE)
  * @time: last access time (seconds)
@@ -780,6 +781,7 @@ struct ubifs_znode {
 	struct ubifs_znode *parent;
 	struct ubifs_znode *cnext;
 	struct ubifs_znode *cparent;
+	struct ubifs_znode *fnext;
 	int ciip;
 	unsigned long flags;
 	time64_t time;
-- 
2.52.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] ubifs: tnc_delete: Don't update collapsed znodes if memory allocation fails

[Zhihao Cheng]

A collapsed znode is updated with 'znode->parent = NULL' before calling
insert_old_idx(), if a memory allocation failure happens, there will
exists non-root znode(without parent) in TNC, which leads a null-ptr-def
problem for TNC traversing, the details shown as:

 file: consists of many data nodes, TNC is:
                   znode_A (lv 2)
  	     /                         \
       znode_B (lvl 1)              znode (lv 1)
        /             \                 |
    znode_C(lv 0)...znode(lv 0)     znode (lv 0)
     |   |     |     |       |          |
  ino_1 ino_f dent dn_0 ... dn_i      dn_i+1

     P1
  truncate(file, 0)
   ubifs_setattr
    ubifs_jnl_truncate
     ubifs_tnc_remove_range
      tnc_delete // dn_* are removed
       do-while

  the TNC looks like:
          znode_A (lv 2, parent = NULL)
		|
	  znode_B (lvl 1, parent = znode_A)
		|
	  znode_C (lv 0, parent = znode_B)
	  |     |     |
	ino_1 ino_f dent

  tnc_delete will collapse the tree:
  // while (znode->child_cnt == 1 && znode->level != 0)
   // round 1
   zp = znode_A
   znode_B = get_znode(c, znode_A, 0)
   znode_B->parent = NULL
   znode_B->iip = 0
   insert_old_idx
   c->zroot = znode_B
   kfree(znode_A)
   // round 2
   zp = znode_B
   znode_C = get_znode(c, znode_B, 0)
   znode_C->parent = NULL
   znode_C->iip = 0
   ENOMEM = insert_old_idx
    return
   c->zroot = znode_C // skip

  the TNC looks like:
          znode_B (lv 1, parent = NULL)
		|
	  znode_C (lv 0, parent = NULL)
	  |     |     |
	ino_1 ino_f dent

      P2
  umount
   kill_ubifs_super
    ubifs_put_super
     ubifs_umount
      destroy_journal
       ubifs_tnc_close
        ubifs_destroy_tnc_tree
	 ubifs_destroy_tnc_subtree
	  znode_C = ubifs_tnc_postorder_first
	  if (zn == znode) // false, zn = znode_C, znode = znode_B
	  zn = ubifs_tnc_postorder_next(c, znode_C) // zn = NULL
	  for (n = 0; n < zn->child_cnt; n++) // null-ptr-def !

The root cause is that zroot & znode->parent updating is not atomic,
which could be interrupted by the insert_old_idx() failure, fix it by
making zroot & znode->parent updating together.

Reproducer is https://bugzilla.kernel.org/show_bug.cgi?id=222130.

Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/ubifs/tnc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index 679afddadf82..c9480d963e4c 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -2611,14 +2611,14 @@ static int tnc_delete(struct ubifs_info *c, struct ubifs_znode *znode, int n)
 			znode = dirty_cow_znode(c, zbr);
 			if (IS_ERR(znode))
 				return PTR_ERR(znode);
-			znode->parent = NULL;
-			znode->iip = 0;
 			if (c->zroot.len) {
 				err = insert_old_idx(c, c->zroot.lnum,
 						     c->zroot.offs);
 				if (err)
 					return err;
 			}
+			znode->parent = NULL;
+			znode->iip = 0;
 			c->zroot.lnum = zbr->lnum;
 			c->zroot.offs = zbr->offs;
 			c->zroot.len = zbr->len;
-- 
2.52.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] ubifs: tnc_delete: Don't free zero-branch znodes if memory allocation fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ubifs: assertion failure on lookups due to zero-child znode after tnc_delete() error
--

commit 5c1854483c2d06b6fd3fc46791b422fcd958956f
Author: Zhihao Cheng <chengzhihao1@huawei.com>

ubifs: tnc_delete: Don't free zero-branch znodes if memory allocation fails

This commit delays the freeing of zero-branch znodes in tnc_delete()
until after all insert_old_idx_znode() calls succeed. It introduces a
local stack list to track znodes to be freed, aiming to prevent a
use-after-free race condition when traversing the TNC tree.

> diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
> --- a/fs/ubifs/tnc.c
> +++ b/fs/ubifs/tnc.c
[ ... ]
> @@ -2542,15 +2543,20 @@ static int tnc_delete(struct ubifs_info *c, struct ubifs_znode *znode, int n)
>  		atomic_long_dec(&c->dirty_zn_cnt);
>  
>  		err = insert_old_idx_znode(c, znode);
> -		if (err)
> +		if (err) {
> +			zn = free_head.fnext;
> +			while (zn) {
> +				struct ubifs_znode *next = zn->fnext;
> +
> +				zn->fnext = NULL;
> +				zn = next;
> +			}
>  			return err;

[Severity: High]
If insert_old_idx_znode() returns an error like -ENOMEM, does this early 
return leave the zero-child znode linked in the TNC tree?

Because we return before removing the znode from its parent's zbranch array, 
the tree retains a node with child_cnt == 0. If a subsequent read operation 
(which is permitted even after the filesystem is forced to read-only mode) 
traverses this znode, it will call ubifs_search_zbranch():

fs/ubifs/tnc_misc.c:ubifs_search_zbranch() {
        int beg = 0, end = znode->child_cnt, mid;
        int cmp;
        const struct ubifs_zbranch *zbr = &znode->zbranch[0];

        ubifs_assert(c, end > beg);
...
}

With end == 0 and beg == 0, this will trigger the assertion failure, which 
can panic the kernel if mounted with assert=panic. Does this error path need 
to remove the znode from its parent's zbranch, or otherwise mark it to 
prevent lookups from tripping the assertion?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014060252.3531356-1-chengzhihao1@huawei.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] ubifs: tnc_delete: Don't free zero-branch znodes if memory allocation fails

[Zhihao Cheng]

在 2026/10/14 14:28, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ubifs: assertion failure on lookups due to zero-child znode after tnc_delete() error
> --
> 
> commit 5c1854483c2d06b6fd3fc46791b422fcd958956f
> Author: Zhihao Cheng <chengzhihao1@huawei.com>
> 
> ubifs: tnc_delete: Don't free zero-branch znodes if memory allocation fails
> 
> This commit delays the freeing of zero-branch znodes in tnc_delete()
> until after all insert_old_idx_znode() calls succeed. It introduces a
> local stack list to track znodes to be freed, aiming to prevent a
> use-after-free race condition when traversing the TNC tree.
> 
>> diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
>> --- a/fs/ubifs/tnc.c
>> +++ b/fs/ubifs/tnc.c
> [ ... ]
>> @@ -2542,15 +2543,20 @@ static int tnc_delete(struct ubifs_info *c, struct ubifs_znode *znode, int n)
>>   		atomic_long_dec(&c->dirty_zn_cnt);
>>   
>>   		err = insert_old_idx_znode(c, znode);
>> -		if (err)
>> +		if (err) {
>> +			zn = free_head.fnext;
>> +			while (zn) {
>> +				struct ubifs_znode *next = zn->fnext;
>> +
>> +				zn->fnext = NULL;
>> +				zn = next;
>> +			}
>>   			return err;
> 
> [Severity: High]
> If insert_old_idx_znode() returns an error like -ENOMEM, does this early
> return leave the zero-child znode linked in the TNC tree?
> 
> Because we return before removing the znode from its parent's zbranch array,
> the tree retains a node with child_cnt == 0. If a subsequent read operation
> (which is permitted even after the filesystem is forced to read-only mode)
> traverses this znode, it will call ubifs_search_zbranch():
> 
> fs/ubifs/tnc_misc.c:ubifs_search_zbranch() {
>          int beg = 0, end = znode->child_cnt, mid;
>          int cmp;
>          const struct ubifs_zbranch *zbr = &znode->zbranch[0];
> 
>          ubifs_assert(c, end > beg);
> ...
> }
> 
> With end == 0 and beg == 0, this will trigger the assertion failure, which
> can panic the kernel if mounted with assert=panic. Does this error path need
> to remove the znode from its parent's zbranch, or otherwise mark it to
> prevent lookups from tripping the assertion?
> 

Hi, the insert_old_idx_znode() makes tnc_delete return immediately even 
level0_znode->child_cnt becomes zero, which could trigger following 
assertion failure in ubifs_search_zbranch(), this is an already existed 
process.
This issue is not severe because after the insert_old_idx_znode error 
occurs, UBIFS has already switched to read-only mode and issued an 
alarm. Subsequent ubifs_assert is merely printing more alarms, unless 
the mount option uses assert=panic.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/