| 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: <32a2196b93ccdac0623175180a26c690e97536f6.camel@ibm.com>
Date: Wed, 3 Dec 2025 23:19:33 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: "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>,
"mehdi.benhadjkhelifa@...il.com" <mehdi.benhadjkhelifa@...il.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 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. :)
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.
> >
> > >
> > > 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.
Powered by blists - more mailing lists