Message-ID: <ae1eb591-ba89-4290-8c17-4cc7816ce666@linux.alibaba.com>
Date: Fri, 23 Aug 2024 14:13:06 +0800
From: Joseph Qi <joseph.qi@...ux.alibaba.com>
To: Julian Sun <sunjunchao2870@...il.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 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.

Thanks,
Joseph

Powered by blists - more mailing lists