linux-kernel - Re: [PATCH v2] ext4: Make block validity check resistent to sb bh corruption

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] [day] [month] [year] [list]

Message-ID: <Z-Y9zO0GnMVLDdUJ@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
Date: Fri, 28 Mar 2025 11:42:28 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, "Theodore Ts'o" <tytso@....edu>,
        Baokun Li <libaokun1@...wei.com>,
        Ritesh Harjani <ritesh.list@...il.com>, Zhang Yi <yi.zhang@...wei.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] ext4: Make block validity check resistent to sb bh
 corruption

On Thu, Mar 27, 2025 at 07:25:30PM +0100, Jan Kara wrote:
> On Thu 27-03-25 23:18:09, Ojaswin Mujoo wrote:
> > Block validity checks need to be skipped in case they are called
> > for journal blocks since they are part of system's protected
> > zone.
> > 
> > Currently, this is done by checking inode->ino against
> > sbi->s_es->s_journal_inum, which is a direct read from the ext4 sb
> > buffer head. If someone modifies this underneath us then the
> > s_journal_inum field might get corrupted. To prevent against this,
> > change the check to directly compare the inode with journal->j_inode.
> > 
> > **Slight change in behavior**: During journal init path,
> > check_block_validity etc might be called for journal inode when
> > sbi->s_journal is not set yet. In this case we now proceed with
> > ext4_inode_block_valid() instead of returning early. Since systems zones
> > have not been set yet, it is okay to proceed so we can perform basic
> > checks on the blocks.
> > 
> > Suggested-by: Baokun Li <libaokun1@...wei.com>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
> 
> Looks good. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@...e.cz>
> 
> One style nit below:
> 
> > diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
> > index 87ee3a17bd29..e8c5525afc67 100644
> > --- a/fs/ext4/block_validity.c
> > +++ b/fs/ext4/block_validity.c
> > @@ -351,10 +351,9 @@ int ext4_check_blockref(const char *function, unsigned int line,
> >  {
> >  	__le32 *bref = p;
> >  	unsigned int blk;
> > +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
> >  
> > -	if (ext4_has_feature_journal(inode->i_sb) &&
> > -	    (inode->i_ino ==
> > -	     le32_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_journal_inum)))
> > +	if (journal && inode == journal->j_inode)
> >  		return 0;
> >  
> >  	while (bref < p+max) {
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 365d31004bd0..8b048be14008 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -384,10 +384,11 @@ static int __check_block_validity(struct inode *inode, const char *func,
> >  				unsigned int line,
> >  				struct ext4_map_blocks *map)
> >  {
> > -	if (ext4_has_feature_journal(inode->i_sb) &&
> > -	    (inode->i_ino ==
> > -	     le32_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_journal_inum)))
> > -		return 0;
> > +	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
> > +
> > +	if (journal && inode == journal->j_inode)
> > +			return 0;
> 
> Bogus indentation here.

Thanks for the review Jan and for catching this. My bad, i missed
running checkpatch on this and somehow this looked okay in vim.

I'll fix this and quickly send a v3.

Regards,
ojaswin
> 
> 								Honza
> -- 
> Jan Kara <jack@...e.com>
> SUSE Labs, CR