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