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

[PATCH v2 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>
---
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@linaro.org

---
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,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH v2 1/2] ARM: ioremap: Sync 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.

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

-- 
2.46.2

[PATCH v2 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

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

[Clement LE GOFFIC]

On 10/16/24 21:15, Linus Walleij wrote:
> 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

Hello Linus,

While ARM TRM says that if Rd is the same of Rn then it can be omitted, 
such syntax causes error on my build.
Looking around for such syntax in the kernel, this line should be :
add	r2, r2, ip, lsr #KASAN_SHADOW_SCALE_SHIFT

Regards,

Clement

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

[Mark Rutland]

On Wed, Oct 16, 2024 at 09:15:21PM +0200, Linus Walleij 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.
> 
> 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-3b098de9db6d@foss.st.com/
> 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>
> 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);
> +		}

This looks good; FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

As a separate thing, I believe we also need to use atomic_read_acquire()
for the reads of vmalloc_seq to pair with the atomic_*_release() on each
update.

Otherwise, this can be reordered, e.g.

	do {
		memcpy_pgd(...);
		seq = atomic_read(&init_mm.context.vmalloc_seq);
		atomic_set_release(&mm->context.vmalloc_seq, seq);
	} while (seq != atomic_read(&init_mm.context.vmalloc_seq)

... and we might fail to copy the relevant table entries from init_mm,
but still think we're up-to-date and update mm's vmalloc_seq.

Ard, does that sound right to you, or am I missing something?

Mark.

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

[Ard Biesheuvel]

On Thu, 17 Oct 2024 at 12:30, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Oct 16, 2024 at 09:15:21PM +0200, Linus Walleij 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.
> >
> > 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-3b098de9db6d@foss.st.com/
> > 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>
> > 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);
> > +             }
>
> This looks good; FWIW:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> As a separate thing, I believe we also need to use atomic_read_acquire()
> for the reads of vmalloc_seq to pair with the atomic_*_release() on each
> update.
>
> Otherwise, this can be reordered, e.g.
>
>         do {
>                 memcpy_pgd(...);
>                 seq = atomic_read(&init_mm.context.vmalloc_seq);
>                 atomic_set_release(&mm->context.vmalloc_seq, seq);
>         } while (seq != atomic_read(&init_mm.context.vmalloc_seq)
>
> ... and we might fail to copy the relevant table entries from init_mm,
> but still think we're up-to-date and update mm's vmalloc_seq.
>

The compiler cannot reorder this as it has to assume that the memcpy()
may have side effects that affect the result of the atomic read.

So the question is whether this CPU can observe the new value of
init_mm.context.vmalloc_seq but still see the old contents of its page
tables in case another CPU is modifying init_mm concurrently.
atomic_read_acquire() definitely seems more appropriate here to
prevent that from happening, and I don't recall why I didn't use that
at the time.

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

[Linus Walleij]

On Thu, Oct 17, 2024 at 12:20 PM Clement LE GOFFIC
<clement.legoffic@foss.st.com> wrote:

> > +     add     r2, ip, lsr #KASAN_SHADOW_SCALE_SHIFT
(...)
> While ARM TRM says that if Rd is the same of Rn then it can be omitted,
> such syntax causes error on my build.
> Looking around for such syntax in the kernel, this line should be :
> add     r2, r2, ip, lsr #KASAN_SHADOW_SCALE_SHIFT

Yeah clearly my compilers allowed it :/

I changed it to the archaic version, will repost as v3.

Please test at your convenience!

Yours,
Linus Walleij

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

[Linus Walleij]

On Thu, Oct 17, 2024 at 2:55 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Oct 17, 2024 at 12:20 PM Clement LE GOFFIC
> <clement.legoffic@foss.st.com> wrote:
>
> > > +     add     r2, ip, lsr #KASAN_SHADOW_SCALE_SHIFT
> (...)
> > While ARM TRM says that if Rd is the same of Rn then it can be omitted,
> > such syntax causes error on my build.
> > Looking around for such syntax in the kernel, this line should be :
> > add     r2, r2, ip, lsr #KASAN_SHADOW_SCALE_SHIFT
>
> Yeah clearly my compilers allowed it :/
>
> I changed it to the archaic version, will repost as v3.

I think I meant the canonical version.. anglo-saxon is
sometimes not my strong card.

Yours,
Linus Walleij

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

[Linus Walleij]

On Wed, Oct 16, 2024 at 9:15 PM 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.
>
> 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-3b098de9db6d@foss.st.com/
> 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>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

As it turns out in my confusion I have missed that the more or less identical
patch with a different subject (talking about recursion) is already submitted
by Melon Liu and waiting in the patch tracker:
https://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=9427/1

I've tested it and it solves the problem equally well.

I even reviewed that and didn't remember it...

I will submit patch 2/2 into the patch tracker and let Melon's
patch deal with this issue.

Yours,
Linus Walleij