[PATCH RFC 0/4] Discuss to add return value in hfs_bnode_read* and hfs_brec_lenoff

[PATCH RFC 0/4] Discuss to add return value in hfs_bnode_read* and hfs_brec_lenoff

[Chenzhi Yang]

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

Hello,

This patchset addresses two issues:

1. Unchecked offset/length leading to out-of-bounds memory access. 
   syzbot has reported such a bug in hfs_bmap_alloc, and hfs_bmap_free
   has a similar potential problem.  

   To mitigate this, I added offset/length validation in `hfs_brec_lenoff`.

   This ensures callers always receive valid parameters, and in case of
   invalid values, an error code will be returned instead of risking
   memory corruption.

2. Use of uninitialized memory due to early return in hfs_bnode_read.

   Recent commits have introduced offset/length validation in hfs_bnode_read. 
   However, when an invalid offset is detected, the function currently 
   returns early without initializing the provided buffer.

   This leads to a scenario where hfs_bnode_read_u16 may call be16_to_cpu
   on uninitialized data.

While there could be multiple ways to fix these issues, adding proper
error return values to the affected functions seems the safest solution.

However, this approach would require substantial changes across the
codebase. In this patch, I only modified a small example function to
illustrate the idea and seek feedback from the community: 
Should we move forward with this direction and extend it more broadly?

In addition, this patch allows xfstests generic/113 to pass, which
previously failed.

Yang Chenzhi (4):
  hfs: add hfs_off_and_len_is_valid helper
  hfs: introduce __hfs_bnode_read* to fix KMSAN uninit-value
  hfs: restruct hfs_bnode_read
  hfs: restructure hfs_brec_lenoff into a returned-value version

 fs/hfs/bfind.c | 12 +++----
 fs/hfs/bnode.c | 87 +++++++++++++++++++++++++++++++++++---------------
 fs/hfs/brec.c  | 12 +++++--
 fs/hfs/btree.c | 21 +++++++++---
 fs/hfs/btree.h | 22 ++++++++++++-
 5 files changed, 115 insertions(+), 39 deletions(-)

-- 
2.43.0

[RFC PATCH 1/4] hfs: add hfs_off_and_len_is_valid helper

[Chenzhi Yang]

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

Introduce a helper function hfs_off_and_len_is_valid, which combines
is_bnode_offset_valid and check_and_correct_request_len.

The motivation is that check_and_correct_request_len correcting the
length may force the caller to continue the execution, but the
corrected length might not match the buffer size, this may trigger a
out-of-bounds memory access. In addition, if the bnode is corrupted,
continuing to read data may trigger unknown bugs.

It is still unclear whether there are special cases where the
length must be corrected, so instead of replacing the existing
logic, this helper function is added.

Signed-off-by: Yang Chenzhi <yang.chenzhi@vivo.com>
---
 fs/hfs/btree.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
index 0e6baee93245..fb69f66409f4 100644
--- a/fs/hfs/btree.h
+++ b/fs/hfs/btree.h
@@ -170,3 +170,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

[RFC PATCH 2/4] hfs: introduce __hfs_bnode_read* to fix KMSAN uninit-value

[Chenzhi Yang]

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

Original bug report:
https://groups.google.com/g/syzkaller-bugs/c/xSRmGOV0xLw/m/D8DA6JGcCAAJ

This bug was fixed by the commit:

commit a431930c9bac518bf99d6b1da526a7f37ddee8d8
Author: Viacheslav Dubeyko <slava@dubeyko.com>
Date:   Thu Jul 3 14:49:12 2025 -0700

However, hfs_bnode_read will return early detecting invalid offset.
In that case, hfs_bnode_read_u16 accesses and uninitialized data variable
via be32_to_cpu(data), which trigger a KMSAN uninit-value report.

This RFC patch introduce __hfs_bnode_read* helpers, which
are returned-value versions of hfs_bnode_read.

In principle, hfs_bnode_read() should return an error, but this API
is widely used across HFS. Some heavy functions such as
hfs_bnode_split() and hfs_brec_insert() do not have robust error
handling. Turning hfs_bnode_read() into an error-returning function
would therefore require significant rework and testing.

This patch is only a minimal attempt to address the issue and to
gather feedback. If error-returning semantics are not desired, simply
initializing the local variable in hfs_bnode_read_u16() and
hfs_bnode_read_u8() would also avoid the problem.

Replace old hfs_bnode_read* in hfs_bnode_dump with __hfs_bnode_read*.
so that an error return can be used to fix the problem

This is not a comprehensive fix yet; more work is needed.

Signed-off-by: Yang Chenzhi <yang.chenzhi@vivo.com>
---
 fs/hfs/bnode.c | 81 +++++++++++++++++++++++++++++++++++++++++++-------
 fs/hfs/btree.h |  1 +
 2 files changed, 71 insertions(+), 11 deletions(-)

diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
index e8cd1a31f247..da0ab993921d 100644
--- a/fs/hfs/bnode.c
+++ b/fs/hfs/bnode.c
@@ -57,6 +57,59 @@ int check_and_correct_requested_length(struct hfs_bnode *node, int off, int len)
 	return 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;
+
+	/* 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;
+	off &= ~PAGE_MASK; /* compute page offset for the first page */
+
+	for (bytes_read = 0; bytes_read < len; bytes_read += bytes_to_read) {
+		if (pagenum >= node->tree->pages_per_bnode)
+			break;
+		page = node->page[pagenum];
+		bytes_to_read = min_t(int, len - bytes_read, PAGE_SIZE - off);
+
+		memcpy_from_page(buf + bytes_read, page, off, bytes_to_read);
+
+		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)
 {
 	struct page *page;
@@ -241,7 +294,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 +305,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/btree.h b/fs/hfs/btree.h
index fb69f66409f4..bf780bf4a016 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);
-- 
2.43.0

[RFC PATCH 3/4] hfs: restruct hfs_bnode_read

[Chenzhi Yang]

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

Since __hfs_bnode_read and hfs_bnode_read have the same
implementation, make hfs_bnode_read call __hfs_bnode_read
and report errors when detected, while keeping the
original function logic unchanged.

Signed-off-by: Yang Chenzhi <yang.chenzhi@vivo.com>
---
 fs/hfs/bnode.c | 34 ++++++----------------------------
 1 file changed, 6 insertions(+), 28 deletions(-)

diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
index da0ab993921d..b0bbaf016b8d 100644
--- a/fs/hfs/bnode.c
+++ b/fs/hfs/bnode.c
@@ -112,40 +112,18 @@ static int __hfs_bnode_read_u8(struct hfs_bnode *node, u8* buf, u16 off)
 
 void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len)
 {
-	struct page *page;
-	int pagenum;
-	int bytes_read;
-	int bytes_to_read;
-
-	if (!is_bnode_offset_valid(node, off))
-		return;
+	int res;
 
-	if (len == 0) {
-		pr_err("requested zero length: "
+	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;
-	}
-
-	len = check_and_correct_requested_length(node, off, len);
-
-	off += node->page_offset;
-	pagenum = off >> PAGE_SHIFT;
-	off &= ~PAGE_MASK; /* compute page offset for the first page */
-
-	for (bytes_read = 0; bytes_read < len; bytes_read += bytes_to_read) {
-		if (pagenum >= node->tree->pages_per_bnode)
-			break;
-		page = node->page[pagenum];
-		bytes_to_read = min_t(int, len - bytes_read, PAGE_SIZE - off);
-
-		memcpy_from_page(buf + bytes_read, page, off, bytes_to_read);
-
-		pagenum++;
-		off = 0; /* page offset only applies to the first page */
 	}
+	return;
 }
 
 u16 hfs_bnode_read_u16(struct hfs_bnode *node, int off)
-- 
2.43.0

[RFC PATCH 4/4] hfs: restructure hfs_brec_lenoff into a returned-value version

[Chenzhi Yang]

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

Restructure hfs_brec_lenoff into a function that validates
both offset and length. This function now returns an error code
to indicate whether the execution is correct.

This helps fix slab out-of-bounds issues in hfs_bmap_alloc

Replace the old hfs_brec_lenoff interface with the new
version.

Signed-off-by: Yang Chenzhi <yang.chenzhi@vivo.com>
---
 fs/hfs/bfind.c | 14 +++++++-------
 fs/hfs/brec.c  | 13 ++++++++++---
 fs/hfs/btree.c | 21 +++++++++++++++++----
 fs/hfs/btree.h |  2 +-
 4 files changed, 35 insertions(+), 15 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/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 bf780bf4a016..78f228e62a86 100644
--- a/fs/hfs/btree.h
+++ b/fs/hfs/btree.h
@@ -117,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 *);
-- 
2.43.0

Re: [PATCH RFC 0/4] Discuss to add return value in hfs_bnode_read* and hfs_brec_lenoff

[Viacheslav Dubeyko]

On Tue, 2025-08-26 at 11:35 +0800, Chenzhi Yang wrote:
> From: Yang Chenzhi <yang.chenzhi@vivo.com>
> 
> Hello,
> 
> This patchset addresses two issues:
> 
> 1. Unchecked offset/length leading to out-of-bounds memory access. 
>    syzbot has reported such a bug in hfs_bmap_alloc, and
> hfs_bmap_free
>    has a similar potential problem.  
> 
>    To mitigate this, I added offset/length validation in
> `hfs_brec_lenoff`.
> 
>    This ensures callers always receive valid parameters, and in case
> of
>    invalid values, an error code will be returned instead of risking
>    memory corruption.
> 
> 2. Use of uninitialized memory due to early return in hfs_bnode_read.
> 
>    Recent commits have introduced offset/length validation in
> hfs_bnode_read. 
>    However, when an invalid offset is detected, the function
> currently 
>    returns early without initializing the provided buffer.
> 
>    This leads to a scenario where hfs_bnode_read_u16 may call
> be16_to_cpu
>    on uninitialized data.
> 
> While there could be multiple ways to fix these issues, adding proper
> error return values to the affected functions seems the safest
> solution.
> 
> However, this approach would require substantial changes across the
> codebase. In this patch, I only modified a small example function to
> illustrate the idea and seek feedback from the community: 
> Should we move forward with this direction and extend it more
> broadly?
> 
> In addition, this patch allows xfstests generic/113 to pass, which
> previously failed.
> 
> Yang Chenzhi (4):
>   hfs: add hfs_off_and_len_is_valid helper
>   hfs: introduce __hfs_bnode_read* to fix KMSAN uninit-value
>   hfs: restruct hfs_bnode_read
>   hfs: restructure hfs_brec_lenoff into a returned-value version
> 
>  fs/hfs/bfind.c | 12 +++----
>  fs/hfs/bnode.c | 87 +++++++++++++++++++++++++++++++++++-------------
> --
>  fs/hfs/brec.c  | 12 +++++--
>  fs/hfs/btree.c | 21 +++++++++---
>  fs/hfs/btree.h | 22 ++++++++++++-
>  5 files changed, 115 insertions(+), 39 deletions(-)

Frankly speaking, I don't see the point to split this fix on several
patches. It's really hard to understand the logic and correctness of
the fix in the current state of patchset. Could you please resend the
fix as one patch? It's not the big modification and one patch will be
more easy to review. Also, as far as I can see, patches depend on each
other and one patch will be more safe than the patchset.

Thanks,
Slava.