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=-6.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 F1299C00A89 for ; Fri, 30 Oct 2020 19:17:04 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4957D2072C for ; Fri, 30 Oct 2020 19:17:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="fdjCuzEK"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="RCDhFlYv" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4957D2072C Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=6cunibRv/fqKmvnqguVWYynrxtZheik3s8IZxZmoo94=; b=fdjCuzEK81kvO6G26jT50PvFw 0lF1svV6rEXsFu1Xtkrr2ytBHEGVKMVTvjF4s20e7dOJwpcgyTBD1i2zLus5wGV6g7SiUOMRNZNvc 5cQ+FA8j96qdklcO8nleEgKTMoC+VQJlkzcOBK1vhKDdpSljWGpHMvMxwBR3xs6PFpsiT949aTBdP IU+jceuO8uQkAEVg05ppClY2wdmFYfwznrCCzc6g083N2Qog3ESiXjI7N1/w18ovftU9m6yldWmZh daTPuNE7l9TDmSoN79FvxdtZ1zBToZrVyf7Zl+QWaxFMdRuXyj4suDVQyv7WxIVHaMPDkbXE7VkRJ SMoF5/MUQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kYZsz-0002DV-H9; Fri, 30 Oct 2020 19:16:29 +0000 Received: from mail-oo1-xc43.google.com ([2607:f8b0:4864:20::c43]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kYZsv-0002CN-KJ for linux-arm-kernel@lists.infradead.org; Fri, 30 Oct 2020 19:16:27 +0000 Received: by mail-oo1-xc43.google.com with SMTP id u5so1846448oot.0 for ; Fri, 30 Oct 2020 12:16:25 -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=Tz920EP50Fk+YfosNHv1Jj5DHiWxm4KdQS/E2ZHYeJwwWpsq8EtC1rbi2qTVAYvhhd nSBZxjLxFRnFIg/q4S0dga5GeWAvY9o6+VhwjLJKVLYWuYVgevf9/wcbG/pOXk14amoK 7jkG8xSviBajZV4pcX+CD424ZcfQYw6GECqyuce0uBCNmcjnOwIjUXHjUbr9PP+HBx7f JB0+n+ZlbEWwT68tuytSvTbPTS5tD/BI4Tx82gwFDO4fpjduHJ+9of8GEQiT4+/BDvBR cwgWPDqvhi1PcRC7cjAULQrJaTF3sTTe6rbNwHFWth1OaUoOvEA1IWOPxu7tFsI4lvx/ jNFg== X-Gm-Message-State: AOAM530Ds/eVV7M5+QTg9gjoJUAcr068MUOIu8oBuYKXkyEDXtOpp/Oc +GJ/4qpm/3MCXnU3bsKD0Y8aYjALALb6huRdxjx5HQ== 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201030_151625_744984_D7950DF5 X-CRM114-Status: GOOD ( 46.05 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Hillf Danton , "open list:DOCUMENTATION" , Peter Zijlstra , Catalin Marinas , Dave Hansen , SeongJae Park , Linux-MM , Eric Dumazet , Alexander Potapenko , "H . Peter Anvin" , Christoph Lameter , Will Deacon , SeongJae Park , Jonathan Corbet , the arch/x86 maintainers , kasan-dev , Ingo Molnar , Vlastimil Babka , David Rientjes , Andrey Ryabinin , =?UTF-8?Q?J=C3=B6rn_Engel?= , Kees Cook , "Paul E . McKenney" , Andrey Konovalov , Borislav Petkov , Andy Lutomirski , Jonathan Cameron , Thomas Gleixner , Andrew Morton , Dmitry Vyukov , Linux ARM , Greg Kroah-Hartman , kernel list , Pekka Enberg , Joonsoo Kim Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel