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 4B79FC4363A for ; Fri, 30 Oct 2020 02:49:44 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A0ED0207DE for ; Fri, 30 Oct 2020 02:49:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="iaLTL98N" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A0ED0207DE 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 256996B0071; Thu, 29 Oct 2020 22:49:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 205306B0072; Thu, 29 Oct 2020 22:49:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0A2E56B0073; Thu, 29 Oct 2020 22:49:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0132.hostedemail.com [216.40.44.132]) by kanga.kvack.org (Postfix) with ESMTP id C54BB6B0071 for ; Thu, 29 Oct 2020 22:49:42 -0400 (EDT) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 66EB7181AEF10 for ; Fri, 30 Oct 2020 02:49:42 +0000 (UTC) X-FDA: 77427061404.30.rose02_100620f27292 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin30.hostedemail.com (Postfix) with ESMTP id 45AB8180B3C85 for ; Fri, 30 Oct 2020 02:49:42 +0000 (UTC) X-HE-Tag: rose02_100620f27292 X-Filterd-Recvd-Size: 11391 Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com [209.85.167.65]) by imf34.hostedemail.com (Postfix) with ESMTP for ; Fri, 30 Oct 2020 02:49:41 +0000 (UTC) Received: by mail-lf1-f65.google.com with SMTP id l2so6015765lfk.0 for ; Thu, 29 Oct 2020 19:49:41 -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=bxX9Me9cwsFEBumfTFk5EKdPQHJ86xf2rO/msdc2EqI=; b=iaLTL98NqU6FBBQxLOjbfNc6+/hVBdtelfbZUgX3oDGyKoKWu35O5PvbnQ9yqiIacZ EJzWLLHfT9z21eWJXYkNJ3hkxuLZH6cPUEChjVxPZNie8VKwO09h5NvkLGxH8rb8eczk bQ33aVScXy3Hl7hXI+bHOQS2lnFt1YX8x7sYgD09huCO9OD92ak4WynH0aH+GNO4BjSF gtiSASThSF9TmJaYQFZMfOga3t8pU4xWUYZfsxAMequzUue6O1eHHLFKWc+BSyzuRMF8 zh1IjwOnFWxof5hzUVLt/oh5T9YfoZFtpGiXPVRPi3Ibr8c1IT87OwTCwlIRIzsDLSxD B46A== 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=bxX9Me9cwsFEBumfTFk5EKdPQHJ86xf2rO/msdc2EqI=; b=oTIdQvIbnac1586wmIyWPNMres/xv60lRwTsCtRbdo7qImyqwRxCkhueGIyrL6aV48 TWPv8edmGRrv6H+fZQfrE9/F5yPQjIeIYe5Y9QWn5AXuHbJJMzb3iVehQ1yaPGQQAG4X iWRi+n0N20wSCDDEmZPR41U4jGogSeb4YekaBS+flgwI6/qFFe+Wm7+WoQN39hW50RJK aLAb8SijJv8AU0N4IF4l+4IjPlh70M0MP4VzB4Oa8urxckQWce/cWKeH3ulQwSm1VMQb dBpWVkDtX/YBw/rczIXBgH+eRBg+2vOb8VPO4sqB9I1uAPZSsvHB/2+ylYGTnUgHvGZ/ FRgA== X-Gm-Message-State: AOAM5319+YfMvJk2lqGqr/gPTQlESD8AvXz4JZ1rGtv2DNcLHGOJKFma /zpe3PTl75Y52w9mhRBWEx9lOb2wIpINeVwCqSycQw== X-Google-Smtp-Source: ABdhPJx+vH+ZHNK7ydX1Nmu3QMRgdDhq55zeSv2xkmDEn8Qj0VKsLIZDBHt4K6upZ92hBQN5BdvkJJpZMYFZ0fdTTy4= X-Received: by 2002:a05:6512:1054:: with SMTP id c20mr23811lfb.576.1604026180208; Thu, 29 Oct 2020 19:49:40 -0700 (PDT) MIME-Version: 1.0 References: <20201029131649.182037-1-elver@google.com> <20201029131649.182037-2-elver@google.com> In-Reply-To: <20201029131649.182037-2-elver@google.com> From: Jann Horn Date: Fri, 30 Oct 2020 03:49:12 +0100 Message-ID: Subject: Re: [PATCH v6 1/9] mm: add Kernel Electric-Fence infrastructure To: Marco Elver 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 , joern@purestorage.com, 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 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. > +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; > + }