From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 89AA1C00A89 for ; Fri, 30 Oct 2020 19:16:27 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E414520797 for ; Fri, 30 Oct 2020 19:16:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="RCDhFlYv" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E414520797 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id F17C56B0036; Fri, 30 Oct 2020 15:16:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EC8F16B005C; Fri, 30 Oct 2020 15:16:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D69336B005D; Fri, 30 Oct 2020 15:16:25 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0139.hostedemail.com [216.40.44.139]) by kanga.kvack.org (Postfix) with ESMTP id AA14D6B0036 for ; Fri, 30 Oct 2020 15:16:25 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 43EFC1EF1 for ; Fri, 30 Oct 2020 19:16:25 +0000 (UTC) X-FDA: 77429547930.09.frame40_1f097a827298 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin09.hostedemail.com (Postfix) with ESMTP id 2659B180AD802 for ; Fri, 30 Oct 2020 19:16:25 +0000 (UTC) X-HE-Tag: frame40_1f097a827298 X-Filterd-Recvd-Size: 12649 Received: from mail-oo1-f65.google.com (mail-oo1-f65.google.com [209.85.161.65]) by imf26.hostedemail.com (Postfix) with ESMTP for ; Fri, 30 Oct 2020 19:16:24 +0000 (UTC) Received: by mail-oo1-f65.google.com with SMTP id n2so1843905ooo.8 for ; Fri, 30 Oct 2020 12:16:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=IphFE7ilzXNXur1141KXwIQedEFrpv7O0T6mxNfExvI=; b=RCDhFlYvuzkhj0RbKq2lgBPQSykTVOFnWK/WcFtrtTkVR3x5GXOu2PzFzOLGUOmzJj mB78wvCQ1fsVzChzlkxfqZuhpB+sUy5eafmPUGWeOXt+v/gjbzoB7Q/okyjNyzjJSmU3 1PasmJcynTSBoknvZQcDTfUVzO9DXaGOOxUcj4fkSyZtYxa5BdcYmYAcTI7jNHVkctHj ZGx547ln5g2kzS1n66nWW0Qhw0ot8uV8tRQiKxp4ivjtCokA/pKy3E9j18NwhrMoD3b8 NFxNVhnauAqXsTFULPRBgYMnAD4frbaY+pkRAbGZKq/sh30IrkkXj6jxp41ah2AujTN8 DMDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=IphFE7ilzXNXur1141KXwIQedEFrpv7O0T6mxNfExvI=; b=NQAZe76dgiQGePQJZIMI77P5fFxCYBdlRz4wfBZfa75OtiIv6/SAoiyvw8WM4Ax5U7 6cI9djyIfap37C76y7jmbHohdhM+DeDfniABZ1s2U4tZeduPTB1XTZHB9+phHwHJGEqf Z9YNww7MAXOsdOQ3nWeJzCy6lMzJHpP9nF49VAhH/e8pxl3x6g3sn6XjfdrYVzieVdWg yY36T13iJptyT9YwMC5srXIZjXf+tkrOMOiCg1WUz4ix1T1KTzS8ikhWpRFtP7Tp0UZG NOWYK6r53wFk4kNHKUWUit/UqmVIhXxFube7bqNK9hGFQEWOIDBrauIdZQEytTyX55oQ 5VNw== X-Gm-Message-State: AOAM533t7Za1BGunXN6hzziYhS0k+5trJ3YxVATIV69r0OlEFW6/q+8X fxSJvAEO13TDEhp7ofFI0ODeMdV1WKDsRHtxMHR0ag== X-Google-Smtp-Source: ABdhPJz+Ar79arAMWb4nHKtD5m+2c2HoTc65MlGNrFoHz2E+7j17Ls4jvjkLcU+UEA7bPTumYtOoXF4YQOvbudM+bJY= X-Received: by 2002:a4a:b28b:: with SMTP id k11mr3076579ooo.54.1604085383351; Fri, 30 Oct 2020 12:16:23 -0700 (PDT) MIME-Version: 1.0 References: <20201029131649.182037-1-elver@google.com> <20201029131649.182037-2-elver@google.com> In-Reply-To: From: Marco Elver Date: Fri, 30 Oct 2020 20:16:11 +0100 Message-ID: Subject: Re: [PATCH v6 1/9] mm: add Kernel Electric-Fence infrastructure To: Jann Horn Cc: Andrew Morton , Alexander Potapenko , "H . Peter Anvin" , "Paul E . McKenney" , Andrey Konovalov , Andrey Ryabinin , Andy Lutomirski , Borislav Petkov , Catalin Marinas , Christoph Lameter , Dave Hansen , David Rientjes , Dmitry Vyukov , Eric Dumazet , Greg Kroah-Hartman , Hillf Danton , Ingo Molnar , Jonathan Cameron , Jonathan Corbet , Joonsoo Kim , =?UTF-8?Q?J=C3=B6rn_Engel?= , Kees Cook , Mark Rutland , Pekka Enberg , Peter Zijlstra , SeongJae Park , Thomas Gleixner , Vlastimil Babka , Will Deacon , "the arch/x86 maintainers" , "open list:DOCUMENTATION" , kernel list , kasan-dev , Linux ARM , Linux-MM , SeongJae Park Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, 30 Oct 2020 at 03:49, Jann Horn wrote: > On Thu, Oct 29, 2020 at 2:17 PM Marco Elver 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. Done, thanks. > > +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. I guess the idea was to not pollute the config in non-EXPERT configs, but it probably doesn't matter much. Changed it to the suggested cleaner approach. > [...] > > +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. Hmm, that really sounds weird... I've changed it. :-) > > + */ > > + 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. Hmm, perhaps if there are corruptions on either side of an object printing both errors (which includes indications of which bytes were corrupted) might give more insights into what went wrong. Printing errors for every canary byte on one side didn't make much sense though, hence the break. Until we see this in the wild, let's err on the side of "more information might be better". > > + } > > + > > + /* 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? Done. > [...] > > +} > [...] > > 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/ ? Done. > > + * 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); print_diff_canary() calculates max_addr using PAGE_ALIGN(), and we won't read from the next page. I think I'll move all this logic into print_diff_canary() to simplify. Thanks, -- Marco