ocfs2: Add a sanity check for corrupted file system.

[PATCH] ocfs2: Add a sanity check for corrupted file system.

Posted by sunjunchao 1 year ago

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2313dda4dc4885c93578
Signed-off-by: sunjunchao <sunjunchao@zspace.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) >
+	    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));
+	}
+
 	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);
-- 
2.39.5

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

Posted by Joseph Qi 11 months, 3 weeks ago


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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=2313dda4dc4885c93578
> Signed-off-by: sunjunchao <sunjunchao@zspace.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);

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

Posted by Julian Sun 11 months, 3 weeks ago

On Tue, 2024-12-31 at 15:16 +0800, Joseph Qi wrote:
> 
> 
> 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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=2313dda4dc4885c93578
> > Signed-off-by: sunjunchao <sunjunchao@zspace.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?

Exactly, actually it was adapted from ocfs2_tools.

> 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()?

Sure, this makes it clearer and more straightforward, will fix it in next version.

> 
> > +           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.

Yes, this is exactly how it should be.
Thanks for your review and comments.

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

Thanks.
-- 
Julian Sun <sunjunchao2870@gmail.com>