linux-kernel - Re: [PATCH] hfs: Validate CNIDs in hfs_read

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] [day] [month] [year] [list]

Message-ID: <559c331f-4838-49fb-95aa-2d1498c8a41e@I-love.SAKURA.ne.jp>
Date: Thu, 9 Oct 2025 21:57:33 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Viacheslav Dubeyko <slava@...eyko.com>,
        George Anthony Vernon <contact@...rnon.com>,
        Viacheslav Dubeyko <Slava.Dubeyko@....com>
Cc: "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
        "frank.li@...o.com" <frank.li@...o.com>,
        "skhan@...uxfoundation.org" <skhan@...uxfoundation.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel-mentees@...ts.linux.dev"
 <linux-kernel-mentees@...ts.linux.dev>,
        "syzbot+97e301b4b82ae803d21b@...kaller.appspotmail.com"
 <syzbot+97e301b4b82ae803d21b@...kaller.appspotmail.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

I found this patch. Please CC: me when posting V2.

On 2025/10/07 22:40, Viacheslav Dubeyko wrote:
> On Sat, 2025-10-04 at 02:25 +0100, George Anthony Vernon wrote:
>> On Fri, Oct 03, 2025 at 10:40:16PM +0000, Viacheslav Dubeyko wrote:
>>> Let's pay respect to previous efforts. I am suggesting to add this
>>> line:
>>>
>>> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
>>>
>>> Are you OK with it?
>> I agree with paying respect to Tetsuo. The kernel docs indicate that
>> the SoB tag
>> isn't used like that. Would the Suggested-by: tag be more
>> appropriate?
>>

I'm not suggesting this change. Therefore, Cc: might match.

But I don't like

  Tested-by: syzbot+97e301b4b82ae803d21b@...kaller.appspotmail.com

line, for syzbot only tested one cnid which was embedded in the
reproducer. My modified reproducer which tests all range still hits
BUG() when the inode number of the record retrieved as a result of
hfs_cat_find_brec(HFS_ROOT_CNID) is HFS_POR_CNID. That is why I push
https://lkml.kernel.org/r/427fcb57-8424-4e52-9f21-7041b2c4ae5b@I-love.SAKURA.ne.jp
as a fix for this problem (and you can propose this patch as a
further sanity check). Unless

>>>
>>>> +{
>>>> +	if (likely(cnid >= HFS_FIRSTUSER_CNID))
>>>> +		return true;
>>>> +
>>>> +	switch (cnid) {
>>>> +	case HFS_POR_CNID:

we disable HFS_POR_CNID case (which I guess it is wrong to do so),
we shall hit BUG() in hfs_write_inode().

>>>> +	case HFS_ROOT_CNID:
>>>> +		return type == HFS_CDR_DIR;
>>>> +	case HFS_EXT_CNID:
>>>> +	case HFS_CAT_CNID:
>>>> +	case HFS_BAD_CNID:
>>>> +	case HFS_EXCH_CNID:
>>>> +		return type == HFS_CDR_FIL;
>>>> +	default:
>>>> +		return false;
>>>



>>> int hfs_write_inode(struct inode *inode, struct writeback_control
>>> *wbc)
>>> {
>>> 	struct inode *main_inode = inode;
>>> 	struct hfs_find_data fd;
>>> 	hfs_cat_rec rec;
>>> 	int res;
>>>
>>> 	hfs_dbg("ino %lu\n", inode->i_ino);
>>> 	res = hfs_ext_write_extent(inode);
>>> 	if (res)
>>> 		return res;
>>>
>>> 	if (inode->i_ino < HFS_FIRSTUSER_CNID) {
>>> 		switch (inode->i_ino) {
>>> 		case HFS_ROOT_CNID:
>>> 			break;
>>> 		case HFS_EXT_CNID:
>>> 			hfs_btree_write(HFS_SB(inode->i_sb)-
>>>> ext_tree);
>>> 			return 0;
>>> 		case HFS_CAT_CNID:
>>> 			hfs_btree_write(HFS_SB(inode->i_sb)-
>>>> cat_tree);
>>> 			return 0;
>>> 		default:
>>> 			BUG();
>>> 			return -EIO;
>>>
>>> I think we need to select something one here. :) I believe we need
>>> to remove
>>> BUG() and return -EIO, finally. What do you think? 

I think that removing this BUG() now is wrong.
Without my patch, the inode number of the record retrieved as a
result of hfs_cat_find_brec(HFS_ROOT_CNID) can be HFS_POR_CNID or
greater than HFS_FIRSTUSER_CNID, which I think is a logical error
in the filesystem image.

>>
>> I think that with validation of inodes in hfs_read_inode this code
>> path should
>> no longer be reachable by poking the kernel interface from userspace.
>> If it is
>> ever reached, it means kernel logic is broken, so it should be
>> treated as a bug.

Your patch is incomplete. Please also apply my patch.