Commit b2268045 authored by Jori Koolstra's avatar Jori Koolstra Committed by Viacheslav Dubeyko
Browse files

hfs: Replace BUG_ON with error handling for CNID count checks 



In a06ec283 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.

Singed-off-by: default avatarJori Koolstra <jkoolstra@xs4all.nl>
Reported-by: default avatar <syzbot+17cc9bb6d8d69b4139f0@syzkaller.appspotmail.com>
Closes: https://syzbot.org/bug?extid=17cc9bb6d8d69b4139f0


Signed-off-by: default avatarJori Koolstra <jkoolstra@xs4all.nl>
Reviewed-by: default avatarViacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: default avatarViacheslav Dubeyko <slava@dubeyko.com>
Link: https://lore.kernel.org/r/20251220191006.2465256-1-jkoolstra@xs4all.nl


Signed-off-by: default avatarViacheslav Dubeyko <slava@dubeyko.com>
parent 413466f3

fs/hfs/dir.c

+11 −4
Original line number Diff line number Diff line
@@ -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;

	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);

	if (IS_ERR(inode))

		return ERR_CAST(inode);



	res = hfs_cat_create(inode->i_ino, dir, &dentry->d_name, inode);

	if (res) {
@@ -254,11 +254,18 @@ static struct dentry *hfs_mkdir(struct mnt_idmap *idmap, struct inode *dir, 
 */

static int hfs_remove(struct inode *dir, struct dentry *dentry)

{

	struct super_block *sb = dir->i_sb;

	struct inode *inode = d_inode(dentry);

	int res;



	if (S_ISDIR(inode->i_mode) && inode->i_size != 2)

		return -ENOTEMPTY;



	if (unlikely(!is_hfs_cnid_counts_valid(sb))) {

	    pr_err("cannot remove file/folder\n");

	    return -ERANGE;

	}



	res = hfs_cat_delete(inode->i_ino, dir, &dentry->d_name);

	if (res)

		return res;

fs/hfs/hfs_fs.h

+1 −0
Original line number Diff line number Diff line
@@ -199,6 +199,7 @@ extern void hfs_delete_inode(struct inode *inode); 
extern const struct xattr_handler * const hfs_xattr_handlers[];



/* mdb.c */

extern bool is_hfs_cnid_counts_valid(struct super_block *sb);

extern int hfs_mdb_get(struct super_block *sb);

extern void hfs_mdb_commit(struct super_block *sb);

extern void hfs_mdb_close(struct super_block *sb);

fs/hfs/inode.c

+24 −6
Original line number Diff line number Diff line
@@ -187,16 +187,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;



	err = -ERANGE;



	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) {

		atomic64_dec(&HFS_SB(sb)->next_id);

		pr_err("cannot create new inode: next CNID exceeds limit\n");

		goto out_discard;

	}

	inode->i_ino = (u32)next_id;

	inode->i_mode = mode;

	inode->i_uid = current_fsuid();
@@ -210,7 +217,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) {

			atomic64_dec(&HFS_SB(sb)->folder_count);

			pr_err("cannot create new inode: folder count exceeds limit\n");

			goto out_discard;

		}

		if (dir->i_ino == HFS_ROOT_CNID)

			HFS_SB(sb)->root_dirs++;

		inode->i_op = &hfs_dir_inode_operations;
@@ -220,7 +231,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) {

			atomic64_dec(&HFS_SB(sb)->file_count);

			pr_err("cannot create new inode: file count exceeds limit\n");

			goto out_discard;

		}

		if (dir->i_ino == HFS_ROOT_CNID)

			HFS_SB(sb)->root_files++;

		inode->i_op = &hfs_file_inode_operations;
@@ -244,6 +259,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)
@@ -252,7 +272,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);

		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--;
@@ -261,7 +280,6 @@ void hfs_delete_inode(struct inode *inode) 
		return;

	}



	BUG_ON(atomic64_read(&HFS_SB(sb)->file_count) > U32_MAX);

	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--;

fs/hfs/mdb.c

+27 −4
Original line number Diff line number Diff line
@@ -64,6 +64,27 @@ static int hfs_get_last_session(struct super_block *sb, 
	return 0;

}



bool is_hfs_cnid_counts_valid(struct super_block *sb)

{

	struct hfs_sb_info *sbi = HFS_SB(sb);

	bool corrupted = false;



	if (unlikely(atomic64_read(&sbi->next_id) > U32_MAX)) {

		pr_warn("next CNID exceeds limit\n");

		corrupted = true;

	}

	if (unlikely(atomic64_read(&sbi->file_count) > U32_MAX)) {

		pr_warn("file count exceeds limit\n");

		corrupted = true;

	}

	if (unlikely(atomic64_read(&sbi->folder_count) > U32_MAX)) {

		pr_warn("folder count exceeds limit\n");

		corrupted = true;

	}



	return !corrupted;

}



/*

 * hfs_mdb_get()

 *
@@ -159,6 +180,11 @@ int hfs_mdb_get(struct super_block *sb) 
	atomic64_set(&HFS_SB(sb)->file_count, be32_to_cpu(mdb->drFilCnt));

	atomic64_set(&HFS_SB(sb)->folder_count, be32_to_cpu(mdb->drDirCnt));



	if (!is_hfs_cnid_counts_valid(sb)) {

		pr_warn("filesystem possibly corrupted, running fsck.hfs is recommended. Mounting read-only.\n");

		sb->s_flags |= SB_RDONLY;

	}



	/* TRY to get the alternate (backup) MDB. */

	sect = part_start + part_size - 2;

	bh = sb_bread512(sb, sect, mdb2);
@@ -212,7 +238,7 @@ int hfs_mdb_get(struct super_block *sb) 


	attrib = mdb->drAtrb;

	if (!(attrib & cpu_to_be16(HFS_SB_ATTRIB_UNMNT))) {

		pr_warn("filesystem was not cleanly unmounted, running fsck.hfs is recommended.  mounting read-only.\n");

		pr_warn("filesystem was not cleanly unmounted, running fsck.hfs is recommended.	Mounting read-only.\n");

		sb->s_flags |= SB_RDONLY;

	}

	if ((attrib & cpu_to_be16(HFS_SB_ATTRIB_SLOCK))) {
@@ -270,15 +296,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);

		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);

		mdb->drFilCnt =

			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)->file_count));

		BUG_ON(atomic64_read(&HFS_SB(sb)->folder_count) > U32_MAX);

		mdb->drDirCnt =

			cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)->folder_count));

fs/hfs/super.c

+3 −0
Original line number Diff line number Diff line
@@ -34,6 +34,7 @@ MODULE_LICENSE("GPL"); 


static int hfs_sync_fs(struct super_block *sb, int wait)

{

	is_hfs_cnid_counts_valid(sb);

	hfs_mdb_commit(sb);

	return 0;

}
@@ -65,6 +66,8 @@ static void flush_mdb(struct work_struct *work) 
	sbi->work_queued = 0;

	spin_unlock(&sbi->work_lock);



	is_hfs_cnid_counts_valid(sb);



	hfs_mdb_commit(sb);

}