| 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: <54e47f6ae96b4ed9bc30bd8c58487fa4d5cb6538.camel@dubeyko.com>
Date: Thu, 20 Nov 2025 11:53:25 -0800
From: Viacheslav Dubeyko <slava@...eyko.com>
To: Jori Koolstra <jkoolstra@...all.nl>, John Paul Adrian Glaubitz
<glaubitz@...sik.fu-berlin.de>, Yangtao Li <frank.li@...o.com>
Cc: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
syzbot+17cc9bb6d8d69b4139f0@...kaller.appspotmail.com
Subject: Re: [PATCH] Replace BUG_ON with error handling in hfs_new_inode()
On Mon, 2025-11-03 at 14:10 +0100, Jori Koolstra wrote:
> In a06ec283e125 next_id, folder_count, and file_count in the super
> block
> info were expanded to 64 bits, and BUG_ONs were added to detect
> overflow. This triggered an error reported by syzbot: if the MDB is
> corrupted, the BUG_ON is triggered. This patch replaces this
> mechanism
> with proper error handling and resolves the syzbot reported bug.
>
> hfs_new_inode() is the only place were the 32-bit limits need to be
> verified, since only in that function can these values be increased.
> Therefore, the checks in hfs_mdb_commit() and hfs_delete_inode() are
> removed.
>
I am terribly sorry, I've missed the patch. But, please, please,
please, add prefix 'hfs:' to the topic. This is the reason why I've
missed the patch. I expected to see something like this:
hfs: Replace BUG_ON with error handling in hfs_new_inode()
I need to process dozens emails every day. So, if I don't see proper
keyword in the topic, then I skip the emails.
> Singed-off-by: Jori Koolstra <jkoolstra@...all.nl>
> Reported-by: syzbot+17cc9bb6d8d69b4139f0@...kaller.appspotmail.com
> Closes: https://syzbot.org/bug?extid=17cc9bb6d8d69b4139f0
> Signed-off-by: Jori Koolstra <jkoolstra@...all.nl>
> ---
> fs/hfs/dir.c | 8 ++++----
> fs/hfs/inode.c | 30 ++++++++++++++++++++++++------
> fs/hfs/mdb.c | 3 ---
> 3 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
> index 86a6b317b474..ee1760305380 100644
> --- a/fs/hfs/dir.c
> +++ b/fs/hfs/dir.c
> @@ -196,8 +196,8 @@ static int hfs_create(struct mnt_idmap *idmap,
> struct inode *dir,
> int res;
>
> inode = hfs_new_inode(dir, &dentry->d_name, mode);
> - if (!inode)
> - return -ENOMEM;
I don't think that this removal is correct. The hfs_new_inode() can
return NULL [1].
> + if (IS_ERR(inode))
> + return PTR_ERR(inode);
>
> res = hfs_cat_create(inode->i_ino, dir, &dentry->d_name,
> inode);
> if (res) {
> @@ -226,8 +226,8 @@ static struct dentry *hfs_mkdir(struct mnt_idmap
> *idmap, struct inode *dir,
> int res;
>
> inode = hfs_new_inode(dir, &dentry->d_name, S_IFDIR | mode);
> - if (!inode)
> - return ERR_PTR(-ENOMEM);
Ditto. Please, take a look here [1].
> + if (IS_ERR(inode))
> + return ERR_CAST(inode);
>
> res = hfs_cat_create(inode->i_ino, dir, &dentry->d_name,
> inode);
> if (res) {
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 9cd449913dc8..beec6fe7e801 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -186,16 +186,23 @@ struct inode *hfs_new_inode(struct inode *dir,
> const struct qstr *name, umode_t
> s64 next_id;
> s64 file_count;
> s64 folder_count;
> + int err = -ENOMEM;
>
> if (!inode)
> - return NULL;
> + goto out_err;
OK. I see. You have modified the hfs_new_inode() with the goal to
return error code instead of NULL.
Frankly speaking, I am not sure that inode is NULL, then it means
always that we are out of memory (-ENOMEM).
> +
> + err = -ENOSPC;
Why do we use -ENOSPC here? If next_id > U32_MAX, then it doesn't mean
that volume is full. Probably, we have corrupted volume, then code
error should be completely different (maybe, -EIO).
>
> mutex_init(&HFS_I(inode)->extents_lock);
> INIT_LIST_HEAD(&HFS_I(inode)->open_dir_list);
> spin_lock_init(&HFS_I(inode)->open_dir_lock);
> hfs_cat_build_key(sb, (btree_key *)&HFS_I(inode)->cat_key,
> dir->i_ino, name);
> next_id = atomic64_inc_return(&HFS_SB(sb)->next_id);
> - BUG_ON(next_id > U32_MAX);
> + if (next_id > U32_MAX) {
> + pr_err("hfs: next file ID exceeds 32-bit limit —
> possible "
> + "superblock corruption");
The 'hfs:' prefix is not necessary here. It could be not only file but
folder ID too. So, maybe, it makes sense to mention "next CNID". The
whole comment needs to be on one line. Also, I believe it makes sense
to recommend run FSCK tool here.
> + goto out_discard;
> + }
> inode->i_ino = (u32)next_id;
> inode->i_mode = mode;
> inode->i_uid = current_fsuid();
> @@ -209,7 +216,11 @@ struct inode *hfs_new_inode(struct inode *dir,
> const struct qstr *name, umode_t
> if (S_ISDIR(mode)) {
> inode->i_size = 2;
> folder_count = atomic64_inc_return(&HFS_SB(sb)-
> >folder_count);
> - BUG_ON(folder_count > U32_MAX);
> + if (folder_count > U32_MAX) {
> + pr_err("hfs: folder count exceeds 32-bit
> limit — possible "
> + "superblock corruption");
Ditto.
> + goto out_discard;
> + }
> if (dir->i_ino == HFS_ROOT_CNID)
> HFS_SB(sb)->root_dirs++;
> inode->i_op = &hfs_dir_inode_operations;
> @@ -219,7 +230,11 @@ struct inode *hfs_new_inode(struct inode *dir,
> const struct qstr *name, umode_t
> } else if (S_ISREG(mode)) {
> HFS_I(inode)->clump_blocks = HFS_SB(sb)->clumpablks;
> file_count = atomic64_inc_return(&HFS_SB(sb)-
> >file_count);
> - BUG_ON(file_count > U32_MAX);
> + if (file_count > U32_MAX) {
> + pr_err("hfs: file count exceeds 32-bit limit
> — possible "
> + "superblock corruption");
Ditto.
> + goto out_discard;
> + }
> if (dir->i_ino == HFS_ROOT_CNID)
> HFS_SB(sb)->root_files++;
> inode->i_op = &hfs_file_inode_operations;
> @@ -243,6 +258,11 @@ struct inode *hfs_new_inode(struct inode *dir,
> const struct qstr *name, umode_t
> hfs_mark_mdb_dirty(sb);
>
> return inode;
> +
> + out_discard:
> + iput(inode);
> + out_err:
> + return ERR_PTR(err);
> }
>
> void hfs_delete_inode(struct inode *inode)
> @@ -251,7 +271,6 @@ void hfs_delete_inode(struct inode *inode)
>
> hfs_dbg("ino %lu\n", inode->i_ino);
> if (S_ISDIR(inode->i_mode)) {
> - BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) >
> U32_MAX);
I don't agree with complete removal of this check. Because, we could
have bugs in file system logic that can increase folder_count
wrongfully above U32_MAX limit.
So, I prefer to have this check in some way. Error code sounds good.
>
> atomic64_dec(&HFS_SB(sb)->folder_count);
> if (HFS_I(inode)->cat_key.ParID ==
> cpu_to_be32(HFS_ROOT_CNID))
> HFS_SB(sb)->root_dirs--;
> @@ -260,7 +279,6 @@ void hfs_delete_inode(struct inode *inode)
> return;
> }
>
> - BUG_ON(atomic64_read(&HFS_SB(sb)->file_count) > U32_MAX);
Ditto. I don't agree with complete removal of this check.
> atomic64_dec(&HFS_SB(sb)->file_count);
> if (HFS_I(inode)->cat_key.ParID ==
> cpu_to_be32(HFS_ROOT_CNID))
> HFS_SB(sb)->root_files--;
> diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
> index 53f3fae60217..1c3fb631cc8e 100644
> --- a/fs/hfs/mdb.c
> +++ b/fs/hfs/mdb.c
> @@ -273,15 +273,12 @@ void hfs_mdb_commit(struct super_block *sb)
> /* These parameters may have been modified, so write
> them back */
> mdb->drLsMod = hfs_mtime();
> mdb->drFreeBks = cpu_to_be16(HFS_SB(sb)-
> >free_ablocks);
> - BUG_ON(atomic64_read(&HFS_SB(sb)->next_id) >
> U32_MAX);
Ditto. I don't agree with complete removal of this check.
> mdb->drNxtCNID =
> cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)-
> >next_id));
> mdb->drNmFls = cpu_to_be16(HFS_SB(sb)->root_files);
> mdb->drNmRtDirs = cpu_to_be16(HFS_SB(sb)-
> >root_dirs);
> - BUG_ON(atomic64_read(&HFS_SB(sb)->file_count) >
> U32_MAX);
Ditto. I don't agree with complete removal of this check.
> mdb->drFilCnt =
> cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)-
> >file_count));
> - BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) >
> U32_MAX);
Ditto. I don't agree with complete removal of this check.
> mdb->drDirCnt =
> cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)-
> >folder_count));
>
Thanks,
Slava.
[1]
https://elixir.bootlin.com/linux/v6.18-rc6/source/fs/hfs/inode.c#L190
Powered by blists - more mailing lists