Re: [PATCH v6 1/9] mm: add Kernel Electric-Fence infrastructure

[Jann Horn <jannh@google.com>]

On Thu, Oct 29, 2020 at 2:17 PM Marco Elver <elver@google.com> wrote:
> This adds the Kernel Electric-Fence (KFENCE) infrastructure. KFENCE is a
> low-overhead sampling-based memory safety error detector of heap
> use-after-free, invalid-free, and out-of-bounds access errors.
[...]
> diff --git a/include/linux/kfence.h b/include/linux/kfence.h
[...]
> +/**
> + * is_kfence_address() - check if an address belongs to KFENCE pool
> + * @addr: address to check
> + *
> + * Return: true or false depending on whether the address is within the KFENCE
> + * object range.
> + *
> + * KFENCE objects live in a separate page range and are not to be intermixed
> + * with regular heap objects (e.g. KFENCE objects must never be added to the
> + * allocator freelists). Failing to do so may and will result in heap
> + * corruptions, therefore is_kfence_address() must be used to check whether
> + * an object requires specific handling.
> + */

It might be worth noting in the comment that this is one of the few
parts of KFENCE that are highly performance-sensitive, since that was
an important point during the review.

> +static __always_inline bool is_kfence_address(const void *addr)
> +{
> +       /*
> +        * The non-NULL check is required in case the __kfence_pool pointer was
> +        * never initialized; keep it in the slow-path after the range-check.
> +        */
> +       return unlikely((unsigned long)((char *)addr - __kfence_pool) < KFENCE_POOL_SIZE && addr);
> +}
[...]
> diff --git a/lib/Kconfig.kfence b/lib/Kconfig.kfence
[...]
> +config KFENCE_STRESS_TEST_FAULTS
> +       int "Stress testing of fault handling and error reporting"
> +       default 0
> +       depends on EXPERT
> +       help
> +         The inverse probability with which to randomly protect KFENCE object
> +         pages, resulting in spurious use-after-frees. The main purpose of
> +         this option is to stress test KFENCE with concurrent error reports
> +         and allocations/frees. A value of 0 disables stress testing logic.
> +
> +         The option is only to test KFENCE; set to 0 if you are unsure.
[...]
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
[...]
> +#ifndef CONFIG_KFENCE_STRESS_TEST_FAULTS /* Only defined with CONFIG_EXPERT. */
> +#define CONFIG_KFENCE_STRESS_TEST_FAULTS 0
> +#endif

I think you can make this prettier by writing the Kconfig
appropriately. See e.g. ARCH_MMAP_RND_BITS:

config ARCH_MMAP_RND_BITS
  int "Number of bits to use for ASLR of mmap base address" if EXPERT
  range ARCH_MMAP_RND_BITS_MIN ARCH_MMAP_RND_BITS_MAX
  default ARCH_MMAP_RND_BITS_DEFAULT if ARCH_MMAP_RND_BITS_DEFAULT
  default ARCH_MMAP_RND_BITS_MIN
  depends on HAVE_ARCH_MMAP_RND_BITS

So instead of 'depends on EXPERT', I think the proper way would be to
append ' if EXPERT' to the line
'int "Stress testing of fault handling and error reporting"', so that
only whether the option is user-visible depends on EXPERT, and
non-EXPERT configs automatically use the default value.

[...]
> +static inline unsigned long metadata_to_pageaddr(const struct kfence_metadata *meta)
> +{
> +       unsigned long offset = (meta - kfence_metadata + 1) * PAGE_SIZE * 2;
> +       unsigned long pageaddr = (unsigned long)&__kfence_pool[offset];
> +
> +       /* The checks do not affect performance; only called from slow-paths. */
> +
> +       /* Only call with a pointer into kfence_metadata. */
> +       if (KFENCE_WARN_ON(meta < kfence_metadata ||
> +                          meta >= kfence_metadata + CONFIG_KFENCE_NUM_OBJECTS))
> +               return 0;
> +
> +       /*
> +        * This metadata object only ever maps to 1 page; verify the calculation
> +        * happens and that the stored address was not corrupted.

nit: This reads a bit weirdly to me. Maybe "; verify that the stored
address is in the expected range"? But feel free to leave it as-is if
you prefer it that way.

> +        */
> +       if (KFENCE_WARN_ON(ALIGN_DOWN(meta->addr, PAGE_SIZE) != pageaddr))
> +               return 0;
> +
> +       return pageaddr;
> +}
[...]
> +/* __always_inline this to ensure we won't do an indirect call to fn. */
> +static __always_inline void for_each_canary(const struct kfence_metadata *meta, bool (*fn)(u8 *))
> +{
> +       const unsigned long pageaddr = ALIGN_DOWN(meta->addr, PAGE_SIZE);
> +       unsigned long addr;
> +
> +       lockdep_assert_held(&meta->lock);
> +
> +       /* Check left of object. */
> +       for (addr = pageaddr; addr < meta->addr; addr++) {
> +               if (!fn((u8 *)addr))
> +                       break;

It could be argued that "return" instead of "break" would be cleaner
here if the API is supposed to be "invoke fn() on each canary byte,
but stop when fn() returns false". But I suppose it doesn't really
matter, so either way is fine.

> +       }
> +
> +       /* Check right of object. */
> +       for (addr = meta->addr + meta->size; addr < pageaddr + PAGE_SIZE; addr++) {
> +               if (!fn((u8 *)addr))
> +                       break;
> +       }
> +}
> +
> +static void *kfence_guarded_alloc(struct kmem_cache *cache, size_t size, gfp_t gfp)
> +{
[...]
> +       /* Set required struct page fields. */
> +       page = virt_to_page(meta->addr);
> +       page->slab_cache = cache;
> +       if (IS_ENABLED(CONFIG_SLUB))
> +               page->objects = 1;
> +       if (IS_ENABLED(CONFIG_SLAB))
> +               page->s_mem = addr;

Maybe move the last 4 lines over into the "hooks for SLAB" and "hooks
for SLUB" patches?

[...]
> +}
[...]
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
[...]
> +/*
> + * Get the number of stack entries to skip get out of MM internals. @type is

s/to skip get out/to skip to get out/ ?

> + * optional, and if set to NULL, assumes an allocation or free stack.
> + */
> +static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries,
> +                           const enum kfence_error_type *type)
[...]
> +void kfence_report_error(unsigned long address, const struct kfence_metadata *meta,
> +                        enum kfence_error_type type)
> +{
[...]
> +       case KFENCE_ERROR_CORRUPTION: {
> +               size_t bytes_to_show = 16;
> +
> +               pr_err("BUG: KFENCE: memory corruption in %pS\n\n", (void *)stack_entries[skipnr]);
> +               pr_err("Corrupted memory at 0x" PTR_FMT " ", (void *)address);
> +
> +               if (address < meta->addr)
> +                       bytes_to_show = min(bytes_to_show, meta->addr - address);
> +               print_diff_canary((u8 *)address, bytes_to_show);

If the object was located on the right side, but with 1 byte padding
to the right due to alignment, and a 1-byte OOB write had clobbered
the canary byte on the right side, we would later detect a
KFENCE_ERROR_CORRUPTION at offset 0xfff inside the page, right? In
that case, I think we'd end up trying to read 15 canary bytes from the
following guard page and take a page fault?

You may want to do something like:

unsigned long canary_end = (address < meta->addr) ? meta->addr :
address | (PAGE_SIZE-1);
bytes_to_show = min(bytes_to_show, canary_end);



> +               pr_cont(" (in kfence-#%zd):\n", object_index);
> +               break;
> +       }