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

Manas Ghandat posted 1 patch 2 years, 2 months ago
fs/jfs/jfs_dmap.c | 57 ++++++++++++++++++++++++++---------------------
1 file changed, 31 insertions(+), 26 deletions(-)
[PATCH] jfs: fix array-index-out-of-bounds in dbAdjTree
Posted by Manas Ghandat 2 years, 2 months ago
Currently there is a bound check missing in the dbAdjTree while
accessing the dmt_stree. To add the required check added the bool is_ctl
which is required to determine the size as suggest in the following
commit.
https://lore.kernel.org/linux-kernel-mentees/f9475918-2186-49b8-b801-6f0f9e75f4fa@oracle.com/

Reported-by: syzbot+39ba34a099ac2e9bd3cb@syzkaller.appspotmail.com
Fixes: https://syzkaller.appspot.com/bug?extid=39ba34a099ac2e9bd3cb
Signed-off-by: Manas Ghandat <ghandatmanas@gmail.com>
---
 fs/jfs/jfs_dmap.c | 57 ++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c
index 88afd108c2dd..57fba21994c3 100644
--- a/fs/jfs/jfs_dmap.c
+++ b/fs/jfs/jfs_dmap.c
@@ -63,10 +63,10 @@
  */
 static void dbAllocBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
 			int nblocks);
-static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval);
-static int dbBackSplit(dmtree_t * tp, int leafno);
-static int dbJoin(dmtree_t * tp, int leafno, int newval);
-static void dbAdjTree(dmtree_t * tp, int leafno, int newval);
+static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval, bool is_ctl);
+static int dbBackSplit(dmtree_t * tp, int leafno, bool is_ctl);
+static int dbJoin(dmtree_t * tp, int leafno, int newval, bool is_ctl);
+static void dbAdjTree(dmtree_t * tp, int leafno, int newval, bool is_ctl);
 static int dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc,
 		    int level);
 static int dbAllocAny(struct bmap * bmp, s64 nblocks, int l2nb, s64 * results);
@@ -2096,7 +2096,7 @@ static int dbFreeDmap(struct bmap * bmp, struct dmap * dp, s64 blkno,
 		 * system.
 		 */
 		if (dp->tree.stree[word] == NOFREE)
-			dbBackSplit((dmtree_t *) & dp->tree, word);
+			dbBackSplit((dmtree_t *) & dp->tree, word, false);
 
 		dbAllocBits(bmp, dp, blkno, nblocks);
 	}
@@ -2182,7 +2182,7 @@ static void dbAllocBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
 			 * the binary system of the leaves if need be.
 			 */
 			dbSplit(tp, word, BUDMIN,
-				dbMaxBud((u8 *) & dp->wmap[word]));
+				dbMaxBud((u8 *) & dp->wmap[word]),false);
 
 			word += 1;
 		} else {
@@ -2222,7 +2222,7 @@ static void dbAllocBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
 				 * system of the leaves to reflect the current
 				 * allocation (size).
 				 */
-				dbSplit(tp, word, size, NOFREE);
+				dbSplit(tp, word, size, NOFREE, false);
 
 				/* get the number of dmap words handled */
 				nw = BUDSIZE(size, BUDMIN);
@@ -2329,7 +2329,7 @@ static int dbFreeBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
 			/* update the leaf for this dmap word.
 			 */
 			rc = dbJoin(tp, word,
-				    dbMaxBud((u8 *) & dp->wmap[word]));
+				    dbMaxBud((u8 *) & dp->wmap[word]),false);
 			if (rc)
 				return rc;
 
@@ -2362,7 +2362,7 @@ static int dbFreeBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
 
 				/* update the leaf.
 				 */
-				rc = dbJoin(tp, word, size);
+				rc = dbJoin(tp, word, size, false);
 				if (rc)
 					return rc;
 
@@ -2514,16 +2514,16 @@ dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc, int level)
 		 * that it is at the front of a binary buddy system.
 		 */
 		if (oldval == NOFREE) {
-			rc = dbBackSplit((dmtree_t *) dcp, leafno);
+			rc = dbBackSplit((dmtree_t *) dcp, leafno, true);
 			if (rc) {
 				release_metapage(mp);
 				return rc;
 			}
 			oldval = dcp->stree[ti];
 		}
-		dbSplit((dmtree_t *) dcp, leafno, dcp->budmin, newval);
+		dbSplit((dmtree_t *) dcp, leafno, dcp->budmin, newval, true);
 	} else {
-		rc = dbJoin((dmtree_t *) dcp, leafno, newval);
+		rc = dbJoin((dmtree_t *) dcp, leafno, newval, true);
 		if (rc) {
 			release_metapage(mp);
 			return rc;
@@ -2554,7 +2554,7 @@ dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc, int level)
 				 */
 				if (alloc) {
 					dbJoin((dmtree_t *) dcp, leafno,
-					       oldval);
+					       oldval, true);
 				} else {
 					/* the dbJoin() above might have
 					 * caused a larger binary buddy system
@@ -2564,9 +2564,9 @@ dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc, int level)
 					 */
 					if (dcp->stree[ti] == NOFREE)
 						dbBackSplit((dmtree_t *)
-							    dcp, leafno);
+							    dcp, leafno, true);
 					dbSplit((dmtree_t *) dcp, leafno,
-						dcp->budmin, oldval);
+						dcp->budmin, oldval, true);
 				}
 
 				/* release the buffer and return the error.
@@ -2614,7 +2614,7 @@ dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc, int level)
  *
  * serialization: IREAD_LOCK(ipbmap) or IWRITE_LOCK(ipbmap) held on entry/exit;
  */
-static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval)
+static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval, bool is_ctl)
 {
 	int budsz;
 	int cursz;
@@ -2636,7 +2636,7 @@ static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval)
 		while (cursz >= splitsz) {
 			/* update the buddy's leaf with its new value.
 			 */
-			dbAdjTree(tp, leafno ^ budsz, cursz);
+			dbAdjTree(tp, leafno ^ budsz, cursz, is_ctl);
 
 			/* on to the next size and buddy.
 			 */
@@ -2648,7 +2648,7 @@ static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval)
 	/* adjust the dmap tree to reflect the specified leaf's new
 	 * value.
 	 */
-	dbAdjTree(tp, leafno, newval);
+	dbAdjTree(tp, leafno, newval, is_ctl);
 }
 
 
@@ -2679,7 +2679,7 @@ static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval)
  *
  * serialization: IREAD_LOCK(ipbmap) or IWRITE_LOCK(ipbmap) held on entry/exit;
  */
-static int dbBackSplit(dmtree_t * tp, int leafno)
+static int dbBackSplit(dmtree_t * tp, int leafno, bool is_ctl)
 {
 	int budsz, bud, w, bsz, size;
 	int cursz;
@@ -2730,7 +2730,7 @@ static int dbBackSplit(dmtree_t * tp, int leafno)
 				 * system in two.
 				 */
 				cursz = leaf[bud] - 1;
-				dbSplit(tp, bud, cursz, cursz);
+				dbSplit(tp, bud, cursz, cursz, is_ctl);
 				break;
 			}
 		}
@@ -2758,7 +2758,7 @@ static int dbBackSplit(dmtree_t * tp, int leafno)
  *
  * RETURN VALUES: none
  */
-static int dbJoin(dmtree_t * tp, int leafno, int newval)
+static int dbJoin(dmtree_t * tp, int leafno, int newval, bool is_ctl)
 {
 	int budsz, buddy;
 	s8 *leaf;
@@ -2813,12 +2813,12 @@ static int dbJoin(dmtree_t * tp, int leafno, int newval)
 			if (leafno < buddy) {
 				/* leafno is the left buddy.
 				 */
-				dbAdjTree(tp, buddy, NOFREE);
+				dbAdjTree(tp, buddy, NOFREE, is_ctl);
 			} else {
 				/* buddy is the left buddy and becomes
 				 * leafno.
 				 */
-				dbAdjTree(tp, leafno, NOFREE);
+				dbAdjTree(tp, leafno, NOFREE, is_ctl);
 				leafno = buddy;
 			}
 
@@ -2831,7 +2831,7 @@ static int dbJoin(dmtree_t * tp, int leafno, int newval)
 
 	/* update the leaf value.
 	 */
-	dbAdjTree(tp, leafno, newval);
+	dbAdjTree(tp, leafno, newval, is_ctl);
 
 	return 0;
 }
@@ -2852,15 +2852,20 @@ static int dbJoin(dmtree_t * tp, int leafno, int newval)
  *
  * RETURN VALUES: none
  */
-static void dbAdjTree(dmtree_t * tp, int leafno, int newval)
+static void dbAdjTree(dmtree_t * tp, int leafno, int newval, bool is_ctl)
 {
 	int lp, pp, k;
-	int max;
+	int max, size;
+
+	size = is_ctl ? CTLTREESIZE : TREESIZE;
 
 	/* pick up the index of the leaf for this leafno.
 	 */
 	lp = leafno + le32_to_cpu(tp->dmt_leafidx);
 
+	if (lp > size || lp < 0)
+		return;
+
 	/* is the current value the same as the old value ?  if so,
 	 * there is nothing to do.
 	 */
-- 
2.37.2
Re: [PATCH] jfs: fix array-index-out-of-bounds in dbAdjTree
Posted by Manas Ghandat 2 years, 2 months ago
Just a friendly remainder

On 17/10/23 17:33, Manas Ghandat wrote:
> Currently there is a bound check missing in the dbAdjTree while
> accessing the dmt_stree. To add the required check added the bool is_ctl
> which is required to determine the size as suggest in the following
> commit.
> https://lore.kernel.org/linux-kernel-mentees/f9475918-2186-49b8-b801-6f0f9e75f4fa@oracle.com/
>
> Reported-by: syzbot+39ba34a099ac2e9bd3cb@syzkaller.appspotmail.com
> Fixes: https://syzkaller.appspot.com/bug?extid=39ba34a099ac2e9bd3cb
> Signed-off-by: Manas Ghandat <ghandatmanas@gmail.com>
> ---
>   fs/jfs/jfs_dmap.c | 57 ++++++++++++++++++++++++++---------------------
>   1 file changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c
> index 88afd108c2dd..57fba21994c3 100644
> --- a/fs/jfs/jfs_dmap.c
> +++ b/fs/jfs/jfs_dmap.c
> @@ -63,10 +63,10 @@
>    */
>   static void dbAllocBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
>   			int nblocks);
> -static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval);
> -static int dbBackSplit(dmtree_t * tp, int leafno);
> -static int dbJoin(dmtree_t * tp, int leafno, int newval);
> -static void dbAdjTree(dmtree_t * tp, int leafno, int newval);
> +static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval, bool is_ctl);
> +static int dbBackSplit(dmtree_t * tp, int leafno, bool is_ctl);
> +static int dbJoin(dmtree_t * tp, int leafno, int newval, bool is_ctl);
> +static void dbAdjTree(dmtree_t * tp, int leafno, int newval, bool is_ctl);
>   static int dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc,
>   		    int level);
>   static int dbAllocAny(struct bmap * bmp, s64 nblocks, int l2nb, s64 * results);
> @@ -2096,7 +2096,7 @@ static int dbFreeDmap(struct bmap * bmp, struct dmap * dp, s64 blkno,
>   		 * system.
>   		 */
>   		if (dp->tree.stree[word] == NOFREE)
> -			dbBackSplit((dmtree_t *) & dp->tree, word);
> +			dbBackSplit((dmtree_t *) & dp->tree, word, false);
>   
>   		dbAllocBits(bmp, dp, blkno, nblocks);
>   	}
> @@ -2182,7 +2182,7 @@ static void dbAllocBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
>   			 * the binary system of the leaves if need be.
>   			 */
>   			dbSplit(tp, word, BUDMIN,
> -				dbMaxBud((u8 *) & dp->wmap[word]));
> +				dbMaxBud((u8 *) & dp->wmap[word]),false);
>   
>   			word += 1;
>   		} else {
> @@ -2222,7 +2222,7 @@ static void dbAllocBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
>   				 * system of the leaves to reflect the current
>   				 * allocation (size).
>   				 */
> -				dbSplit(tp, word, size, NOFREE);
> +				dbSplit(tp, word, size, NOFREE, false);
>   
>   				/* get the number of dmap words handled */
>   				nw = BUDSIZE(size, BUDMIN);
> @@ -2329,7 +2329,7 @@ static int dbFreeBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
>   			/* update the leaf for this dmap word.
>   			 */
>   			rc = dbJoin(tp, word,
> -				    dbMaxBud((u8 *) & dp->wmap[word]));
> +				    dbMaxBud((u8 *) & dp->wmap[word]),false);
>   			if (rc)
>   				return rc;
>   
> @@ -2362,7 +2362,7 @@ static int dbFreeBits(struct bmap * bmp, struct dmap * dp, s64 blkno,
>   
>   				/* update the leaf.
>   				 */
> -				rc = dbJoin(tp, word, size);
> +				rc = dbJoin(tp, word, size, false);
>   				if (rc)
>   					return rc;
>   
> @@ -2514,16 +2514,16 @@ dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc, int level)
>   		 * that it is at the front of a binary buddy system.
>   		 */
>   		if (oldval == NOFREE) {
> -			rc = dbBackSplit((dmtree_t *) dcp, leafno);
> +			rc = dbBackSplit((dmtree_t *) dcp, leafno, true);
>   			if (rc) {
>   				release_metapage(mp);
>   				return rc;
>   			}
>   			oldval = dcp->stree[ti];
>   		}
> -		dbSplit((dmtree_t *) dcp, leafno, dcp->budmin, newval);
> +		dbSplit((dmtree_t *) dcp, leafno, dcp->budmin, newval, true);
>   	} else {
> -		rc = dbJoin((dmtree_t *) dcp, leafno, newval);
> +		rc = dbJoin((dmtree_t *) dcp, leafno, newval, true);
>   		if (rc) {
>   			release_metapage(mp);
>   			return rc;
> @@ -2554,7 +2554,7 @@ dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc, int level)
>   				 */
>   				if (alloc) {
>   					dbJoin((dmtree_t *) dcp, leafno,
> -					       oldval);
> +					       oldval, true);
>   				} else {
>   					/* the dbJoin() above might have
>   					 * caused a larger binary buddy system
> @@ -2564,9 +2564,9 @@ dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc, int level)
>   					 */
>   					if (dcp->stree[ti] == NOFREE)
>   						dbBackSplit((dmtree_t *)
> -							    dcp, leafno);
> +							    dcp, leafno, true);
>   					dbSplit((dmtree_t *) dcp, leafno,
> -						dcp->budmin, oldval);
> +						dcp->budmin, oldval, true);
>   				}
>   
>   				/* release the buffer and return the error.
> @@ -2614,7 +2614,7 @@ dbAdjCtl(struct bmap * bmp, s64 blkno, int newval, int alloc, int level)
>    *
>    * serialization: IREAD_LOCK(ipbmap) or IWRITE_LOCK(ipbmap) held on entry/exit;
>    */
> -static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval)
> +static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval, bool is_ctl)
>   {
>   	int budsz;
>   	int cursz;
> @@ -2636,7 +2636,7 @@ static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval)
>   		while (cursz >= splitsz) {
>   			/* update the buddy's leaf with its new value.
>   			 */
> -			dbAdjTree(tp, leafno ^ budsz, cursz);
> +			dbAdjTree(tp, leafno ^ budsz, cursz, is_ctl);
>   
>   			/* on to the next size and buddy.
>   			 */
> @@ -2648,7 +2648,7 @@ static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval)
>   	/* adjust the dmap tree to reflect the specified leaf's new
>   	 * value.
>   	 */
> -	dbAdjTree(tp, leafno, newval);
> +	dbAdjTree(tp, leafno, newval, is_ctl);
>   }
>   
>   
> @@ -2679,7 +2679,7 @@ static void dbSplit(dmtree_t * tp, int leafno, int splitsz, int newval)
>    *
>    * serialization: IREAD_LOCK(ipbmap) or IWRITE_LOCK(ipbmap) held on entry/exit;
>    */
> -static int dbBackSplit(dmtree_t * tp, int leafno)
> +static int dbBackSplit(dmtree_t * tp, int leafno, bool is_ctl)
>   {
>   	int budsz, bud, w, bsz, size;
>   	int cursz;
> @@ -2730,7 +2730,7 @@ static int dbBackSplit(dmtree_t * tp, int leafno)
>   				 * system in two.
>   				 */
>   				cursz = leaf[bud] - 1;
> -				dbSplit(tp, bud, cursz, cursz);
> +				dbSplit(tp, bud, cursz, cursz, is_ctl);
>   				break;
>   			}
>   		}
> @@ -2758,7 +2758,7 @@ static int dbBackSplit(dmtree_t * tp, int leafno)
>    *
>    * RETURN VALUES: none
>    */
> -static int dbJoin(dmtree_t * tp, int leafno, int newval)
> +static int dbJoin(dmtree_t * tp, int leafno, int newval, bool is_ctl)
>   {
>   	int budsz, buddy;
>   	s8 *leaf;
> @@ -2813,12 +2813,12 @@ static int dbJoin(dmtree_t * tp, int leafno, int newval)
>   			if (leafno < buddy) {
>   				/* leafno is the left buddy.
>   				 */
> -				dbAdjTree(tp, buddy, NOFREE);
> +				dbAdjTree(tp, buddy, NOFREE, is_ctl);
>   			} else {
>   				/* buddy is the left buddy and becomes
>   				 * leafno.
>   				 */
> -				dbAdjTree(tp, leafno, NOFREE);
> +				dbAdjTree(tp, leafno, NOFREE, is_ctl);
>   				leafno = buddy;
>   			}
>   
> @@ -2831,7 +2831,7 @@ static int dbJoin(dmtree_t * tp, int leafno, int newval)
>   
>   	/* update the leaf value.
>   	 */
> -	dbAdjTree(tp, leafno, newval);
> +	dbAdjTree(tp, leafno, newval, is_ctl);
>   
>   	return 0;
>   }
> @@ -2852,15 +2852,20 @@ static int dbJoin(dmtree_t * tp, int leafno, int newval)
>    *
>    * RETURN VALUES: none
>    */
> -static void dbAdjTree(dmtree_t * tp, int leafno, int newval)
> +static void dbAdjTree(dmtree_t * tp, int leafno, int newval, bool is_ctl)
>   {
>   	int lp, pp, k;
> -	int max;
> +	int max, size;
> +
> +	size = is_ctl ? CTLTREESIZE : TREESIZE;
>   
>   	/* pick up the index of the leaf for this leafno.
>   	 */
>   	lp = leafno + le32_to_cpu(tp->dmt_leafidx);
>   
> +	if (lp > size || lp < 0)
> +		return;
> +
>   	/* is the current value the same as the old value ?  if so,
>   	 * there is nothing to do.
>   	 */