| 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: <4b620e91b43f86dceed88ed2f73b1ff1e72bff6c.camel@ibm.com>
Date: Mon, 1 Dec 2025 23:04:15 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: "brauner@...nel.org" <brauner@...nel.org>,
"glaubitz@...sik.fu-berlin.de"
<glaubitz@...sik.fu-berlin.de>,
"frank.li@...o.com" <frank.li@...o.com>,
"slava@...eyko.com" <slava@...eyko.com>, "jack@...e.cz" <jack@...e.cz>,
"mehdi.benhadjkhelifa@...il.com" <mehdi.benhadjkhelifa@...il.com>,
"sandeen@...hat.com" <sandeen@...hat.com>
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>,
"syzbot+ad45f827c88778ff7df6@...kaller.appspotmail.com"
<syzbot+ad45f827c88778ff7df6@...kaller.appspotmail.com>,
"stable@...r.kernel.org" <stable@...r.kernel.org>,
"skhan@...uxfoundation.org" <skhan@...uxfoundation.org>
Subject: Re: [PATCH v3 1/2] hfs: ensure sb->s_fs_info is always cleaned up
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?
Thanks,
Slava.
>
> 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,
> };
Powered by blists - more mailing lists