On Wed, 27 Apr 2022 at 09:11, <cgel.zte@xxxxxxxxx> wrote: > > From: xu xin <xu.xin16@xxxxxxxxxx> > > 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/ > Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx> > --- > 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 >
