Skip to contentSkip to conversation

kasan: a few HW_TAGS fixes

PATCH In review Andrey Konovalov 4 participants 8 messages Dec 9, 2020

Subsystems (3)
Memory management arm64 KASAN
lore
Share & tools
lore

Andrey Konovalov

author
#0 lore raw

cover kasan: a few HW_TAGS fixes

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(-)
Signature · 1 line
2.29.2.576.ga3fc446d84-goog
author
#1 lore raw

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

__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 <[email protected]>

---

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(-)
mm/kasan/hw_tags.c +2 -2
index c91f2c06ecb5..55bd6f09c70f 100644
@@ -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;
Signature · 1 line
2.29.2.576.ga3fc446d84-goog
Reviewed-by on v1 1/2
#2 lore raw

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

Quoted 4 lines · 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 <[email protected]>
Reviewed-by Marco Elver <[email protected]>
Quoted 14 lines

---

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(-)
mm/kasan/hw_tags.c +0 -0
index c91f2c06ecb5..55bd6f09c70f 100644

mm/kasan/hw_tags.c static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init; · line 46

Quoted 5 lines
mm/kasan/hw_tags.c +1 -1
@@ -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.

mm/kasan/hw_tags.c static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init; · line 47

Quoted 1 line
mm/kasan/hw_tags.c +0 -0
 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

#3 lore raw

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

Quoted 33 lines · Marco Elver
Quoted 4 lines · 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 <[email protected]>
Reviewed-by Marco Elver <[email protected]>
Quoted 14 lines

---

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(-)
mm/kasan/hw_tags.c +0 -0
index c91f2c06ecb5..55bd6f09c70f 100644

mm/kasan/hw_tags.c static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init; · line 46

Quoted 5 lines
mm/kasan/hw_tags.c +1 -1
@@ -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.

Quoted 5 lines

mm/kasan/hw_tags.c static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init; · line 47

Quoted 1 line
mm/kasan/hw_tags.c +0 -0
 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?

Signature · 1 line
Kees Cook
#4 lore raw

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

Quoted 35 lines · Kees Cook
Quoted 33 lines · Marco Elver
Quoted 4 lines · 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 <[email protected]>
Reviewed-by Marco Elver <[email protected]>
Quoted 14 lines

---

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(-)
mm/kasan/hw_tags.c +0 -0
index c91f2c06ecb5..55bd6f09c70f 100644

mm/kasan/hw_tags.c static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init; · line 46

Quoted 5 lines
mm/kasan/hw_tags.c +1 -1
@@ -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.

Quoted 8 lines
Quoted 5 lines

mm/kasan/hw_tags.c static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init; · line 47

Quoted 1 line
mm/kasan/hw_tags.c +0 -0
 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/[email protected]/

-- Marco

author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/Kconfig +1 -1
index 6fefab9041d8..62a7668976a2 100644
@@ -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
Signature · 1 line
2.29.2.576.ga3fc446d84-goog
Reviewed-by on v1 2/2
#6 lore raw

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

Quoted 7 lines · 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.

Quoted 8 lines

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 <[email protected]>
Reviewed-by Marco Elver <[email protected]>
Quoted 20 lines
---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/Kconfig +1 -2
index 6fefab9041d8..62a7668976a2 100644
@@ -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

maintainer
#7 lore raw

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

Quoted 10 lines · On Wed, 9 Dec 2020 19:51:05 +0100 Marco Elver <[email protected]> wrote:
Quoted 8 lines

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 <[email protected]>
Reviewed-by Marco Elver <[email protected]>

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