LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] kfence: fix is_kfence_address() for addresses below KFENCE_POOL_SIZE
@ 2021-08-18 13:03 Marco Elver
  2021-08-18 13:40 ` Marco Elver
  0 siblings, 1 reply; 3+ messages in thread
From: Marco Elver @ 2021-08-18 13:03 UTC (permalink / raw)
  To: elver, akpm
  Cc: glider, dvyukov, linux-kernel, linux-mm, kasan-dev,
	Kuan-Ying Lee, stable

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);
 }
 
 /**
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] kfence: fix is_kfence_address() for addresses below KFENCE_POOL_SIZE
  2021-08-18 13:03 [PATCH] kfence: fix is_kfence_address() for addresses below KFENCE_POOL_SIZE Marco Elver
@ 2021-08-18 13:40 ` Marco Elver
  2021-08-18 13:43   ` Alexander Potapenko
  0 siblings, 1 reply; 3+ messages in thread
From: Marco Elver @ 2021-08-18 13:40 UTC (permalink / raw)
  To: elver, akpm
  Cc: glider, dvyukov, linux-kernel, linux-mm, kasan-dev,
	Kuan-Ying Lee, stable, Jann Horn

+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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] kfence: fix is_kfence_address() for addresses below KFENCE_POOL_SIZE
  2021-08-18 13:40 ` Marco Elver
@ 2021-08-18 13:43   ` Alexander Potapenko
  0 siblings, 0 replies; 3+ messages in thread
From: Alexander Potapenko @ 2021-08-18 13:43 UTC (permalink / raw)
  To: Marco Elver
  Cc: Andrew Morton, Dmitriy Vyukov, LKML,
	Linux Memory Management List, kasan-dev, Kuan-Ying Lee, stable,
	Jann Horn

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 != 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>
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 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



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-08-18 13:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18 13:03 [PATCH] kfence: fix is_kfence_address() for addresses below KFENCE_POOL_SIZE Marco Elver
2021-08-18 13:40 ` Marco Elver
2021-08-18 13:43   ` Alexander Potapenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).