Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220427071100.3844081-1-xu.xin16@zte.com.cn>
In-Reply-To: <20220427071100.3844081-1-xu.xin16@zte.com.cn>
From:   Marco Elver <elver@google.com>
Date:   Wed, 27 Apr 2022 09:33:52 +0200
Message-ID: <CANpmjNM8hKG+HH+pBR4cDLcU-sUWFO6t4CF89bt5uess0Zm3dg@mail.gmail.com>
Subject: Re: [PATCH] mm/kfence: fix a potential NULL pointer dereference
To:     cgel.zte@gmail.com
Cc:     glider@google.com, akpm@linux-foundation.org, dvyukov@google.com,
        kasan-dev@googlegroups.com, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, xu xin <xu.xin16@zte.com.cn>,
        Zeal Robot <zealci@zte.com.cn>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>