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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 56613C433FE for ; Wed, 26 Oct 2022 18:39:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 83F2D8E0002; Wed, 26 Oct 2022 14:39:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7EEF78E0001; Wed, 26 Oct 2022 14:39:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 68FCD8E0002; Wed, 26 Oct 2022 14:39:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 559608E0001 for ; Wed, 26 Oct 2022 14:39:31 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 1E5C280A52 for ; Wed, 26 Oct 2022 18:39:31 +0000 (UTC) X-FDA: 80063963742.17.F1D1E7B Received: from mail-yw1-f170.google.com (mail-yw1-f170.google.com [209.85.128.170]) by imf25.hostedemail.com (Postfix) with ESMTP id B3469A0002 for ; Wed, 26 Oct 2022 18:39:30 +0000 (UTC) Received: by mail-yw1-f170.google.com with SMTP id 00721157ae682-333a4a5d495so157244247b3.10 for ; Wed, 26 Oct 2022 11:39:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=+MwHo6Qga4mSu6bM9BYxPybQBccWW4RB4pLaj/uM33c=; b=XC8ba0cqlyThtgd4nAAgZNe9v+eTvs3vaP6RE5b2mbhrFDVg2P6zPl6O68UH/Cx0i+ Fbkpu18iwkuaQc01y+zK8D5iuabkAa1QwKwPskEsdbtUN/rLVPY7KLD+T0SNEm6+LMPd iQX6XQeDYMgnqwZXAbQkKNLH3IKY/X0hKQBEtYBpTRtFgKVaK+/XrSOafLJ4M1WV+lTG tMNrIaBml3uJy8wCVgApRivPCZgYwCo5mSxQ4RhDZvLJBC8H7TBwUuuvwwBdd3rFpql/ gwzzMkneAYiTjSbXXGAWp5E9j9Ud8zblc1Q3ducpGWw6BfZ5kCcJpyF96RgAoSrwuekz 8B7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=+MwHo6Qga4mSu6bM9BYxPybQBccWW4RB4pLaj/uM33c=; b=SFTSmlTrY9/B35rLmSlUDNAB353FZfg4n/4eLMtREUxPg6erGa+w8m+kXCIfnk9O2I op7KpB7dVzDCBSSmu5xL4CFvcmyEYQ1KFSTiTNb1dSldBsdv+3bMJjrxzbZhEpjc9uza bBPK5PGoJanKC0HwXv6xps2iuziTLgbNpzFJKt0/lmxefaUMY8YO3/O0BZIV4/VUqLo1 dgPF2IqBaP7ua9TWOU3/3ZSkU6Hzla45YlrPPfXCc1UxgcHqeeQomGLCiLvOipwd3sQG Bk2X4Z+pJ86+3KYlpk4J4L3rCF70PI1s0r3l0at2Fhr/nHxz43f0ZVfouDPqlA6oa3m1 YFXw== X-Gm-Message-State: ACrzQf0ScgMWJvMtVjCQ/t8HpZ1yAsfbx1yfO5WBGO3UuuLnDMVnX+KS AGybF+HTlKSrDaRsIb75vCYahK1P0XNA2wvSeobVpg== X-Google-Smtp-Source: AMsMyM6Y0VTZnBTVggiaNBtGSYOeXlSM2y2O2MPA9suG3Zj2YayxQnqxP42bjnv7aAXJRuOj9FFmuS2ePl41GwfccyA= X-Received: by 2002:a81:5807:0:b0:36a:b20c:a88a with SMTP id m7-20020a815807000000b0036ab20ca88amr22855090ywb.324.1666809569760; Wed, 26 Oct 2022 11:39:29 -0700 (PDT) MIME-Version: 1.0 References: <20221025221755.3810809-1-glider@google.com> In-Reply-To: From: Alexander Potapenko Date: Wed, 26 Oct 2022 11:38:53 -0700 Message-ID: Subject: Re: [PATCH] x86/uaccess: instrument copy_from_user_nmi() To: Peter Zijlstra , Marco Elver Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , Dave Hansen , Kees Cook , x86@kernel.org Content-Type: multipart/alternative; boundary="00000000000050a53805ebf4587f" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1666809570; a=rsa-sha256; cv=none; b=rrXuxsTvQG1M6kq1bt8ZyKH9DYXaBDPIxqoT9hzN02HDdzcpKUxaulnUoQEFihjRruPUly kse9z6Ihz7KUYddshrzNhvPT0rjHSfY5hVTH6t2IHs7cBVQOFPKM7YTQcn8vhGjQa+kSTv Sph8d67s6PhrexbnqEuvDQ+0blE5XjI= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=XC8ba0cq; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of glider@google.com designates 209.85.128.170 as permitted sender) smtp.mailfrom=glider@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1666809570; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=+MwHo6Qga4mSu6bM9BYxPybQBccWW4RB4pLaj/uM33c=; b=wI9Ih1bAJI2M8sUJWHk+5xTEeRip+eHwwRP8GSQHmWayRe920kyrO3vnh/Eaw015P2vm0i N3ljvGuDx7+YOJP0ZTC/abJDYO69bEfpedAJaCAF680AW74+Co/db0ZJhr8D9z2oC4qMvi a8kOmBpt4Fjv4x4NR+vPf7BJ6KO1INs= X-Stat-Signature: ff7waeyeyjbrqnmrq143kpp34cj47my3 X-Rspam-User: X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: B3469A0002 Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=XC8ba0cq; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of glider@google.com designates 209.85.128.170 as permitted sender) smtp.mailfrom=glider@google.com X-HE-Tag: 1666809570-509003 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: --00000000000050a53805ebf4587f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Oct 26, 2022 at 2:31 AM Peter Zijlstra wrote= : > 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 > > Cc: Dave Hansen > > Cc: Kees Cook > > Cc: x86@kernel.org > > Signed-off-by: Alexander Potapenko > > --- > > 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 > > #include > > +#include > > > > #include > > > > @@ -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 =3D 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. > > Good question. I think the implicit assumption is that every function in include/linux/instrumented.h must be NMI-safe (and IRQ safe as well). For KASAN I believe it to be the case: kasan_check_read() and kasan_check_write() are pretty simple, and in the worst case we'll get a spinlock in kasan_report(), which is quite unlikely to be nested (that's a KASAN bug report interrupted by an NMI, which in turn contains a KASAN bug)= . KCSAN also appears to be lockless and may only suffer from the nested bug report case (still super-rare). Marco, am I correct? For KMSAN the particular kmsan_unpoison_memory() is just a loop doing a memset() of several memory regions belonging to different pages, it doesn't even perform reporting. 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. Overall, I think we are safe for now, but I'm a bit afraid this may easily get out of hand if someone adds more tool hooks to instrumented.h > Also did this want: > > Fixes: 59298997df89 ("x86/uaccess: avoid check_object_size() in > copy_from_user_nmi()") > > ? > Ah, this explains why it started popping up. Yes, the Fixes: tag is relevant here. --=20 Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Stra=C3=9Fe, 33 80636 M=C3=BCnchen Gesch=C3=A4ftsf=C3=BChrer: Paul Manicle, Liana Sebastian Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg --00000000000050a53805ebf4587f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Oct 26, 2022 at 2:31 AM Peter= Zijlstra <peterz@infradead.org<= /a>> wrote:
O= n 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>
> ---
>=C2=A0 arch/x86/lib/usercopy.c | 3 +++
>=C2=A0 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 @@
>=C2=A0
>=C2=A0 #include <linux/uaccess.h>
>=C2=A0 #include <linux/export.h>
> +#include <linux/instrumented.h>
>=C2=A0
>=C2=A0 #include <asm/tlbflush.h>
>=C2=A0
> @@ -44,7 +45,9 @@ copy_from_user_nmi(void *to, const void __user *from= , unsigned long n)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 * called from other contexts.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pagefault_disable();
> +=C2=A0 =C2=A0 =C2=A0instrument_copy_from_user_before(to, from, n); >=C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D raw_copy_from_user(to, from, n);
> +=C2=A0 =C2=A0 =C2=A0instrument_copy_from_user_after(to, from, n, ret)= ;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pagefault_enable();
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;

Is all that instrumentation NMI safe? ISTR having seen locks in some of
that *SAN stuff.

Good question.
I think the implicit assumpt= ion is that every function in include/linux/instrumented.h must be NMI-safe= (and IRQ safe as well).

For KASAN I believe it to= be the case: kasan_check_read() and kasan_check_write() are pretty simple,= and in the worst case we'll get a spinlock in kasan_report(), which is= quite unlikely to be nested (that's a KASAN bug report interrupted by = an NMI, which in turn contains a KASAN bug).
KCSAN also appears t= o be lockless and may only suffer from the nested bug report case (still su= per-rare). Marco, am I correct?

For KMSAN the part= icular kmsan_unpoison_memory() is just a loop doing a memset() of several m= emory regions belonging to different pages, it doesn't even perform rep= orting.
A bigger issue from the NMI perspective is probably havin= g=C2=A0__msan_poison_alloca() inserted in every non-noinstr kernel function= , because that hook may acquire the stackdepot lock.

Overall, I think we are safe for now, but I'm a bit afraid this may = easily get out of hand if someone adds more tool hooks to instrumented.h
=C2=A0
Also did this want:

Fixes: 59298997df89 ("x86/uaccess: avoid check_object_size() in copy_f= rom_user_nmi()")

?

Ah, this explains why it started popping = up.
Yes, the Fixes: tag is relevant here.

--
Alexa= nder Potapenko
Software Engineer

Google Germany GmbH
Erika-Man= n-Stra=C3=9Fe, 33
80636 M=C3=BCnchen

Gesch=C3=A4ftsf=C3=BChrer: P= aul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 8= 6891
Sitz der Gesellschaft: Hamburg
--00000000000050a53805ebf4587f--