* Re: [PATCH] lkdtm: Add kfence read after free crash type
2023-11-29 20:42 ` [PATCH] lkdtm: Add kfence read after free crash type Stephen Boyd
@ 2023-11-29 20:54 ` Marco Elver
0 siblings, 0 replies; 2+ messages in thread
From: Marco Elver @ 2023-11-29 20:54 UTC (permalink / raw)
To: Stephen Boyd
Cc: Kees Cook, linux-kernel, patches, Arnd Bergmann,
Greg Kroah-Hartman, Alexander Potapenko, Dmitry Vyukov,
kasan-dev, linux-mm
On Wed, 29 Nov 2023 at 21:42, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Adding kfence folks (will add on v2).
>
> Quoting Kees Cook (2023-11-29 12:22:27)
> > On Mon, Nov 27, 2023 at 03:49:45PM -0800, Stephen Boyd wrote:
> > > Add the ability to allocate memory from kfence and trigger a read after
> > > free on that memory to validate that kfence is working properly. This is
> > > used by ChromeOS integration tests to validate that kfence errors can be
> > > collected on user devices and parsed properly.
> >
> > This looks really good; thanks for adding this!
> >
> > >
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > > drivers/misc/lkdtm/heap.c | 64 +++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 64 insertions(+)
> > >
> > > diff --git a/drivers/misc/lkdtm/heap.c b/drivers/misc/lkdtm/heap.c
> > > index 0ce4cbf6abda..608872bcc7e0 100644
> > > --- a/drivers/misc/lkdtm/heap.c
> > > +++ b/drivers/misc/lkdtm/heap.c
> > > @@ -4,6 +4,7 @@
> > > * page allocation and slab allocations.
> > > */
> > > #include "lkdtm.h"
> > > +#include <linux/kfence.h>
> > > #include <linux/slab.h>
> > > #include <linux/vmalloc.h>
> > > #include <linux/sched.h>
> > > @@ -132,6 +133,66 @@ static void lkdtm_READ_AFTER_FREE(void)
> > > kfree(val);
> > > }
> > >
> > > +#if IS_ENABLED(CONFIG_KFENCE)
> >
> > I really try hard to avoid having tests disappear depending on configs,
> > and instead report the expected failure case (as you have). Can this be
> > built without the IS_ENABLED() tests?
> >
>
> We need IS_ENABLED() for the kfence_sample_interval variable. I suppose
> if the config isn't set that variable can be assumed as zero and then
> the timeout would hit immediately. We can either define the name
> 'kfence_sample_interval' as 0 in the header, or put an ifdef in the
> function.
I think it's fair to put it in the kfence header, so you don't need
the #ifdefs in the test code.
We didn't think anyone should depend on kfence_sample_interval outside
KFENCE code, but probably only tests would anyway.
> ---8<---
> diff --git a/drivers/misc/lkdtm/heap.c b/drivers/misc/lkdtm/heap.c
> index 4f467d3972a6..574d0aa726dc 100644
> --- a/drivers/misc/lkdtm/heap.c
> +++ b/drivers/misc/lkdtm/heap.c
> @@ -138,6 +138,14 @@ static void lkdtm_KFENCE_READ_AFTER_FREE(void)
> int *base, val, saw;
> unsigned long timeout, resched_after;
> size_t len = 1024;
> + unsigned long interval;
> +
> +#ifdef CONFIG_KFENCE
> + interval = kfence_sample_interval;
> +#else
> + interval = 0;
> +#endif
> +
> /*
> * The slub allocator will use the either the first word or
> * the middle of the allocation to store the free pointer,
> @@ -150,13 +158,13 @@ static void lkdtm_KFENCE_READ_AFTER_FREE(void)
> * 100x the sample interval should be more than enough to ensure we get
> * a KFENCE allocation eventually.
> */
> - timeout = jiffies + msecs_to_jiffies(100 * kfence_sample_interval);
> + timeout = jiffies + msecs_to_jiffies(100 * interval);
> /*
> * Especially for non-preemption kernels, ensure the allocation-gate
> * timer can catch up: after @resched_after, every failed allocation
> * attempt yields, to ensure the allocation-gate timer is scheduled.
> */
> - resched_after = jiffies + msecs_to_jiffies(kfence_sample_interval);
> + resched_after = jiffies + msecs_to_jiffies(interval);
> do {
> base = kmalloc(len, GFP_KERNEL);
> if (!base) {
>
> ---8<----
> diff --git a/include/linux/kfence.h b/include/linux/kfence.h
> index 401af4757514..88100cc9caba 100644
> --- a/include/linux/kfence.h
> +++ b/include/linux/kfence.h
> @@ -223,6 +223,8 @@ bool __kfence_obj_info(struct kmem_obj_info *kpp,
> void *object, struct slab *sla
>
> #else /* CONFIG_KFENCE */
>
> +#define kfence_sample_interval (0)
> +
> static inline bool is_kfence_address(const void *addr) { return false; }
> static inline void kfence_alloc_pool_and_metadata(void) { }
> static inline void kfence_init(void) { }
Acked-by: Marco Elver <elver@google.com>
FWIW, I've occasionally been using repeatedly invoked READ_AFTER_FREE
to test if KFENCE is working. Having a dedicated test like this seems
more reliable though.
^ permalink raw reply [flat|nested] 2+ messages in thread