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> <CANpmjNPX0SANJ6oDoxDecMfvbZXFhk4qCuaYPyWT1M8FNpy_vw@mail.gmail.com>
In-Reply-To: <CANpmjNPX0SANJ6oDoxDecMfvbZXFhk4qCuaYPyWT1M8FNpy_vw@mail.gmail.com>
From:   Alexander Potapenko <glider@google.com>
Date:   Wed, 18 Aug 2021 15:43:09 +0200
Message-ID: <CAG_fn=WvyuFbDyx5g8qkjak7H87htc=yk6+5hazXgK5nMZvx1Q@mail.gmail.com>
Subject: Re: [PATCH] kfence: fix is_kfence_address() for addresses below KFENCE_POOL_SIZE
To:     Marco Elver <elver@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Dmitriy Vyukov <dvyukov@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux Memory Management List <linux-mm@kvack.org>,
        kasan-dev <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"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Aug 18, 2021 at 3:40 PM Marco Elver <elver@google.com> wrote:
>
> +Cc Jann
>
> On Wed, 18 Aug 2021 at 15:03, Marco Elver <elver@google.com> wrote:
> >
> > Originally the addr !=3D NULL check was meant to take care of the case
> > where __kfence_pool =3D=3D NULL (KFENCE is disabled). However, this doe=
s 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 o=
r
> > any other faulting access with addr < KFENCE_POOL_SIZE. While the kerne=
l
> > 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 !=3D 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>
Acked-by: Alexander Potapenko <glider@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 poi=
nter was
> > -        * never initialized; keep it in the slow-path after the range-=
check.
> > +        * The __kfence_pool !=3D NULL check is required to deal with t=
he case
> > +        * where __kfence_pool =3D=3D 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+8Nade=
uiu1Byv+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=3D=3DNULL. Simply checking
> __kfence_pool!=3DNULL 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


--=20
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Stra=C3=9Fe, 33
80636 M=C3=BCnchen

Gesch=C3=A4ftsf=C3=BChrer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg