[PATCH v7] hfs: validate record ID against requested CNID in hfs_cat_find_brec()

[Tetsuo Handa]

syzbot is reporting that BUG() in hfs_write_inode() fires upon unmount
operation when the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) is not HFS_ROOT_CNID, for
commit b905bafdea21 ("hfs: Sanity check the root record") checked
the record size and the record type but did not check the inode number.

Initially, Viacheslav Dubeyko was assuming that we can fix this problem
by adding validation to hfs_read_inode(), and George Anthony Vernon is
proposing a patch that adds validation to hfs_read_inode().

While I am not against adding validation to hfs_read_inode(), treating
an inode which is not the root inode as if the root inode is a logical
error which should be rejected regardless of whether we hit BUG() or not.
And we confirmed that we can't fix this logical error by adding validation
to hfs_read_inode().

Since hfs_brec_read() doesn't receive which type of struct (one of "struct
hfs_cat_file", "struct hfs_cat_dir" or "struct hfs_cat_thread") does the
caller of hfs_brec_read() want to read as function argument, currently
it is impossible for hfs_brec_read() to return 0 only when the retrieved
record is what the caller wants, despite the HFS's on-disk layout includes
type of struct within the retrieved record.

Therefore, validate the inode number within hfs_cat_find_brec(), for
it seems that Viacheslav Dubeyko does not want to touch hfs_fill_super().
Unfortunately, we cannot remove rec.type == HFS_CDR_DIR test from
hfs_fill_super() despite hfs_cat_find_brec() guarantees that it returns 0
only when the retrieved cnid is the requested one, for the retrieved cnid
by chance matches the requested cnid with HFS_CDR_FIL type when when the
caller wants HFS_CDR_DIR. Also, since hfs_cat_find_brec() does not return
which record type it found, hfs_fill_super() needs to call hfs_bnode_read()
again. Not optimal but inevitable if we don't want to touch
hfs_fill_super()...

Reported-by: syzbot+97e301b4b82ae803d21b@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=97e301b4b82ae803d21b
Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
---
To: Viacheslav Dubeyko

Since hfs_cat_find_brec() is currently called from only hfs_fill_super() with
cnid == HFS_ROOT_CNID, we know that hfs_cat_find_brec() can assume that the caller wants
HFS_CDR_DIR. But since I didn't get response from you on the optimized version
at https://lore.kernel.org/all/9f66743b-70e0-4886-884e-5203f5c02ed8@xxxxxxxxxxxxxxxxxxx/ ,
I can't understand what you meant from the "Please, check the HFS's on-disk layout." response
at https://lore.kernel.org/all/c732a6e39e833b4b63499059cd388b753e1b2297.camel@xxxxxxxxxx/ .
Do you want to add "which type of struct the caller wants" argument to hfs_brec_read()
so that we can validate record type and fd->entrylength inside hfs_brec_read() ?
Do you want to add "which type of struct the caller wants" argument to hfs_cat_find_brec()
so that we can eliminate the need to call hfs_bnode_read() from both functions?
Unless you have a plan to call hfs_cat_find_brec() from other locations, we should avoid
adding unused code path.

 fs/hfs/catalog.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index 7f5339ee57c1..bac0218a736c 100644
--- a/fs/hfs/catalog.c
+++ b/fs/hfs/catalog.c
@@ -208,7 +208,20 @@ int hfs_cat_find_brec(struct super_block *sb, u32 cnid,
 		return -EIO;
 	}
 	memcpy(fd->search_key->cat.CName.name, rec.thread.CName.name, len);
-	return hfs_brec_find(fd);
+	res = hfs_brec_find(fd);
+	if (res)
+		return res;
+	if (rec.type == HFS_CDR_THD && fd->entrylength == sizeof(rec.dir)) {
+		hfs_bnode_read(fd->bnode, &rec.dir, fd->entryoffset, sizeof(rec.dir));
+		if (rec.type == HFS_CDR_DIR && cnid == be32_to_cpu(rec.dir.DirID))
+			return 0;
+	} else if (rec.type == HFS_CDR_FTH && fd->entrylength == sizeof(rec.file)) {
+		hfs_bnode_read(fd->bnode, &rec.file, fd->entryoffset, sizeof(rec.file));
+		if (rec.type == HFS_CDR_FIL && cnid == be32_to_cpu(rec.file.FlNum))
+			return 0;
+	}
+	pr_err("found corrupted record in catalog\n");
+	return -EIO;
 }
 
 static inline
-- 
2.54.0