Message ID | 20221025221755.3810809-1-glider@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp1247294wru; Tue, 25 Oct 2022 15:26:21 -0700 (PDT) X-Google-Smtp-Source: AMsMyM546XJ4LIqBOFIgR9TmVbPt/eISMpWpgtJZMfELT35NZhol9QWJBnAbDzki0UnASRhohq5a X-Received: by 2002:a05:6402:2989:b0:44e:90d0:b9ff with SMTP id eq9-20020a056402298900b0044e90d0b9ffmr37521853edb.110.1666736781352; Tue, 25 Oct 2022 15:26:21 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666736781; cv=none; d=google.com; s=arc-20160816; b=WFog2PwxcXtcLpAaxNpcYg0opQVTHmH3/72hOArUMmp12LMePqjhCxzbteKQmB7k3Y HY9NOo7tnO3RUr4JOSZBs9F+EL2OoQLduHvIOk+IqSdZktotmpLXzW8WoCZgXy2TpADO A7xdyHBl6Se5mD1V2/za2R7Ys7XI2AfN1XsdVRqcAMajUzW4v1dTEWqHSH5OKImUxOHo SdeI7kzXCTwjimOh17NoGlpXyGgNNL5ijrQz4d9D90DiThjuUfLAH1Gb+Lpb/KumezN1 MdHyY1Ea9U7nKfG3e/sdwr34KCM23/6I49N6Rc3MqxVFT4HayI6URIeJNofZKn+Mzmq/ 7qIQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:mime-version:date :dkim-signature; bh=ZQGssT3M/U7b4XMGY/asVcg61n5AcupNDo7Gu+AvRuY=; b=sd9+wgk2z8CU9ExDepNODGeve8e9Ldhez+x5b5KR7zm8t++3ThfoO2tqTka9odODpK dNYLyAW3EmnVi3hh4XUnbpK3+Wq6CjEkjQ2AQsINwrzoh5RO0WNDNxYflbM+P4aE8ZU7 Pvi3VVnbJfvvN0WqpF7SL/g+oVC7ky+3cU8xQaxwsm6Q2pwn9r04MJ/6o2trqvh6yIJm EorUekxQNYBFZ1+Z80vMqrICbiQh2QVcqQwBCXODi+weds8guIrSfo4eINhJ9bk+IJCb Yq8vl3wa0LJ53Uj8ux6y7wsbRfQ2uIJlgQ8ivx9VBEpFauBX5FklklR4AdSaRi3uLLyK IyIA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=GZzG9I3E; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id h10-20020a056402280a00b00451e1aae675si4576758ede.547.2022.10.25.15.25.56; Tue, 25 Oct 2022 15:26:21 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=GZzG9I3E; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232560AbiJYWSF (ORCPT <rfc822;pwkd43@gmail.com> + 99 others); Tue, 25 Oct 2022 18:18:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40360 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232518AbiJYWSC (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 25 Oct 2022 18:18:02 -0400 Received: from mail-ej1-x64a.google.com (mail-ej1-x64a.google.com [IPv6:2a00:1450:4864:20::64a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C0CE5C58 for <linux-kernel@vger.kernel.org>; Tue, 25 Oct 2022 15:18:01 -0700 (PDT) Received: by mail-ej1-x64a.google.com with SMTP id qk31-20020a1709077f9f00b00791a3e02c80so3522391ejc.21 for <linux-kernel@vger.kernel.org>; Tue, 25 Oct 2022 15:18:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=ZQGssT3M/U7b4XMGY/asVcg61n5AcupNDo7Gu+AvRuY=; b=GZzG9I3EC4BdTG42DnBMyd1ivXSjjTelzZgMfx5pqBuLbH+hAXxGKOtyb79pJSdisX /cpLkpV914ei9LOOwDEcu7sTj/B25/nINBuG35/Tk8G54bHHoc+ny2ycz1Wam8uSNmGd tY/cb0kPSQi17ZqKRk4v3fjjk3EuSGaByehhMseFU4VSIV3fseJw2rBBP+T7eVNHsmf0 tVDy8K7dQhxLyCes+qB6UaAS/ALtPRdJJfJCqOT8iExcL/IzM25Ullu5r3UeNdoSZqBy KYA/w+Zg554ckMQspY6Vt30Ly/d4YYC5g2WsdbKbuwghxsCBeOYps6smx3sA2/cHwrxs r7qw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=ZQGssT3M/U7b4XMGY/asVcg61n5AcupNDo7Gu+AvRuY=; b=BN+l2Zz8BhMwTjL4epUgCAn25AWTWFwo1xkRkgaIbITCHSQOziXlqHrzR3nUkGxD0S bdbuKj9hLRJu+zS/TopaF7pM5N5alQr7MV41RyBJhUQF2Urfg09NJYlGJet2S592S6ag /otSS4MIx24j7TxMwJSEJt9lLrmXuarp0ZQjZ7ulAHQ3/EKwU3Km6zchxo/Hl9v7tRZj QoM5tIjLdx2LExjMl52vb23wH5RPU9EFVat2/nkvv1fKwKNPPsyqwdR+r8JR+SavavQU rNzBNQHjt53sEGWyVDOyXpwbC9Ya6cvzlDPWEA+tKDxWnOn6m745pvWHev/ApBf64wcb q+Wg== X-Gm-Message-State: ACrzQf2V6GrH1p1IVkzBO5s14c4z6fslM0INL8VSj5GQ5qsgogMgXHeE ZoeiUj9JERMrEE9kGmnSGsHEn3yfwps= X-Received: from glider.muc.corp.google.com ([2a00:79e0:9c:201:2e8:4dec:7a62:7bda]) (user=glider job=sendgmr) by 2002:a05:6402:b2f:b0:461:701e:877a with SMTP id bo15-20020a0564020b2f00b00461701e877amr18471401edb.82.1666736280249; Tue, 25 Oct 2022 15:18:00 -0700 (PDT) Date: Wed, 26 Oct 2022 00:17:55 +0200 Mime-Version: 1.0 X-Mailer: git-send-email 2.38.0.135.g90850a2211-goog Message-ID: <20221025221755.3810809-1-glider@google.com> Subject: [PATCH] x86/uaccess: instrument copy_from_user_nmi() From: Alexander Potapenko <glider@google.com> To: glider@google.com Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>, Dave Hansen <dave.hansen@linux.intel.com>, Kees Cook <keescook@chromium.org>, x86@kernel.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747700187269329935?= X-GMAIL-MSGID: =?utf-8?q?1747700187269329935?= |
Series |
x86/uaccess: instrument copy_from_user_nmi()
|
|
Commit Message
Alexander Potapenko
Oct. 25, 2022, 10:17 p.m. UTC
Make sure usercopy hooks from linux/instrumented.h are invoked for
copy_from_user_nmi().
This fixes KMSAN false positives reported when dumping opcodes for a
stack trace.
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: x86@kernel.org
Signed-off-by: Alexander Potapenko <glider@google.com>
---
arch/x86/lib/usercopy.c | 3 +++
1 file changed, 3 insertions(+)
Comments
On Wed, Oct 26, 2022 at 12:17:55AM +0200, Alexander Potapenko wrote: > Make sure usercopy hooks from linux/instrumented.h are invoked for > copy_from_user_nmi(). > This fixes KMSAN false positives reported when dumping opcodes for a > stack trace. > > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: x86@kernel.org > Signed-off-by: Alexander Potapenko <glider@google.com> > --- > arch/x86/lib/usercopy.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c > index f1bb186171562..24b48af274173 100644 > --- a/arch/x86/lib/usercopy.c > +++ b/arch/x86/lib/usercopy.c > @@ -6,6 +6,7 @@ > > #include <linux/uaccess.h> > #include <linux/export.h> > +#include <linux/instrumented.h> > > #include <asm/tlbflush.h> > > @@ -44,7 +45,9 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n) > * called from other contexts. > */ > pagefault_disable(); > + instrument_copy_from_user_before(to, from, n); > ret = raw_copy_from_user(to, from, n); > + instrument_copy_from_user_after(to, from, n, ret); > pagefault_enable(); > > return ret; Is all that instrumentation NMI safe? ISTR having seen locks in some of that *SAN stuff. Also did this want: Fixes: 59298997df89 ("x86/uaccess: avoid check_object_size() in copy_from_user_nmi()") ?
On Wed, Oct 26, 2022 at 11:38:53AM -0700, Alexander Potapenko wrote: > A bigger issue from the NMI perspective is probably > having __msan_poison_alloca() inserted in every non-noinstr kernel > function, because that hook may acquire the stackdepot lock. *urgghhh* that's broken, that must not be. There is a *TON* of NMI functions that are non-noinstr. What's worse, it seems to do a memory allocation as well, and that's out the window with PREEMPT_RT where you can't do even GFP_ATOMIC from regular IRQ context. That function is wholly unacceptable to be added to every kernel function.
On Thu, Oct 27, 2022 at 1:05 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Oct 26, 2022 at 11:38:53AM -0700, Alexander Potapenko wrote: > > A bigger issue from the NMI perspective is probably > > having __msan_poison_alloca() inserted in every non-noinstr kernel > > function, because that hook may acquire the stackdepot lock. > > *urgghhh* that's broken, that must not be. There is a *TON* of NMI > functions that are non-noinstr. __msan_poison_alloca() is guarded by kmsan_in_runtime(), which is currently implemented as: static __always_inline bool kmsan_in_runtime(void) { if ((hardirq_count() >> HARDIRQ_SHIFT) > 1) return true; return kmsan_get_context()->kmsan_in_runtime; } I think the easiest way to fix the NMI situation would be adding "if in_nmi() return true"? Currently that will render kmsan_unpoison_memory() useless in NMI context, but I think we don't need a check for kmsan_in_runtime() there, because unpoisoning is self-contained and normally does not recurse (guess we can tolerate a pr_err() on the rare assertion violation path?) > What's worse, it seems to do a memory allocation as well, and that's out > the window with PREEMPT_RT where you can't do even GFP_ATOMIC from > regular IRQ context. Yes, there's a lazy call to alloc_pages() in lib/stackdepot.c that is done when we run out of storage space. It would be a pity to ignore everything that is happening inside regular IRQs (by making kmsan_in_runtime() bail out on in_irq()) - I think we occasionally see errors from there, e.g. https://syzkaller.appspot.com/bug?id=233563e79a8e00f86412eb3d2fb4eb1f425e70c3 We could make stackdepot avoid allocating memory in IRQs/NMIs and hope that calls to __msan_poison_alloca() from regular contexts keep up with draining the storage from interrupts. Another option would be to preallocate a very big chunk of memory for stackdepot and never do allocations again. These tricks won't however save us from acquiring depot_lock from lib/stackdepot.c every time we want to create a new origin. But that should not be a problem by itself, because we always do kmsan_enter_runtime() before accessing the stack depot - i.e. it won't be taken recursively. Given that PREEMPT_RT is not the default at the moment, shall we make KMSAN incompatible with it for the time being? > That function is wholly unacceptable to be added to every kernel > function. >
On Thu, Oct 27, 2022 at 11:26:50AM -0700, Alexander Potapenko wrote: > On Thu, Oct 27, 2022 at 1:05 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Wed, Oct 26, 2022 at 11:38:53AM -0700, Alexander Potapenko wrote: > > > A bigger issue from the NMI perspective is probably > > > having __msan_poison_alloca() inserted in every non-noinstr kernel > > > function, because that hook may acquire the stackdepot lock. > > > > *urgghhh* that's broken, that must not be. There is a *TON* of NMI > > functions that are non-noinstr. > > __msan_poison_alloca() is guarded by kmsan_in_runtime(), which is > currently implemented as: > > static __always_inline bool kmsan_in_runtime(void) > { > if ((hardirq_count() >> HARDIRQ_SHIFT) > 1) > return true; > return kmsan_get_context()->kmsan_in_runtime; > } > > I think the easiest way to fix the NMI situation would be adding "if > in_nmi() return true"? It might help to look through these threads: https://lore.kernel.org/lkml/20220916135953.1320601-1-keescook@chromium.org/ https://lore.kernel.org/all/20220919201648.2250764-1-keescook@chromium.org/ I wandered around attempting to deal with in_nmi(), etc. And in the end just drop the attempt to cover it. It's worth noting that copy_from_user_nmi() exists on 1 architecture and has exactly 1 call-site...
On Thu, Oct 27, 2022 at 11:58:35AM -0700, Kees Cook wrote: > I wandered around attempting to deal with in_nmi(), etc. And in > the end just drop the attempt to cover it. It's worth noting that > copy_from_user_nmi() exists on 1 architecture and has exactly 1 > call-site... Yeah, back when I wrote it, it was a lot more complicated because we could not reliably take #PF from NMI context; it did manual page-walks, kmap_atomic()s and mempcy(). That's all fixed now and it's really mostly a rudiment -- except for these instrumentation issues it seems.
On Thu, Oct 27, 2022 at 11:58 AM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Oct 27, 2022 at 11:26:50AM -0700, Alexander Potapenko wrote: > > On Thu, Oct 27, 2022 at 1:05 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > On Wed, Oct 26, 2022 at 11:38:53AM -0700, Alexander Potapenko wrote: > > > > A bigger issue from the NMI perspective is probably > > > > having __msan_poison_alloca() inserted in every non-noinstr kernel > > > > function, because that hook may acquire the stackdepot lock. > > > > > > *urgghhh* that's broken, that must not be. There is a *TON* of NMI > > > functions that are non-noinstr. > > > > __msan_poison_alloca() is guarded by kmsan_in_runtime(), which is > > currently implemented as: > > > > static __always_inline bool kmsan_in_runtime(void) > > { > > if ((hardirq_count() >> HARDIRQ_SHIFT) > 1) > > return true; > > return kmsan_get_context()->kmsan_in_runtime; > > } > > > > I think the easiest way to fix the NMI situation would be adding "if > > in_nmi() return true"? > > It might help to look through these threads: > > https://lore.kernel.org/lkml/20220916135953.1320601-1-keescook@chromium.org/ > https://lore.kernel.org/all/20220919201648.2250764-1-keescook@chromium.org/ Sorry, I missed that letter, should have responded earlier. > I wandered around attempting to deal with in_nmi(), etc. And in > the end just drop the attempt to cover it. It's worth noting that > copy_from_user_nmi() exists on 1 architecture and has exactly 1 > call-site... It doesn't really matter for KASAN, because a missing addressability check is a matter of missing some (possibly rare) bugs. For KMSAN a missing initialization will result in false positives, and we already started seeing them: show_opcodes() copies data to a local and prints it, but without a call to kmsan_unpoison_memory() it will result in error reports about opcodes[] being uninitialized. So for this particular case I want to ensure kmsan_unpoison_memory() can be called from NMI context (by removing the kmsan_in_runtime() check from it), but to be on the safe side we'll also have to do nothing in __msan_poison_alloca() under in_nmi(). > -- > Kees Cook -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Paul Manicle, Liana Sebastian Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c index f1bb186171562..24b48af274173 100644 --- a/arch/x86/lib/usercopy.c +++ b/arch/x86/lib/usercopy.c @@ -6,6 +6,7 @@ #include <linux/uaccess.h> #include <linux/export.h> +#include <linux/instrumented.h> #include <asm/tlbflush.h> @@ -44,7 +45,9 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n) * called from other contexts. */ pagefault_disable(); + instrument_copy_from_user_before(to, from, n); ret = raw_copy_from_user(to, from, n); + instrument_copy_from_user_after(to, from, n, ret); pagefault_enable(); return ret;