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
__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(-)
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
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(-)
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
On Wed, Dec 09, 2020 at 07:49:36PM +0100, Marco Elver wrote:
> On Wed, 9 Dec 2020 at 19:24, Andrey Konovalov <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Marco Elver <[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(-)
> >
> > 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
On Wed, 9 Dec 2020 at 19:57, Kees Cook <[email protected]> wrote:
>
> On Wed, Dec 09, 2020 at 07:49:36PM +0100, Marco Elver wrote:
> > On Wed, 9 Dec 2020 at 19:24, Andrey Konovalov <[email protected]> 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 <[email protected]>
> >
> > Reviewed-by: Marco Elver <[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(-)
> > >
> > > 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/[email protected]/
-- Marco
On Wed, 9 Dec 2020 at 19:24, Andrey Konovalov <[email protected]> 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 <[email protected]>
Reviewed-by: Marco Elver <[email protected]>
> ---
> 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
>
On Wed, 9 Dec 2020 at 19:24, Andrey Konovalov <[email protected]> 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 <[email protected]>
Reviewed-by: Marco Elver <[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(-)
>
> 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
On Wed, 9 Dec 2020 19:51:05 +0100 Marco Elver <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Marco Elver <[email protected]>
Thanks. I simply dropped
kasan-arm64-dont-allow-sw_tags-with-arm64_mte.patch.