linux-kernel - Re: [PATCH v2 2/2] hfs: Update sanity check of the root record

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d8cc277f-c14c-4aee-ac0b-cce2938232d8@I-love.SAKURA.ne.jp>
Date: Fri, 14 Nov 2025 23:18:02 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>,
        George Anthony Vernon <contact@...rnon.com>
Cc: "frank.li@...o.com" <frank.li@...o.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel-mentees@...ts.linux.dev"
 <linux-kernel-mentees@...ts.linux.dev>,
        "slava@...eyko.com"
 <slava@...eyko.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "syzbot+97e301b4b82ae803d21b@...kaller.appspotmail.com"
 <syzbot+97e301b4b82ae803d21b@...kaller.appspotmail.com>,
        "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
        "skhan@...uxfoundation.org" <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2 2/2] hfs: Update sanity check of the root record

On 2025/11/12 7:56, Viacheslav Dubeyko wrote:
> The file system is mounted only if hfs_fill_super() created root node and return
> 0 [1]. However, if hfs_iget() return bad inode [2] and we will call
> is_bad_inode() here [3]:
> 
> 	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
> 	hfs_find_exit(&fd);
> 	if (!root_inode || is_bad_inode(root_inode)) <-- call will be here
> 		goto bail_no_root;
> 
> then, mount will fail. So, no successful mount will happen because
> is_valid_cnid() will manage the check in hfs_read_inode().

Do you admit that mounting (and optionally fuzzing on) a bad inode (an inode
which was subjected to make_bad_inode()) is useless?

Adding is_bad_inode() check without corresponding iput() in error path causes
an inode leak bug. Also, error code will differ (my patch returns -EIO while
your approach will return -EINVAL).

Honestly speaking, I don't like use of make_bad_inode(). make_bad_inode() might
change file type. Also, I worry that make_bad_inode() causes a subtle race bug
like https://syzkaller.appspot.com/bug?extid=b7c3ba8cdc2f6cf83c21 which has not
come to a conclusion.

Why can't we remove make_bad_inode() usage from hfs_read_inode() and return non-0 value
(so that inode_insert5() will return NULL and iget5_locked() will call destroy_inode()
and return NULL) when hfs_read_inode() encountered an invalid entry?