| Message ID | 20241016-arm-kasan-vmalloc-crash-v2-1-0a52fd086eef@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix KASAN crash when using KASAN_VMALLOC | expand |
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.
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.
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
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
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(-)