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

[Andrey Konovalov <andreyknvl@gmail.com>]

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.