linux-ext4 - Re: [PATCH 2/7] ext4: avoid potential buffer_head leak in __ext4_new

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0351b3b1-e84a-9e41-a492-743f5bbea910@huawei.com>
Date: Wed, 14 Aug 2024 16:48:31 +0800
From: Zhang Yi <yi.zhang@...wei.com>
To: Kemeng Shi <shikemeng@...weicloud.com>
CC: <linux-ext4@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<tytso@....edu>, <adilger.kernel@...ger.ca>
Subject: Re: [PATCH 2/7] ext4: avoid potential buffer_head leak in
 __ext4_new_inode

On 2024/8/13 20:07, Kemeng Shi wrote:
> If a group is marked EXT4_GROUP_INFO_IBITMAP_CORRUPT after it's inode
> bitmap buffer_head was successfully verified, then __ext4_new_inode
> will get a valid inode_bitmap_bh of a corrupted group from
> ext4_read_inode_bitmap in which case inode_bitmap_bh misses a release.
> Hnadle "IS_ERR(inode_bitmap_bh)" and group corruption separately like
> how ext4_free_inode does to avoid buffer_head leak.
> 
> Signed-off-by: Kemeng Shi <shikemeng@...weicloud.com>
> ---
>  fs/ext4/ialloc.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index ad7f13976dc6..f24a238b6b09 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -1054,9 +1054,13 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
>  		brelse(inode_bitmap_bh);
>  		inode_bitmap_bh = ext4_read_inode_bitmap(sb, group);
>  		/* Skip groups with suspicious inode tables */
> -		if (((!(sbi->s_mount_state & EXT4_FC_REPLAY))
> -		     && EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) ||
> -		    IS_ERR(inode_bitmap_bh)) {
> +		if (IS_ERR(inode_bitmap_bh)) {
> +			inode_bitmap_bh = NULL;
> +			goto next_group;
> +		}
> +		if (!(sbi->s_mount_state & EXT4_FC_REPLAY) &&
> +		    EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) {
> +			brelse(inode_bitmap_bh);
>  			inode_bitmap_bh = NULL;

Wouldn't the inode_bitmap_bh be brelsed in the next loop or the end of this
function? why not just goto next_group?

Thanks,
Yi.

>  			goto next_group;
>  		}
>