| 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: <df71a155cb7b9cad2cc2d8c4262dfcc075f18946.camel@gmail.com>
Date: Fri, 30 Aug 2024 19:22:47 +0800
From: Julian Sun <sunjunchao2870@...il.com>
To: Joseph Qi <joseph.qi@...ux.alibaba.com>, ocfs2-devel@...ts.linux.dev
Cc: lbec@...lplan.org, mark@...heh.com,
syzbot+05b9b39d8bdfe1a0861f@...kaller.appspotmail.com,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, Heming Zhao <heming.zhao@...e.com>
Subject: Re: [PATCH v2] ocfs2: fix null-ptr-deref when journal load failed.
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@...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..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@...il.com>
Powered by blists - more mailing lists