On 11/16/20 12:19 PM, Dmitry Vyukov wrote:

On Mon, Nov 16, 2020 at 1:16 PM Catalin Marinas [off-list ref] wrote:

quoted

On Mon, Nov 16, 2020 at 12:50:00PM +0100, Marco Elver wrote:

quoted

On Mon, 16 Nov 2020 at 11:59, Dmitry Vyukov [off-list ref] wrote:

quoted

On Thu, Oct 29, 2020 at 8:57 PM 'Andrey Konovalov' via kasan-dev
[off-list ref] wrote:

quoted

On Tue, Oct 27, 2020 at 1:44 PM Dmitry Vyukov [off-list ref] wrote:

quoted

On Thu, Oct 22, 2020 at 3:19 PM Andrey Konovalov [off-list ref] wrote:

quoted

There's a config option CONFIG_KASAN_STACK that has to be enabled for
KASAN to use stack instrumentation and perform validity checks for
stack variables.

There's no need to unpoison stack when CONFIG_KASAN_STACK is not enabled.
Only call kasan_unpoison_task_stack[_below]() when CONFIG_KASAN_STACK is
enabled.

Signed-off-by: Andrey Konovalov <redacted>
Link: https://linux-review.googlesource.com/id/If8a891e9fe01ea543e00b576852685afec0887e3
---
 arch/arm64/kernel/sleep.S        |  2 +-
 arch/x86/kernel/acpi/wakeup_64.S |  2 +-
 include/linux/kasan.h            | 10 ++++++----
 mm/kasan/common.c                |  2 ++
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index ba40d57757d6..bdadfa56b40e 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S

@@ -133,7 +133,7 @@ SYM_FUNC_START(_cpu_resume)
         */
        bl      cpu_do_resume

-#ifdef CONFIG_KASAN
+#if defined(CONFIG_KASAN) && CONFIG_KASAN_STACK
        mov     x0, sp
        bl      kasan_unpoison_task_stack_below
 #endif

diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
index c8daa92f38dc..5d3a0b8fd379 100644
--- a/arch/x86/kernel/acpi/wakeup_64.S
+++ b/arch/x86/kernel/acpi/wakeup_64.S

@@ -112,7 +112,7 @@ SYM_FUNC_START(do_suspend_lowlevel)
        movq    pt_regs_r14(%rax), %r14
        movq    pt_regs_r15(%rax), %r15

-#ifdef CONFIG_KASAN
+#if defined(CONFIG_KASAN) && CONFIG_KASAN_STACK
        /*
         * The suspend path may have poisoned some areas deeper in the stack,
         * which we now need to unpoison.

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 3f3f541e5d5f..7be9fb9146ac 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h

@@ -68,8 +68,6 @@ static inline void kasan_disable_current(void) {}

 void kasan_unpoison_memory(const void *address, size_t size);

-void kasan_unpoison_task_stack(struct task_struct *task);
-
 void kasan_alloc_pages(struct page *page, unsigned int order);
 void kasan_free_pages(struct page *page, unsigned int order);

@@ -114,8 +112,6 @@ void kasan_restore_multi_shot(bool enabled);

 static inline void kasan_unpoison_memory(const void *address, size_t size) {}

-static inline void kasan_unpoison_task_stack(struct task_struct *task) {}
-
 static inline void kasan_alloc_pages(struct page *page, unsigned int order) {}
 static inline void kasan_free_pages(struct page *page, unsigned int order) {}

@@ -167,6 +163,12 @@ static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }

 #endif /* CONFIG_KASAN */

+#if defined(CONFIG_KASAN) && CONFIG_KASAN_STACK

&& defined(CONFIG_KASAN_STACK) for consistency

CONFIG_KASAN_STACK is different from other KASAN configs. It's always
defined, and its value is what controls whether stack instrumentation
is enabled.

Not sure why we did this instead of the following, but okay.

 config KASAN_STACK
-       int
-       default 1 if KASAN_STACK_ENABLE || CC_IS_GCC
-       default 0
+       bool
+       default y if KASAN_STACK_ENABLE || CC_IS_GCC
+       default n

I wondered the same, but then looking at scripts/Makefile.kasan I
think it's because we directly pass it to the compiler:
    ...
    $(call cc-param,asan-stack=$(CONFIG_KASAN_STACK)) \
    ...

Try this instead:

      $(call cc-param,asan-stack=$(if $(CONFIG_KASAN_STACK),1,0)) \

We could have just 1 config instead of 2 as well.
For gcc we could do no prompt and default value y, and for clang --
prompt and default value n. I think it should do what we need.

I agree with Catalin's proposal since it should simplify things.

Nit: 'default n' is the default hence I do not think it should be required
explicitly.

-- 
Regards,
Vincenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel