linux-kernel - Re: [PATCH] ocfs2: Add a sanity check for corrupted file system.

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: <df33d96d-e8fb-4de9-99cd-f21e0eee8195@linux.alibaba.com>
Date: Tue, 31 Dec 2024 15:16:55 +0800
From: Joseph Qi <joseph.qi@...ux.alibaba.com>
To: sunjunchao <sunjunchao2870@...il.com>, ocfs2-devel@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Cc: mark@...heh.com, jlbec@...lplan.org, sunjunchao <sunjunchao@...ace.cn>,
 syzbot+2313dda4dc4885c93578@...kaller.appspotmail.com
Subject: Re: [PATCH] ocfs2: Add a sanity check for corrupted file system.



On 2024/12/10 21:08, sunjunchao wrote:
> Hi,
> 
> Recently syzbot reported a use-after-free issue[1].
> 
> The root cause of the problem is that the journal
> inode recorded in this file system image is corrupted.
> The value of "di->id2.i_list.l_next_free_rec" is 8193,
> which is greater than the value of "di->id2.i_list.l_count" (19).
> 
> To solve this problem, an additional check should be added
> during the validity check. If the check fails, an error will
> be returned and the file system will be set to read-only.
> Also correct the l_next_free_rec value if online check is triggered,
> same as what fsck.ocfs2 does.
> 
> [1]: https://lore.kernel.org/all/67577778.050a0220.a30f1.01bc.GAE@google.com/T/
> 
> Reported-and-tested-by: syzbot+2313dda4dc4885c93578@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=2313dda4dc4885c93578
> Signed-off-by: sunjunchao <sunjunchao@...ace.cn>
> ---
>  fs/ocfs2/inode.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index 2cc5c99fe941..d3df54467d73 100644
> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -1358,6 +1358,21 @@ void ocfs2_refresh_inode(struct inode *inode,
>  	spin_unlock(&OCFS2_I(inode)->ip_lock);
>  }
>  
> +static int has_extents(struct ocfs2_dinode *di)
> +{
> +	/* inodes flagged with other stuff in id2 */
> +	if (di->i_flags & (OCFS2_SUPER_BLOCK_FL | OCFS2_LOCAL_ALLOC_FL |
> +			   OCFS2_CHAIN_FL | OCFS2_DEALLOC_FL))
> +		return 0;
> +	/* i_flags doesn't indicate when id2 is a fast symlink */
> +	if (S_ISLNK(di->i_mode) && di->i_size && di->i_clusters == 0)
> +		return 0;
> +	if (di->i_dyn_features & OCFS2_INLINE_DATA_FL)
> +		return 0;
> +
> +	return 1;
> +}
> +
>  int ocfs2_validate_inode_block(struct super_block *sb,
>  			       struct buffer_head *bh)
>  {
> @@ -1386,6 +1401,15 @@ int ocfs2_validate_inode_block(struct super_block *sb,
>  
>  	rc = -EINVAL;
>  
> +	if (has_extents(di) && le16_to_cpu(di->id2.i_list.l_next_free_rec) >

Seems has_extent() is used to identify type of extent list?
IMO, ocfs2_validate_inode_block() is a common validation function, so I
don't it is proper to check extent list here.
So how about add the check into ocfs2_get_clusters_nocache()?

> +	    le16_to_cpu(di->id2.i_list.l_count)) {
> +		rc = ocfs2_error(sb, "corrupted dinode #%llu: next_free_rec is %u, count is %u\n",
> +				 (unsigned long long)bh->b_blocknr,
> +				 le16_to_cpu(di->id2.i_list.l_next_free_rec),
> +				 le16_to_cpu(di->id2.i_list.l_count));
> +		goto bail;
> +	}
> +
>  	if (!OCFS2_IS_VALID_DINODE(di)) {
>  		rc = ocfs2_error(sb, "Invalid dinode #%llu: signature = %.*s\n",
>  				 (unsigned long long)bh->b_blocknr, 7,
> @@ -1483,6 +1507,16 @@ static int ocfs2_filecheck_validate_inode_block(struct super_block *sb,
>  		rc = -OCFS2_FILECHECK_ERR_GENERATION;
>  	}
>  
> +	if (has_extents(di) && le16_to_cpu(di->id2.i_list.l_next_free_rec) >
> +	    le16_to_cpu(di->id2.i_list.l_count)) {
> +		mlog(ML_ERROR,
> +		     "Filecheck: invalid dinode #%llu: l_next_free_rec is %u, l_count is %u\n",
> +		     (unsigned long long)bh->b_blocknr,
> +		     le16_to_cpu(di->id2.i_list.l_next_free_rec),
> +		     le16_to_cpu(di->id2.i_list.l_count));
> +		rc = -OCFS2_FILECHECK_ERR_FAILED;
> +	}
> +
>  bail:
>  	return rc;
>  }
> @@ -1547,6 +1581,16 @@ static int ocfs2_filecheck_repair_inode_block(struct super_block *sb,
>  		     le32_to_cpu(di->i_fs_generation));
>  	}
>  
> +	if (has_extents(di) && le16_to_cpu(di->id2.i_list.l_next_free_rec) >
> +	    le16_to_cpu(di->id2.i_list.l_count)) {
> +		di->id2.i_list.l_next_free_rec = di->id2.i_list.l_count;
> +		changed = 1;
> +		mlog(ML_ERROR,
> +		     "Filecheck: reset dinode #%llu: l_next_free_rec to %u\n",
> +		     (unsigned long long)bh->b_blocknr,
> +		     le16_to_cpu(di->id2.i_list.l_next_free_rec));
> +	}
> +

For file check, I'd like to post it as a separate patch.

Thanks,
Joseph

>  	if (changed || ocfs2_validate_meta_ecc(sb, bh->b_data, &di->i_check)) {
>  		ocfs2_compute_meta_ecc(sb, bh->b_data, &di->i_check);
>  		mark_buffer_dirty(bh);