linux-kernel - Re: [PATCH v3] hfs: remove BUG() from hfs_release_folio()/hfs_test_inode()/hfs_write

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: <bb8d0438-6db4-4032-ba44-f7b4155d2cef@I-love.SAKURA.ne.jp>
Date: Thu, 24 Jul 2025 15:55:42 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>,
        "willy@...radead.org" <willy@...radead.org>
Cc: "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
        "frank.li@...o.com" <frank.li@...o.com>,
        "slava@...eyko.com" <slava@...eyko.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH v3] hfs: remove BUG() from
 hfs_release_folio()/hfs_test_inode()/hfs_write_inode()

Then, something like below change?

--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -318,6 +318,9 @@ static int hfs_read_inode(struct inode *inode, void *data)
        struct hfs_iget_data *idata = data;
        struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
        hfs_cat_rec *rec;
+       /* https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID */
+       static const u16 bad_cnid_list = (1 << 0) | (1 << 6) | (1 << 7) | (1 << 8) |
+               (1 << 9) | (1 << 10) | (1 << 11) | (1 << 12) | (1 << 13);

        HFS_I(inode)->flags = 0;
        HFS_I(inode)->rsrc_inode = NULL;
@@ -358,6 +361,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
                inode->i_op = &hfs_file_inode_operations;
                inode->i_fop = &hfs_file_operations;
                inode->i_mapping->a_ops = &hfs_aops;
+               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
+                       make_bad_inode(inode);
                break;
        case HFS_CDR_DIR:
                inode->i_ino = be32_to_cpu(rec->dir.DirID);
@@ -368,6 +373,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
                                      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
                inode->i_op = &hfs_dir_inode_operations;
                inode->i_fop = &hfs_dir_operations;
+               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
+                       make_bad_inode(inode);
                break;
        default:
                make_bad_inode(inode);



But I can't be convinced that above change is sufficient, for if I do

+		static u8 serial;
+               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
+                       inode->i_ino = (serial++) % 16;

instead of

+               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
+                       make_bad_inode(inode);

, the reproducer still hits BUG() for 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15
because hfs_write_inode() handles only 2, 3 and 4.

        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;
                }
        }

Unless this is because I'm modifying in-kernel memory than filesystem image,
we will have to remove BUG() line.

On 2025/07/24 3:43, Viacheslav Dubeyko wrote:
> This could be defined in Catalog File (maybe not). I didn't find anything
> related to this in HFS specification.

https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID
says "the CNID of zero is never used and serves as a nil value." That is,
I think we can reject inode->i_ino == 0 case.

But I'm not sure for other values up to 15, expect values noted as "introduced
with HFS Plus". We could filter values in bad_cnid_list bitmap, but filtering
undefined values might not be sufficient for preserving BUG() line.