Re: [PATCH 2/2] riscv: Add KASAN support

From: Nick Hu
Date: Tue Sep 03 2019 - 22:25:32 EST

Next message: Atsushi Yamamoto: "====INQUIRY====URGENT PLEASE===="
Previous message: Andrew Jeffery: "[PATCH v3] mmc: sdhci-of-aspeed: Depend on CONFIG_OF_ADDRESS"
In reply to: Daniel Axtens: "Re: [PATCH 2/2] riscv: Add KASAN support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Daniel,

On Wed, Sep 04, 2019 at 01:08:51AM +1000, Daniel Axtens wrote:
> Nick Hu <nickhu@xxxxxxxxxxxxx> writes:
>
> > Hi Christoph,
> >
> > Thanks for your reply. I will answer one by one.
> >
> > Hi Alexander,
> >
> > Would you help me for the question about SOFTIRQENTRY_TEXT?
> >
> > On Mon, Aug 12, 2019 at 11:10:50PM +0800, Christoph Hellwig wrote:
> >> > 2. KASAN can't debug the modules since the modules are allocated in VMALLOC
> >> > area. We mapped the shadow memory, which corresponding to VMALLOC area,
> >> > to the kasan_early_shadow_page because we don't have enough physical space
> >> > for all the shadow memory corresponding to VMALLOC area.
> >>
> >> How do other architectures solve this problem?
> >>
> > Other archs like arm64 and x86 allocate modules in their module region.
>
> I've run in to a similar difficulty in ppc64. My approach has been to
> add a generic feature to allow kasan to handle vmalloc areas:
>
> https://lore.kernel.org/linux-mm/20190903145536.3390-1-dja@xxxxxxxxxx/
>
> I link this with ppc64 in this series:
>
> https://lore.kernel.org/linuxppc-dev/20190806233827.16454-1-dja@xxxxxxxxxx/
>
> However, see Christophe Leroy's comments: he thinks I should take a
> different approach in a number of places, including just adding a
> separate module area. I haven't had time to think through all of his
> proposals yet; in particular I'd want to think through what the
> implication of a separate module area is for KASLR.
>
> Regards,
> Daniel
>

Thanks for the advice! I would study on it.

Regards,
Nick

> >
> >> > @@ -54,6 +54,8 @@ config RISCV
> >> > select EDAC_SUPPORT
> >> > select ARCH_HAS_GIGANTIC_PAGE
> >> > select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
> >> > + select GENERIC_STRNCPY_FROM_USER if KASAN
> >>
> >> Is there any reason why we can't always enabled this? Also just
> >> enabling the generic efficient strncpy_from_user should probably be
> >> a separate patch.
> >>
> > You're right, always enable it would be better.
> >
> >> > + select HAVE_ARCH_KASAN if MMU
> >>
> >> Based on your cover letter this should be if MMU && 64BIT
> >>
> >> > #define __HAVE_ARCH_MEMCPY
> >> > extern asmlinkage void *memcpy(void *, const void *, size_t);
> >> > +extern asmlinkage void *__memcpy(void *, const void *, size_t);
> >> >
> >> > #define __HAVE_ARCH_MEMMOVE
> >> > extern asmlinkage void *memmove(void *, const void *, size_t);
> >> > +extern asmlinkage void *__memmove(void *, const void *, size_t);
> >> > +
> >> > +#define memcpy(dst, src, len) __memcpy(dst, src, len)
> >> > +#define memmove(dst, src, len) __memmove(dst, src, len)
> >> > +#define memset(s, c, n) __memset(s, c, n)
> >>
> >> This looks weird and at least needs a very good comment. Also
> >> with this we effectively don't need the non-prefixed prototypes
> >> anymore. Also you probably want to split the renaming of the mem*
> >> routines into a separate patch with a proper changelog.
> >>
> > I made some mistakes on this porting, this would be better:
> >
> > #define __HAVE_ARCH_MEMSET
> > extern asmlinkage void *memset(void *, int, size_t);
> > extern asmlinkage void *__memset(void *, int, size_t);
> >
> > #define __HAVE_ARCH_MEMCPY
> > extern asmlinkage void *memcpy(void *, const void *, size_t);
> > extern asmlinkage void *__memcpy(void *, const void *, size_t);
> >
> > #define __HAVE_ARCH_MEMMOVE
> > extern asmlinkage void *memmove(void *, const void *, size_t);
> > extern asmlinkage void *__memmove(void *, const void *, size_t);
> >
> > #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
> >
> > #define memcpy(dst, src, len) __memcpy(dst, src, len)
> > #define memmove(dst, src, len) __memmove(dst, src, len)
> > #define memset(s, c, n) __memset(s, c, n)
> >
> > #endif
> >
> >> > #include <asm/tlbflush.h>
> >> > #include <asm/thread_info.h>
> >> >
> >> > +#ifdef CONFIG_KASAN
> >> > +#include <asm/kasan.h>
> >> > +#endif
> >>
> >> Any good reason to not just always include the header?
> >>
> > Nope, I would remove the '#ifdef CONFIG_KASAN', and do the logic in the header
> > instead.
> >
> >> > +
> >> > #ifdef CONFIG_DUMMY_CONSOLE
> >> > struct screen_info screen_info = {
> >> > .orig_video_lines = 30,
> >> > @@ -64,12 +68,17 @@ void __init setup_arch(char **cmdline_p)
> >> >
> >> > setup_bootmem();
> >> > paging_init();
> >> > +
> >> > unflatten_device_tree();
> >>
> >> spurious whitespace change.
> >>
> >> > diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> >> > index 23cd1a9..9700980 100644
> >> > --- a/arch/riscv/kernel/vmlinux.lds.S
> >> > +++ b/arch/riscv/kernel/vmlinux.lds.S
> >> > @@ -46,6 +46,7 @@ SECTIONS
> >> > KPROBES_TEXT
> >> > ENTRY_TEXT
> >> > IRQENTRY_TEXT
> >> > + SOFTIRQENTRY_TEXT
> >>
> >> Hmm. What is the relation to kasan here? Maybe we should add this
> >> separately with a good changelog?
> >>
> > There is a commit for it:
> >
> > Author: Alexander Potapenko <glider@xxxxxxxxxx>
> > Date: Fri Mar 25 14:22:05 2016 -0700
> >
> > arch, ftrace: for KASAN put hard/soft IRQ entries into separate sections
> >
> > KASAN needs to know whether the allocation happens in an IRQ handler.
> > This lets us strip everything below the IRQ entry point to reduce the
> > number of unique stack traces needed to be stored.
> >
> > Move the definition of __irq_entry to <linux/interrupt.h> so that the
> > users don't need to pull in <linux/ftrace.h>. Also introduce the
> > __softirq_entry macro which is similar to __irq_entry, but puts the
> > corresponding functions to the .softirqentry.text section.
> >
> > After reading the patch I understand that soft/hard IRQ entries should be
> > separated for KASAN to work, but why?
> >
> > Alexender, do you have any comments on this?
> >
> >> > +++ b/arch/riscv/mm/kasan_init.c
> >> > @@ -0,0 +1,102 @@
> >> > +// SPDX-License-Identifier: GPL-2.0
> >>
> >> This probably also wants a copyright statement.
> >>
> >> > + // init for swapper_pg_dir
> >>
> >> Please use /* */ style comments.
> >
> > --
> > You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@xxxxxxxxxxxxxxxxx
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20190814074417.GA21929%40andestech.com.

Next message: Atsushi Yamamoto: "====INQUIRY====URGENT PLEASE===="
Previous message: Andrew Jeffery: "[PATCH v3] mmc: sdhci-of-aspeed: Depend on CONFIG_OF_ADDRESS"
In reply to: Daniel Axtens: "Re: [PATCH 2/2] riscv: Add KASAN support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]