Message-Id: <20250827064018.327046-1-yang.chenzhi@vivo.com>
Date: Wed, 27 Aug 2025 14:40:18 +0800
From: Chenzhi Yang <yang.chenzhi@...o.com>
To: slava@...eyko.com,
	glaubitz@...sik.fu-berlin.de,
	frank.li@...o.com
Cc: linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Yang Chenzhi <yang.chenzhi@...o.com>
Subject: [RFC PATCH v2] hfs: add return values to hfs_brec_lenoff and hfs_bnode_read to improve robustness

From: Yang Chenzhi <yang.chenzhi@...o.com>

This patch addresses two issues in the hfs filesystem:

1. out-of-bounds access in hfs_bmap_alloc

Analysis can be found here:
https://lore.kernel.org/all/20250818141734.8559-2-yang.chenzhi@vivo.com/

With specially crafted offsets from syzbot, hfs_brec_lenoff()
could return invalid offset and length values.

This patch introduces a return value for hfs_brec_lenoff().
The function now validates offset and length:
  - if invalid, it returns an error code;
  - if valid, it returns 0.

All callers of hfs_brec_lenoff() are updated to check its return
value before using offset and length, thus preventing out-of-bounds access.

2. potential use of uninitialized memory in hfs_bnode_dump

Related bug report:
https://syzkaller.appspot.com/bug?extid=f687659f3c2acfa34201

This bug was previously fixed in commit:
commit a431930c9bac518bf99d6b1da526a7f37ddee8d8

However, a new syzbot report indicated a KMSAN use-uninit-value.
The root cause is that hfs_bnode_dump() calls hfs_bnode_read_u16()
with an invalid offset.
  - hfs_bnode_read() detects the invalid offset and returns immediately;
  - Back in hfs_bnode_read_u16(), be16_to_cpu() was then called on an
    uninitialized variable.

To address this, the intended direction is for hfs_bnode_read()
to return a status code (0 on success, negative errno on failure)
so that callers can detect errors and exit early, avoiding the use
of uninitialized memory.

However, hfs_bnode_read() is widely used, this patch does not modify
it directly. Instead, new __hfs_bnode_read*() helper functions are
introduced, which mirror the original behavior but add offset/length
validation and return values.

For now, only the hfs_bnode_dump() code path is updated to use these
helpers in order to validate the feasibility of this approach.

After applying the patch, the xfstests quick suite was run:
  - The previously failing generic/113 test now passes;
  - All other test cases remain unchanged.

-------------------------------------------

The main idea of this patch is to:
Add explicit return values to critical functions so that
invalid offset/length values are reported via error codes;

Require all callers to check return values, ensuring
invalid parameters are not propagated further;

Improve the overall robustness of the HFS codebase and
protect against syzbot-crafted invalid inputs.

RFC: feedback is requested on whether adding return values
to hfs_brec_lenoff() and hfs_bnode_read() in this manner
is an acceptable direction, and if such safety improvements
should be expanded more broadly within the HFS subsystem.

Signed-off-by: Yang Chenzhi <yang.chenzhi@...o.com>
---
 fs/hfs/bfind.c | 14 ++++----
 fs/hfs/bnode.c | 87 +++++++++++++++++++++++++++++++++++---------------
 fs/hfs/brec.c  | 13 ++++++--
 fs/hfs/btree.c | 21 +++++++++---
 fs/hfs/btree.h | 21 +++++++++++-
 5 files changed, 116 insertions(+), 40 deletions(-)

diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
index 34e9804e0f36..aea6edd4d830 100644
--- a/fs/hfs/bfind.c
+++ b/fs/hfs/bfind.c
@@ -61,16 +61,16 @@ int __hfs_brec_find(struct hfs_bnode *bnode, struct hfs_find_data *fd)
 	u16 off, len, keylen;
 	int rec;
 	int b, e;
-	int res;
+	int res, ret;
 
 	b = 0;
 	e = bnode->num_recs - 1;
 	res = -ENOENT;
 	do {
 		rec = (e + b) / 2;
-		len = hfs_brec_lenoff(bnode, rec, &off);
+		ret = hfs_brec_lenoff(bnode, rec, &off, &len);
 		keylen = hfs_brec_keylen(bnode, rec);
-		if (keylen == 0) {
+		if (keylen == 0 || ret) {
 			res = -EINVAL;
 			goto fail;
 		}
@@ -87,9 +87,9 @@ int __hfs_brec_find(struct hfs_bnode *bnode, struct hfs_find_data *fd)
 			e = rec - 1;
 	} while (b <= e);
 	if (rec != e && e >= 0) {
-		len = hfs_brec_lenoff(bnode, e, &off);
+		ret = hfs_brec_lenoff(bnode, e, &off, &len);
 		keylen = hfs_brec_keylen(bnode, e);
-		if (keylen == 0) {
+		if (keylen == 0 || ret) {
 			res = -EINVAL;
 			goto fail;
 		}
@@ -223,9 +223,9 @@ int hfs_brec_goto(struct hfs_find_data *fd, int cnt)
 		fd->record += cnt;
 	}
 
-	len = hfs_brec_lenoff(bnode, fd->record, &off);
+	res = hfs_brec_lenoff(bnode, fd->record, &off, &len);
 	keylen = hfs_brec_keylen(bnode, fd->record);
-	if (keylen == 0) {
+	if (keylen == 0 || res) {
 		res = -EINVAL;
 		goto out;
 	}
diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
index e8cd1a31f247..b0bbaf016b8d 100644
--- a/fs/hfs/bnode.c
+++ b/fs/hfs/bnode.c
@@ -57,26 +57,16 @@ int check_and_correct_requested_length(struct hfs_bnode *node, int off, int len)
 	return len;
 }
 
-void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len)
+int __hfs_bnode_read(struct hfs_bnode *node, void *buf, u16 off, u16 len)
 {
 	struct page *page;
 	int pagenum;
 	int bytes_read;
 	int bytes_to_read;
 
-	if (!is_bnode_offset_valid(node, off))
-		return;
-
-	if (len == 0) {
-		pr_err("requested zero length: "
-		       "NODE: id %u, type %#x, height %u, "
-		       "node_size %u, offset %d, len %d\n",
-		       node->this, node->type, node->height,
-		       node->tree->node_size, off, len);
-		return;
-	}
-
-	len = check_and_correct_requested_length(node, off, len);
+	/* len = 0 is invalid: prevent use of an uninitalized buffer*/
+	if (!len || !hfs_off_and_len_is_valid(node, off, len))
+		return -EINVAL;
 
 	off += node->page_offset;
 	pagenum = off >> PAGE_SHIFT;
@@ -93,6 +83,47 @@ void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len)
 		pagenum++;
 		off = 0; /* page offset only applies to the first page */
 	}
+
+	return 0;
+}
+
+static int __hfs_bnode_read_u16(struct hfs_bnode *node, u16* buf, u16 off)
+{
+	__be16 data;
+	int res;
+
+	res = __hfs_bnode_read(node, (void*)(&data), off, 2);
+	if (res)
+		return res;
+	*buf = be16_to_cpu(data);
+	return 0;
+}
+
+
+static int __hfs_bnode_read_u8(struct hfs_bnode *node, u8* buf, u16 off)
+{
+	int res;
+
+	res = __hfs_bnode_read(node, (void*)(&buf), off, 2);
+	if (res)
+		return res;
+	return 0;
+}
+
+void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len)
+{
+	int res;
+
+	len = check_and_correct_requested_length(node, off, len);
+	res = __hfs_bnode_read(node, buf, (u16)off, (u16)len);
+	if (res) {
+		pr_err("hfs_bnode_read error: "
+		       "NODE: id %u, type %#x, height %u, "
+		       "node_size %u, offset %d, len %d\n",
+		       node->this, node->type, node->height,
+		       node->tree->node_size, off, len);
+	}
+	return;
 }
 
 u16 hfs_bnode_read_u16(struct hfs_bnode *node, int off)
@@ -241,7 +272,8 @@ void hfs_bnode_dump(struct hfs_bnode *node)
 {
 	struct hfs_bnode_desc desc;
 	__be32 cnid;
-	int i, off, key_off;
+	int i, res;
+	u16 off, key_off;
 
 	hfs_dbg(BNODE_MOD, "bnode: %d\n", node->this);
 	hfs_bnode_read(node, &desc, 0, sizeof(desc));
@@ -251,23 +283,28 @@ void hfs_bnode_dump(struct hfs_bnode *node)
 
 	off = node->tree->node_size - 2;
 	for (i = be16_to_cpu(desc.num_recs); i >= 0; off -= 2, i--) {
-		key_off = hfs_bnode_read_u16(node, off);
+		res = __hfs_bnode_read_u16(node, &key_off, off);
+		if (res) return;
 		hfs_dbg_cont(BNODE_MOD, " %d", key_off);
 		if (i && node->type == HFS_NODE_INDEX) {
-			int tmp;
-
-			if (node->tree->attributes & HFS_TREE_VARIDXKEYS)
-				tmp = (hfs_bnode_read_u8(node, key_off) | 1) + 1;
-			else
+			u8 tmp, data;
+			if (node->tree->attributes & HFS_TREE_VARIDXKEYS) {
+				res = __hfs_bnode_read_u8(node, &data, key_off);
+				if (res) return;
+				tmp = (data | 1) + 1;
+			} else {
 				tmp = node->tree->max_key_len + 1;
-			hfs_dbg_cont(BNODE_MOD, " (%d,%d",
-				     tmp, hfs_bnode_read_u8(node, key_off));
+			}
+			res = __hfs_bnode_read_u8(node, &data, key_off);
+			if (res) return;
+			hfs_dbg_cont(BNODE_MOD, " (%d,%d", tmp, data);
 			hfs_bnode_read(node, &cnid, key_off + tmp, 4);
 			hfs_dbg_cont(BNODE_MOD, ",%d)", be32_to_cpu(cnid));
 		} else if (i && node->type == HFS_NODE_LEAF) {
-			int tmp;
+			u8 tmp;
 
-			tmp = hfs_bnode_read_u8(node, key_off);
+			res = __hfs_bnode_read_u8(node, &tmp, key_off);
+			if (res) return;
 			hfs_dbg_cont(BNODE_MOD, " (%d)", tmp);
 		}
 	}
diff --git a/fs/hfs/brec.c b/fs/hfs/brec.c
index 896396554bcc..d7026a3ffeea 100644
--- a/fs/hfs/brec.c
+++ b/fs/hfs/brec.c
@@ -16,15 +16,22 @@ static int hfs_brec_update_parent(struct hfs_find_data *fd);
 static int hfs_btree_inc_height(struct hfs_btree *tree);
 
 /* Get the length and offset of the given record in the given node */
-u16 hfs_brec_lenoff(struct hfs_bnode *node, u16 rec, u16 *off)
+int hfs_brec_lenoff(struct hfs_bnode *node, u16 rec, u16 *off, u16 *len)
 {
 	__be16 retval[2];
 	u16 dataoff;
+	int res;
 
 	dataoff = node->tree->node_size - (rec + 2) * 2;
-	hfs_bnode_read(node, retval, dataoff, 4);
+	res = __hfs_bnode_read(node, retval, dataoff, 4);
+	if (res)
+		return -EINVAL;
 	*off = be16_to_cpu(retval[1]);
-	return be16_to_cpu(retval[0]) - *off;
+	*len = be16_to_cpu(retval[0]) - *off;
+	if (!hfs_off_and_len_is_valid(node, *off, *len) ||
+			*off < sizeof(struct hfs_bnode_desc))
+		return -EINVAL;
+	return 0;
 }
 
 /* Get the length of the key from a keyed record */
diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
index e86e1e235658..b13582dcc27a 100644
--- a/fs/hfs/btree.c
+++ b/fs/hfs/btree.c
@@ -301,7 +301,9 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
 	node = hfs_bnode_find(tree, nidx);
 	if (IS_ERR(node))
 		return node;
-	len = hfs_brec_lenoff(node, 2, &off16);
+	res = hfs_brec_lenoff(node, 2, &off16, &len);
+	if (res)
+		return ERR_PTR(res);
 	off = off16;
 
 	off += node->page_offset;
@@ -347,7 +349,9 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
 			return next_node;
 		node = next_node;
 
-		len = hfs_brec_lenoff(node, 0, &off16);
+		res = hfs_brec_lenoff(node, 0, &off16, &len);
+		if (res)
+			return ERR_PTR(res);
 		off = off16;
 		off += node->page_offset;
 		pagep = node->page + (off >> PAGE_SHIFT);
@@ -363,6 +367,7 @@ void hfs_bmap_free(struct hfs_bnode *node)
 	u16 off, len;
 	u32 nidx;
 	u8 *data, byte, m;
+	int res;
 
 	hfs_dbg(BNODE_MOD, "btree_free_node: %u\n", node->this);
 	tree = node->tree;
@@ -370,7 +375,9 @@ void hfs_bmap_free(struct hfs_bnode *node)
 	node = hfs_bnode_find(tree, 0);
 	if (IS_ERR(node))
 		return;
-	len = hfs_brec_lenoff(node, 2, &off);
+	res = hfs_brec_lenoff(node, 2, &off, &len);
+	if (res)
+		goto fail;
 	while (nidx >= len * 8) {
 		u32 i;
 
@@ -394,7 +401,9 @@ void hfs_bmap_free(struct hfs_bnode *node)
 			hfs_bnode_put(node);
 			return;
 		}
-		len = hfs_brec_lenoff(node, 0, &off);
+		res = hfs_brec_lenoff(node, 0, &off, &len);
+		if (res)
+			goto fail;
 	}
 	off += node->page_offset + nidx / 8;
 	page = node->page[off >> PAGE_SHIFT];
@@ -415,4 +424,8 @@ void hfs_bmap_free(struct hfs_bnode *node)
 	hfs_bnode_put(node);
 	tree->free_nodes++;
 	mark_inode_dirty(tree->inode);
+	return;
+fail:
+	hfs_bnode_put(node);
+	pr_err("fail to free a bnode due to invalid off or len\n");
 }
diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
index 0e6baee93245..78f228e62a86 100644
--- a/fs/hfs/btree.h
+++ b/fs/hfs/btree.h
@@ -94,6 +94,7 @@ extern struct hfs_bnode * hfs_bmap_alloc(struct hfs_btree *);
 extern void hfs_bmap_free(struct hfs_bnode *node);
 
 /* bnode.c */
+extern int __hfs_bnode_read(struct hfs_bnode *, void *, u16, u16);
 extern void hfs_bnode_read(struct hfs_bnode *, void *, int, int);
 extern u16 hfs_bnode_read_u16(struct hfs_bnode *, int);
 extern u8 hfs_bnode_read_u8(struct hfs_bnode *, int);
@@ -116,7 +117,7 @@ extern void hfs_bnode_get(struct hfs_bnode *);
 extern void hfs_bnode_put(struct hfs_bnode *);
 
 /* brec.c */
-extern u16 hfs_brec_lenoff(struct hfs_bnode *, u16, u16 *);
+extern int hfs_brec_lenoff(struct hfs_bnode *, u16, u16 *, u16 *);
 extern u16 hfs_brec_keylen(struct hfs_bnode *, u16);
 extern int hfs_brec_insert(struct hfs_find_data *, void *, int);
 extern int hfs_brec_remove(struct hfs_find_data *);
@@ -170,3 +171,21 @@ struct hfs_btree_header_rec {
 						   max key length. use din catalog
 						   b-tree but not in extents
 						   b-tree (hfsplus). */
+static inline
+bool hfs_off_and_len_is_valid(struct hfs_bnode *node, u16 off, u16 len)
+{
+	bool ret = true;
+	if (off > node->tree->node_size ||
+			off + len > node->tree->node_size)
+		ret = false;
+
+	if (!ret) {
+		pr_err("requested invalid offset: "
+		       "NODE: id %u, type %#x, height %u, "
+		       "node_size %u, offset %u, length %u\n",
+		       node->this, node->type, node->height,
+		       node->tree->node_size, off, len);
+	}
+
+	return ret;
+}
-- 
2.43.0

Powered by blists - more mailing lists