Re: [PATCH] ubifs: check the length of index branches before reading them

[Zhihao Cheng <chengzhihao1@huawei.com>]

在 2026/10/24 20:30, Alejandro Pacheco 写道:
> From: Alejandro Pacheco <ale@alepacheco.dev>
> 
> read_znode() validates each branch of the index node it reads: the LEB
> number and offset of every branch and, in a level 0 znode, the length of
> the leaf node the branch points to, against c->ranges[]. In a znode above
> level 0, where each branch points to another index node, the length is
> not checked:
> 
> 	if (znode->level)
> 		continue;
> 
> When such a child is loaded, ubifs_load_znode() calls read_znode() for
> it, which allocates c->max_idx_node_sz bytes and has ubifs_read_node()
> read zbr->len bytes into them. Before the read, that length is bounded
> by the space left in the LEB but not by the size of the buffer; the
> length in the node header is compared with it and with c->ranges[] only
> after the read. So an image with a branch length larger than
> c->max_idx_node_sz, but within the LEB, can cause a heap buffer overflow.
> 
> dbg_check_old_index(), which walks the old index on the media when the
> chk_index debugging check is enabled, reads each child index node by its
> branch length the same way, into a buffer sized for an index node of
> c->fanout branches, without checking the length.
> 
> validate_master() already checks the root index node's length,
> c->zroot.len, against c->ranges[UBIFS_IDX_NODE]. Give every other index
> branch the same check before it is used for a read: in read_znode(), by
> running the length check level 0 branches get with type UBIFS_IDX_NODE,
> and in dbg_check_old_index(). Images written by UBIFS or mkfs.ubifs are
> not affected: an index node is ubifs_idx_node_sz() of its 1 to fanout
> branches long, which is the range in c->ranges[UBIFS_IDX_NODE], and
> ubifs_read_node() already rejects a length outside it, though only after
> the read.
> 
> This needs a crafted image (the CRC of the index node holding the branch
> catches random corruption), and mounting UBIFS needs CAP_SYS_ADMIN in
> the initial user namespace.
> 
> It was tested on a KASAN UML kernel built from this tree, with UBI on a
> 64 MiB mtdram device and the chk_index debugging check enabled. A tree of
> 1,830 files in 30 directories was copied to a freshly formatted volume,
> read back after a remount, partly deleted and copied again, and read
> back after another remount; the same tree was read back from a second
> volume written by mkfs.ubifs. Across the remounts, read_znode() read 909
> index nodes above level 0, and dbg_check_old_index() walked the old
> index at 20 commits, the whole index of each volume as it was on the
> media among them, with no KASAN report, warning or UBIFS error. This only
> runs the new checks on valid index nodes: no image with a bad branch
> length was made. It builds with no new warnings (W=1, x86_64 defconfig
> with UBIFS and KASAN), and checkpatch.pl --strict reports nothing.
> 
> An LLM coding assistant was used to find this bug and to write this
> patch and its changelog.
> 
> Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alejandro Pacheco <ale@alepacheco.dev>
> ---
>   fs/ubifs/commit.c   | 5 +++++
>   fs/ubifs/tnc_misc.c | 6 +++---
>   2 files changed, 8 insertions(+), 3 deletions(-)
> 

Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
> diff --git a/fs/ubifs/commit.c b/fs/ubifs/commit.c
> index 5b3a840098b..0e50b584248 100644
> --- a/fs/ubifs/commit.c
> +++ b/fs/ubifs/commit.c
> @@ -696,6 +696,11 @@ int dbg_check_old_index(struct ubifs_info *c, struct ubifs_zbranch *zroot)
>   		lnum = le32_to_cpu(br->lnum);
>   		offs = le32_to_cpu(br->offs);
>   		len = le32_to_cpu(br->len);
> +		if (len < c->ranges[UBIFS_IDX_NODE].min_len ||
> +		    len > c->ranges[UBIFS_IDX_NODE].max_len) {
> +			err = 8;
> +			goto out_dump;
> +		}
>   		key_read(c, &br->key, &lower_key);
>   		if (iip + 1 < le16_to_cpu(idx->child_cnt)) {
>   			br = ubifs_idx_branch(c, idx, iip + 1);
> diff --git a/fs/ubifs/tnc_misc.c b/fs/ubifs/tnc_misc.c
> index 10b222dc6a5..e3ae17977c9 100644
> --- a/fs/ubifs/tnc_misc.c
> +++ b/fs/ubifs/tnc_misc.c
> @@ -357,9 +357,9 @@ static int read_znode(struct ubifs_info *c, struct ubifs_zbranch *zzbr,
>   		}
>   
>   		if (znode->level)
> -			continue;
> -
> -		type = key_type(c, &zbr->key);
> +			type = UBIFS_IDX_NODE;
> +		else
> +			type = key_type(c, &zbr->key);
>   		if (c->ranges[type].max_len == 0) {
>   			if (zbr->len != c->ranges[type].len) {
>   				ubifs_err(c, "bad target node (type %d) length (%d)",
> .
>