Re: [PATCH] mm/kfence: fix a potential NULL pointer dereference

Marco Elver
2022-04-27T07:34:34Z

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
>