| 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
| ||
|
Message-ID: <8cb50ca3-8ccc-461e-866c-bb322ef8bfc6@I-love.SAKURA.ne.jp>
Date: Fri, 25 Jul 2025 07:05:32 +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()
On 2025/07/25 4:49, Viacheslav Dubeyko wrote:
> On Thu, 2025-07-24 at 15:55 +0900, Tetsuo Handa wrote:
>> 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 */
>
> We already have all declarations in hfs.h:
>
> /* Some special File ID numbers */
> #define HFS_POR_CNID 1 /* Parent Of the Root */
> #define HFS_ROOT_CNID 2 /* ROOT directory */
> #define HFS_EXT_CNID 3 /* EXTents B-tree */
> #define HFS_CAT_CNID 4 /* CATalog B-tree */
> #define HFS_BAD_CNID 5 /* BAD blocks file */
> #define HFS_ALLOC_CNID 6 /* ALLOCation file (HFS+) */
> #define HFS_START_CNID 7 /* STARTup file (HFS+) */
> #define HFS_ATTR_CNID 8 /* ATTRibutes file (HFS+) */
> #define HFS_EXCH_CNID 15 /* ExchangeFiles temp id */
> #define HFS_FIRSTUSER_CNID 16
These declarations does not define 14, and some flags are never used despite
being declared here.
>
> So, adding the link here doesn't make any sense.
>
>> + static const u16 bad_cnid_list = (1 << 0) | (1 << 6) | (1 << 7) | (1 << 8) |
>> + (1 << 9) | (1 << 10) | (1 << 11) | (1 << 12) | (1 << 13);
Some of values in this constant are not declared.
>
> I don't see any sense to introduce flags here. First of all, please, don't use
> hardcoded values but you should use declared constants from hfs.h (for example,
> HFS_EXT_CNID instead of 3). Secondly, you can simply compare the i_ino with
> constants, for example:
This will save a lot of computational power compared to switch().
>
> bool is_inode_id_invalid(u64 ino) {
> switch (inode->i_ino) {
> case HFS_EXT_CNID:
> ...
> return true;
>
> }
>
> return false;
> }
>
> Thirdly, you can introduce an inline function that can do such check. And it
> make sense to introduce constant for the case of zero value.
>
> Why have you missed HFS_EXT_CNID, HFS_CAT_CNID? These values cannot used in
> hfs_read_inode().
Is hfs_read_inode() never called for HFS_EXT_CNID and HFS_CAT_CNID ?
>
>>
>> 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);
>
> It looks pretty complicated. You can simply use one above-mentioned function
> with the check:
>
> if (is_inode_id_invalid(be32_to_cpu(rec->dir.DirID)))
> <goto to make bad inode>
>
> We can simply check the the inode ID in the beginning of the whole action:
>
> <Make the check here>
> inode->i_ino = be32_to_cpu(rec->file.FlNum);
> inode->i_mode = S_IRUGO | S_IXUGO;
> if (!(rec->file.Flags & HFS_FIL_LOCK))
> inode->i_mode |= S_IWUGO;
> inode->i_mode &= ~hsb->s_file_umask;
> inode->i_mode |= S_IFREG;
> inode_set_mtime_to_ts(inode,
> inode_set_atime_to_ts(inode,
> inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->file.MdDat))));
> inode->i_op = &hfs_file_inode_operations;
> inode->i_fop = &hfs_file_operations;
> inode->i_mapping->a_ops = &hfs_aops;
>
> It doesn't make any sense to construct inode if we will make in bad inode,
> finally. Don't waste computational power. :)
>
>> 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);
>
> We already have make_bad_inode(inode) as default action. So, simply jump there.
>
>> 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;
>
> I don't see the point in flags introduction. It makes logic very complicated.
The point of this change is to excecise inode->i_ino for all values between 0 and 15.
Some of values between 0 and 15 must be valid as inode->i_ino , doesn't these? Then,
>
>>
>> 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.
>>
>
> How can we go into hfs_write_inode() if we created the bad inode for invalid
> inode ID? How is it possible?
are all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 invalid value for hfs_read_inode() ?
If all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 are invalid value for hfs_read_inode(),
and 3 and 4 are also invalid value for hfs_read_inode(), hfs_read_inode() would accept only 2.
Something is crazily wrong.
Can we really filter some of values between 0 and 15 at hfs_read_inode() ?
Powered by blists - more mailing lists