From: Yang Chenzhi <yang.chenzhi@xxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxx> --- 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
