Re: [PATCH] jfs: fix array-index-out-of-bounds

[Dave Kleikamp <dave.kleikamp@oracle.com>]

On 9/23/24 11:52PM, Ghanshyam Agrawal wrote:
> On Tue, Sep 24, 2024 at 2:15 AM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> Le 23/09/2024 à 05:35, Ghanshyam Agrawal a écrit :
>>> On Sun, Sep 22, 2024 at 8:35 PM Christophe JAILLET
>>> <christophe.jaillet@wanadoo.fr> wrote:
>>>>
>>>> Le 22/09/2024 à 13:00, Ghanshyam Agrawal a écrit :
>>>>> In some cases, dn_numag may be greater than MAXAG which may
>>>>> result in an array-index-out-of-bounds in dbNextAG. Added
>>>>> a check to return an error code before we crash.
>>>>>
>>>>> Reported-by: syzbot+808f3f84407f08a93022@syzkaller.appspotmail.com
>>>>> Closes: https://syzkaller.appspot.com/bug?extid=808f3f84407f08a93022
>>>>> Signed-off-by: Ghanshyam Agrawal <ghanshyam1898@gmail.com>
>>>>> ---
>>>>>     fs/jfs/jfs_imap.c | 3 +++
>>>>>     1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/fs/jfs/jfs_imap.c b/fs/jfs/jfs_imap.c
>>>>> index 2ec35889ad24..5088da13e8f1 100644
>>>>> --- a/fs/jfs/jfs_imap.c
>>>>> +++ b/fs/jfs/jfs_imap.c
>>>>> @@ -1360,6 +1360,9 @@ int diAlloc(struct inode *pip, bool dir, struct inode *ip)
>>>>>         if (agno < 0 || agno > dn_numag)
>>>>>                 return -EIO;
>>>>>
>>>>> +     if (unlikely(dn_numag > MAXAG))
>>>>
>>>> Hi,
>>>>
>>>> looking at other places with checks with MAXAG, I wonder if it should be >=?
>>>>
>>>> CJ
>>>>
>>>>> +             return -EIO;
>>>>> +
>>>>>         if (atomic_read(&JFS_SBI(pip->i_sb)->bmap->db_active[agno])) {
>>>>>                 /*
>>>>>                  * There is an open file actively growing.  We want to
>>>>
>>>
>>> Hello Christophe,
>>>
>>> Thanks for reviewing my code. I believe the greater than symbol I have
>>> set is correct in this case.
>>
>> I think it's not.
>>
>> If you have "if (unlikely(dn_numag > MAXAG))", then
>>
>>          - dn_numag can be = MAXAG
>>          - [2] - so, agno can be = MAXAG as well
>>          - [3] - and, accessing memory past the end of the array will happen,
>> because db_active is atomic_t db_active[MAXAG];
>>          - BUG
>>
>> Or I miss something obvious?
>>
>>> Can you please check it thoroughly and letme know wny it should be >= ?
>>
>> Well, usually things don't work that way.
>>
>> YOU propose to fix something, which is nice. So YOU should explain why
>> it is correct.
>>
>> If I'm correct, the way to see that your fix is incomplete is just in
>> the 3 or 4 lines just above and below your change.
>>
>> You've been told what could be wrong, you could have checked yourself.
>> Or explained the reasoning that makes you think it is correct.
>>
>>
>>
>> Sorry if my answer looks rude, it is not my intend. I just read your
>> answer as "can you do my home work for me", which is certainly not you
>> intend either.
>>
>> So, no hard felling, but a bit disappointed by the lack of curiosity.
>>
>> CJ
>>
>>>
>>> Thanks & Regards,
>>> Ghanshyam Agrawal
>>>
>>
>>
>> [1]: https://elixir.bootlin.com/linux/v6.11/source/fs/jfs/jfs_imap.c#L1363
>>
>> [2]: https://elixir.bootlin.com/linux/v6.11/source/fs/jfs/jfs_imap.c#L1363
>>
>> [3]: https://elixir.bootlin.com/linux/v6.11/source/fs/jfs/jfs_imap.c#L1366
>>
> 
> Hello Christophe,
> 
> Thanks for your detailed answer and comments. I had done my research
> and couldn't find the reason to change the operator and then requested
> for your clarification. I should have been able to do that. I will work on that.
> 
> Thank you for taking time to explain me your thoughts. You are right.
> The operator should indeed be >=.
> 
> I also just found out that someone recently fixed this bug and their patch
> has gotten accepted. I wonder how I could have found that out before
> working on my patch. Since they neither sent the patch to syzkaller for
> testing nor did they include the fixes tag with a syzkaller link, I couldn't
> find it out. Maybe they found this bug from some other channel and
> not syzkaller. I will find a way to address this as well.

Just catching up on this thread.

Thank you for taking the time and effort to fix this and thanks 
Christophe for catching the >= thing.

The prior patch that fixed this was posted to the jfs-discussion group, 
which is pretty low-volume. It could be somewhere you can check if you 
are looking at other problems in jfs in the future. Sorry you had to 
duplicate that work.

Thanks again,
Shaggy

> 
> Thanks again for taking time to review my patch and discuss it
> with me.
> 
> Best Regards,
> Ghanshyam Agrawal