Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210818130300.2482437-1-elver@google.com>
In-Reply-To: <20210818130300.2482437-1-elver@google.com>
From:   Marco Elver <elver@google.com>
Date:   Wed, 18 Aug 2021 15:40:04 +0200
Message-ID: <CANpmjNPX0SANJ6oDoxDecMfvbZXFhk4qCuaYPyWT1M8FNpy_vw@mail.gmail.com>
Subject: Re: [PATCH] kfence: fix is_kfence_address() for addresses below KFENCE_POOL_SIZE
To:     elver@google.com, akpm@linux-foundation.org
Cc:     glider@google.com, dvyukov@google.com,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        kasan-dev@googlegroups.com,
        Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>,
        stable@vger.kernel.org, Jann Horn <jannh@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

+Cc Jann

On Wed, 18 Aug 2021 at 15:03, Marco Elver <elver@google.com> wrote:
>
> Originally the addr != NULL check was meant to take care of the case
> where __kfence_pool == NULL (KFENCE is disabled). However, this does not
> work for addresses where addr > 0 && addr < KFENCE_POOL_SIZE.
>
> This can be the case on NULL-deref where addr > 0 && addr < PAGE_SIZE or
> any other faulting access with addr < KFENCE_POOL_SIZE. While the kernel
> would likely crash, the stack traces and report might be confusing due
> to double faults upon KFENCE's attempt to unprotect such an address.
>
> Fix it by just checking that __kfence_pool != NULL instead.
>
> Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure")
> Reported-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>
> Signed-off-by: Marco Elver <elver@google.com>
> Cc: <stable@vger.kernel.org>    [5.12+]
> ---
>  include/linux/kfence.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/kfence.h b/include/linux/kfence.h
> index a70d1ea03532..3fe6dd8a18c1 100644
> --- a/include/linux/kfence.h
> +++ b/include/linux/kfence.h
> @@ -51,10 +51,11 @@ extern atomic_t kfence_allocation_gate;
>  static __always_inline bool is_kfence_address(const void *addr)
>  {
>         /*
> -        * The non-NULL check is required in case the __kfence_pool pointer was
> -        * never initialized; keep it in the slow-path after the range-check.
> +        * The __kfence_pool != NULL check is required to deal with the case
> +        * where __kfence_pool == NULL && addr < KFENCE_POOL_SIZE. Keep it in
> +        * the slow-path after the range-check!
>          */
> -       return unlikely((unsigned long)((char *)addr - __kfence_pool) < KFENCE_POOL_SIZE && addr);
> +       return unlikely((unsigned long)((char *)addr - __kfence_pool) < KFENCE_POOL_SIZE && __kfence_pool);
>  }

Jann, I recall discussing this check somewhere around:
https://lore.kernel.org/linux-doc/CAG48ez0D1+hStZaDOigwbqNqFHJAJtXK+8Nadeuiu1Byv+xp5A@mail.gmail.com/

I think you pointed out initially that we need another check, but
somehow that turned into '&& addr' -- I think that's what we ended up
with because of worry about another memory load, which is clearly
wrong as that only works if addr==NULL. Simply checking
__kfence_pool!=NULL is enough. I also checked codegen, and the
compiler is smart enough to not reload the global __kfence_pool.

Wanted to call it out, just in case you see something even more
efficient (probably the only way to do better is to get rid of the 2nd
branch, which I don't think is possible). :-)

Thanks,
-- Marco