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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Closes: https://syzkaller.appspot.com/bug?extid=808f3f84407f08a93022
> > > Signed-off-by: Ghanshyam Agrawal <ghanshyam1898@xxxxxxxxx>
> > > ---
> > >    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