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

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

[Joseph Qi]

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@linux.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@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..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.

Thanks,
Joseph

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

[Julian Sun]

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@linux.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@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..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@gmail.com>