Message-ID: <37703318b16023c0b6d7e5e2acac0412e22747b4.camel@gmail.com>
Date: Fri, 23 Aug 2024 16:10:47 +0800
From: Julian Sun <sunjunchao2870@...il.com>
To: Joseph Qi <joseph.qi@...ux.alibaba.com>
Cc: ocfs2-devel@...ts.linux.dev, jlbec@...lplan.org, mark@...heh.com, 
	syzbot+05b9b39d8bdfe1a0861f@...kaller.appspotmail.com, 
	"linux-kernel@...r.kernel.org"
	 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ocfs2: fix null-ptr-deref when journal load failed.

On Fri, 2024-08-23 at 14:13 +0800, Joseph Qi wrote:
> 
> 
> On 8/23/24 10:22 AM, Julian Sun wrote:
> > On Fri, 2024-08-23 at 09:59 +0800, Joseph Qi wrote:
> > > 
> > > 
> > > On 8/20/24 11:19 PM, Julian Sun wrote:
> > > > Joseph Qi <joseph.qi@...ux.alibaba.com> 于2024年8月20日周二 21:03写道：
> > > > > 
> > > > > 
> > > > > 
> > > > > On 8/19/24 9:11 PM, Julian Sun wrote:
> > > > > > During the mounting process, if the jbd2_journal_load()
> > > > > > call fails, it will internally invoke journal_reset()
> > > > > > ->journal_fail_superblock(), which sets journal-
> > > > > > >j_sb_buffer
> > > > > 
> > > > > 
> > > > > > This description is not right.
> > > > > > journal_reset() fails because of too short journal, then
> > > > > > lead
> > > > > > to
> > > > > > jbd2_journal_load() fails with NULL j_sb_buffer.
> > > > yeah. That's exactly what I described.
> > > > > 
> > > > > > to NULL. Subsequently, ocfs2_journal_shutdown() calls
> > > > > > jbd2_journal_flush()->jbd2_cleanup_journal_tail()->
> > > > > > __jbd2_update_log_tail()->jbd2_journal_update_sb_log_tail()
> > > > > > ->lock_buffer(journal->j_sb_buffer), resulting in a
> > > > > > null-pointer dereference error.
> > > > > > 
> > > > > > To resolve this issue, a new state OCFS2_JOURNAL_INITED
> > > > > > has been introduced to replace the previous functionality
> > > > > > of OCFS2_JOURNAL_LOADED, the original OCFS2_JOURNAL_LOADED
> > > > > > is only set when ocfs2_journal_load() is successful.
> > > > > 
> > > > > 
> > > > > > Or set OCFS2_JOURNAL_LOADED only after JBD2_LOADED?
> > > > I don't think this is correct. We first call
> > > > ocfs2_journal_init(),
> > > > which allocates some resources, before calling
> > > > jbd2_journal_load().
> > > > If
> > > > ocfs2_journal_init() succeeds but jbd2_journal_load() fails,
> > > > this
> > > > solution may lead to a resource leak.
> > > > If there is anything important I'm missing, please let me know,
> > > > thanks.
> > > > 
> > > 
> > > Okay, seems except iput(inode) and kfree(journal), we may have to
> > > do
> > > the
> > > following cleanup:
> > > 1) ocfs2_inode_unlock(journal->j_inode);
> > > 2) brelse(journal->j_bh);
> > > 3) OCFS2_I(inode)->ip_open_count--
> > > 4) jbd2_journal_destroy()
> > > ...
> > > 
> > > So it seems that introducing a new state will be more clear.
> > > 
> > > > > > The jbd2_journal_flush() function is allowed to be called
> > > > > > only when this flag is set. The logic here is that if the
> > > > > > journal has even not been successfully loaded, there is
> > > > > > no need to flush the journal.
> > > > > > 
> > > > > > Link:
> > > > > > https://syzkaller.appspot.com/bug?extid=05b9b39d8bdfe1a0861f
> > > > > > Reported-by:
> > > > > > syzbot+05b9b39d8bdfe1a0861f@...kaller.appspotmail.com
> > > > > > Signed-off-by: Julian Sun <sunjunchao2870@...il.com>
> > > > > > ---
> > > > > >  fs/ocfs2/journal.c | 9 ++++++---
> > > > > >  fs/ocfs2/journal.h | 1 +
> > > > > >  2 files changed, 7 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> > > > > > index 530fba34f6d3..6f837296048f 100644
> > > > > > --- a/fs/ocfs2/journal.c
> > > > > > +++ b/fs/ocfs2/journal.c
> > > > > > @@ -968,7 +968,7 @@ int ocfs2_journal_init(struct
> > > > > > ocfs2_super
> > > > > > *osb, int *dirty)
> > > > > > 
> > > > > >       ocfs2_set_journal_params(osb);
> > > > > > 
> > > > > > -     journal->j_state = OCFS2_JOURNAL_LOADED;
> > > > > > +     journal->j_state = OCFS2_JOURNAL_INITED;
> > > > > > 
> > > > > >       status = 0;
> > > > > >  done:
> > > > > > @@ -1039,6 +1039,7 @@ void ocfs2_journal_shutdown(struct
> > > > > > ocfs2_super *osb)
> > > > > >       int status = 0;
> > > > > >       struct inode *inode = NULL;
> > > > > >       int num_running_trans = 0;
> > > > > > +     enum ocfs2_journal_state state;
> > > > > > 
> > > > > >       BUG_ON(!osb);
> > > > > > 
> > > > > > @@ -1047,8 +1048,9 @@ void ocfs2_journal_shutdown(struct
> > > > > > ocfs2_super *osb)
> > > > > >               goto done;
> > > > > > 
> > > > > >       inode = journal->j_inode;
> > > > > > +     state = journal->j_state;
> > > > > > 
> > > > > > -     if (journal->j_state != OCFS2_JOURNAL_LOADED)
> > > > > > +     if (state != OCFS2_JOURNAL_INITED)
> > > 
> > > This is not right.
> > > What if journal has already been loaded?
> > Hi, Joseph
> > 
> > Thanks for your review and comments.
> > 
> > I'm not sure if I fully understand what you mean. 
> > Because the functionality of OCFS2_JOURNAL_INITED is completely
> > equivalent to the original OCFS2_JOURNAL_LOADED, so do you mean
> > that
> > there might be an issue with the original OCFS2_JOURNAL_LOADED? If
> > so,
> > I will dig it into and try to fix.
> > But in any case, that should be a separate patch.
> > 
> > If there is any misunderstanding, please let me know, thanks.
> 
> Now you separate original OCFS2_JOURNAL_LOADED into
> OCFS2_JOURNAL_INITED
> and OCFS2_JOURNAL_LOADED. And ocfs2_journal_shutdown() will be called
> after OCFS2_JOURNAL_LOADED is set, e.g. normal umount, or error
> happens
> after ocfs2_check_volume(). You changes break the this logic.
> 
> BTW, cc linux-kernel as well.

I see. Thanks very much for the clarification.
I will send patch v2 to fix it.
> 
> Thanks,
> Joseph

Thanks,
-- 
Julian Sun <sunjunchao2870@...il.com>

Powered by blists - more mailing lists