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

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

[Ghanshyam Agrawal]

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))
+		return -EIO;
+
 	if (atomic_read(&JFS_SBI(pip->i_sb)->bmap->db_active[agno])) {
 		/*
 		 * There is an open file actively growing.  We want to
-- 
2.34.1

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

[Christophe JAILLET]

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

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

[Ghanshyam Agrawal]

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. Can you please check it thoroughly and let
me know wny it should be >= ?

Thanks & Regards,
Ghanshyam Agrawal

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

[Christophe JAILLET]

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

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

[Ghanshyam Agrawal]

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.

Thanks again for taking time to review my patch and discuss it
with me.

Best Regards,
Ghanshyam Agrawal

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

[Dave Kleikamp]

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