Re: [PATCH] kasan: guard release_free_meta() shadow access with kasan_arch_is_ready()

Re: [PATCH] kasan: guard release_free_meta() shadow access with kasan_arch_is_ready()

[Andrew Morton]

On Tue, 13 Feb 2024 14:39:58 +1100 Benjamin Gray <bgray@linux.ibm.com> wrote:

> release_free_meta() accesses the shadow directly through the path
> 
>   kasan_slab_free
>     __kasan_slab_free
>       kasan_release_object_meta
>         release_free_meta
>           kasan_mem_to_shadow
> 
> There are no kasan_arch_is_ready() guards here, allowing an oops when
> the shadow is not initialized. The oops can be seen on a Power8 KVM
> guest.
> 
> This patch adds the guard to release_free_meta(), as it's the first
> level that specifically requires the shadow.
> 
> It is safe to put the guard at the start of this function, before the
> stack put: only kasan_save_free_info() can initialize the saved stack,
> which itself is guarded with kasan_arch_is_ready() by its caller
> poison_slab_object(). If the arch becomes ready before
> release_free_meta() then we will not observe KASAN_SLAB_FREE_META in the
> object's shadow, so we will not put an uninitialized stack either.
> 
> ...
>
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -522,6 +522,9 @@ static void release_alloc_meta(struct kasan_alloc_meta *meta)
>  
>  static void release_free_meta(const void *object, struct kasan_free_meta *meta)
>  {
> +	if (!kasan_arch_is_ready())
> +		return;
> +
>  	/* Check if free meta is valid. */
>  	if (*(u8 *)kasan_mem_to_shadow(object) != KASAN_SLAB_FREE_META)
>  		return;

I'll add
Fixes: 63b85ac56a64 ("kasan: stop leaking stack trace handles")

Re: [PATCH] kasan: guard release_free_meta() shadow access with kasan_arch_is_ready()

[Andrey Konovalov]

On Tue, Feb 13, 2024 at 4:40 AM Benjamin Gray <bgray@linux.ibm.com> wrote:
>
> release_free_meta() accesses the shadow directly through the path
>
>   kasan_slab_free
>     __kasan_slab_free
>       kasan_release_object_meta
>         release_free_meta
>           kasan_mem_to_shadow
>
> There are no kasan_arch_is_ready() guards here, allowing an oops when
> the shadow is not initialized. The oops can be seen on a Power8 KVM
> guest.
>
> This patch adds the guard to release_free_meta(), as it's the first
> level that specifically requires the shadow.
>
> It is safe to put the guard at the start of this function, before the
> stack put: only kasan_save_free_info() can initialize the saved stack,
> which itself is guarded with kasan_arch_is_ready() by its caller
> poison_slab_object(). If the arch becomes ready before
> release_free_meta() then we will not observe KASAN_SLAB_FREE_META in the
> object's shadow, so we will not put an uninitialized stack either.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
>
> ---
>
> I am interested in removing the need for kasan_arch_is_ready() entirely,
> as it mostly acts like a separate check of kasan_enabled().

Dropping kasan_arch_is_ready() calls from KASAN internals and instead
relying on kasan_enabled() checks in include/linux/kasan.h would be
great!

I filed a bug about this a while ago:
https://bugzilla.kernel.org/show_bug.cgi?id=217049

> Currently
> both are necessary, but I think adding a kasan_enabled() guard to
> check_region_inline() makes kasan_enabled() a superset of
> kasan_arch_is_ready().

Sounds good to me. I would also go through the list of other exported
KASAN functions to check whether any of them also need a
kasan_enabled() check. At least kasan_unpoison_task_stack() seems to
be one of them.

> Allowing an arch to override kasan_enabled() can then let us replace it
> with a static branch that we enable somewhere in boot (for PowerPC,
> after we use a bunch of generic code to parse the device tree to
> determine how we want to configure the MMU). This should generally work
> OK I think, as HW tags already does this,

We can also add something like CONFIG_ARCH_HAS_KASAN_FLAG_ENABLE and
only use a static branch only on those architectures where it's
required.

> but I did have to add another
> patch for an uninitialised data access it introduces.

What was this data access? Is this something we need to fix in the mainline?

> On the other hand, KASAN does more than shadow based sanitisation, so
> we'd be disabling that in early boot too.

I think the things that we need to handle before KASAN is enabled is
kasan_cache_create() and kasan_metadata_size() (if these can even
called before KASAN is enabled). Otherwise, KASAN just collects
metadata, which is useless without shadow memory-based reporting
anyway.

> ---
>  mm/kasan/generic.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index df6627f62402..032bf3e98c24 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -522,6 +522,9 @@ static void release_alloc_meta(struct kasan_alloc_meta *meta)
>
>  static void release_free_meta(const void *object, struct kasan_free_meta *meta)
>  {
> +       if (!kasan_arch_is_ready())
> +               return;
> +
>         /* Check if free meta is valid. */
>         if (*(u8 *)kasan_mem_to_shadow(object) != KASAN_SLAB_FREE_META)
>                 return;
> --
> 2.43.0
>

For the patch itself as a fix:

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>

Thanks!

Re: [PATCH] kasan: guard release_free_meta() shadow access with kasan_arch_is_ready()

[Andrey Konovalov]

On Thu, Feb 15, 2024 at 12:25 AM Benjamin Gray <bgray@linux.ibm.com> wrote:
>
> > We can also add something like CONFIG_ARCH_HAS_KASAN_FLAG_ENABLE and
> > only use a static branch only on those architectures where it's
> > required.

Perhaps CONFIG_ARCH_USES_KASAN_ENABLED would be a better name.

> That works too, PowerPC should only need a static branch when
> CONFIG_KASAN is enabled.
>
> Loongarch is also a kasan_arch_is_ready() user though, so I'm not sure
> if they'd still need it for something?

And UM as well.

We can start with a change that makes kasan_flag_enabled and
kasan_enabled() work for the Generic mode, define a
kasan_init_generic() function that switches kasan_flag_enabled (and
prints the "KernelAddressSanitizer initialized" message?), and then
call kasan_init_generic() from kasan_init() in the arch code (perhaps
even for all arches to unify the "initialized" message?).

And then we can ask Loongarch and UM people to test the change.

Both Loongarch and UM define kasan_arch_is_ready() as the value of
their global enable flags, which they switch in kasan_init(). So I
would think using kasan_enabled() should just work (minding the
metadata-related parts).

> > What was this data access? Is this something we need to fix in the
> > mainline?
>
> I don't believe so (though I spent a while debugging it before I
> realised I had introduced it by changing kasan_enabled() dynamically).
>
> In kasan_cache_create() we unconditionally allocate a metadata buffer,
> but the kasan_init_slab_obj() call to initialise it is guarded by
> kasan_enabled(). But later parts of the code only check the presence of
> the buffer before using it, so bad things happen if kasan_enabled()
> later turns on (I was getting some error about invalid lock state).

Ah, makes sense. Yeah, these metadata init functions should work even
before kasan_flag_enabled is switched on then.