[PATCH 0/2] Fix KASAN crash when using KASAN_VMALLOC

[PATCH 0/2] Fix KASAN crash when using KASAN_VMALLOC

[Linus Walleij]

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-3b098de9db6d@foss.st.com/

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>
---
Linus Walleij (2):
      ARM: ioremap: Flush 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        | 7 +++++++
 2 files changed, 15 insertions(+)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20241015-arm-kasan-vmalloc-crash-fcbd51416457

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH 1/2] ARM: ioremap: Flush PGDs for VMALLOC shadow

[Linus Walleij]

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.

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-3b098de9db6d@foss.st.com/
Reported-by: Clement LE GOFFIC <clement.legoffic@foss.st.com>
Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/mm/ioremap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 794cfea9f9d4..449f1f04814c 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>
@@ -125,6 +126,12 @@ void __check_vmalloc_seq(struct mm_struct *mm)
 		       pgd_offset_k(VMALLOC_START),
 		       sizeof(pgd_t) * (pgd_index(VMALLOC_END) -
 					pgd_index(VMALLOC_START)));
+		if (IS_ENABLED(CONFIG_KASAN_VMALLOC)) {
+			memcpy(pgd_offset(mm, (unsigned long)kasan_mem_to_shadow((void *)VMALLOC_START)),
+			       pgd_offset_k((unsigned long)kasan_mem_to_shadow((void *)VMALLOC_START)),
+			       sizeof(pgd_t) * (pgd_index((unsigned long)kasan_mem_to_shadow((void *)VMALLOC_END)) -
+						pgd_index((unsigned long)kasan_mem_to_shadow((void *)VMALLOC_START))));
+		}
 		/*
 		 * Use a store-release so that other CPUs that observe the
 		 * counter's new value are guaranteed to see the results of the

-- 
2.46.2

Re: [PATCH 1/2] ARM: ioremap: Flush PGDs for VMALLOC shadow

[Russell King (Oracle)]

On Tue, Oct 15, 2024 at 11:37:14PM +0200, Linus Walleij wrote:
> @@ -125,6 +126,12 @@ void __check_vmalloc_seq(struct mm_struct *mm)
>  		       pgd_offset_k(VMALLOC_START),
>  		       sizeof(pgd_t) * (pgd_index(VMALLOC_END) -
>  					pgd_index(VMALLOC_START)));
> +		if (IS_ENABLED(CONFIG_KASAN_VMALLOC)) {
> +			memcpy(pgd_offset(mm, (unsigned long)kasan_mem_to_shadow((void *)VMALLOC_START)),
> +			       pgd_offset_k((unsigned long)kasan_mem_to_shadow((void *)VMALLOC_START)),
> +			       sizeof(pgd_t) * (pgd_index((unsigned long)kasan_mem_to_shadow((void *)VMALLOC_END)) -
> +						pgd_index((unsigned long)kasan_mem_to_shadow((void *)VMALLOC_START))));

Maybe the following would be more readable:

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)));
}

		seq = ...;
		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);
> +		}

?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/2] ARM: ioremap: Flush PGDs for VMALLOC shadow

[Ard Biesheuvel]

On Tue, 15 Oct 2024 at 23:37, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> 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.
>
> 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-3b098de9db6d@foss.st.com/
> Reported-by: Clement LE GOFFIC <clement.legoffic@foss.st.com>
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  arch/arm/mm/ioremap.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index 794cfea9f9d4..449f1f04814c 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>
> @@ -125,6 +126,12 @@ void __check_vmalloc_seq(struct mm_struct *mm)
>                        pgd_offset_k(VMALLOC_START),
>                        sizeof(pgd_t) * (pgd_index(VMALLOC_END) -
>                                         pgd_index(VMALLOC_START)));
> +               if (IS_ENABLED(CONFIG_KASAN_VMALLOC)) {
> +                       memcpy(pgd_offset(mm, (unsigned long)kasan_mem_to_shadow((void *)VMALLOC_START)),
> +                              pgd_offset_k((unsigned long)kasan_mem_to_shadow((void *)VMALLOC_START)),
> +                              sizeof(pgd_t) * (pgd_index((unsigned long)kasan_mem_to_shadow((void *)VMALLOC_END)) -
> +                                               pgd_index((unsigned long)kasan_mem_to_shadow((void *)VMALLOC_START))));
> +               }

+1 to Russell's suggestion to change this wall of text into something legible.

Then, there is another part to this: in arch/arm/kernel/traps.c, we
have the following code

void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
{
    if (start < VMALLOC_END && end > VMALLOC_START)
        atomic_inc_return_release(&init_mm.context.vmalloc_seq);
}

where we only bump vmalloc_seq if the updated region overlaps with the
vmalloc region, so this will need a similar treatment afaict.

Re: [PATCH 1/2] ARM: ioremap: Flush PGDs for VMALLOC shadow

[Linus Walleij]

On Wed, Oct 16, 2024 at 1:33 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> > @@ -125,6 +126,12 @@ void __check_vmalloc_seq(struct mm_struct *mm)
(...)
> Then, there is another part to this: in arch/arm/kernel/traps.c, we
> have the following code
>
> void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
> {
>     if (start < VMALLOC_END && end > VMALLOC_START)
>         atomic_inc_return_release(&init_mm.context.vmalloc_seq);
> }
>
> where we only bump vmalloc_seq if the updated region overlaps with the
> vmalloc region, so this will need a similar treatment afaict.

Not really, right? We bump init_mm.context.vmalloc_seq if the address
overlaps the entire vmalloc area.

Then the previously patched __check_vmalloc_seq() will check that
atomic counter and copy the PGD entries, and with the code in this
patch it will also copy (sync) the corresponding shadow memory
at that point.

Yours,
Linus Walleij

Re: [PATCH 1/2] ARM: ioremap: Flush PGDs for VMALLOC shadow

[Ard Biesheuvel]

On Wed, 16 Oct 2024 at 20:38, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Oct 16, 2024 at 1:33 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > > @@ -125,6 +126,12 @@ void __check_vmalloc_seq(struct mm_struct *mm)
> (...)
> > Then, there is another part to this: in arch/arm/kernel/traps.c, we
> > have the following code
> >
> > void arch_sync_kernel_mappings(unsigned long start, unsigned long end)
> > {
> >     if (start < VMALLOC_END && end > VMALLOC_START)
> >         atomic_inc_return_release(&init_mm.context.vmalloc_seq);
> > }
> >
> > where we only bump vmalloc_seq if the updated region overlaps with the
> > vmalloc region, so this will need a similar treatment afaict.
>
> Not really, right? We bump init_mm.context.vmalloc_seq if the address
> overlaps the entire vmalloc area.
>
> Then the previously patched __check_vmalloc_seq() will check that
> atomic counter and copy the PGD entries, and with the code in this
> patch it will also copy (sync) the corresponding shadow memory
> at that point.
>

Yes, so we rely on the fact that changes to the vmalloc area and
changes to the associated shadow mappings always occur in combination,
right?

I think that should probably be safe, but we have to be sure.

Re: [PATCH 1/2] ARM: ioremap: Flush PGDs for VMALLOC shadow

[Linus Walleij]

On Wed, Oct 16, 2024 at 8:50 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> Yes, so we rely on the fact that changes to the vmalloc area and
> changes to the associated shadow mappings always occur in combination,
> right?

Yes otherwise it is pretty much the definition of a KASAN violation.

Mostly it "just works" because all low-level operations emitted by the
compiler and all memcpy() (etc) are patched to do any memory access
in tandem, this vmalloc_seq-thing was a big confusion for me.

I'll send out the revised patches so people can test!

Yours,
Linus Walleij

[PATCH 2/2] ARM: entry: Do a dummy read from VMAP shadow

[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-3b098de9db6d@foss.st.com/
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..12a4040a04ff 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, 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

-- 
2.46.2