| 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: <3237e199-2375-4064-9a28-134836b00606@gmail.com>
Date: Thu, 4 Dec 2025 13:19:36 +0100
From: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@...il.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>,
"glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
"frank.li@...o.com" <frank.li@...o.com>,
"brauner@...nel.org" <brauner@...nel.org>,
"slava@...eyko.com" <slava@...eyko.com>,
"sandeen@...hat.com" <sandeen@...hat.com>, "jack@...e.cz" <jack@...e.cz>
Cc: "khalid@...nel.org" <khalid@...nel.org>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
"david.hunter.linux@...il.com" <david.hunter.linux@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-kernel-mentees@...ts.linuxfoundation.org"
<linux-kernel-mentees@...ts.linuxfoundation.org>,
"skhan@...uxfoundation.org" <skhan@...uxfoundation.org>,
"stable@...r.kernel.org" <stable@...r.kernel.org>,
"syzbot+ad45f827c88778ff7df6@...kaller.appspotmail.com"
<syzbot+ad45f827c88778ff7df6@...kaller.appspotmail.com>
Subject: Re: [PATCH v3 1/2] hfs: ensure sb->s_fs_info is always cleaned up
On 12/4/25 12:19 AM, Viacheslav Dubeyko wrote:
> On Tue, 2025-12-02 at 11:16 +0100, Mehdi Ben Hadj Khelifa wrote:
>> On 12/2/25 12:04 AM, Viacheslav Dubeyko wrote:
>>> On Mon, 2025-12-01 at 23:23 +0100, Mehdi Ben Hadj Khelifa wrote:
>>>> When hfs was converted to the new mount api a bug was introduced by
>>>> changing the allocation pattern of sb->s_fs_info. If setup_bdev_super()
>>>> fails after a new superblock has been allocated by sget_fc(), but before
>>>> hfs_fill_super() takes ownership of the filesystem-specific s_fs_info
>>>> data it was leaked.
>>>>
>>>> Fix this by freeing sb->s_fs_info in hfs_kill_super().
>>>>
>>>> Cc: stable@...r.kernel.org
>>>> Fixes: ffcd06b6d13b ("hfs: convert hfs to use the new mount api")
>>>> Reported-by: syzbot+ad45f827c88778ff7df6@...kaller.appspotmail.com
>>>> Closes: https://syzkaller.appspot.com/bug?extid=ad45f827c88778ff7df6
>>>> Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@....com>
>>>> Signed-off-by: Christian Brauner <brauner@...nel.org>
>>>> Signed-off-by: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@...il.com>
>>>> ---
>>>> fs/hfs/mdb.c | 35 ++++++++++++++---------------------
>>>> fs/hfs/super.c | 10 +++++++++-
>>>> 2 files changed, 23 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
>>>> index 53f3fae60217..f28cd24dee84 100644
>>>> --- a/fs/hfs/mdb.c
>>>> +++ b/fs/hfs/mdb.c
>>>> @@ -92,7 +92,7 @@ int hfs_mdb_get(struct super_block *sb)
>>>> /* See if this is an HFS filesystem */
>>>> bh = sb_bread512(sb, part_start + HFS_MDB_BLK, mdb);
>>>> if (!bh)
>>>> - goto out;
>>>> + return -EIO;
>>>
>>> Frankly speaking, I don't see the point to rework the hfs_mdb_get() method so
>>> intensively. We had pretty good pattern before:
>>>
>>> int hfs_mdb_get(struct super_block *sb) {
>>> if (something_happens)
>>> goto out;
>>>
>>> if (something_happens_and_we_need_free_buffer)
>>> goto out_bh;
>>>
>>> return 0;
>>>
>>> out_bh:
>>> brelse(bh);
>>> out:
>>> return -EIO;
>>> }
>>>
>>> The point here that we have error management logic in one place. Now you have
>>> spread this logic through the whole function. It makes function more difficult
>>> to manage and we can introduce new bugs. Could you please localize your change
>>> without reworking this pattern of error situation management? Also, it will make
>>> the patch more compact. Could you please rework the patch?
>>>
>> This change in particular is made by christian. As he mentionned in one
>> of his emails to my patches[1], his logic was that hfs_mdb_put() should
>> only be called in fill_super() which cleans everything up and that the
>> cleanup labels don't make sense here which is why he spread the logic of
>> cleanup across the function. Maybe he can give us more input on this
>> since it wasn't my code.
>>
>> [1]:https://lore.kernel.org/all/20251119-delfin-bioladen-6bf291941d4f@brauner/
>>>
>
> I am not against of not calling the hfs_mdb_put() in hfs_mdb_get(). But if I am
> trying to rework some method significantly, guys are not happy at all about it.
> :) I am slightly worried about such significant rework of hfs_mdb_get() because
> we potentially could introduce some new bugs. And I definitely will have the
> conflict with another patch under review. :)
>
Totally understandable. If I was to make that change I would probably
seperate it from the fix (except the part where we delete freeing the
s_fs_info struct). But I guess christian wanted to do the whole
refactoring since it was related and it made more sense as he explained it.
> I've spent some more time for reviewing the patch again. And I think I can
> accept it as it is. Currently, I don't see any serious issue in hfs_mdb_get().
> It is simply my code style preferences. :) But people can see it in different
> ways.
>
Thanks for you time and effort!
>>>
>>>>
>>>> if (mdb->drSigWord == cpu_to_be16(HFS_SUPER_MAGIC))
>>>> break;
>>>> @@ -102,13 +102,14 @@ int hfs_mdb_get(struct super_block *sb)
>>>> * (should do this only for cdrom/loop though)
>>>> */
>>>> if (hfs_part_find(sb, &part_start, &part_size))
>>>> - goto out;
>>>> + return -EIO;
>>>> }
>>>>
>>>> HFS_SB(sb)->alloc_blksz = size = be32_to_cpu(mdb->drAlBlkSiz);
>>>> if (!size || (size & (HFS_SECTOR_SIZE - 1))) {
>>>> pr_err("bad allocation block size %d\n", size);
>>>> - goto out_bh;
>>>> + brelse(bh);
>>>> + return -EIO;
>>>> }
>>>>
>>>> size = min(HFS_SB(sb)->alloc_blksz, (u32)PAGE_SIZE);
>>>> @@ -125,14 +126,16 @@ int hfs_mdb_get(struct super_block *sb)
>>>> brelse(bh);
>>>> if (!sb_set_blocksize(sb, size)) {
>>>> pr_err("unable to set blocksize to %u\n", size);
>>>> - goto out;
>>>> + return -EIO;
>>>> }
>>>>
>>>> bh = sb_bread512(sb, part_start + HFS_MDB_BLK, mdb);
>>>> if (!bh)
>>>> - goto out;
>>>> - if (mdb->drSigWord != cpu_to_be16(HFS_SUPER_MAGIC))
>>>> - goto out_bh;
>>>> + return -EIO;
>>>> + if (mdb->drSigWord != cpu_to_be16(HFS_SUPER_MAGIC)) {
>>>> + brelse(bh);
>>>> + return -EIO;
>>>> + }
>>>>
>>>> HFS_SB(sb)->mdb_bh = bh;
>>>> HFS_SB(sb)->mdb = mdb;
>>>> @@ -174,7 +177,7 @@ int hfs_mdb_get(struct super_block *sb)
>>>>
>>>> HFS_SB(sb)->bitmap = kzalloc(8192, GFP_KERNEL);
>>>> if (!HFS_SB(sb)->bitmap)
>>>> - goto out;
>>>> + return -EIO;
>>>>
>>>> /* read in the bitmap */
>>>> block = be16_to_cpu(mdb->drVBMSt) + part_start;
>>>> @@ -185,7 +188,7 @@ int hfs_mdb_get(struct super_block *sb)
>>>> bh = sb_bread(sb, off >> sb->s_blocksize_bits);
>>>> if (!bh) {
>>>> pr_err("unable to read volume bitmap\n");
>>>> - goto out;
>>>> + return -EIO;
>>>> }
>>>> off2 = off & (sb->s_blocksize - 1);
>>>> len = min((int)sb->s_blocksize - off2, size);
>>>> @@ -199,12 +202,12 @@ int hfs_mdb_get(struct super_block *sb)
>>>> HFS_SB(sb)->ext_tree = hfs_btree_open(sb, HFS_EXT_CNID, hfs_ext_keycmp);
>>>> if (!HFS_SB(sb)->ext_tree) {
>>>> pr_err("unable to open extent tree\n");
>>>> - goto out;
>>>> + return -EIO;
>>>> }
>>>> HFS_SB(sb)->cat_tree = hfs_btree_open(sb, HFS_CAT_CNID, hfs_cat_keycmp);
>>>> if (!HFS_SB(sb)->cat_tree) {
>>>> pr_err("unable to open catalog tree\n");
>>>> - goto out;
>>>> + return -EIO;
>>>> }
>>>>
>>>> attrib = mdb->drAtrb;
>>>> @@ -229,12 +232,6 @@ int hfs_mdb_get(struct super_block *sb)
>>>> }
>>>>
>>>> return 0;
>>>> -
>>>> -out_bh:
>>>> - brelse(bh);
>>>> -out:
>>>> - hfs_mdb_put(sb);
>>>> - return -EIO;
>>>> }
>>>>
>>>> /*
>>>> @@ -359,8 +356,6 @@ void hfs_mdb_close(struct super_block *sb)
>>>> * Release the resources associated with the in-core MDB. */
>>>> void hfs_mdb_put(struct super_block *sb)
>>>> {
>>>> - if (!HFS_SB(sb))
>>>> - return;
>>>> /* free the B-trees */
>>>> hfs_btree_close(HFS_SB(sb)->ext_tree);
>>>> hfs_btree_close(HFS_SB(sb)->cat_tree);
>>>> @@ -373,6 +368,4 @@ void hfs_mdb_put(struct super_block *sb)
>>>> unload_nls(HFS_SB(sb)->nls_disk);
>>>>
>>>> kfree(HFS_SB(sb)->bitmap);
>>>> - kfree(HFS_SB(sb));
>>>> - sb->s_fs_info = NULL;
>>>> }
>>>> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
>>>> index 47f50fa555a4..df289cbdd4e8 100644
>>>> --- a/fs/hfs/super.c
>>>> +++ b/fs/hfs/super.c
>>>> @@ -431,10 +431,18 @@ static int hfs_init_fs_context(struct fs_context *fc)
>>>> return 0;
>>>> }
>>>>
>>>> +static void hfs_kill_super(struct super_block *sb)
>>>> +{
>>>> + struct hfs_sb_info *hsb = HFS_SB(sb);
>>>> +
>>>> + kill_block_super(sb);
>>>> + kfree(hsb);
>>>> +}
>>>> +
>>>> static struct file_system_type hfs_fs_type = {
>>>> .owner = THIS_MODULE,
>>>> .name = "hfs",
>>>> - .kill_sb = kill_block_super,
>>>> + .kill_sb = hfs_kill_super,
>>>> .fs_flags = FS_REQUIRES_DEV,
>>>> .init_fs_context = hfs_init_fs_context,
>>>> };
>
> Looks good. Thanks a lot for the fix.
>
> Reviewed-by: Viacheslav Dubeyko <slava@...eyko.com>
>
> Thanks,
> Slava.
Best Regards,
Mehdi Ben Hadj khelifa
Powered by blists - more mailing lists