| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20250826033557.127367-3-yang.chenzhi@vivo.com>
Date: Tue, 26 Aug 2025 11:35:55 +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 2/4] hfs: introduce __hfs_bnode_read* to fix KMSAN uninit-value
From: Yang Chenzhi <yang.chenzhi@...o.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@...eyko.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@...o.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
Powered by blists - more mailing lists