This problem reported by Clement LE GOFFIC manifest when using CONFIG_KASAN_IN_VMALLOC and VMAP_STACK: https://lore.kernel.org/linux-arm-kernel/a1a1d062-f3a2-4d05-9836-3b098de9db6...
After some analysis it seems we are missing to sync the VMALLOC shadow memory in top level PGD to all CPUs.
Add some code to perform this sync, and the bug appears to go away.
As suggested by Ard, also perform a dummy read from the shadow memory of the new VMAP_STACK in the low level assembly.
Signed-off-by: Linus Walleij linus.walleij@linaro.org --- Changes in v3: - Collect Mark Rutlands ACK on patch 1 - Change the simplified assembly add r2, ip, lsr #n to the canonical add r2, r2, ip, lsr #n in patch 2. - Link to v2: https://lore.kernel.org/r/20241016-arm-kasan-vmalloc-crash-v2-0-0a52fd086eef...
Changes in v2: - Implement the two helper functions suggested by Russell making the KASAN PGD copying less messy. - Link to v1: https://lore.kernel.org/r/20241015-arm-kasan-vmalloc-crash-v1-0-dbb23592ca83...
--- Linus Walleij (2): ARM: ioremap: Sync PGDs for VMALLOC shadow ARM: entry: Do a dummy read from VMAP shadow
arch/arm/kernel/entry-armv.S | 8 ++++++++ arch/arm/mm/ioremap.c | 25 +++++++++++++++++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) --- base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc change-id: 20241015-arm-kasan-vmalloc-crash-fcbd51416457
Best regards,
When sync:ing the VMALLOC area to other CPUs, make sure to also sync the KASAN shadow memory for the VMALLOC area, so that we don't get stale entries for the shadow memory in the top level PGD.
Since we are now copying PGDs in two instances, create a helper function named memcpy_pgd() to do the actual copying, and create a helper to map the addresses of VMALLOC_START and VMALLOC_END into the corresponding shadow memory.
Cc: stable@vger.kernel.org Fixes: 565cbaad83d8 ("ARM: 9202/1: kasan: support CONFIG_KASAN_VMALLOC") Link: https://lore.kernel.org/linux-arm-kernel/a1a1d062-f3a2-4d05-9836-3b098de9db6... Reported-by: Clement LE GOFFIC clement.legoffic@foss.st.com Suggested-by: Mark Rutland mark.rutland@arm.com Suggested-by: Russell King (Oracle) linux@armlinux.org.uk Acked-by: Mark Rutland mark.rutland@arm.com Signed-off-by: Linus Walleij linus.walleij@linaro.org --- arch/arm/mm/ioremap.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c index 794cfea9f9d4..94586015feed 100644 --- a/arch/arm/mm/ioremap.c +++ b/arch/arm/mm/ioremap.c @@ -23,6 +23,7 @@ */ #include <linux/module.h> #include <linux/errno.h> +#include <linux/kasan.h> #include <linux/mm.h> #include <linux/vmalloc.h> #include <linux/io.h> @@ -115,16 +116,32 @@ int ioremap_page(unsigned long virt, unsigned long phys, } EXPORT_SYMBOL(ioremap_page);
+static unsigned long arm_kasan_mem_to_shadow(unsigned long addr) +{ + return (unsigned long)kasan_mem_to_shadow((void *)addr); +} + +static void memcpy_pgd(struct mm_struct *mm, unsigned long start, + unsigned long end) +{ + memcpy(pgd_offset(mm, start), pgd_offset_k(start), + sizeof(pgd_t) * (pgd_index(end) - pgd_index(start))); +} + void __check_vmalloc_seq(struct mm_struct *mm) { int seq;
do { seq = atomic_read(&init_mm.context.vmalloc_seq); - memcpy(pgd_offset(mm, VMALLOC_START), - pgd_offset_k(VMALLOC_START), - sizeof(pgd_t) * (pgd_index(VMALLOC_END) - - pgd_index(VMALLOC_START))); + memcpy_pgd(mm, VMALLOC_START, VMALLOC_END); + if (IS_ENABLED(CONFIG_KASAN_VMALLOC)) { + unsigned long start = + arm_kasan_mem_to_shadow(VMALLOC_START); + unsigned long end = + arm_kasan_mem_to_shadow(VMALLOC_END); + memcpy_pgd(mm, start, end); + } /* * Use a store-release so that other CPUs that observe the * counter's new value are guaranteed to see the results of the
On 10/17/24 14:59, Linus Walleij wrote:
[...] +static unsigned long arm_kasan_mem_to_shadow(unsigned long addr) +{
- return (unsigned long)kasan_mem_to_shadow((void *)addr);
+}
`kasan_mem_to_shadow` function symbol is only exported with : CONFIG_KASAN_GENERIC or defined(CONFIG_KASAN_SW_TAGS) from kasan.h
To me, the if condition you added below should be expanded with those two macros.
[...] void __check_vmalloc_seq(struct mm_struct *mm) { int seq; do { seq = atomic_read(&init_mm.context.vmalloc_seq);
memcpy(pgd_offset(mm, VMALLOC_START),
pgd_offset_k(VMALLOC_START),
sizeof(pgd_t) * (pgd_index(VMALLOC_END) -
pgd_index(VMALLOC_START)));
memcpy_pgd(mm, VMALLOC_START, VMALLOC_END);
if (IS_ENABLED(CONFIG_KASAN_VMALLOC)) {
unsigned long start =
arm_kasan_mem_to_shadow(VMALLOC_START);
unsigned long end =
arm_kasan_mem_to_shadow(VMALLOC_END);
memcpy_pgd(mm, start, end);
/*}
- Use a store-release so that other CPUs that observe the
- counter's new value are guaranteed to see the results of the
Otherwise it compiles with KASAN enabled, I am running some tests with your patches.
Regards,
Clément
On Thu, Oct 17, 2024 at 4:22 PM Clement LE GOFFIC clement.legoffic@foss.st.com wrote:
On 10/17/24 14:59, Linus Walleij wrote:
[...]
+static unsigned long arm_kasan_mem_to_shadow(unsigned long addr) +{
return (unsigned long)kasan_mem_to_shadow((void *)addr);
+}
`kasan_mem_to_shadow` function symbol is only exported with : CONFIG_KASAN_GENERIC or defined(CONFIG_KASAN_SW_TAGS) from kasan.h
To me, the if condition you added below should be expanded with those two macros.
(...)
if (IS_ENABLED(CONFIG_KASAN_VMALLOC)) {
Let's check this with the KASAN authors, I think looking for CONFIG_KASAN_VMALLOC should be enough as it is inside the if KASAN clause in lib/Kconfig.kasan, i.e. the symbol KASAN must be enabled for CONFIG_KASAN_VMALLOC to be enabled, and if KASAN is enabled then either KASAN_GENERIC or KASAN_SW_TAGS is enabled (the third option KASAN_HW_TAGS, also known as memory tagging is only available on ARM64 and we are not ARM64.)
But I might be wrong! Kconfig regularly bites me in the foot...
Yours, Linus Walleij
When switching task, in addition to a dummy read from the new VMAP stack, also do a dummy read from the VMAP stack's corresponding KASAN shadow memory to sync things up in the new MM context.
Cc: stable@vger.kernel.org Fixes: a1c510d0adc6 ("ARM: implement support for vmap'ed stacks") Link: https://lore.kernel.org/linux-arm-kernel/a1a1d062-f3a2-4d05-9836-3b098de9db6... Reported-by: Clement LE GOFFIC clement.legoffic@foss.st.com Suggested-by: Ard Biesheuvel ardb@kernel.org Signed-off-by: Linus Walleij linus.walleij@linaro.org --- arch/arm/kernel/entry-armv.S | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S index 1dfae1af8e31..ef6a657c8d13 100644 --- a/arch/arm/kernel/entry-armv.S +++ b/arch/arm/kernel/entry-armv.S @@ -25,6 +25,7 @@ #include <asm/tls.h> #include <asm/system_info.h> #include <asm/uaccess-asm.h> +#include <asm/kasan_def.h>
#include "entry-header.S" #include <asm/probes.h> @@ -561,6 +562,13 @@ ENTRY(__switch_to) @ entries covering the vmalloc region. @ ldr r2, [ip] +#ifdef CONFIG_KASAN_VMALLOC + @ Also dummy read from the KASAN shadow memory for the new stack if we + @ are using KASAN + mov_l r2, KASAN_SHADOW_OFFSET + add r2, r2, ip, lsr #KASAN_SHADOW_SCALE_SHIFT + ldr r2, [r2] +#endif #endif
@ When CONFIG_THREAD_INFO_IN_TASK=n, the update of SP itself is what
linux-stable-mirror@lists.linaro.org