| Message ID | 20220427071100.3844081-1-xu.xin16@zte.com.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/kfence: fix a potential NULL pointer dereference | expand |
On Wed, 27 Apr 2022 at 09:11, <cgel.zte@gmail.com> wrote: > > From: xu xin <xu.xin16@zte.com.cn> > > In __kfence_free(), the returned 'meta' from addr_to_metadata() > might be NULL just as the implementation of addr_to_metadata() > shows. > > Let's add a check of the pointer 'meta' to avoid NULL pointer > dereference. The patch brings three changes: > > 1. Add checks in both kfence_free() and __kfence_free(); > 2. kfence_free is not inline function any longer and new inline > function '__try_free_kfence_meta' is introduced. This is very bad for performance (see below). > 3. The check of is_kfence_address() is not required for > __kfence_free() now because __kfence_free has done the check in > addr_to_metadata(); > > Reported-by: Zeal Robot <zealci@zte.com.cn> Is this a static analysis robot? Please show a real stack trace with an actual NULL-deref. Nack - please see: https://lore.kernel.org/all/CANpmjNO5-o1B9r2eYS_482RBVJSyPoHSnV2t+M8fJdFzBf6d2A@mail.gmail.com/ > Signed-off-by: xu xin <xu.xin16@zte.com.cn> > --- > include/linux/kfence.h | 10 ++-------- > mm/kfence/core.c | 30 +++++++++++++++++++++++++++--- > 2 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/include/linux/kfence.h b/include/linux/kfence.h > index 726857a4b680..fbf6391ab53c 100644 > --- a/include/linux/kfence.h > +++ b/include/linux/kfence.h > @@ -160,7 +160,7 @@ void *kfence_object_start(const void *addr); > * __kfence_free() - release a KFENCE heap object to KFENCE pool > * @addr: object to be freed > * > - * Requires: is_kfence_address(addr) > + * Requires: is_kfence_address(addr), but now it's unnecessary (As an aside, something can't be required and be unnecessary at the same time.) There's a reason it was designed this way - is_kfence_address() is much cheaper than a full call. > * Release a KFENCE object and mark it as freed. > */ > @@ -179,13 +179,7 @@ void __kfence_free(void *addr); > * allocator's free codepath. The allocator must check the return value to > * determine if it was a KFENCE object or not. > */ > -static __always_inline __must_check bool kfence_free(void *addr) > -{ > - if (!is_kfence_address(addr)) > - return false; > - __kfence_free(addr); > - return true; > -} > +bool __must_check kfence_free(void *addr); There's a reason is_kfence_address() is inline here, because this function is actually called in relatively hot paths, and a simple load+cmp is much cheaper than a call! > /** > * kfence_handle_page_fault() - perform page fault handling for KFENCE pages > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > index 6e69986c3f0d..1405585369b3 100644 > --- a/mm/kfence/core.c > +++ b/mm/kfence/core.c > @@ -1048,10 +1048,10 @@ void *kfence_object_start(const void *addr) > return meta ? (void *)meta->addr : NULL; > } > > -void __kfence_free(void *addr) > -{ > - struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr); > > +/* Require: meta is not NULL*/ > +static __always_inline void __try_free_kfence_meta(struct kfence_metadata *meta) > +{ > #ifdef CONFIG_MEMCG > KFENCE_WARN_ON(meta->objcg); > #endif > @@ -1067,6 +1067,30 @@ void __kfence_free(void *addr) > kfence_guarded_free(addr, meta, false); > } > > +void __kfence_free(void *addr) > +{ > + struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr); > + > + if (!meta) { > + kfence_report_error(addr, false, NULL, NULL, KFENCE_ERROR_INVALID); > + return; > + } > + > + __try_free_kfence_meta(meta); > +} > + > +bool __must_check kfence_free(void *addr) > +{ > + struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr); > + > + if (!meta) > + return false; > + > + __try_free_kfence_meta(meta); > + > + return true; > +} > + > bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs *regs) > { > const int page_index = (addr - (unsigned long)__kfence_pool) / PAGE_SIZE; > -- > 2.25.1 >
On Wed, Apr 27, 2022 at 09:33:52AM +0200, Marco Elver wrote: > On Wed, 27 Apr 2022 at 09:11, <cgel.zte@gmail.com> wrote: > > > > From: xu xin <xu.xin16@zte.com.cn> > > > > In __kfence_free(), the returned 'meta' from addr_to_metadata() > > might be NULL just as the implementation of addr_to_metadata() > > shows. > > > > Let's add a check of the pointer 'meta' to avoid NULL pointer > > dereference. The patch brings three changes: > > > > 1. Add checks in both kfence_free() and __kfence_free(); > > 2. kfence_free is not inline function any longer and new inline > > function '__try_free_kfence_meta' is introduced. > > This is very bad for performance (see below). > > > 3. The check of is_kfence_address() is not required for > > __kfence_free() now because __kfence_free has done the check in > > addr_to_metadata(); > > > > Reported-by: Zeal Robot <zealci@zte.com.cn> > > Is this a static analysis robot? Please show a real stack trace with > an actual NULL-deref. > > Nack - please see: > https://lore.kernel.org/all/CANpmjNO5-o1B9r2eYS_482RBVJSyPoHSnV2t+M8fJdFzBf6d2A@mail.gmail.com/ > Thanks for your reply. It's from static analysis indeed and no actual NULL-deref event happened yet. I'm just worried that what if address at the edge of __kfence_pool and thus addr_to_metadata() returns NULL. Is is just a guess, I'm not sure. But if __kfence_free make sure that the given address never is at the edge of __kfence_pool, then the calculation and check in addr_to_metadata() is extra performance consumption: "index = (addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2) - 1; if (index < 0 || index >= CONFIG_KFENCE_NUM_OBJECTS) 240 return NULL;" > >Signed-off-by: xu xin <xu.xin16@zte.com.cn> > > --- > > include/linux/kfence.h | 10 ++-------- > > mm/kfence/core.c | 30 +++++++++++++++++++++++++++--- > > 2 files changed, 29 insertions(+), 11 deletions(-) > > > > diff --git a/include/linux/kfence.h b/include/linux/kfence.h > > index 726857a4b680..fbf6391ab53c 100644 > > --- a/include/linux/kfence.h > > +++ b/include/linux/kfence.h > > @@ -160,7 +160,7 @@ void *kfence_object_start(const void *addr); > > * __kfence_free() - release a KFENCE heap object to KFENCE pool > > * @addr: object to be freed > > * > > - * Requires: is_kfence_address(addr) > > + * Requires: is_kfence_address(addr), but now it's unnecessary > > (As an aside, something can't be required and be unnecessary at the same time.) Oh, I'm sorry for this. In my opinion, inner addr_to_metadata(), is_kfence_address is executed for the second time, so not necessary here.
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hnaz-mm/master]
url: https://github.com/intel-lab-lkp/linux/commits/cgel-zte-gmail-com/mm-kfence-fix-a-potential-NULL-pointer-dereference/20220427-151258
base: https://github.com/hnaz/linux-mm master
config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20220427/202204271645.QTJoeela-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/920e9e639493bc72bee803c763f09760e3acd063
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review cgel-zte-gmail-com/mm-kfence-fix-a-potential-NULL-pointer-dereference/20220427-151258
git checkout 920e9e639493bc72bee803c763f09760e3acd063
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash mm/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
mm/kfence/core.c: In function '__try_free_kfence_meta':
mm/kfence/core.c:1067:37: error: 'addr' undeclared (first use in this function)
1067 | kfence_guarded_free(addr, meta, false);
| ^~~~
mm/kfence/core.c:1067:37: note: each undeclared identifier is reported only once for each function it appears in
mm/kfence/core.c: In function '__kfence_free':
>> mm/kfence/core.c:1075:37: warning: passing argument 1 of 'kfence_report_error' makes integer from pointer without a cast [-Wint-conversion]
1075 | kfence_report_error(addr, false, NULL, NULL, KFENCE_ERROR_INVALID);
| ^~~~
| |
| void *
In file included from mm/kfence/core.c:37:
mm/kfence/kfence.h:129:40: note: expected 'long unsigned int' but argument is of type 'void *'
129 | void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *regs,
| ~~~~~~~~~~~~~~^~~~~~~
vim +/kfence_report_error +1075 mm/kfence/core.c
1069
1070 void __kfence_free(void *addr)
1071 {
1072 struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);
1073
1074 if (!meta) {
> 1075 kfence_report_error(addr, false, NULL, NULL, KFENCE_ERROR_INVALID);
1076 return;
1077 }
1078
1079 __try_free_kfence_meta(meta);
1080 }
1081
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hnaz-mm/master]
url: https://github.com/intel-lab-lkp/linux/commits/cgel-zte-gmail-com/mm-kfence-fix-a-potential-NULL-pointer-dereference/20220427-151258
base: https://github.com/hnaz/linux-mm master
config: arm-buildonly-randconfig-r004-20220427 (https://download.01.org/0day-ci/archive/20220427/202204271916.aTcNyVdc-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 1cddcfdc3c683b393df1a5c9063252eb60e52818)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/920e9e639493bc72bee803c763f09760e3acd063
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review cgel-zte-gmail-com/mm-kfence-fix-a-potential-NULL-pointer-dereference/20220427-151258
git checkout 920e9e639493bc72bee803c763f09760e3acd063
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash mm/kfence/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
mm/kfence/core.c:1067:23: error: use of undeclared identifier 'addr'
kfence_guarded_free(addr, meta, false);
^
>> mm/kfence/core.c:1075:23: warning: incompatible pointer to integer conversion passing 'void *' to parameter of type 'unsigned long' [-Wint-conversion]
kfence_report_error(addr, false, NULL, NULL, KFENCE_ERROR_INVALID);
^~~~
mm/kfence/kfence.h:129:40: note: passing argument to parameter 'address' here
void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *regs,
^
1 warning and 1 error generated.
vim +1075 mm/kfence/core.c
1069
1070 void __kfence_free(void *addr)
1071 {
1072 struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr);
1073
1074 if (!meta) {
> 1075 kfence_report_error(addr, false, NULL, NULL, KFENCE_ERROR_INVALID);
1076 return;
1077 }
1078
1079 __try_free_kfence_meta(meta);
1080 }
1081
Hi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on hnaz-mm/master]
url: https://github.com/intel-lab-lkp/linux/commits/cgel-zte-gmail-com/mm-kfence-fix-a-potential-NULL-pointer-dereference/20220427-151258
base: https://github.com/hnaz/linux-mm master
config: x86_64-randconfig-a014 (https://download.01.org/0day-ci/archive/20220427/202204272015.3JRd9BKR-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 1cddcfdc3c683b393df1a5c9063252eb60e52818)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/920e9e639493bc72bee803c763f09760e3acd063
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review cgel-zte-gmail-com/mm-kfence-fix-a-potential-NULL-pointer-dereference/20220427-151258
git checkout 920e9e639493bc72bee803c763f09760e3acd063
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> mm/kfence/core.c:1067:23: error: use of undeclared identifier 'addr'
kfence_guarded_free(addr, meta, false);
^
mm/kfence/core.c:1075:23: warning: incompatible pointer to integer conversion passing 'void *' to parameter of type 'unsigned long' [-Wint-conversion]
kfence_report_error(addr, false, NULL, NULL, KFENCE_ERROR_INVALID);
^~~~
mm/kfence/kfence.h:129:40: note: passing argument to parameter 'address' here
void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *regs,
^
1 warning and 1 error generated.
vim +/addr +1067 mm/kfence/core.c
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1050
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1051
920e9e639493bc7 xu xin 2022-04-27 1052 /* Require: meta is not NULL*/
920e9e639493bc7 xu xin 2022-04-27 1053 static __always_inline void __try_free_kfence_meta(struct kfence_metadata *meta)
920e9e639493bc7 xu xin 2022-04-27 1054 {
8f0b36497303487 Muchun Song 2022-04-01 1055 #ifdef CONFIG_MEMCG
8f0b36497303487 Muchun Song 2022-04-01 1056 KFENCE_WARN_ON(meta->objcg);
8f0b36497303487 Muchun Song 2022-04-01 1057 #endif
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1058 /*
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1059 * If the objects of the cache are SLAB_TYPESAFE_BY_RCU, defer freeing
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1060 * the object, as the object page may be recycled for other-typed
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1061 * objects once it has been freed. meta->cache may be NULL if the cache
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1062 * was destroyed.
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1063 */
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1064 if (unlikely(meta->cache && (meta->cache->flags & SLAB_TYPESAFE_BY_RCU)))
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1065 call_rcu(&meta->rcu_head, rcu_guarded_free);
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1066 else
0ce20dd840897b1 Alexander Potapenko 2021-02-25 @1067 kfence_guarded_free(addr, meta, false);
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1068 }
0ce20dd840897b1 Alexander Potapenko 2021-02-25 1069
diff --git a/include/linux/kfence.h b/include/linux/kfence.h index 726857a4b680..fbf6391ab53c 100644 --- a/include/linux/kfence.h +++ b/include/linux/kfence.h @@ -160,7 +160,7 @@ void *kfence_object_start(const void *addr); * __kfence_free() - release a KFENCE heap object to KFENCE pool * @addr: object to be freed * - * Requires: is_kfence_address(addr) + * Requires: is_kfence_address(addr), but now it's unnecessary * * Release a KFENCE object and mark it as freed. */ @@ -179,13 +179,7 @@ void __kfence_free(void *addr); * allocator's free codepath. The allocator must check the return value to * determine if it was a KFENCE object or not. */ -static __always_inline __must_check bool kfence_free(void *addr) -{ - if (!is_kfence_address(addr)) - return false; - __kfence_free(addr); - return true; -} +bool __must_check kfence_free(void *addr); /** * kfence_handle_page_fault() - perform page fault handling for KFENCE pages diff --git a/mm/kfence/core.c b/mm/kfence/core.c index 6e69986c3f0d..1405585369b3 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -1048,10 +1048,10 @@ void *kfence_object_start(const void *addr) return meta ? (void *)meta->addr : NULL; } -void __kfence_free(void *addr) -{ - struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr); +/* Require: meta is not NULL*/ +static __always_inline void __try_free_kfence_meta(struct kfence_metadata *meta) +{ #ifdef CONFIG_MEMCG KFENCE_WARN_ON(meta->objcg); #endif @@ -1067,6 +1067,30 @@ void __kfence_free(void *addr) kfence_guarded_free(addr, meta, false); } +void __kfence_free(void *addr) +{ + struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr); + + if (!meta) { + kfence_report_error(addr, false, NULL, NULL, KFENCE_ERROR_INVALID); + return; + } + + __try_free_kfence_meta(meta); +} + +bool __must_check kfence_free(void *addr) +{ + struct kfence_metadata *meta = addr_to_metadata((unsigned long)addr); + + if (!meta) + return false; + + __try_free_kfence_meta(meta); + + return true; +} + bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs *regs) { const int page_index = (addr - (unsigned long)__kfence_pool) / PAGE_SIZE;