Re: [PATCH v2 2/2] UML: add support for KASAN under x86_64

[David Gow]

On Tue, May 31, 2022 at 2:03 AM Andrey Konovalov <andreyknvl@xxxxxxxxx> wrote:
>
> On Fri, May 27, 2022 at 8:56 PM David Gow <davidgow@xxxxxxxxxx> wrote:
> >
> > From: Patricia Alfonso <trishalfonso@xxxxxxxxxx>
> >
> > Make KASAN run on User Mode Linux on x86_64.
> >
> > The UML-specific KASAN initializer uses mmap to map the roughly 2.25TB
> > of shadow memory to the location defined by KASAN_SHADOW_OFFSET.
> > kasan_init() utilizes constructors to initialize KASAN before main().
> >
> > The location of the KASAN shadow memory, starting at
> > KASAN_SHADOW_OFFSET, can be configured using the KASAN_SHADOW_OFFSET
> > option. UML uses roughly 18TB of address space, and KASAN requires 1/8th
> > of this. The default location of this offset is 0x100000000000, which
> > keeps it out-of-the-way even on UML setups with more "physical" memory.
> >
> > For low-memory setups, 0x7fff8000 can be used instead, which fits in an
> > immediate and is therefore faster, as suggested by Dmitry Vyukov. There
> > is usually enough free space at this location; however, it is a config
> > option so that it can be easily changed if needed.
> >
> > Note that, unlike KASAN on other architectures, vmalloc allocations
> > still use the shadow memory allocated upfront, rather than allocating
> > and free-ing it per-vmalloc allocation.
> >
> > Also note that, while UML supports both KASAN in inline mode
> > (CONFIG_KASAN_INLINE) and static linking (CONFIG_STATIC_LINK), it does
> > not support both at the same time.
> >
> > Signed-off-by: Patricia Alfonso <trishalfonso@xxxxxxxxxx>
> > Co-developed-by: Vincent Whitchurch <vincent.whitchurch@xxxxxxxx>
> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@xxxxxxxx>
> > Signed-off-by: David Gow <davidgow@xxxxxxxxxx>
>
> Hi David,
>
> Thanks for working on this!
>
> > diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
> > index a4f07de21771..c993d99116f2 100644
> > --- a/mm/kasan/shadow.c
> > +++ b/mm/kasan/shadow.c
> > @@ -295,9 +295,29 @@ int kasan_populate_vmalloc(unsigned long addr, unsigned long size)
> >                 return 0;
> >
> >         shadow_start = (unsigned long)kasan_mem_to_shadow((void *)addr);
> > -       shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
> >         shadow_end = (unsigned long)kasan_mem_to_shadow((void *)addr + size);
> > -       shadow_end = ALIGN(shadow_end, PAGE_SIZE);
> > +
> > +       /*
> > +        * User Mode Linux maps enough shadow memory for all of physical memory
> > +        * at boot, so doesn't need to allocate more on vmalloc, just clear it.
>
> Should this say "for all of _virtual_ memory"?
>
> Otherwise, this is confusing. All KASAN-enabled architectures map
> shadow for physical memory. And they still need map shadow for
> vmalloc() separately. This is what kasan_populate_vmalloc() is for.
>

Yup, this was a mistake on my part: the original RFC for KASAN/UML
only allocated enough shadow memory to cover physical memory, but it
was changed in v1 (which I'd forgotten).

I've updated the comment in v3:
https://lore.kernel.org/lkml/20220630074757.2739000-2-davidgow@xxxxxxxxxx/

> > +        *
> > +        * If another architecture chooses to go down the same path, we should
> > +        * replace this check for CONFIG_UML with something more generic, such
> > +        * as:
> > +        * - A CONFIG_KASAN_NO_SHADOW_ALLOC option, which architectures could set
> > +        * - or, a way of having architecture-specific versions of these vmalloc
> > +        *   and module shadow memory allocation options.
>
> I think this part above and the first sentence below belong to the
> commit changelog, not to a comment.
>

While I think there's _some_ sense in leaving this in the comment (as
a bit of a reminder / TODO), given that the commit changelog is more
ephemeral, I've moved it to the commit message for v3. This will be
easy to find via git blame, while not cluttering the actual file, so
seems an okay spot for it.

Cheers,
-- David



> > +        *
> > +        * For the time being, though, this check works. The remaining CONFIG_UML
> > +        * checks in this file exist for the same reason.
> > +        */
> > +       if (IS_ENABLED(CONFIG_UML)) {
> > +               __memset((void *)shadow_start, KASAN_VMALLOC_INVALID, shadow_end - shadow_start);
> > +               return 0;
> > +       }
> > +
> > +       shadow_start = PAGE_ALIGN_DOWN(shadow_start);
> > +       shadow_end = PAGE_ALIGN(shadow_end);
> >
> >         ret = apply_to_page_range(&init_mm, shadow_start,
> >                                   shadow_end - shadow_start,
> > @@ -466,6 +486,10 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end,
> >
> >         if (shadow_end > shadow_start) {
> >                 size = shadow_end - shadow_start;
> > +               if (IS_ENABLED(CONFIG_UML)) {
> > +                       __memset(shadow_start, KASAN_SHADOW_INIT, shadow_end - shadow_start);
> > +                       return;
> > +               }
> >                 apply_to_existing_page_range(&init_mm,
> >                                              (unsigned long)shadow_start,
> >                                              size, kasan_depopulate_vmalloc_pte,
> > @@ -531,6 +555,11 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
> >         if (WARN_ON(!PAGE_ALIGNED(shadow_start)))
> >                 return -EINVAL;
> >
> > +       if (IS_ENABLED(CONFIG_UML)) {
> > +               __memset((void *)shadow_start, KASAN_SHADOW_INIT, shadow_size);
> > +               return 0;
> > +       }
> > +
> >         ret = __vmalloc_node_range(shadow_size, 1, shadow_start,
> >                         shadow_start + shadow_size,
> >                         GFP_KERNEL,
> > @@ -554,6 +583,9 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
> >
> >  void kasan_free_module_shadow(const struct vm_struct *vm)
> >  {
> > +       if (IS_ENABLED(CONFIG_UML))
> > +               return;
> > +
> >         if (vm->flags & VM_KASAN)
> >                 vfree(kasan_mem_to_shadow(vm->addr));
> >  }
> > --
> > 2.36.1.124.g0e6072fb45-goog
> >
>
> Thanks!

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature