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 D79F1C433F5 for ; Mon, 2 May 2022 17:01:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2BC3E6B0071; Mon, 2 May 2022 13:01:07 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 26CBF6B0073; Mon, 2 May 2022 13:01:07 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 10D7C6B0074; Mon, 2 May 2022 13:01:07 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.27]) by kanga.kvack.org (Postfix) with ESMTP id 019136B0071 for ; Mon, 2 May 2022 13:01:06 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id CBB002A033 for ; Mon, 2 May 2022 17:01:06 +0000 (UTC) X-FDA: 79421418132.30.0D61B00 Received: from mail-yw1-f181.google.com (mail-yw1-f181.google.com [209.85.128.181]) by imf26.hostedemail.com (Postfix) with ESMTP id 6700E140082 for ; Mon, 2 May 2022 17:01:03 +0000 (UTC) Received: by mail-yw1-f181.google.com with SMTP id 00721157ae682-2f7ca2ce255so154457017b3.7 for ; Mon, 02 May 2022 10:01:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=ZDNXgBhMz20++Q4yigO612T0eYygF7r9J92FIVtim7Y=; b=JgdYu4i8LXmMnXLXrkcT7kHOAMHmAidhNS6U0ueSLoMV2xM914ojCWAfQT3FwPR+1l PoLOfIxEoZUnpUcLAHkqdzewTFZmlPHzf2P4KtVSykjNiUaSjHEZ4JXGrO87YwYEfY29 Z+QTsgSlKNHz0OH4JLROLS99pvqLcEwt40oWqVFt9/peS0coGpNEREGC2CeybE9udTkZ PBSCKT1mEj4EUvIGaFwp/YC2T3K64TVZ9MIoJHwe/x1WPyoerlcNsHxE9mUxBRzI6/O/ dA+GoQpNe65P4xsbfG25zceIOp2apEavuWjynEw+O1ENeDzWJe9o6sXQ0vlxCo8yyC69 UBEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=ZDNXgBhMz20++Q4yigO612T0eYygF7r9J92FIVtim7Y=; b=D/YWrh/mYVEf9ZQPYcCskaHf/WJYRs1fsx3Zf75XR+LbxdynyaQYS0CB6IOjjnve8h D8NWE6u3pEvSOtHuyK/B1Rusbw7tuI8ylXsrI/NFvr7KfwNwWFnGH80UlmHNksC921i3 NJIaxxTwI/Iiq1il/6rPoXZ0DG4A5lg500KGk/PQjxohiiCxGrx+bL8RGkEK5MC7gjr6 PF5Oq7rvaH3bSKzkYewBhfxD+5OTiDXaWIjI/WHtr9fmDvAZYEoAu9md3pALu+GlrOCl wXi3DVAosGPf3TWAHR1hFJVWdu5vHTtw6WmhGrzgP5ua1iFXjuW2W+K/YpnbbzFatui6 awTA== X-Gm-Message-State: AOAM532QpkFR7A8EUgDEcZwbsmd9hIO6QyUnfQf2qKq2xvdpC1lolGAs Itc/Mc1dzz33aLGEUu1fkct1rN7M7H3WKyeUAvZmjA== X-Google-Smtp-Source: ABdhPJy9WXshcsyYnhmFBRjyjSz+xXgG968fsEWEpZo4KzR8Ug2a15AebqyxN7IoToxv1Gm1KUB+/NnO5+jeZO0Mdww= X-Received: by 2002:a81:1f8b:0:b0:2f8:5846:445e with SMTP id f133-20020a811f8b000000b002f85846445emr11776773ywf.50.1651510864112; Mon, 02 May 2022 10:01:04 -0700 (PDT) MIME-Version: 1.0 References: <20220426164315.625149-1-glider@google.com> <20220426164315.625149-29-glider@google.com> <87a6c6y7mg.ffs@tglx> In-Reply-To: <87a6c6y7mg.ffs@tglx> From: Alexander Potapenko Date: Mon, 2 May 2022 19:00:28 +0200 Message-ID: Subject: Re: [PATCH v3 28/46] kmsan: entry: handle register passing from uninstrumented code To: Thomas Gleixner Cc: Alexander Viro , Andrew Morton , Andrey Konovalov , Andy Lutomirski , Arnd Bergmann , Borislav Petkov , Christoph Hellwig , Christoph Lameter , David Rientjes , Dmitry Vyukov , Eric Dumazet , Greg Kroah-Hartman , Herbert Xu , Ilya Leoshkevich , Ingo Molnar , Jens Axboe , Joonsoo Kim , Kees Cook , Marco Elver , Mark Rutland , Matthew Wilcox , "Michael S. Tsirkin" , Pekka Enberg , Peter Zijlstra , Petr Mladek , Steven Rostedt , Vasily Gorbik , Vegard Nossum , Vlastimil Babka , kasan-dev , Linux Memory Management List , Linux-Arch , LKML Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 6700E140082 X-Stat-Signature: a46386cz9gu4wn8kine8wte9np91qpy4 X-Rspam-User: Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=JgdYu4i8; spf=pass (imf26.hostedemail.com: domain of glider@google.com designates 209.85.128.181 as permitted sender) smtp.mailfrom=glider@google.com; dmarc=pass (policy=reject) header.from=google.com X-HE-Tag: 1651510863-751194 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, Apr 27, 2022 at 3:32 PM Thomas Gleixner wrote: > > On Tue, Apr 26 2022 at 18:42, Alexander Potapenko wrote: > > Can you please use 'entry:' as prefix. Slapping kmsan in front of > everything does not really make sense. Sure, will do. > > Replace instrumentation_begin() with instrumentation_begin_with_r= egs() > > to let KMSAN handle the non-instrumented code and unpoison pt_regs > > passed from the instrumented part. > > That should be: > > from the non-instrumented part > or > passed to the instrumented part > > right? That should be "from the non-instrumented part", you are right. > > --- a/kernel/entry/common.c > > +++ b/kernel/entry/common.c > > @@ -23,7 +23,7 @@ static __always_inline void __enter_from_user_mode(st= ruct pt_regs *regs) > > CT_WARN_ON(ct_state() !=3D CONTEXT_USER); > > user_exit_irqoff(); > > > > - instrumentation_begin(); > > + instrumentation_begin_with_regs(regs); > > I can see what you are trying to do, but this will end up doing the same > thing over and over. Let's just look at a syscall. > > __visible noinstr void do_syscall_64(struct pt_regs *regs, int nr) > { > ... > nr =3D syscall_enter_from_user_mode(regs, nr) > > __enter_from_user_mode(regs) > ..... > instrumentation_begin_with_regs(regs); > .... > > instrumentation_begin_with_regs(regs); > .... > > instrumentation_begin_with_regs(regs); > > if (!do_syscall_x64(regs, nr) && !do_syscall_x32(regs, nr) && nr = !=3D -1) { > /* Invalid system call, but still a system call. */ > regs->ax =3D __x64_sys_ni_syscall(regs); > } > > instrumentation_end(); > > syscall_exit_to_user_mode(regs); > instrumentation_begin_with_regs(regs); > __syscall_exit_to_user_mode_work(regs); > instrumentation_end(); > __exit_to_user_mode(); > > That means you memset state four times and unpoison regs four times. I'm > not sure whether that's desired. Regarding the regs, you are right. It should be enough to unpoison the regs at idtentry prologue instead. I tried that initially, but IIRC it required patching each of the DEFINE_IDTENTRY_XXX macros, which already use instrumentation_begin(). This decision can probably be revisited. As for the state, what we are doing here is still not enough, although it appears to work. Every time an instrumented function calls another function, it sets up the metadata for the function arguments in the per-task struct kmsan_context_state. Similarly, every instrumented function expects its caller to put the metadata into that structure. Now, if a non-instrumented function (e.g. every `noinstr` function) calls an instrumented one (which happens inside the instrumentation_begin()/instrumentation_end() region), nobody sets up the state for that instrumented function, so it may report false positives when accessing its arguments, if there are leftover poisoned values in the state. To overcome this problem, ideally we need to wipe kmsan_context_state every time a call from the non-instrumented function occurs. But this cannot be done automatically exactly because we cannot instrument the named function :) We therefore apply an approximation, wiping the state at the point of the first transition between instrumented and non-instrumented code. Because poison values are generally rare, and instrumented regions tend to be short, it is unlikely that further calls from the same non-instrumented function will result in false positives. Yet it is not completely impossible, so wiping the state for the second/third etc. time won't hurt. > > instrumentation_begin()/end() are not really suitable IMO. They were > added to allow objtool to validate that nothing escapes into > instrumentable code unless annotated accordingly. An alternative to this would be adding some extra code unpoisoning the state to every non-instrumented function that contains an instrumented region. That code would have to precede the first instrumentation_begin() anyway, so I thought it would be reasonable to piggyback on the existing annotation. > > Thanks, > > tglx --=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 Diese E-Mail ist vertraulich. Falls Sie diese f=C3=A4lschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, l=C3=B6schen Sie alle Kopien und Anh=C3=A4nge davon und lassen Sie = mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.