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

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

[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(-)

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)",