Re: [PATCH v2] ocfs2: fix null-ptr-deref when journal load failed.

[Julian Sun <sunjunchao2870@gmail.com>]

On Fri, 2024-08-30 at 18:58 +0800, Joseph Qi wrote:
> 
> 
> On 8/30/24 6:11 PM, Julian Sun wrote:
> > On Fri, 2024-08-30 at 17:40 +0800, Joseph Qi wrote:
> > > 
> > > 
> > > On 8/23/24 4:31 PM, Julian Sun wrote:
> > > > During the mounting process, if journal_reset() fails
> > > > because of too short journal, then lead to
> > > > jbd2_journal_load() fails with NULL j_sb_buffer.
> > > > 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.
> > > > 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@syzkaller.appspotmail.com
> > > > Signed-off-by: Julian Sun <sunjunchao2870@gmail.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..da0ffcc5de0a 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 && state !=
> > > > OCFS2_JOURNAL_LOADED)
> > > >                 goto done;
> > > >  
> > > >         /* need to inc inode use count - jbd2_journal_destroy
> > > > will
> > > > iput. */
> > > > @@ -1076,7 +1078,7 @@ void ocfs2_journal_shutdown(struct
> > > > ocfs2_super *osb)
> > > >  
> > > >         BUG_ON(atomic_read(&(osb->journal->j_num_trans)) != 0);
> > > >  
> > > > -       if (ocfs2_mount_local(osb)) {
> > > > +       if (ocfs2_mount_local(osb) && state ==
> > > > OCFS2_JOURNAL_LOADED) {
> > > 
> > > The only intent of the new introduced state is to identify if
> > > journal
> > > is
> > > truly loaded or not.
> > > So it seems that the simplest way to fix this is just check
> > > JBD2_LOADED
> > > here.
> > > 
> > > if (ocfs2_mount_local(osb) &&
> > >     (journal->j_journal->j_flags & JBD2_LOADED)) {
> > >         ...
> > > }
> > Hi, Joseph, thanks for your review and comments. 
> > 
> > Yeah! It's absolutely the simplest and cleanest way to fix this
> > issue.
> > Thanks for your suggestion.
> > > 
> > > BTW, could you please also replace 'osb->journal->j_num_trans' to
> > > 'journal->j_num_trans'?
> > Sure.
> > > 
> > > >                 jbd2_journal_lock_updates(journal->j_journal);
> > > >                 status = jbd2_journal_flush(journal->j_journal,
> > > > 0);
> > > >                 jbd2_journal_unlock_updates(journal-
> > > > >j_journal);
> > > > @@ -1174,6 +1176,7 @@ int ocfs2_journal_load(struct
> > > > ocfs2_journal
> > > > *journal, int local, int replayed)
> > > >                 }
> > > >         } else
> > > >                 osb->commit_task = NULL;
> > > > +       journal->j_state = OCFS2_JOURNAL_LOADED;
> > > 
> > > It seems that this has to be moved just after
> > > jbd2_journal_load().
> > > Anyway, I don't think we have to introduce a new state. See
> > > above.
> > > 
> > Agreed. And now OCFS2_JOURNAL_LOADED is set when
> > ocfs2_journal_init()
> > succeed, it may led to some misunderstanding: the journal was not
> > really loaded when OCFS2_JOURNAL_LOADED was set. I would like to
> > rename
> > it to OCFS2_JOURNAL_INITED in another patch to make it clearer.
> 
> Umm... You can treat it as ocfs2 journal state only, not jbd2.
> Also jbd2 doesn't have 'initialized' state either.
> So I don't think we have to do this change in the fix.
> 
> Joseph  
Got it. Thanks for your explanation.

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