From: xu xin <[email protected]>
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.
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 <[email protected]>
Signed-off-by: xu xin <[email protected]>
---
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
*
* 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;
--
2.25.1
On Wed, 27 Apr 2022 at 09:11, <[email protected]> wrote:
>
> From: xu xin <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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, <[email protected]> wrote:
> >
> > From: xu xin <[email protected]>
> >
> > 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 <[email protected]>
>
> 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 <[email protected]>
> > ---
> > 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/[email protected]/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 <[email protected]>
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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/[email protected]/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 <[email protected]>
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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/[email protected]/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 <[email protected]>
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp