| Message ID | f7e26047d2fb7b963aebb894a23989cd830265bd.1640036051.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan, vmalloc, arm64: add vmalloc tagging support for SW/HW_TAGS | expand |
On Mon, Dec 20, 2021 at 11:02 PM <andrey.konovalov@linux.dev> wrote: > > From: Andrey Konovalov <andreyknvl@google.com> > > Allow disabling vmalloc() tagging for HW_TAGS KASAN via a kasan.vmalloc > command line switch. > > This is a fail-safe switch intended for production systems that enable > HW_TAGS KASAN. In case vmalloc() tagging ends up having an issue not > detected during testing but that manifests in production, kasan.vmalloc > allows to turn vmalloc() tagging off while leaving page_alloc/slab > tagging on. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > --- > > Changes v1->v2: > - Mark kasan_arg_stacktrace as __initdata instead of __ro_after_init. > - Combine KASAN_ARG_VMALLOC_DEFAULT and KASAN_ARG_VMALLOC_ON switch > cases. > --- > mm/kasan/hw_tags.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- > mm/kasan/kasan.h | 6 ++++++ > 2 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c > index 99230e666c1b..657b23cebe28 100644 > --- a/mm/kasan/hw_tags.c > +++ b/mm/kasan/hw_tags.c > @@ -32,6 +32,12 @@ enum kasan_arg_mode { > KASAN_ARG_MODE_ASYMM, > }; > > +enum kasan_arg_vmalloc { > + KASAN_ARG_VMALLOC_DEFAULT, > + KASAN_ARG_VMALLOC_OFF, > + KASAN_ARG_VMALLOC_ON, > +}; > + > enum kasan_arg_stacktrace { > KASAN_ARG_STACKTRACE_DEFAULT, > KASAN_ARG_STACKTRACE_OFF, > @@ -40,6 +46,7 @@ enum kasan_arg_stacktrace { > > static enum kasan_arg kasan_arg __ro_after_init; > static enum kasan_arg_mode kasan_arg_mode __ro_after_init; > +static enum kasan_arg_vmalloc kasan_arg_vmalloc __initdata; > static enum kasan_arg_stacktrace kasan_arg_stacktrace __initdata; > > /* Whether KASAN is enabled at all. */ > @@ -50,6 +57,9 @@ EXPORT_SYMBOL(kasan_flag_enabled); > enum kasan_mode kasan_mode __ro_after_init; > EXPORT_SYMBOL_GPL(kasan_mode); > > +/* Whether to enable vmalloc tagging. */ > +DEFINE_STATIC_KEY_FALSE(kasan_flag_vmalloc); > + > /* Whether to collect alloc/free stack traces. */ > DEFINE_STATIC_KEY_FALSE(kasan_flag_stacktrace); > > @@ -89,6 +99,23 @@ static int __init early_kasan_mode(char *arg) > } > early_param("kasan.mode", early_kasan_mode); > > +/* kasan.vmalloc=off/on */ > +static int __init early_kasan_flag_vmalloc(char *arg) > +{ > + if (!arg) > + return -EINVAL; > + > + if (!strcmp(arg, "off")) > + kasan_arg_vmalloc = KASAN_ARG_VMALLOC_OFF; > + else if (!strcmp(arg, "on")) > + kasan_arg_vmalloc = KASAN_ARG_VMALLOC_ON; > + else > + return -EINVAL; > + > + return 0; > +} > +early_param("kasan.vmalloc", early_kasan_flag_vmalloc); > + > /* kasan.stacktrace=off/on */ > static int __init early_kasan_flag_stacktrace(char *arg) > { > @@ -172,6 +199,18 @@ void __init kasan_init_hw_tags(void) > break; > } > > + switch (kasan_arg_vmalloc) { > + case KASAN_ARG_VMALLOC_DEFAULT: > + /* Default to enabling vmalloc tagging. */ > + fallthrough; > + case KASAN_ARG_VMALLOC_ON: > + static_branch_enable(&kasan_flag_vmalloc); > + break; > + case KASAN_ARG_VMALLOC_OFF: > + /* Do nothing, kasan_flag_vmalloc keeps its default value. */ > + break; > + } I think we should be setting the default when defining the static key (e.g. in this case it should be DEFINE_STATIC_KEY_TRUE), so that: - the _DEFAULT case is always empty; - the _ON case explicitly enables the static branch - the _OFF case explicitly disables the branch This way we'll only need to change DEFINE_STATIC_KEY_TRUE to DEFINE_STATIC_KEY_FALSE if we want to change the default, but we don't have to mess up with the rest of the code. Right now the switch statement is confusing, because the _OFF case refers to some "default" value, whereas the _DEFAULT one actively changes the state. I see that this code is copied from kasan_flag_stacktrace implementation, and my comment also applies there (but I don't insist on fixing that one right now).
On Tue, Dec 21, 2021 at 3:43 PM Alexander Potapenko <glider@google.com> wrote: > > > > > + switch (kasan_arg_vmalloc) { > > + case KASAN_ARG_VMALLOC_DEFAULT: > > + /* Default to enabling vmalloc tagging. */ > > + fallthrough; > > + case KASAN_ARG_VMALLOC_ON: > > + static_branch_enable(&kasan_flag_vmalloc); > > + break; > > + case KASAN_ARG_VMALLOC_OFF: > > + /* Do nothing, kasan_flag_vmalloc keeps its default value. */ > > + break; > > + } > > I think we should be setting the default when defining the static key > (e.g. in this case it should be DEFINE_STATIC_KEY_TRUE), so that: > - the _DEFAULT case is always empty; > - the _ON case explicitly enables the static branch > - the _OFF case explicitly disables the branch > This way we'll only need to change DEFINE_STATIC_KEY_TRUE to > DEFINE_STATIC_KEY_FALSE if we want to change the default, but we don't > have to mess up with the rest of the code. > Right now the switch statement is confusing, because the _OFF case > refers to some "default" value, whereas the _DEFAULT one actively > changes the state. > > I see that this code is copied from kasan_flag_stacktrace > implementation, and my comment also applies there (but I don't insist > on fixing that one right now). Will do in v5. Thanks!
diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c index 99230e666c1b..657b23cebe28 100644 --- a/mm/kasan/hw_tags.c +++ b/mm/kasan/hw_tags.c @@ -32,6 +32,12 @@ enum kasan_arg_mode { KASAN_ARG_MODE_ASYMM, }; +enum kasan_arg_vmalloc { + KASAN_ARG_VMALLOC_DEFAULT, + KASAN_ARG_VMALLOC_OFF, + KASAN_ARG_VMALLOC_ON, +}; + enum kasan_arg_stacktrace { KASAN_ARG_STACKTRACE_DEFAULT, KASAN_ARG_STACKTRACE_OFF, @@ -40,6 +46,7 @@ enum kasan_arg_stacktrace { static enum kasan_arg kasan_arg __ro_after_init; static enum kasan_arg_mode kasan_arg_mode __ro_after_init; +static enum kasan_arg_vmalloc kasan_arg_vmalloc __initdata; static enum kasan_arg_stacktrace kasan_arg_stacktrace __initdata; /* Whether KASAN is enabled at all. */ @@ -50,6 +57,9 @@ EXPORT_SYMBOL(kasan_flag_enabled); enum kasan_mode kasan_mode __ro_after_init; EXPORT_SYMBOL_GPL(kasan_mode); +/* Whether to enable vmalloc tagging. */ +DEFINE_STATIC_KEY_FALSE(kasan_flag_vmalloc); + /* Whether to collect alloc/free stack traces. */ DEFINE_STATIC_KEY_FALSE(kasan_flag_stacktrace); @@ -89,6 +99,23 @@ static int __init early_kasan_mode(char *arg) } early_param("kasan.mode", early_kasan_mode); +/* kasan.vmalloc=off/on */ +static int __init early_kasan_flag_vmalloc(char *arg) +{ + if (!arg) + return -EINVAL; + + if (!strcmp(arg, "off")) + kasan_arg_vmalloc = KASAN_ARG_VMALLOC_OFF; + else if (!strcmp(arg, "on")) + kasan_arg_vmalloc = KASAN_ARG_VMALLOC_ON; + else + return -EINVAL; + + return 0; +} +early_param("kasan.vmalloc", early_kasan_flag_vmalloc); + /* kasan.stacktrace=off/on */ static int __init early_kasan_flag_stacktrace(char *arg) { @@ -172,6 +199,18 @@ void __init kasan_init_hw_tags(void) break; } + switch (kasan_arg_vmalloc) { + case KASAN_ARG_VMALLOC_DEFAULT: + /* Default to enabling vmalloc tagging. */ + fallthrough; + case KASAN_ARG_VMALLOC_ON: + static_branch_enable(&kasan_flag_vmalloc); + break; + case KASAN_ARG_VMALLOC_OFF: + /* Do nothing, kasan_flag_vmalloc keeps its default value. */ + break; + } + switch (kasan_arg_stacktrace) { case KASAN_ARG_STACKTRACE_DEFAULT: /* Default to enabling stack trace collection. */ @@ -184,8 +223,9 @@ void __init kasan_init_hw_tags(void) break; } - pr_info("KernelAddressSanitizer initialized (hw-tags, mode=%s, stacktrace=%s)\n", + pr_info("KernelAddressSanitizer initialized (hw-tags, mode=%s, vmalloc=%s, stacktrace=%s)\n", kasan_mode_info(), + kasan_vmalloc_enabled() ? "on" : "off", kasan_stack_collection_enabled() ? "on" : "off"); } @@ -218,6 +258,9 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size, u8 tag; unsigned long redzone_start, redzone_size; + if (!kasan_vmalloc_enabled()) + return (void *)start; + if (!is_vmalloc_or_module_addr(start)) return (void *)start; diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h index 020f3e57a03f..49a5d5e2e948 100644 --- a/mm/kasan/kasan.h +++ b/mm/kasan/kasan.h @@ -12,6 +12,7 @@ #include <linux/static_key.h> #include "../slab.h" +DECLARE_STATIC_KEY_FALSE(kasan_flag_vmalloc); DECLARE_STATIC_KEY_FALSE(kasan_flag_stacktrace); enum kasan_mode { @@ -22,6 +23,11 @@ enum kasan_mode { extern enum kasan_mode kasan_mode __ro_after_init; +static inline bool kasan_vmalloc_enabled(void) +{ + return static_branch_likely(&kasan_flag_vmalloc); +} + static inline bool kasan_stack_collection_enabled(void) { return static_branch_unlikely(&kasan_flag_stacktrace);