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;