[PATCH] jfs: fix slab-out-of-bounds Read in dtSearch

Manas Ghandat posted 1 patch 2 years, 2 months ago
There is a newer version of this series
fs/jfs/jfs_dtree.c | 3 +++
1 file changed, 3 insertions(+)
[PATCH] jfs: fix slab-out-of-bounds Read in dtSearch
Posted by Manas Ghandat 2 years, 2 months ago
Currently while searching for current page in the sorted entry table
of the page there is a out of bound access. Added a bound check to fix
the error.

Reported-by: syzbot+9924e2a08d9ba0fd4ce2@syzkaller.appspotmail.com
Fixes: https://syzkaller.appspot.com/bug?extid=9924e2a08d9ba0fd4ce2
Signed-off-by: Manas Ghandat <ghandatmanas@gmail.com>
---
 fs/jfs/jfs_dtree.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/jfs/jfs_dtree.c b/fs/jfs/jfs_dtree.c
index 92b7c533407c..cf67d32d5b7f 100644
--- a/fs/jfs/jfs_dtree.c
+++ b/fs/jfs/jfs_dtree.c
@@ -633,6 +633,9 @@ int dtSearch(struct inode *ip, struct component_name * key, ino_t * data,
 		for (base = 0, lim = p->header.nextindex; lim; lim >>= 1) {
 			index = base + (lim >> 1);
 
+			if (stbl[index] > 128 || stbl[index] < 0)
+				goto out;
+
 			if (p->header.flag & BT_LEAF) {
 				/* uppercase leaf name to compare */
 				cmp =
-- 
2.37.2
Re: [PATCH] jfs: fix slab-out-of-bounds Read in dtSearch
Posted by kernel test robot 2 years, 1 month ago
Hi Manas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on kleikamp-shaggy/jfs-next]
[also build test WARNING on linus/master v6.6-rc7 next-20231023]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Manas-Ghandat/jfs-fix-slab-out-of-bounds-Read-in-dtSearch/20231017-152500
base:   https://github.com/kleikamp/linux-shaggy jfs-next
patch link:    https://lore.kernel.org/r/20231016171130.15952-1-ghandatmanas%40gmail.com
patch subject: [PATCH] jfs: fix slab-out-of-bounds Read in dtSearch
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20231024/202310241933.iekefgtT-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231024/202310241933.iekefgtT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310241933.iekefgtT-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/jfs/jfs_dtree.c:636:20: warning: result of comparison of constant 128 with expression of type 's8' (aka 'signed char') is always false [-Wtautological-constant-out-of-range-compare]
                           if (stbl[index] > 128 || stbl[index] < 0)
                               ~~~~~~~~~~~ ^ ~~~
   1 warning generated.


vim +636 fs/jfs/jfs_dtree.c

   555	
   556	/*
   557	 *	dtSearch()
   558	 *
   559	 * function:
   560	 *	Search for the entry with specified key
   561	 *
   562	 * parameter:
   563	 *
   564	 * return: 0 - search result on stack, leaf page pinned;
   565	 *	   errno - I/O error
   566	 */
   567	int dtSearch(struct inode *ip, struct component_name * key, ino_t * data,
   568		     struct btstack * btstack, int flag)
   569	{
   570		int rc = 0;
   571		int cmp = 1;		/* init for empty page */
   572		s64 bn;
   573		struct metapage *mp;
   574		dtpage_t *p;
   575		s8 *stbl;
   576		int base, index, lim;
   577		struct btframe *btsp;
   578		pxd_t *pxd;
   579		int psize = 288;	/* initial in-line directory */
   580		ino_t inumber;
   581		struct component_name ciKey;
   582		struct super_block *sb = ip->i_sb;
   583	
   584		ciKey.name = kmalloc_array(JFS_NAME_MAX + 1, sizeof(wchar_t),
   585					   GFP_NOFS);
   586		if (!ciKey.name) {
   587			rc = -ENOMEM;
   588			goto dtSearch_Exit2;
   589		}
   590	
   591	
   592		/* uppercase search key for c-i directory */
   593		UniStrcpy(ciKey.name, key->name);
   594		ciKey.namlen = key->namlen;
   595	
   596		/* only uppercase if case-insensitive support is on */
   597		if ((JFS_SBI(sb)->mntflag & JFS_OS2) == JFS_OS2) {
   598			ciToUpper(&ciKey);
   599		}
   600		BT_CLR(btstack);	/* reset stack */
   601	
   602		/* init level count for max pages to split */
   603		btstack->nsplit = 1;
   604	
   605		/*
   606		 *	search down tree from root:
   607		 *
   608		 * between two consecutive entries of <Ki, Pi> and <Kj, Pj> of
   609		 * internal page, child page Pi contains entry with k, Ki <= K < Kj.
   610		 *
   611		 * if entry with search key K is not found
   612		 * internal page search find the entry with largest key Ki
   613		 * less than K which point to the child page to search;
   614		 * leaf page search find the entry with smallest key Kj
   615		 * greater than K so that the returned index is the position of
   616		 * the entry to be shifted right for insertion of new entry.
   617		 * for empty tree, search key is greater than any key of the tree.
   618		 *
   619		 * by convention, root bn = 0.
   620		 */
   621		for (bn = 0;;) {
   622			/* get/pin the page to search */
   623			DT_GETPAGE(ip, bn, mp, psize, p, rc);
   624			if (rc)
   625				goto dtSearch_Exit1;
   626	
   627			/* get sorted entry table of the page */
   628			stbl = DT_GETSTBL(p);
   629	
   630			/*
   631			 * binary search with search key K on the current page.
   632			 */
   633			for (base = 0, lim = p->header.nextindex; lim; lim >>= 1) {
   634				index = base + (lim >> 1);
   635	
 > 636				if (stbl[index] > 128 || stbl[index] < 0)
   637					goto out;
   638	
   639				if (p->header.flag & BT_LEAF) {
   640					/* uppercase leaf name to compare */
   641					cmp =
   642					    ciCompare(&ciKey, p, stbl[index],
   643						      JFS_SBI(sb)->mntflag);
   644				} else {
   645					/* router key is in uppercase */
   646	
   647					cmp = dtCompare(&ciKey, p, stbl[index]);
   648	
   649	
   650				}
   651				if (cmp == 0) {
   652					/*
   653					 *	search hit
   654					 */
   655					/* search hit - leaf page:
   656					 * return the entry found
   657					 */
   658					if (p->header.flag & BT_LEAF) {
   659						inumber = le32_to_cpu(
   660				((struct ldtentry *) & p->slot[stbl[index]])->inumber);
   661	
   662						/*
   663						 * search for JFS_LOOKUP
   664						 */
   665						if (flag == JFS_LOOKUP) {
   666							*data = inumber;
   667							rc = 0;
   668							goto out;
   669						}
   670	
   671						/*
   672						 * search for JFS_CREATE
   673						 */
   674						if (flag == JFS_CREATE) {
   675							*data = inumber;
   676							rc = -EEXIST;
   677							goto out;
   678						}
   679	
   680						/*
   681						 * search for JFS_REMOVE or JFS_RENAME
   682						 */
   683						if ((flag == JFS_REMOVE ||
   684						     flag == JFS_RENAME) &&
   685						    *data != inumber) {
   686							rc = -ESTALE;
   687							goto out;
   688						}
   689	
   690						/*
   691						 * JFS_REMOVE|JFS_FINDDIR|JFS_RENAME
   692						 */
   693						/* save search result */
   694						*data = inumber;
   695						btsp = btstack->top;
   696						btsp->bn = bn;
   697						btsp->index = index;
   698						btsp->mp = mp;
   699	
   700						rc = 0;
   701						goto dtSearch_Exit1;
   702					}
   703	
   704					/* search hit - internal page:
   705					 * descend/search its child page
   706					 */
   707					goto getChild;
   708				}
   709	
   710				if (cmp > 0) {
   711					base = index + 1;
   712					--lim;
   713				}
   714			}
   715	
   716			/*
   717			 *	search miss
   718			 *
   719			 * base is the smallest index with key (Kj) greater than
   720			 * search key (K) and may be zero or (maxindex + 1) index.
   721			 */
   722			/*
   723			 * search miss - leaf page
   724			 *
   725			 * return location of entry (base) where new entry with
   726			 * search key K is to be inserted.
   727			 */
   728			if (p->header.flag & BT_LEAF) {
   729				/*
   730				 * search for JFS_LOOKUP, JFS_REMOVE, or JFS_RENAME
   731				 */
   732				if (flag == JFS_LOOKUP || flag == JFS_REMOVE ||
   733				    flag == JFS_RENAME) {
   734					rc = -ENOENT;
   735					goto out;
   736				}
   737	
   738				/*
   739				 * search for JFS_CREATE|JFS_FINDDIR:
   740				 *
   741				 * save search result
   742				 */
   743				*data = 0;
   744				btsp = btstack->top;
   745				btsp->bn = bn;
   746				btsp->index = base;
   747				btsp->mp = mp;
   748	
   749				rc = 0;
   750				goto dtSearch_Exit1;
   751			}
   752	
   753			/*
   754			 * search miss - internal page
   755			 *
   756			 * if base is non-zero, decrement base by one to get the parent
   757			 * entry of the child page to search.
   758			 */
   759			index = base ? base - 1 : base;
   760	
   761			/*
   762			 * go down to child page
   763			 */
   764		      getChild:
   765			/* update max. number of pages to split */
   766			if (BT_STACK_FULL(btstack)) {
   767				/* Something's corrupted, mark filesystem dirty so
   768				 * chkdsk will fix it.
   769				 */
   770				jfs_error(sb, "stack overrun!\n");
   771				BT_STACK_DUMP(btstack);
   772				rc = -EIO;
   773				goto out;
   774			}
   775			btstack->nsplit++;
   776	
   777			/* push (bn, index) of the parent page/entry */
   778			BT_PUSH(btstack, bn, index);
   779	
   780			/* get the child page block number */
   781			pxd = (pxd_t *) & p->slot[stbl[index]];
   782			bn = addressPXD(pxd);
   783			psize = lengthPXD(pxd) << JFS_SBI(ip->i_sb)->l2bsize;
   784	
   785			/* unpin the parent page */
   786			DT_PUTPAGE(mp);
   787		}
   788	
   789	      out:
   790		DT_PUTPAGE(mp);
   791	
   792	      dtSearch_Exit1:
   793	
   794		kfree(ciKey.name);
   795	
   796	      dtSearch_Exit2:
   797	
   798		return rc;
   799	}
   800	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH] jfs: fix slab-out-of-bounds Read in dtSearch
Posted by Dan Carpenter 2 years, 1 month ago
Hi Manas,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Manas-Ghandat/jfs-fix-slab-out-of-bounds-Read-in-dtSearch/20231017-152500
base:   https://github.com/kleikamp/linux-shaggy jfs-next
patch link:    https://lore.kernel.org/r/20231016171130.15952-1-ghandatmanas%40gmail.com
patch subject: [PATCH] jfs: fix slab-out-of-bounds Read in dtSearch
config: i386-randconfig-141-20231022 (https://download.01.org/0day-ci/archive/20231024/202310241724.Ed02yUz9-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231024/202310241724.Ed02yUz9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202310241724.Ed02yUz9-lkp@intel.com/

smatch warnings:
fs/jfs/jfs_dtree.c:636 dtSearch() warn: impossible condition '(stbl[index] > 128) => ((-128)-127 > 128)'

vim +636 fs/jfs/jfs_dtree.c

^1da177e4c3f41 Linus Torvalds   2005-04-16  567  int dtSearch(struct inode *ip, struct component_name * key, ino_t * data,
^1da177e4c3f41 Linus Torvalds   2005-04-16  568  	     struct btstack * btstack, int flag)
^1da177e4c3f41 Linus Torvalds   2005-04-16  569  {
^1da177e4c3f41 Linus Torvalds   2005-04-16  570  	int rc = 0;
^1da177e4c3f41 Linus Torvalds   2005-04-16  571  	int cmp = 1;		/* init for empty page */
^1da177e4c3f41 Linus Torvalds   2005-04-16  572  	s64 bn;
^1da177e4c3f41 Linus Torvalds   2005-04-16  573  	struct metapage *mp;
^1da177e4c3f41 Linus Torvalds   2005-04-16  574  	dtpage_t *p;
^1da177e4c3f41 Linus Torvalds   2005-04-16  575  	s8 *stbl;
                                                        ^^^^^^^^

^1da177e4c3f41 Linus Torvalds   2005-04-16  576  	int base, index, lim;
^1da177e4c3f41 Linus Torvalds   2005-04-16  577  	struct btframe *btsp;
^1da177e4c3f41 Linus Torvalds   2005-04-16  578  	pxd_t *pxd;
^1da177e4c3f41 Linus Torvalds   2005-04-16  579  	int psize = 288;	/* initial in-line directory */
^1da177e4c3f41 Linus Torvalds   2005-04-16  580  	ino_t inumber;
^1da177e4c3f41 Linus Torvalds   2005-04-16  581  	struct component_name ciKey;
^1da177e4c3f41 Linus Torvalds   2005-04-16  582  	struct super_block *sb = ip->i_sb;
^1da177e4c3f41 Linus Torvalds   2005-04-16  583  
6da2ec56059c3c Kees Cook        2018-06-12  584  	ciKey.name = kmalloc_array(JFS_NAME_MAX + 1, sizeof(wchar_t),
6da2ec56059c3c Kees Cook        2018-06-12  585  				   GFP_NOFS);
09aaa749f637b1 Joe Perches      2007-11-13  586  	if (!ciKey.name) {
^1da177e4c3f41 Linus Torvalds   2005-04-16  587  		rc = -ENOMEM;
^1da177e4c3f41 Linus Torvalds   2005-04-16  588  		goto dtSearch_Exit2;
^1da177e4c3f41 Linus Torvalds   2005-04-16  589  	}
^1da177e4c3f41 Linus Torvalds   2005-04-16  590  
^1da177e4c3f41 Linus Torvalds   2005-04-16  591  
^1da177e4c3f41 Linus Torvalds   2005-04-16  592  	/* uppercase search key for c-i directory */
^1da177e4c3f41 Linus Torvalds   2005-04-16  593  	UniStrcpy(ciKey.name, key->name);
^1da177e4c3f41 Linus Torvalds   2005-04-16  594  	ciKey.namlen = key->namlen;
^1da177e4c3f41 Linus Torvalds   2005-04-16  595  
^1da177e4c3f41 Linus Torvalds   2005-04-16  596  	/* only uppercase if case-insensitive support is on */
^1da177e4c3f41 Linus Torvalds   2005-04-16  597  	if ((JFS_SBI(sb)->mntflag & JFS_OS2) == JFS_OS2) {
^1da177e4c3f41 Linus Torvalds   2005-04-16  598  		ciToUpper(&ciKey);
^1da177e4c3f41 Linus Torvalds   2005-04-16  599  	}
^1da177e4c3f41 Linus Torvalds   2005-04-16  600  	BT_CLR(btstack);	/* reset stack */
^1da177e4c3f41 Linus Torvalds   2005-04-16  601  
^1da177e4c3f41 Linus Torvalds   2005-04-16  602  	/* init level count for max pages to split */
^1da177e4c3f41 Linus Torvalds   2005-04-16  603  	btstack->nsplit = 1;
^1da177e4c3f41 Linus Torvalds   2005-04-16  604  
^1da177e4c3f41 Linus Torvalds   2005-04-16  605  	/*
^1da177e4c3f41 Linus Torvalds   2005-04-16  606  	 *	search down tree from root:
^1da177e4c3f41 Linus Torvalds   2005-04-16  607  	 *
^1da177e4c3f41 Linus Torvalds   2005-04-16  608  	 * between two consecutive entries of <Ki, Pi> and <Kj, Pj> of
^1da177e4c3f41 Linus Torvalds   2005-04-16  609  	 * internal page, child page Pi contains entry with k, Ki <= K < Kj.
^1da177e4c3f41 Linus Torvalds   2005-04-16  610  	 *
^1da177e4c3f41 Linus Torvalds   2005-04-16  611  	 * if entry with search key K is not found
^1da177e4c3f41 Linus Torvalds   2005-04-16  612  	 * internal page search find the entry with largest key Ki
^1da177e4c3f41 Linus Torvalds   2005-04-16  613  	 * less than K which point to the child page to search;
^1da177e4c3f41 Linus Torvalds   2005-04-16  614  	 * leaf page search find the entry with smallest key Kj
^1da177e4c3f41 Linus Torvalds   2005-04-16  615  	 * greater than K so that the returned index is the position of
^1da177e4c3f41 Linus Torvalds   2005-04-16  616  	 * the entry to be shifted right for insertion of new entry.
^1da177e4c3f41 Linus Torvalds   2005-04-16  617  	 * for empty tree, search key is greater than any key of the tree.
^1da177e4c3f41 Linus Torvalds   2005-04-16  618  	 *
^1da177e4c3f41 Linus Torvalds   2005-04-16  619  	 * by convention, root bn = 0.
^1da177e4c3f41 Linus Torvalds   2005-04-16  620  	 */
^1da177e4c3f41 Linus Torvalds   2005-04-16  621  	for (bn = 0;;) {
^1da177e4c3f41 Linus Torvalds   2005-04-16  622  		/* get/pin the page to search */
^1da177e4c3f41 Linus Torvalds   2005-04-16  623  		DT_GETPAGE(ip, bn, mp, psize, p, rc);
^1da177e4c3f41 Linus Torvalds   2005-04-16  624  		if (rc)
^1da177e4c3f41 Linus Torvalds   2005-04-16  625  			goto dtSearch_Exit1;
^1da177e4c3f41 Linus Torvalds   2005-04-16  626  
^1da177e4c3f41 Linus Torvalds   2005-04-16  627  		/* get sorted entry table of the page */
^1da177e4c3f41 Linus Torvalds   2005-04-16  628  		stbl = DT_GETSTBL(p);
^1da177e4c3f41 Linus Torvalds   2005-04-16  629  
^1da177e4c3f41 Linus Torvalds   2005-04-16  630  		/*
^1da177e4c3f41 Linus Torvalds   2005-04-16  631  		 * binary search with search key K on the current page.
^1da177e4c3f41 Linus Torvalds   2005-04-16  632  		 */
^1da177e4c3f41 Linus Torvalds   2005-04-16  633  		for (base = 0, lim = p->header.nextindex; lim; lim >>= 1) {
^1da177e4c3f41 Linus Torvalds   2005-04-16  634  			index = base + (lim >> 1);
^1da177e4c3f41 Linus Torvalds   2005-04-16  635  
7812e358b5edde Manas Ghandat    2023-10-16 @636  			if (stbl[index] > 128 || stbl[index] < 0)

If it's stbl is an s8 so it can't be > 128.

7812e358b5edde Manas Ghandat    2023-10-16  637  				goto out;
7812e358b5edde Manas Ghandat    2023-10-16  638  
^1da177e4c3f41 Linus Torvalds   2005-04-16  639  			if (p->header.flag & BT_LEAF) {
^1da177e4c3f41 Linus Torvalds   2005-04-16  640  				/* uppercase leaf name to compare */
^1da177e4c3f41 Linus Torvalds   2005-04-16  641  				cmp =
^1da177e4c3f41 Linus Torvalds   2005-04-16  642  				    ciCompare(&ciKey, p, stbl[index],
^1da177e4c3f41 Linus Torvalds   2005-04-16  643  					      JFS_SBI(sb)->mntflag);
^1da177e4c3f41 Linus Torvalds   2005-04-16  644  			} else {
^1da177e4c3f41 Linus Torvalds   2005-04-16  645  				/* router key is in uppercase */
^1da177e4c3f41 Linus Torvalds   2005-04-16  646  
^1da177e4c3f41 Linus Torvalds   2005-04-16  647  				cmp = dtCompare(&ciKey, p, stbl[index]);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki