[PATCH mm 0/2] kasan: a few HW_TAGS fixes

[PATCH mm 0/2] kasan: a few HW_TAGS fixes

[Andrey Konovalov]

Hi Andrew,

Could you please squash the first one into
"kasan: add and integrate kasan boot parameters".

And instead of applying the second one, it's better to just drop
"kasan, arm64: don't allow SW_TAGS with ARM64_MTE".

Thanks!

Andrey Konovalov (2):
  kasan: don't use read-only static keys
  Revert "kasan, arm64: don't allow SW_TAGS with ARM64_MTE"

 arch/arm64/Kconfig | 2 +-
 mm/kasan/hw_tags.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.29.2.576.ga3fc446d84-goog

[PATCH mm 1/2] kasan: don't use read-only static keys

[Andrey Konovalov]

__ro_after_init static keys are incompatible with usage in loadable kernel
modules and cause crashes. Don't use those, use normal static keys.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

---

This fix can be squashed into
"kasan: add and integrate kasan boot parameters".

---
 mm/kasan/hw_tags.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
index c91f2c06ecb5..55bd6f09c70f 100644
--- a/mm/kasan/hw_tags.c
+++ b/mm/kasan/hw_tags.c
@@ -43,11 +43,11 @@ static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init;
 static enum kasan_arg_fault kasan_arg_fault __ro_after_init;
 
 /* Whether KASAN is enabled at all. */
-DEFINE_STATIC_KEY_FALSE_RO(kasan_flag_enabled);
+DEFINE_STATIC_KEY_FALSE(kasan_flag_enabled);
 EXPORT_SYMBOL(kasan_flag_enabled);
 
 /* Whether to collect alloc/free stack traces. */
-DEFINE_STATIC_KEY_FALSE_RO(kasan_flag_stacktrace);
+DEFINE_STATIC_KEY_FALSE(kasan_flag_stacktrace);
 
 /* Whether panic or disable tag checking on fault. */
 bool kasan_flag_panic __ro_after_init;
-- 
2.29.2.576.ga3fc446d84-goog

[PATCH mm 2/2] Revert "kasan, arm64: don't allow SW_TAGS with ARM64_MTE"

[Andrey Konovalov]

This reverts "kasan, arm64: don't allow SW_TAGS with ARM64_MTE".

In earlier versions on the hardware tag-based KASAN patchset in-kernel
MTE used to be always enabled when CONFIG_ARM64_MTE is on. This caused
conflicts with the software tag-based KASAN mode.

This is no logner the case: in-kernel MTE is never enabled unless the
CONFIG_KASAN_HW_TAGS is enabled, so there are no more conflicts with
CONFIG_KASAN_SW_TAGS.

Allow CONFIG_KASAN_SW_TAGS to be enabled even when CONFIG_ARM64_MTE is
enabled.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6fefab9041d8..62a7668976a2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -135,7 +135,7 @@ config ARM64
 	select HAVE_ARCH_JUMP_LABEL
 	select HAVE_ARCH_JUMP_LABEL_RELATIVE
 	select HAVE_ARCH_KASAN if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
-	select HAVE_ARCH_KASAN_SW_TAGS if (HAVE_ARCH_KASAN && !ARM64_MTE)
+	select HAVE_ARCH_KASAN_SW_TAGS if HAVE_ARCH_KASAN
 	select HAVE_ARCH_KASAN_HW_TAGS if (HAVE_ARCH_KASAN && ARM64_MTE)
 	select HAVE_ARCH_KFENCE
 	select HAVE_ARCH_KGDB
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH mm 1/2] kasan: don't use read-only static keys

[Marco Elver]

On Wed, 9 Dec 2020 at 19:24, Andrey Konovalov <andreyknvl@google.com> wrote:
> __ro_after_init static keys are incompatible with usage in loadable kernel
> modules and cause crashes. Don't use those, use normal static keys.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Reviewed-by: Marco Elver <elver@google.com>

> ---
>
> This fix can be squashed into
> "kasan: add and integrate kasan boot parameters".
>
> ---
>  mm/kasan/hw_tags.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> index c91f2c06ecb5..55bd6f09c70f 100644
> --- a/mm/kasan/hw_tags.c
> +++ b/mm/kasan/hw_tags.c
> @@ -43,11 +43,11 @@ static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init;
>  static enum kasan_arg_fault kasan_arg_fault __ro_after_init;
>
>  /* Whether KASAN is enabled at all. */
> -DEFINE_STATIC_KEY_FALSE_RO(kasan_flag_enabled);
> +DEFINE_STATIC_KEY_FALSE(kasan_flag_enabled);

Side-node: This appears to be just a bad interface; I think the macro
DEFINE_STATIC_KEY_FALSE_RO() is error-prone, if it can't be guaranteed
that this is always safe, since the presence of the macro encourages
its use and we'll inevitably run into this problem again.

>  EXPORT_SYMBOL(kasan_flag_enabled);

DEFINE_STATIC_KEY_FALSE_RO() + EXPORT_SYMBOL() is an immediate bug.
Given its use has not increased substantially since its introduction,
it may be safer to consider its removal.

Thanks,
-- Marco

Re: [PATCH mm 2/2] Revert "kasan, arm64: don't allow SW_TAGS with ARM64_MTE"

[Marco Elver]

On Wed, 9 Dec 2020 at 19:24, Andrey Konovalov <andreyknvl@google.com> wrote:
>
> This reverts "kasan, arm64: don't allow SW_TAGS with ARM64_MTE".
>
> In earlier versions on the hardware tag-based KASAN patchset in-kernel
> MTE used to be always enabled when CONFIG_ARM64_MTE is on. This caused
> conflicts with the software tag-based KASAN mode.
>
> This is no logner the case: in-kernel MTE is never enabled unless the
> CONFIG_KASAN_HW_TAGS is enabled, so there are no more conflicts with
> CONFIG_KASAN_SW_TAGS.
>
> Allow CONFIG_KASAN_SW_TAGS to be enabled even when CONFIG_ARM64_MTE is
> enabled.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Reviewed-by: Marco Elver <elver@google.com>


> ---
>  arch/arm64/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 6fefab9041d8..62a7668976a2 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -135,7 +135,7 @@ config ARM64
>         select HAVE_ARCH_JUMP_LABEL
>         select HAVE_ARCH_JUMP_LABEL_RELATIVE
>         select HAVE_ARCH_KASAN if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
> -       select HAVE_ARCH_KASAN_SW_TAGS if (HAVE_ARCH_KASAN && !ARM64_MTE)
> +       select HAVE_ARCH_KASAN_SW_TAGS if HAVE_ARCH_KASAN
>         select HAVE_ARCH_KASAN_HW_TAGS if (HAVE_ARCH_KASAN && ARM64_MTE)
>         select HAVE_ARCH_KFENCE
>         select HAVE_ARCH_KGDB
> --
> 2.29.2.576.ga3fc446d84-goog
>

Re: [PATCH mm 1/2] kasan: don't use read-only static keys

[Kees Cook]

On Wed, Dec 09, 2020 at 07:49:36PM +0100, Marco Elver wrote:
> On Wed, 9 Dec 2020 at 19:24, Andrey Konovalov <andreyknvl@google.com> wrote:
> > __ro_after_init static keys are incompatible with usage in loadable kernel
> > modules and cause crashes. Don't use those, use normal static keys.
> >
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> 
> Reviewed-by: Marco Elver <elver@google.com>
> 
> > ---
> >
> > This fix can be squashed into
> > "kasan: add and integrate kasan boot parameters".
> >
> > ---
> >  mm/kasan/hw_tags.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> > index c91f2c06ecb5..55bd6f09c70f 100644
> > --- a/mm/kasan/hw_tags.c
> > +++ b/mm/kasan/hw_tags.c
> > @@ -43,11 +43,11 @@ static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init;
> >  static enum kasan_arg_fault kasan_arg_fault __ro_after_init;
> >
> >  /* Whether KASAN is enabled at all. */
> > -DEFINE_STATIC_KEY_FALSE_RO(kasan_flag_enabled);
> > +DEFINE_STATIC_KEY_FALSE(kasan_flag_enabled);
> 
> Side-node: This appears to be just a bad interface; I think the macro
> DEFINE_STATIC_KEY_FALSE_RO() is error-prone, if it can't be guaranteed
> that this is always safe, since the presence of the macro encourages
> its use and we'll inevitably run into this problem again.
> 
> >  EXPORT_SYMBOL(kasan_flag_enabled);
> 
> DEFINE_STATIC_KEY_FALSE_RO() + EXPORT_SYMBOL() is an immediate bug.
> Given its use has not increased substantially since its introduction,
> it may be safer to consider its removal.

Right -- it seems the export is the problem, not the RO-ness. What is
actually trying to change the flag after __init?

-- 
Kees Cook

Re: [PATCH mm 1/2] kasan: don't use read-only static keys

[Marco Elver]

On Wed, 9 Dec 2020 at 19:57, Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Dec 09, 2020 at 07:49:36PM +0100, Marco Elver wrote:
> > On Wed, 9 Dec 2020 at 19:24, Andrey Konovalov <andreyknvl@google.com> wrote:
> > > __ro_after_init static keys are incompatible with usage in loadable kernel
> > > modules and cause crashes. Don't use those, use normal static keys.
> > >
> > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> >
> > Reviewed-by: Marco Elver <elver@google.com>
> >
> > > ---
> > >
> > > This fix can be squashed into
> > > "kasan: add and integrate kasan boot parameters".
> > >
> > > ---
> > >  mm/kasan/hw_tags.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> > > index c91f2c06ecb5..55bd6f09c70f 100644
> > > --- a/mm/kasan/hw_tags.c
> > > +++ b/mm/kasan/hw_tags.c
> > > @@ -43,11 +43,11 @@ static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init;
> > >  static enum kasan_arg_fault kasan_arg_fault __ro_after_init;
> > >
> > >  /* Whether KASAN is enabled at all. */
> > > -DEFINE_STATIC_KEY_FALSE_RO(kasan_flag_enabled);
> > > +DEFINE_STATIC_KEY_FALSE(kasan_flag_enabled);
> >
> > Side-node: This appears to be just a bad interface; I think the macro
> > DEFINE_STATIC_KEY_FALSE_RO() is error-prone, if it can't be guaranteed
> > that this is always safe, since the presence of the macro encourages
> > its use and we'll inevitably run into this problem again.
> >
> > >  EXPORT_SYMBOL(kasan_flag_enabled);
> >
> > DEFINE_STATIC_KEY_FALSE_RO() + EXPORT_SYMBOL() is an immediate bug.
> > Given its use has not increased substantially since its introduction,
> > it may be safer to consider its removal.
>
> Right -- it seems the export is the problem, not the RO-ness. What is
> actually trying to change the flag after __init?

It seems to want to add it to a list on module loads:
https://lore.kernel.org/lkml/20201208125129.GY2414@hirez.programming.kicks-ass.net/

-- Marco

Re: [PATCH mm 2/2] Revert "kasan, arm64: don't allow SW_TAGS with ARM64_MTE"

[Andrew Morton]

On Wed, 9 Dec 2020 19:51:05 +0100 Marco Elver <elver@google.com> wrote:

> > This is no logner the case: in-kernel MTE is never enabled unless the
> > CONFIG_KASAN_HW_TAGS is enabled, so there are no more conflicts with
> > CONFIG_KASAN_SW_TAGS.
> >
> > Allow CONFIG_KASAN_SW_TAGS to be enabled even when CONFIG_ARM64_MTE is
> > enabled.
> >
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> 
> Reviewed-by: Marco Elver <elver@google.com>

Thanks.  I simply dropped
kasan-arm64-dont-allow-sw_tags-with-arm64_mte.patch.