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

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAG-BmocmdoLv5AR0p69J6JMOwrrQAn32ProJWzH=x5AyeZCfeA@mail.gmail.com>
Date: Tue, 24 Sep 2024 10:22:42 +0530
From: Ghanshyam Agrawal <ghanshyam1898@...il.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: shaggy@...nel.org, ghandatmanas@...il.com, jlayton@...nel.org, 
	eadavis@...com, brauner@...nel.org, jfs-discussion@...ts.sourceforge.net, 
	linux-kernel@...r.kernel.org, 
	syzbot+808f3f84407f08a93022@...kaller.appspotmail.com
Subject: Re: [PATCH] jfs: fix array-index-out-of-bounds

On Tue, Sep 24, 2024 at 2:15 AM Christophe JAILLET
<christophe.jaillet@...adoo.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@...adoo.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@...kaller.appspotmail.com
> >>> Closes: https://syzkaller.appspot.com/bug?extid=808f3f84407f08a93022
> >>> Signed-off-by: Ghanshyam Agrawal <ghanshyam1898@...il.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