* [PATCH] jfs: fix array-index-out-of-bounds @ 2024-09-22 11:00 Ghanshyam Agrawal 2024-09-22 15:05 ` Christophe JAILLET 0 siblings, 1 reply; 6+ messages in thread From: Ghanshyam Agrawal @ 2024-09-22 11:00 UTC (permalink / raw) To: shaggy, ghandatmanas, jlayton, eadavis, brauner, ghanshyam1898 Cc: jfs-discussion, linux-kernel, syzbot+808f3f84407f08a93022 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jfs: fix array-index-out-of-bounds 2024-09-22 11:00 [PATCH] jfs: fix array-index-out-of-bounds Ghanshyam Agrawal @ 2024-09-22 15:05 ` Christophe JAILLET 2024-09-23 3:35 ` Ghanshyam Agrawal 0 siblings, 1 reply; 6+ messages in thread From: Christophe JAILLET @ 2024-09-22 15:05 UTC (permalink / raw) To: Ghanshyam Agrawal, shaggy, ghandatmanas, jlayton, eadavis, brauner Cc: jfs-discussion, linux-kernel, syzbot+808f3f84407f08a93022 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jfs: fix array-index-out-of-bounds 2024-09-22 15:05 ` Christophe JAILLET @ 2024-09-23 3:35 ` Ghanshyam Agrawal 2024-09-23 20:45 ` Christophe JAILLET 0 siblings, 1 reply; 6+ messages in thread From: Ghanshyam Agrawal @ 2024-09-23 3:35 UTC (permalink / raw) To: Christophe JAILLET Cc: shaggy, ghandatmanas, jlayton, eadavis, brauner, jfs-discussion, linux-kernel, syzbot+808f3f84407f08a93022 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jfs: fix array-index-out-of-bounds 2024-09-23 3:35 ` Ghanshyam Agrawal @ 2024-09-23 20:45 ` Christophe JAILLET 2024-09-24 4:52 ` Ghanshyam Agrawal 0 siblings, 1 reply; 6+ messages in thread From: Christophe JAILLET @ 2024-09-23 20:45 UTC (permalink / raw) To: Ghanshyam Agrawal Cc: shaggy, ghandatmanas, jlayton, eadavis, brauner, jfs-discussion, linux-kernel, syzbot+808f3f84407f08a93022 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jfs: fix array-index-out-of-bounds 2024-09-23 20:45 ` Christophe JAILLET @ 2024-09-24 4:52 ` Ghanshyam Agrawal 2024-09-24 13:39 ` Dave Kleikamp 0 siblings, 1 reply; 6+ messages in thread From: Ghanshyam Agrawal @ 2024-09-24 4:52 UTC (permalink / raw) To: Christophe JAILLET Cc: shaggy, ghandatmanas, jlayton, eadavis, brauner, jfs-discussion, linux-kernel, syzbot+808f3f84407f08a93022 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] jfs: fix array-index-out-of-bounds 2024-09-24 4:52 ` Ghanshyam Agrawal @ 2024-09-24 13:39 ` Dave Kleikamp 0 siblings, 0 replies; 6+ messages in thread From: Dave Kleikamp @ 2024-09-24 13:39 UTC (permalink / raw) To: Ghanshyam Agrawal, Christophe JAILLET Cc: ghandatmanas, jlayton, eadavis, brauner, jfs-discussion, linux-kernel, syzbot+808f3f84407f08a93022 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-24 13:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-09-22 11:00 [PATCH] jfs: fix array-index-out-of-bounds Ghanshyam Agrawal 2024-09-22 15:05 ` Christophe JAILLET 2024-09-23 3:35 ` Ghanshyam Agrawal 2024-09-23 20:45 ` Christophe JAILLET 2024-09-24 4:52 ` Ghanshyam Agrawal 2024-09-24 13:39 ` Dave Kleikamp
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®