Re: [PATCH] hfs: Validate CNIDs in hfs_read_inode

Tetsuo Handa
2025-10-09T12:58:19Z

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@I-love.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@syzkaller.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.