| 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
| ||
|
Message-ID: <e63add409dc9fa30c3f1461c7a32e73acdd3cd0e.camel@gmail.com>
Date: Tue, 31 Dec 2024 16:14:32 +0800
From: Julian Sun <sunjunchao2870@...il.com>
To: Joseph Qi <joseph.qi@...ux.alibaba.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 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@...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?
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@...il.com>
Powered by blists - more mailing lists