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

[Julian Sun <sunjunchao2870@gmail.com>]

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>