fs/jfs/jfs_dmap.c | 4 ++++ 1 file changed, 4 insertions(+)
Currently while searching for dmtree_t for sufficient free blocks there
is an array out of bounds while getting element in tp->dm_stree. Added
the required bound check.
Ps: After I added the check I am getting the following log
[ 22.661748][ T4425] ERROR: (device loop0): dbAllocAny: unable to allocate blocks
[ 22.661748][ T4425]
[ 22.665536][ T4425] ERROR: (device loop0): remounting filesystem as read-only
[ 22.667856][ T4425] jfs_mkdir: dtInsert returned -EIO
[ 22.669750][ T4425] ERROR: (device loop0): txAbort:
I was wondering if these checks are significant of not?
Signed-off-by: Manas Ghandat <ghandatmanas@gmail.com>
Reported-by: syzbot+aea1ad91e854d0a83e04@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=aea1ad91e854d0a83e04
---
fs/jfs/jfs_dmap.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c
index a14a0f18a4c4..5af17b2287be 100644
--- a/fs/jfs/jfs_dmap.c
+++ b/fs/jfs/jfs_dmap.c
@@ -2948,6 +2948,10 @@ static int dbFindLeaf(dmtree_t * tp, int l2nb, int *leafidx)
/* sufficient free space found. move to the next
* level (or quit if this is the last level).
*/
+
+ if (x + n > TREESIZE)
+ return -ENOSPC;
+
if (l2nb <= tp->dmt_stree[x + n])
break;
}
--
2.37.2On 8/29/23 11:52AM, Manas Ghandat wrote: > Currently while searching for dmtree_t for sufficient free blocks there > is an array out of bounds while getting element in tp->dm_stree. Added > the required bound check. > > Ps: After I added the check I am getting the following log > > [ 22.661748][ T4425] ERROR: (device loop0): dbAllocAny: unable to allocate blocks > [ 22.661748][ T4425] > [ 22.665536][ T4425] ERROR: (device loop0): remounting filesystem as read-only > [ 22.667856][ T4425] jfs_mkdir: dtInsert returned -EIO > [ 22.669750][ T4425] ERROR: (device loop0): txAbort: > > I was wondering if these checks are significant of not? This won't work. dbFindLeaf() can be called from dbFindCtl() with struct dmapctl whose stree index can be as high as CTLTREESIZE which is larger than TREESIZE. A check against CTLTREESIZE might be better than nothing at all but won't necessarily detect an overflow. Currently, dbFindLeaf doesn't have anything to tell it which tree it is working on. We could pass in the treesize as an argument to dbFindCtl() if we can't come up with something simpler. Shaggy > > Signed-off-by: Manas Ghandat <ghandatmanas@gmail.com> > Reported-by: syzbot+aea1ad91e854d0a83e04@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=aea1ad91e854d0a83e04 > --- > fs/jfs/jfs_dmap.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c > index a14a0f18a4c4..5af17b2287be 100644 > --- a/fs/jfs/jfs_dmap.c > +++ b/fs/jfs/jfs_dmap.c > @@ -2948,6 +2948,10 @@ static int dbFindLeaf(dmtree_t * tp, int l2nb, int *leafidx) > /* sufficient free space found. move to the next > * level (or quit if this is the last level). > */ > + > + if (x + n > TREESIZE) > + return -ENOSPC; > + > if (l2nb <= tp->dmt_stree[x + n]) > break; > }
I was wondering if we could implement a get_tree_size macro wherein we could find the tree size so that we can do the comparison. SInce the tp->dmt_stree is an array we can get its size and fix the out of bounds. Would this thing work? On 30/08/23 00:08, Dave Kleikamp wrote: > This won't work. dbFindLeaf() can be called from dbFindCtl() with > struct dmapctl whose stree index can be as high as CTLTREESIZE which > is larger than TREESIZE. A check against CTLTREESIZE might be better > than nothing at all but won't necessarily detect an overflow. > Currently, dbFindLeaf doesn't have anything to tell it which tree it > is working on. > > We could pass in the treesize as an argument to dbFindCtl() if we > can't come up with something simpler. > > Shaggy > >> >> Signed-off-by: Manas Ghandat <ghandatmanas@gmail.com> >> Reported-by: syzbot+aea1ad91e854d0a83e04@syzkaller.appspotmail.com >> Closes: https://syzkaller.appspot.com/bug?extid=aea1ad91e854d0a83e04 >> --- >> fs/jfs/jfs_dmap.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c >> index a14a0f18a4c4..5af17b2287be 100644 >> --- a/fs/jfs/jfs_dmap.c >> +++ b/fs/jfs/jfs_dmap.c >> @@ -2948,6 +2948,10 @@ static int dbFindLeaf(dmtree_t * tp, int l2nb, >> int *leafidx) >> /* sufficient free space found. move to the next >> * level (or quit if this is the last level). >> */ >> + >> + if (x + n > TREESIZE) >> + return -ENOSPC; >> + >> if (l2nb <= tp->dmt_stree[x + n]) >> break; >> }
On 8/31/23 10:19AM, Manas Ghandat wrote: > I was wondering if we could implement a get_tree_size macro wherein we > could find the tree size so that we can do the comparison. SInce the > tp->dmt_stree is an array we can get its size and fix the out of bounds. > Would this thing work? dmtree_t is a union of two nearly identical structures that both contain an stree. The only real difference in the structures is the size of the stree, so dbFindLeaf doesn't really know which is being used by the caller. > > On 30/08/23 00:08, Dave Kleikamp wrote: >> This won't work. dbFindLeaf() can be called from dbFindCtl() with >> struct dmapctl whose stree index can be as high as CTLTREESIZE which >> is larger than TREESIZE. A check against CTLTREESIZE might be better >> than nothing at all but won't necessarily detect an overflow. >> Currently, dbFindLeaf doesn't have anything to tell it which tree it >> is working on. >> >> We could pass in the treesize as an argument to dbFindCtl() if we >> can't come up with something simpler. >> >> Shaggy >> >>> >>> Signed-off-by: Manas Ghandat <ghandatmanas@gmail.com> >>> Reported-by: syzbot+aea1ad91e854d0a83e04@syzkaller.appspotmail.com >>> Closes: https://syzkaller.appspot.com/bug?extid=aea1ad91e854d0a83e04 >>> --- >>> fs/jfs/jfs_dmap.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c >>> index a14a0f18a4c4..5af17b2287be 100644 >>> --- a/fs/jfs/jfs_dmap.c >>> +++ b/fs/jfs/jfs_dmap.c >>> @@ -2948,6 +2948,10 @@ static int dbFindLeaf(dmtree_t * tp, int l2nb, >>> int *leafidx) >>> /* sufficient free space found. move to the next >>> * level (or quit if this is the last level). >>> */ >>> + >>> + if (x + n > TREESIZE) >>> + return -ENOSPC; >>> + >>> if (l2nb <= tp->dmt_stree[x + n]) >>> break; >>> }
Actually I was talking about the stree attribute of dmtree which is present in both dmaptree and dmapctl. Link : https://elixir.bootlin.com/linux/v6.5.1/source/fs/jfs/jfs_dmap.h#L168 Since it is an array we can add a check for the size of array like the code below. + if (x + n > (sizeof(tp->dmt_stree)/sizeof(s8))) + return -ENOSPC; On 01/09/23 22:38, Dave Kleikamp wrote: > On 8/31/23 10:19AM, Manas Ghandat wrote: >> I was wondering if we could implement a get_tree_size macro wherein >> we could find the tree size so that we can do the comparison. SInce >> the tp->dmt_stree is an array we can get its size and fix the out of >> bounds. Would this thing work? > > dmtree_t is a union of two nearly identical structures that both > contain an stree. The only real difference in the structures is the > size of the stree, so dbFindLeaf doesn't really know which is being > used by the caller. >
On 9/2/23 12:45PM, Manas Ghandat wrote: > Actually I was talking about the stree attribute of dmtree which is > present in both dmaptree and dmapctl. > > Link : > https://elixir.bootlin.com/linux/v6.5.1/source/fs/jfs/jfs_dmap.h#L168 > > Since it is an array we can add a check for the size of array like the > code below. > > > + if (x + n > (sizeof(tp->dmt_stree)/sizeof(s8))) > + return -ENOSPC; That will always evaluate to the same thing (the size of stree in dmapctl). It would be valid, since that is the larger number, but it wouldn't catch all overflows of dmaptree's stree. > > On 01/09/23 22:38, Dave Kleikamp wrote: >> On 8/31/23 10:19AM, Manas Ghandat wrote: >>> I was wondering if we could implement a get_tree_size macro wherein >>> we could find the tree size so that we can do the comparison. SInce >>> the tp->dmt_stree is an array we can get its size and fix the out of >>> bounds. Would this thing work? >> >> dmtree_t is a union of two nearly identical structures that both >> contain an stree. The only real difference in the structures is the >> size of the stree, so dbFindLeaf doesn't really know which is being >> used by the caller. >>
© 2016 - 2025 Red Hat, Inc.