HFS syzbot BUG: test patch validating metadata before hfs_write_inode()

HFS syzbot BUG: test patch validating metadata before hfs_write_inode()

[David Maximiliano Hermitte]

Hi Slava,

I followed the direction you pointed out and tested a small HFS validation patch for the syzbot crash path.

This is not intended as a final upstream patch yet. I am sending it as a test patch to demonstrate the crash direction and to ask whether this is the right validation point. If the direction is correct, I would appreciate guidance on the preferred upstream placement and syntax before a formal submission.

My understanding is that the BUG() in hfs_write_inode() is the endpoint, not the root cause. This test patch keeps hfs_write_inode() intact and rejects corrupted HFS metadata earlier in hfs_read_inode() and hfs_brec_read().

Local QEMU result:

BEFORE_UNPATCHED:
- repro_started=true
- kernel_bug_seen=true
- verdict=BEFORE_BUG_REPRODUCED

AFTER_V04_PATCHED:
- repro_started=true
- clean_hfs_rejects=50
- kernel_bug=False
- oops=False
- kasan=False
- hfs_write_inode=False
- verdict=AFTER_REPRO_RAN_NO_BUG_SEEN

checkpatch.pl --strict:
- 0 errors, 0 warnings

The test patch is included below inline. Full QEMU serial logs are available if useful.

Please let me know whether this validation direction matches what you expect, or whether the fix should be placed differently.

Best regards,
David

--- test patch follows ---

diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
index d56e47b..5f56cab 100644
--- a/fs/hfs/bfind.c
+++ b/fs/hfs/bfind.c
@@ -174,8 +174,10 @@ int hfs_brec_read(struct hfs_find_data *fd, void *rec, u32 rec_len)
 	res = hfs_brec_find(fd);
 	if (res)
 		return res;
+	if (fd->entryoffset < 0 || fd->entrylength <= 0)
+		return -EFSCORRUPTED;
 	if (fd->entrylength > rec_len)
-		return -EINVAL;
+		return -EFSCORRUPTED;
 	hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd->entrylength);
 	return 0;
 }
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 878535d..beefb47 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -369,6 +369,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
 	rec = idata->rec;
 	switch (rec->type) {
 	case HFS_CDR_FIL:
+		if (be32_to_cpu(rec->file.FlNum) < HFS_FIRSTUSER_CNID)
+			return -EFSCORRUPTED;
 		if (!HFS_IS_RSRC(inode)) {
 			hfs_inode_read_fork(inode, rec->file.ExtRec, rec->file.LgLen,
 					    rec->file.PyLen, be16_to_cpu(rec->file.ClpSize));
@@ -390,6 +392,9 @@ static int hfs_read_inode(struct inode *inode, void *data)
 		inode->i_mapping->a_ops = &hfs_aops;
 		break;
 	case HFS_CDR_DIR:
+		if (be32_to_cpu(rec->dir.DirID) < HFS_FIRSTUSER_CNID &&
+		    be32_to_cpu(rec->dir.DirID) != HFS_ROOT_CNID)
+			return -EFSCORRUPTED;
 		inode->i_ino = be32_to_cpu(rec->dir.DirID);
 		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
 		HFS_I(inode)->fs_blocks = 0;

Re: HFS syzbot BUG: test patch validating metadata before hfs_write_inode()

[Viacheslav Dubeyko]

Hi David,

On Fri, 2026-06-12 at 16:20 +0000, David Maximiliano Hermitte wrote:
> Hi Slava,
> 
> I followed the direction you pointed out and tested a small HFS
> validation patch for the syzbot crash path.
> 
> This is not intended as a final upstream patch yet. I am sending it
> as a test patch to demonstrate the crash direction and to ask whether
> this is the right validation point. If the direction is correct, I
> would appreciate guidance on the preferred upstream placement and
> syntax before a formal submission.
> 
> My understanding is that the BUG() in hfs_write_inode() is the
> endpoint, not the root cause. This test patch keeps hfs_write_inode()
> intact and rejects corrupted HFS metadata earlier in hfs_read_inode()
> and hfs_brec_read().
> 
> Local QEMU result:
> 
> BEFORE_UNPATCHED:
> - repro_started=true
> - kernel_bug_seen=true
> - verdict=BEFORE_BUG_REPRODUCED
> 
> AFTER_V04_PATCHED:
> - repro_started=true
> - clean_hfs_rejects=50
> - kernel_bug=False
> - oops=False
> - kasan=False
> - hfs_write_inode=False
> - verdict=AFTER_REPRO_RAN_NO_BUG_SEEN
> 
> checkpatch.pl --strict:
> - 0 errors, 0 warnings
> 
> The test patch is included below inline. Full QEMU serial logs are
> available if useful.
> 
> Please let me know whether this validation direction matches what you
> expect, or whether the fix should be placed differently.
> 

Sorry, I am in personal trip right now. And I have not much time and
capabilities for deep review and discussion. I am going to be back to
normal routine around June 26th. I hope our discussion and fix can wait
two weeks. :) Could you please ping me and revive the discussion around
that time?

Thanks,
Slava.

> Best regards,
> David
> 
> --- test patch follows ---
> 
> diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
> index d56e47b..5f56cab 100644
> --- a/fs/hfs/bfind.c
> +++ b/fs/hfs/bfind.c
> @@ -174,8 +174,10 @@ int hfs_brec_read(struct hfs_find_data *fd, void
> *rec, u32 rec_len)
>  	res = hfs_brec_find(fd);
>  	if (res)
>  		return res;
> +	if (fd->entryoffset < 0 || fd->entrylength <= 0)
> +		return -EFSCORRUPTED;
>  	if (fd->entrylength > rec_len)
> -		return -EINVAL;
> +		return -EFSCORRUPTED;
>  	hfs_bnode_read(fd->bnode, rec, fd->entryoffset, fd-
> >entrylength);
>  	return 0;
>  }
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 878535d..beefb47 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -369,6 +369,8 @@ static int hfs_read_inode(struct inode *inode,
> void *data)
>  	rec = idata->rec;
>  	switch (rec->type) {
>  	case HFS_CDR_FIL:
> +		if (be32_to_cpu(rec->file.FlNum) <
> HFS_FIRSTUSER_CNID)
> +			return -EFSCORRUPTED;
>  		if (!HFS_IS_RSRC(inode)) {
>  			hfs_inode_read_fork(inode, rec->file.ExtRec,
> rec->file.LgLen,
>  					    rec->file.PyLen,
> be16_to_cpu(rec->file.ClpSize));
> @@ -390,6 +392,9 @@ static int hfs_read_inode(struct inode *inode,
> void *data)
>  		inode->i_mapping->a_ops = &hfs_aops;
>  		break;
>  	case HFS_CDR_DIR:
> +		if (be32_to_cpu(rec->dir.DirID) < HFS_FIRSTUSER_CNID
> &&
> +		    be32_to_cpu(rec->dir.DirID) != HFS_ROOT_CNID)
> +			return -EFSCORRUPTED;
>  		inode->i_ino = be32_to_cpu(rec->dir.DirID);
>  		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
>  		HFS_I(inode)->fs_blocks = 0;