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 1ED2DC4727E for ; Wed, 7 Oct 2020 14:15:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B35FD208C7 for ; Wed, 7 Oct 2020 14:15:05 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="WCPWazuY" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728562AbgJGOPE (ORCPT ); Wed, 7 Oct 2020 10:15:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57698 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728265AbgJGOPE (ORCPT ); Wed, 7 Oct 2020 10:15:04 -0400 Received: from mail-ed1-x541.google.com (mail-ed1-x541.google.com [IPv6:2a00:1450:4864:20::541]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AE184C061755 for ; Wed, 7 Oct 2020 07:15:02 -0700 (PDT) Received: by mail-ed1-x541.google.com with SMTP id x1so2367714eds.1 for ; Wed, 07 Oct 2020 07:15:02 -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=MIy1jMlxCOJ0Y5mtMw96kWf8MP2ZqHwIF09OmCxHd/s=; b=WCPWazuYKodAut6Dvw8x3qNRztwfkOsar0G2liazhPuQzvDvjg7Jukxi9uz54DhGNr Sf7OZr+4DI1+Tkas8S2Bif5F8O4CRhY4MehqWbqnsq3bOrD4HTjd/kb1acw5Tqguoqtv Rllh5USbqhULrd8U2UrO1YzU9hBKuL+LRjrkNln91SE0GleTS8gS/v2OlBk2x+kARzLU wB8a+PXnZkS5+d9nkv6+TUW5EH+GjQb1mCBjM/+WA55NTw6KPCP5yQvlsnCQ5UlPIr30 jZl7ZGzHonK7hTlTbseBMPmmbTE23a89djvc1Z00KXGwgfUCf7c3uztATm1ENiA0CjV1 m8Gg== 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=MIy1jMlxCOJ0Y5mtMw96kWf8MP2ZqHwIF09OmCxHd/s=; b=BAUEK8ODklESM3NOXcZTDidJwnO05mdsgOIpKuCyh9nvJAa4cDOlsFYx2u6nJKVuGP moGxuwZipWCwWCCWiUp1ScMrTeLa45mfFqjbCNRkWjLGAtRCCU6bp0+6AJXtlCNu7mPD rDT7LfH+1Mzgd1kDgeGxpB+CROH4XV5Moav8VsNjX0PSDQgQRA1zSpCr/D0j2BzIhx02 utYsnDxD86mHjtlMKI0JxEGnHYl/NsrJFX3QHBJc9MeQNnJAeKjy3wJv2FDFqN6eWrrm kT5Lku2G9h2CZvrilZQZLaM3Xah+DMdAW9R1k0GA7OS8WU5JBa663yv7ahPfSYlaB00R p8cw== X-Gm-Message-State: AOAM5333Ns9jtrvwnMGm+fIJOQLWyFz4g7r35+/MKPRa1qupdG9T4mJU /g4NZJj57GXAP5yh0+VFPCtG/SzRgntl2tLz4xaTYQ== X-Google-Smtp-Source: ABdhPJwR/EYROZuqARz6G7n4XpMFS+ALLZjqv14b7utyykG9ti0A+TvzrhTUupRkmJLlxrAGOJoyhi7AI4L2KuqW7xA= X-Received: by 2002:a05:6402:b0e:: with SMTP id bm14mr3934250edb.259.1602080101068; Wed, 07 Oct 2020 07:15:01 -0700 (PDT) MIME-Version: 1.0 References: <20200929133814.2834621-1-elver@google.com> <20200929133814.2834621-3-elver@google.com> In-Reply-To: From: Jann Horn Date: Wed, 7 Oct 2020 16:14:34 +0200 Message-ID: Subject: Re: [PATCH v4 02/11] x86, kfence: enable KFENCE for x86 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 , 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 Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 7, 2020 at 3:09 PM Marco Elver wrote: > On Fri, 2 Oct 2020 at 07:45, Jann Horn wrote: > > On Tue, Sep 29, 2020 at 3:38 PM Marco Elver wrote: > > > Add architecture specific implementation details for KFENCE and enable > > > KFENCE for the x86 architecture. In particular, this implements the > > > required interface in for setting up the pool and > > > providing helper functions for protecting and unprotecting pages. > > > > > > For x86, we need to ensure that the pool uses 4K pages, which is done > > > using the set_memory_4k() helper function. > > [...] > > > diff --git a/arch/x86/include/asm/kfence.h b/arch/x86/include/asm/kfence.h > > [...] > > > +/* Protect the given page and flush TLBs. */ > > > +static inline bool kfence_protect_page(unsigned long addr, bool protect) > > > +{ > > > + unsigned int level; > > > + pte_t *pte = lookup_address(addr, &level); > > > + > > > + if (!pte || level != PG_LEVEL_4K) > > > > Do we actually expect this to happen, or is this just a "robustness" > > check? If we don't expect this to happen, there should be a WARN_ON() > > around the condition. > > It's not obvious here, but we already have this covered with a WARN: > the core.c code has a KFENCE_WARN_ON, which disables KFENCE on a > warning. So for this specific branch: Can it ever happen? If not, please either remove it or add WARN_ON(). That serves two functions: It ensures that if something unexpected happens, we see a warning, and it hints to people reading the code "this isn't actually expected to happen, you don't have to wrack your brain trying to figure out for which scenario this branch is intended". > > > + return false; > > > + > > > + if (protect) > > > + set_pte(pte, __pte(pte_val(*pte) & ~_PAGE_PRESENT)); > > > + else > > > + set_pte(pte, __pte(pte_val(*pte) | _PAGE_PRESENT)); > > > > Hmm... do we have this helper (instead of using the existing helpers > > for modifying memory permissions) to work around the allocation out of > > the data section? > > I just played around with using the set_memory.c functions, to remind > myself why this didn't work. I experimented with using > set_memory_{np,p}() functions; set_memory_p() isn't implemented, but > is easily added (which I did for below experiment). However, this > didn't quite work: [...] > For one, smp_call_function_many_cond() doesn't want to be called with > interrupts disabled, and we may very well get a KFENCE allocation or > page fault with interrupts disabled / within interrupts. > > Therefore, to be safe, we should avoid IPIs. set_direct_map_invalid_noflush() does that, too, I think? And that's already implemented for both arm64 and x86. > It follows that setting > the page attribute is best-effort, and we can tolerate some > inaccuracy. Lazy fault handling should take care of faults after we > set the page as PRESENT. [...] > > Shouldn't kfence_handle_page_fault() happen after prefetch handling, > > at least? Maybe directly above the "oops" label? > > Good question. AFAIK it doesn't matter, as is_kfence_address() should > never apply for any of those that follow, right? In any case, it > shouldn't hurt to move it down. is_prefetch() ignores any #PF not caused by instruction fetch if it comes from kernel mode and the faulting instruction is one of the PREFETCH* instructions. (Which is not supposed to happen - the processor should just be ignoring the fault for PREFETCH instead of generating an exception AFAIK. But the comments say that this is about CPU bugs and stuff.) While this is probably not a big deal anymore partly because the kernel doesn't use software prefetching in many places anymore, it seems to me like, in principle, this could also cause page faults that should be ignored in KFENCE regions if someone tries to do PREFETCH on an out-of-bounds array element or a dangling pointer or something. 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 54F6EC4363D for ; Wed, 7 Oct 2020 14:15:09 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id C6E9F212CC for ; Wed, 7 Oct 2020 14:15:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="WCPWazuY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C6E9F212CC 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 021BB6B0070; Wed, 7 Oct 2020 10:15:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EC48E6B0071; Wed, 7 Oct 2020 10:15:07 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D8B246B0072; Wed, 7 Oct 2020 10:15:07 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0099.hostedemail.com [216.40.44.99]) by kanga.kvack.org (Postfix) with ESMTP id A857C6B0070 for ; Wed, 7 Oct 2020 10:15:07 -0400 (EDT) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 3A8EF181AC9C6 for ; Wed, 7 Oct 2020 14:15:07 +0000 (UTC) X-FDA: 77345326254.13.dock54_1606696271d0 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin13.hostedemail.com (Postfix) with ESMTP id 1350F18140B9D for ; Wed, 7 Oct 2020 14:15:03 +0000 (UTC) X-HE-Tag: dock54_1606696271d0 X-Filterd-Recvd-Size: 8142 Received: from mail-ed1-f66.google.com (mail-ed1-f66.google.com [209.85.208.66]) by imf50.hostedemail.com (Postfix) with ESMTP for ; Wed, 7 Oct 2020 14:15:02 +0000 (UTC) Received: by mail-ed1-f66.google.com with SMTP id l16so2348457eds.3 for ; Wed, 07 Oct 2020 07:15:02 -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=MIy1jMlxCOJ0Y5mtMw96kWf8MP2ZqHwIF09OmCxHd/s=; b=WCPWazuYKodAut6Dvw8x3qNRztwfkOsar0G2liazhPuQzvDvjg7Jukxi9uz54DhGNr Sf7OZr+4DI1+Tkas8S2Bif5F8O4CRhY4MehqWbqnsq3bOrD4HTjd/kb1acw5Tqguoqtv Rllh5USbqhULrd8U2UrO1YzU9hBKuL+LRjrkNln91SE0GleTS8gS/v2OlBk2x+kARzLU wB8a+PXnZkS5+d9nkv6+TUW5EH+GjQb1mCBjM/+WA55NTw6KPCP5yQvlsnCQ5UlPIr30 jZl7ZGzHonK7hTlTbseBMPmmbTE23a89djvc1Z00KXGwgfUCf7c3uztATm1ENiA0CjV1 m8Gg== 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=MIy1jMlxCOJ0Y5mtMw96kWf8MP2ZqHwIF09OmCxHd/s=; b=Sr33uw977nGYT5yIrhQwPd0N3j9GDO9KWzorcEX+rz6jP+dJWRRpax5vRRT58bqGzQ ammjkEphZiN3vPbpsmPEJnFmDFgIUbIvW37z1Df7E+cP0cC5SAeYfcuUrN4w4Wl36p02 qlhCxYNGOOnTL1Kkcysu7fqTftASIT1vcz4LN9GblIGWYdcFX+m+ioDqcprFazrljl4W ULXKnPbJRajrrl9Pw8k2iGKDV2Pc2Km1X7l0CO25l7748ybKVZKUXopmEc/+O2H2jBW+ /HY2F9RQ06KbraA8+hJpa72xacKrGIikmqg6VnBx2SdpKdeXcuLChBaVAIfuhVbk+TM0 fC7w== X-Gm-Message-State: AOAM532/OaN0036xah88BOKq/5EmiOuou5yZLH/K9wA9v5nYy6ZbMfV+ EjeAbX2vc5pH1T0dWGUlqzYWQx6EfaCeNDbYDutAeA== X-Google-Smtp-Source: ABdhPJwR/EYROZuqARz6G7n4XpMFS+ALLZjqv14b7utyykG9ti0A+TvzrhTUupRkmJLlxrAGOJoyhi7AI4L2KuqW7xA= X-Received: by 2002:a05:6402:b0e:: with SMTP id bm14mr3934250edb.259.1602080101068; Wed, 07 Oct 2020 07:15:01 -0700 (PDT) MIME-Version: 1.0 References: <20200929133814.2834621-1-elver@google.com> <20200929133814.2834621-3-elver@google.com> In-Reply-To: From: Jann Horn Date: Wed, 7 Oct 2020 16:14:34 +0200 Message-ID: Subject: Re: [PATCH v4 02/11] x86, kfence: enable KFENCE for x86 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 , 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 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 Wed, Oct 7, 2020 at 3:09 PM Marco Elver wrote: > On Fri, 2 Oct 2020 at 07:45, Jann Horn wrote: > > On Tue, Sep 29, 2020 at 3:38 PM Marco Elver wrote: > > > Add architecture specific implementation details for KFENCE and enable > > > KFENCE for the x86 architecture. In particular, this implements the > > > required interface in for setting up the pool and > > > providing helper functions for protecting and unprotecting pages. > > > > > > For x86, we need to ensure that the pool uses 4K pages, which is done > > > using the set_memory_4k() helper function. > > [...] > > > diff --git a/arch/x86/include/asm/kfence.h b/arch/x86/include/asm/kfence.h > > [...] > > > +/* Protect the given page and flush TLBs. */ > > > +static inline bool kfence_protect_page(unsigned long addr, bool protect) > > > +{ > > > + unsigned int level; > > > + pte_t *pte = lookup_address(addr, &level); > > > + > > > + if (!pte || level != PG_LEVEL_4K) > > > > Do we actually expect this to happen, or is this just a "robustness" > > check? If we don't expect this to happen, there should be a WARN_ON() > > around the condition. > > It's not obvious here, but we already have this covered with a WARN: > the core.c code has a KFENCE_WARN_ON, which disables KFENCE on a > warning. So for this specific branch: Can it ever happen? If not, please either remove it or add WARN_ON(). That serves two functions: It ensures that if something unexpected happens, we see a warning, and it hints to people reading the code "this isn't actually expected to happen, you don't have to wrack your brain trying to figure out for which scenario this branch is intended". > > > + return false; > > > + > > > + if (protect) > > > + set_pte(pte, __pte(pte_val(*pte) & ~_PAGE_PRESENT)); > > > + else > > > + set_pte(pte, __pte(pte_val(*pte) | _PAGE_PRESENT)); > > > > Hmm... do we have this helper (instead of using the existing helpers > > for modifying memory permissions) to work around the allocation out of > > the data section? > > I just played around with using the set_memory.c functions, to remind > myself why this didn't work. I experimented with using > set_memory_{np,p}() functions; set_memory_p() isn't implemented, but > is easily added (which I did for below experiment). However, this > didn't quite work: [...] > For one, smp_call_function_many_cond() doesn't want to be called with > interrupts disabled, and we may very well get a KFENCE allocation or > page fault with interrupts disabled / within interrupts. > > Therefore, to be safe, we should avoid IPIs. set_direct_map_invalid_noflush() does that, too, I think? And that's already implemented for both arm64 and x86. > It follows that setting > the page attribute is best-effort, and we can tolerate some > inaccuracy. Lazy fault handling should take care of faults after we > set the page as PRESENT. [...] > > Shouldn't kfence_handle_page_fault() happen after prefetch handling, > > at least? Maybe directly above the "oops" label? > > Good question. AFAIK it doesn't matter, as is_kfence_address() should > never apply for any of those that follow, right? In any case, it > shouldn't hurt to move it down. is_prefetch() ignores any #PF not caused by instruction fetch if it comes from kernel mode and the faulting instruction is one of the PREFETCH* instructions. (Which is not supposed to happen - the processor should just be ignoring the fault for PREFETCH instead of generating an exception AFAIK. But the comments say that this is about CPU bugs and stuff.) While this is probably not a big deal anymore partly because the kernel doesn't use software prefetching in many places anymore, it seems to me like, in principle, this could also cause page faults that should be ignored in KFENCE regions if someone tries to do PREFETCH on an out-of-bounds array element or a dangling pointer or something. 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=-7.5 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 57498C4363D for ; Wed, 7 Oct 2020 14:16:34 +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 B52C4208C7 for ; Wed, 7 Oct 2020 14:16:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="mdKh5J4m"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="WCPWazuY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B52C4208C7 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=bFQEjjaMJap7q3M38/GijfOI7V3N8RQncuboq4C4/9c=; b=mdKh5J4maEVfmVkr26YfZOhSA bMswXYg15QNwo4OVSlSn+DSWLKxRgEY9XcbZp/uKpxoyHrrOP9Wzm2WgC0jj7yaoQ+fInYdEH+e4i ON+IIWK/ObHQaEsjQ2iwh+6EqX0bVYkM1AfwZ4OuJZhIybZ/mo/sWj4MjuGdAMqymdG9AUedA8Zxr Ail7iicudnR8Yy7pHI1XOc1qEcgQzEceHkghqsNyiHnAGO0uDuhUsa6IhoBQuQOVaV2dKfnNL/OVX WAwywdI5gnbiwhAVczx4/Eub81LonrUrwTKlKs0kzLjgqMDxo07RaxQwC74r0NpmY7tRYkBS/6Y7G qf51RCAag==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kQADg-0002Pn-QR; Wed, 07 Oct 2020 14:15:04 +0000 Received: from mail-ed1-x544.google.com ([2a00:1450:4864:20::544]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kQADe-0002Om-Rk for linux-arm-kernel@lists.infradead.org; Wed, 07 Oct 2020 14:15:03 +0000 Received: by mail-ed1-x544.google.com with SMTP id v19so2333340edx.9 for ; Wed, 07 Oct 2020 07:15:02 -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=MIy1jMlxCOJ0Y5mtMw96kWf8MP2ZqHwIF09OmCxHd/s=; b=WCPWazuYKodAut6Dvw8x3qNRztwfkOsar0G2liazhPuQzvDvjg7Jukxi9uz54DhGNr Sf7OZr+4DI1+Tkas8S2Bif5F8O4CRhY4MehqWbqnsq3bOrD4HTjd/kb1acw5Tqguoqtv Rllh5USbqhULrd8U2UrO1YzU9hBKuL+LRjrkNln91SE0GleTS8gS/v2OlBk2x+kARzLU wB8a+PXnZkS5+d9nkv6+TUW5EH+GjQb1mCBjM/+WA55NTw6KPCP5yQvlsnCQ5UlPIr30 jZl7ZGzHonK7hTlTbseBMPmmbTE23a89djvc1Z00KXGwgfUCf7c3uztATm1ENiA0CjV1 m8Gg== 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=MIy1jMlxCOJ0Y5mtMw96kWf8MP2ZqHwIF09OmCxHd/s=; b=jd7zyuBOof09wkY/lrt6rRjV4N+qPbN8o9iJL65jVYQtVuEFo011ZB0TEyU953l4uo WSEORmaUVkoAaNiikwZtPA4W6GEVh+qhGKuEvogO/3RFXGY8NwTM6lO7lTzFBJO+w/Vq kE5KOjhTLaUMVqOyvXjqVbNI5GVZvuGe4NsE0QvJ/g43ieiUMny6ktjJC0btZqAqwlYb O/z+txaDdUaBhukFFEHdDb+J/qLEg4w0Xfk768IuLOt0v+PNBZhPrfzg4zRqv2JqAl6R j21DW7CS62YcYQ1viU8PMPCTp3pN+5NyaZG3IM5z8RQPeDgOoKHuBVuFNOgiy0eHyn+R PBpQ== X-Gm-Message-State: AOAM530NmM8Fq8vBFhhYxyD9hGtNe4aJc8gHVOVrVMXl0Z5TcPbwI0/z UXKiPiHXNkt0vil9RqCt7vz6O+po/kDzuEqg8h4SSg== X-Google-Smtp-Source: ABdhPJwR/EYROZuqARz6G7n4XpMFS+ALLZjqv14b7utyykG9ti0A+TvzrhTUupRkmJLlxrAGOJoyhi7AI4L2KuqW7xA= X-Received: by 2002:a05:6402:b0e:: with SMTP id bm14mr3934250edb.259.1602080101068; Wed, 07 Oct 2020 07:15:01 -0700 (PDT) MIME-Version: 1.0 References: <20200929133814.2834621-1-elver@google.com> <20200929133814.2834621-3-elver@google.com> In-Reply-To: From: Jann Horn Date: Wed, 7 Oct 2020 16:14:34 +0200 Message-ID: Subject: Re: [PATCH v4 02/11] x86, kfence: enable KFENCE for x86 To: Marco Elver X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201007_101502_963449_08D97AB6 X-CRM114-Status: GOOD ( 36.91 ) 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 , 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 , 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 Wed, Oct 7, 2020 at 3:09 PM Marco Elver wrote: > On Fri, 2 Oct 2020 at 07:45, Jann Horn wrote: > > On Tue, Sep 29, 2020 at 3:38 PM Marco Elver wrote: > > > Add architecture specific implementation details for KFENCE and enable > > > KFENCE for the x86 architecture. In particular, this implements the > > > required interface in for setting up the pool and > > > providing helper functions for protecting and unprotecting pages. > > > > > > For x86, we need to ensure that the pool uses 4K pages, which is done > > > using the set_memory_4k() helper function. > > [...] > > > diff --git a/arch/x86/include/asm/kfence.h b/arch/x86/include/asm/kfence.h > > [...] > > > +/* Protect the given page and flush TLBs. */ > > > +static inline bool kfence_protect_page(unsigned long addr, bool protect) > > > +{ > > > + unsigned int level; > > > + pte_t *pte = lookup_address(addr, &level); > > > + > > > + if (!pte || level != PG_LEVEL_4K) > > > > Do we actually expect this to happen, or is this just a "robustness" > > check? If we don't expect this to happen, there should be a WARN_ON() > > around the condition. > > It's not obvious here, but we already have this covered with a WARN: > the core.c code has a KFENCE_WARN_ON, which disables KFENCE on a > warning. So for this specific branch: Can it ever happen? If not, please either remove it or add WARN_ON(). That serves two functions: It ensures that if something unexpected happens, we see a warning, and it hints to people reading the code "this isn't actually expected to happen, you don't have to wrack your brain trying to figure out for which scenario this branch is intended". > > > + return false; > > > + > > > + if (protect) > > > + set_pte(pte, __pte(pte_val(*pte) & ~_PAGE_PRESENT)); > > > + else > > > + set_pte(pte, __pte(pte_val(*pte) | _PAGE_PRESENT)); > > > > Hmm... do we have this helper (instead of using the existing helpers > > for modifying memory permissions) to work around the allocation out of > > the data section? > > I just played around with using the set_memory.c functions, to remind > myself why this didn't work. I experimented with using > set_memory_{np,p}() functions; set_memory_p() isn't implemented, but > is easily added (which I did for below experiment). However, this > didn't quite work: [...] > For one, smp_call_function_many_cond() doesn't want to be called with > interrupts disabled, and we may very well get a KFENCE allocation or > page fault with interrupts disabled / within interrupts. > > Therefore, to be safe, we should avoid IPIs. set_direct_map_invalid_noflush() does that, too, I think? And that's already implemented for both arm64 and x86. > It follows that setting > the page attribute is best-effort, and we can tolerate some > inaccuracy. Lazy fault handling should take care of faults after we > set the page as PRESENT. [...] > > Shouldn't kfence_handle_page_fault() happen after prefetch handling, > > at least? Maybe directly above the "oops" label? > > Good question. AFAIK it doesn't matter, as is_kfence_address() should > never apply for any of those that follow, right? In any case, it > shouldn't hurt to move it down. is_prefetch() ignores any #PF not caused by instruction fetch if it comes from kernel mode and the faulting instruction is one of the PREFETCH* instructions. (Which is not supposed to happen - the processor should just be ignoring the fault for PREFETCH instead of generating an exception AFAIK. But the comments say that this is about CPU bugs and stuff.) While this is probably not a big deal anymore partly because the kernel doesn't use software prefetching in many places anymore, it seems to me like, in principle, this could also cause page faults that should be ignored in KFENCE regions if someone tries to do PREFETCH on an out-of-bounds array element or a dangling pointer or something. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel