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: <20210403051325.683071-1-pcc@google.com>
In-Reply-To: <20210403051325.683071-1-pcc@google.com>
From:   Marco Elver <elver@google.com>
Date:   Sat, 3 Apr 2021 12:03:19 +0200
Message-ID: <CANpmjNOzaOJY5K+Sq78AF5N1_6=1kv3rXZ6w+XPuEf9G+cd8iA@mail.gmail.com>
Subject: Re: [PATCH] kfence: unpoison pool region before use
To:     Peter Collingbourne <pcc@google.com>
Cc:     Dmitry Vyukov <dvyukov@google.com>,
        Alexander Potapenko <glider@google.com>,
        Evgenii Stepanov <eugenis@google.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, 3 Apr 2021 at 07:13, Peter Collingbourne <pcc@google.com> wrote:
> If the memory region allocated by KFENCE had previously been poisoned,
> any validity checks done using kasan_byte_accessible() will fail. Fix
> it by unpoisoning the memory before using it as the pool region.
>
> Link: https://linux-review.googlesource.com/id/I0af99e9f1c25eaf7e1ec295836b5d148d76940c5
> Signed-off-by: Peter Collingbourne <pcc@google.com>

Thanks, at a high level this seems reasonable, because we always want
to ensure that KFENCE memory remains unpoisoned with KASAN on. FWIW I
subjected a config with KFENCE+KASAN (generic, SW_TAGS, and HW_TAGS)
to syzkaller testing and ran kfence_test:

  Tested-by: Marco Elver <elver@google.com>


However, it is unclear to me under which circumstances we actually
need this, i.e. something would grab some memblock memory, somehow
poison it, and then release the memory back during early boot (note,
kfence_alloc_pool() is called before slab setup). If we can somehow
understand what actually did this, perhaps it'd help tell us if this
actually needs fixing in KFENCE or it's the other thing that needs a
fix.

Given all this is happening during really early boot, I'd expect no or
very few calls to kasan_poison() until kfence_alloc_pool() is called.
We can probably debug it more by having kasan_poison() do a "if
(!__kfence_pool) dump_stack();" somewhere. Can you try this on the
system where you can repro the problem? I tried this just now on the
latest mainline kernel, and saw 0 calls until kfence_alloc_pool().

Thanks,
-- Marco

> ---
>  mm/kfence/core.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index d53c91f881a4..bb22b0cf77aa 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -633,13 +633,19 @@ static DECLARE_DELAYED_WORK(kfence_timer, toggle_allocation_gate);
>
>  void __init kfence_alloc_pool(void)
>  {
> +       void *pool;
> +
>         if (!kfence_sample_interval)
>                 return;
>
> -       __kfence_pool = memblock_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
> -
> -       if (!__kfence_pool)
> +       pool = memblock_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
> +       if (!pool) {
>                 pr_err("failed to allocate pool\n");
> +               return;
> +       }
> +
> +       kasan_unpoison_range(pool, KFENCE_POOL_SIZE);
> +       __kfence_pool = pool;
>  }
>
>  void __init kfence_init(void)
> --
> 2.31.0.208.g409f899ff0-goog
>