From: Alexander Potapenko <glider@google.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
Andrew Morton <akpm@linux-foundation.org>,
Andrey Konovalov <andreyknvl@google.com>,
Andy Lutomirski <luto@kernel.org>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Arnd Bergmann <arnd@arndb.de>, Borislav Petkov <bp@alien8.de>,
Christoph Hellwig <hch@lst.de>, Christoph Lameter <cl@linux.com>,
David Rientjes <rientjes@google.com>,
Dmitry Vyukov <dvyukov@google.com>,
Eric Dumazet <edumazet@google.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Herbert Xu <herbert@gondor.apana.org.au>,
Ilya Leoshkevich <iii@linux.ibm.com>,
Ingo Molnar <mingo@redhat.com>, Jens Axboe <axboe@kernel.dk>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Kees Cook <keescook@chromium.org>, Marco Elver <elver@google.com>,
Matthew Wilcox <willy@infradead.org>,
"Michael S. Tsirkin" <mst@redhat.com>,
Pekka Enberg <penberg@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Petr Mladek <pmladek@suse.com>,
Steven Rostedt <rostedt@goodmis.org>,
Thomas Gleixner <tglx@linutronix.de>,
Vasily Gorbik <gor@linux.ibm.com>,
Vegard Nossum <vegard.nossum@oracle.com>,
Vlastimil Babka <vbabka@suse.cz>,
linux-mm@kvack.org, linux-arch@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 25/43] kmsan: skip shadow checks in files doing context switches
Date: Wed, 15 Dec 2021 17:28:21 +0100 [thread overview]
Message-ID: <CAG_fn=XOOoCQhEkN1oeOXUX99P+AQ+ApPiUQXPFxR6yeT-Tf=w@mail.gmail.com> (raw)
In-Reply-To: <Ybn39Z5dwcbrbs0O@FVFF77S0Q05N>
On Wed, Dec 15, 2021 at 3:13 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Dec 14, 2021 at 05:20:32PM +0100, Alexander Potapenko wrote:
> > When instrumenting functions, KMSAN obtains the per-task state (mostly
> > pointers to metadata for function arguments and return values) once per
> > function at its beginning.
>
> How does KMSAN instrumentation acquire the per-task state? What's used as the
> base for that?
>
To preserve kernel ABI (so that instrumented functions can call
non-instrumented ones and vice versa) KMSAN uses a per-task struct
that keeps shadow values of function call parameters and return
values:
struct kmsan_context_state {
char param_tls[...];
char retval_tls[...];
char va_arg_tls[...];
char va_arg_origin_tls[...];
u64 va_arg_overflow_size_tls;
depot_stack_handle_t param_origin_tls[...];
depot_stack_handle_t retval_origin_tls;
};
It is mostly dealt with by the compiler, so its layout isn't really important.
The compiler inserts a call to __msan_get_context_state() at the
beginning of every instrumented function to obtain a pointer to that
struct.
Then, every time a function pointer is used, a value is returned, or
another function is called, the compiler adds code that updates the
shadow values in this struct.
E.g. the following function:
int sum(int a, int b) {
...
result = a + b;
return result;
}
will now look as follows:
int sum(int a, int b) {
struct kmsan_context_state *kcs = __msan_get_context_state();
int s_a = ((int)kcs->param_tls)[0]; // shadow of a
int s_b = ((int)kcs->param_tls)[1]; // shadow of b
...
result = a + b;
s_result = s_a | s_b;
((int)kcs->retval_tls)[0] = s_result; // returned shadow
return result;
}
> >
> > To deal with that, we need to apply __no_kmsan_checks to the functions
> > performing context switching - this will result in skipping all KMSAN
> > shadow checks and marking newly created values as initialized,
> > preventing all false positive reports in those functions. False negatives
> > are still possible, but we expect them to be rare and impersistent.
> >
> > To improve maintainability, we choose to apply __no_kmsan_checks not
> > just to a handful of functions, but to the whole files that may perform
> > context switching - this is done via KMSAN_ENABLE_CHECKS:=n.
> > This decision can be reconsidered in the future, when KMSAN won't need
> > so much attention.
>
> I worry this might be the wrong approach (and I've given some rationale below),
> but it's not clear to me exactly how this goes wrong. Could you give an example
> flow where stale data gets used?
The scheme I described above works well until a context switch occurs.
Then, IIUC, at some point `current` changes, so that the previously
fetched KMSAN context state becomes stale:
void foo(...) {
baz(...);
// context switch here changes `current`
baz(...);
}
In this case we'll have foo() setting up kmsan_context_state for the
old task when calling bar(), but bar() taking shadow for its arguments
from the new task's kmsan_context_state.
Does this make sense?
> As above, the actual context-switch occurs in arch code --I assume the
> out-of-line call *must* act as a clobber from the instrumentation's PoV or we'd
> have many more problems.
Treating a function call as a clobber of kmsan_context_state() is
actually an interesting idea.
Adding yet another call to __msan_get_context_state() after every
function call may sound harsh, but we already instrument every memory
access anyway.
What remains unclear is handling the return value of the innermost
function that performed the switch: it will be saved to the old task's
state, but taken from that of the new task.
> I also didn't spot any *explciit* state switching
> being added there that would seem to affect KMSAN.
>
> ... so I don't understand why checks need to be inhibited for the core sched code.
In fact for a long time there were only three functions annotated with
__no_kmsan_checks right in arch/x86/kernel/process_64.c and
kernel/sched/core.c
We decided to apply this attribute to every function in both files,
just to make sure nothing breaks too early while upstreaming KMSAN.
next prev parent reply other threads:[~2021-12-15 16:29 UTC|newest]
Thread overview: 75+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-14 16:20 [PATCH 00/43] Add KernelMemorySanitizer infrastructure Alexander Potapenko
2021-12-14 16:20 ` [PATCH 01/43] arch/x86: add missing include to sparsemem.h Alexander Potapenko
2021-12-14 16:20 ` [PATCH 02/43] stackdepot: reserve 5 extra bits in depot_stack_handle_t Alexander Potapenko
2021-12-14 16:20 ` [PATCH 03/43] kasan: common: adapt to the new prototype of __stack_depot_save() Alexander Potapenko
2021-12-14 16:20 ` [PATCH 04/43] instrumented.h: allow instrumenting both sides of copy_from_user() Alexander Potapenko
2021-12-14 16:20 ` [PATCH 05/43] asm: x86: instrument usercopy in get_user() and __put_user_size() Alexander Potapenko
2021-12-14 16:20 ` [PATCH 06/43] asm-generic: instrument usercopy in cacheflush.h Alexander Potapenko
2021-12-14 16:20 ` [PATCH 07/43] compiler_attributes.h: add __disable_sanitizer_instrumentation Alexander Potapenko
2021-12-15 13:24 ` Mark Rutland
2021-12-15 13:33 ` Marco Elver
2021-12-14 16:20 ` [PATCH 08/43] kmsan: add ReST documentation Alexander Potapenko
2021-12-14 16:20 ` [PATCH 09/43] kmsan: introduce __no_sanitize_memory and __no_kmsan_checks Alexander Potapenko
2021-12-15 13:27 ` Mark Rutland
2021-12-14 16:20 ` [PATCH 10/43] kmsan: pgtable: reduce vmalloc space Alexander Potapenko
2021-12-15 13:36 ` Mark Rutland
2022-03-18 14:14 ` Alexander Potapenko
2021-12-14 16:20 ` [PATCH 11/43] libnvdimm/pfn_dev: increase MAX_STRUCT_PAGE_SIZE Alexander Potapenko
2021-12-14 16:20 ` [PATCH 12/43] kcsan: clang: retire CONFIG_KCSAN_KCOV_BROKEN Alexander Potapenko
2021-12-15 13:33 ` Mark Rutland
2021-12-15 13:39 ` Marco Elver
2021-12-15 14:43 ` Mark Rutland
2022-03-18 14:34 ` Alexander Potapenko
2021-12-14 16:20 ` [PATCH 13/43] kmsan: add KMSAN runtime core Alexander Potapenko
2021-12-14 16:34 ` Greg Kroah-Hartman
2021-12-16 10:33 ` Alexander Potapenko
2021-12-17 16:22 ` Greg Kroah-Hartman
2022-03-21 14:12 ` Alexander Potapenko
[not found] ` <CACT4Y+Y_torRwzh0eDMn+pJh=YT26hGrAPA499WqN1dV+4bDHA@mail.gmail.com>
2022-03-21 13:17 ` Alexander Potapenko
2021-12-14 16:20 ` [PATCH 14/43] MAINTAINERS: add entry for KMSAN Alexander Potapenko
2021-12-14 16:20 ` [PATCH 15/43] kmsan: mm: maintain KMSAN metadata for page operations Alexander Potapenko
2021-12-14 16:20 ` [PATCH 16/43] kmsan: mm: call KMSAN hooks from SLUB code Alexander Potapenko
2022-01-07 17:22 ` Vlastimil Babka
2022-03-25 13:15 ` Alexander Potapenko
2021-12-14 16:20 ` [PATCH 17/43] kmsan: handle task creation and exiting Alexander Potapenko
2021-12-14 16:20 ` [PATCH 18/43] kmsan: unpoison @tlb in arch_tlb_gather_mmu() Alexander Potapenko
2021-12-14 16:20 ` [PATCH 19/43] kmsan: init: call KMSAN initialization routines Alexander Potapenko
2021-12-14 16:20 ` [PATCH 20/43] instrumented.h: add KMSAN support Alexander Potapenko
2021-12-14 16:20 ` [PATCH 21/43] kmsan: mark noinstr as __no_sanitize_memory Alexander Potapenko
2021-12-15 13:49 ` Mark Rutland
2021-12-14 16:20 ` [PATCH 22/43] kmsan: initialize the output of READ_ONCE_NOCHECK() Alexander Potapenko
2021-12-14 16:20 ` [PATCH 23/43] kmsan: make READ_ONCE_TASK_STACK() return initialized values Alexander Potapenko
2021-12-14 16:20 ` [PATCH 24/43] kmsan: disable KMSAN instrumentation for certain kernel parts Alexander Potapenko
2021-12-15 13:53 ` Mark Rutland
2021-12-14 16:20 ` [PATCH 25/43] kmsan: skip shadow checks in files doing context switches Alexander Potapenko
2021-12-15 14:13 ` Mark Rutland
2021-12-15 16:28 ` Alexander Potapenko [this message]
2021-12-15 17:22 ` Mark Rutland
2021-12-14 16:20 ` [PATCH 26/43] kmsan: virtio: check/unpoison scatterlist in vring_map_one_sg() Alexander Potapenko
2022-01-06 12:46 ` Michael S. Tsirkin
2021-12-14 16:20 ` [PATCH 27/43] x86: kmsan: add iomem support Alexander Potapenko
2021-12-14 16:20 ` [PATCH 28/43] kmsan: dma: unpoison DMA mappings Alexander Potapenko
2021-12-14 16:20 ` [PATCH 29/43] kmsan: handle memory sent to/from USB Alexander Potapenko
2021-12-14 16:20 ` [PATCH 30/43] kmsan: add tests for KMSAN Alexander Potapenko
2021-12-14 16:20 ` [PATCH 31/43] kmsan: disable strscpy() optimization under KMSAN Alexander Potapenko
2021-12-14 16:20 ` [PATCH 32/43] crypto: kmsan: disable accelerated configs " Alexander Potapenko
2021-12-14 16:20 ` [PATCH 33/43] kmsan: disable physical page merging in biovec Alexander Potapenko
2021-12-15 14:17 ` Mark Rutland
2021-12-15 16:30 ` Alexander Potapenko
2021-12-14 16:20 ` [PATCH 34/43] kmsan: block: skip bio block merging logic for KMSAN Alexander Potapenko
2021-12-14 16:20 ` [PATCH 35/43] x86: kmsan: use __msan_ string functions where possible Alexander Potapenko
2021-12-14 16:20 ` [PATCH 36/43] x86: kmsan: sync metadata pages on page fault Alexander Potapenko
2021-12-14 16:20 ` [PATCH 37/43] x86: kasan: kmsan: support CONFIG_GENERIC_CSUM on x86, enable it for KASAN/KMSAN Alexander Potapenko
2021-12-14 16:20 ` [PATCH 38/43] x86: fs: kmsan: disable CONFIG_DCACHE_WORD_ACCESS Alexander Potapenko
2021-12-14 16:20 ` [PATCH 39/43] x86: kmsan: handle register passing from uninstrumented code Alexander Potapenko
2021-12-17 21:51 ` Thomas Gleixner
2021-12-20 14:35 ` Alexander Potapenko
2021-12-14 16:20 ` [PATCH 40/43] kmsan: kcov: unpoison area->list in kcov_remote_area_put() Alexander Potapenko
2021-12-14 16:20 ` [PATCH 41/43] security: kmsan: fix interoperability with auto-initialization Alexander Potapenko
2021-12-14 16:38 ` Greg Kroah-Hartman
2021-12-14 17:00 ` Alexander Potapenko
2021-12-14 17:33 ` Greg Kroah-Hartman
2021-12-14 16:20 ` [PATCH 42/43] objtool: kmsan: list KMSAN API functions as uaccess-safe Alexander Potapenko
2021-12-14 16:20 ` [PATCH 43/43] x86: kmsan: enable KMSAN builds for x86 Alexander Potapenko
2021-12-14 16:36 ` [PATCH 00/43] Add KernelMemorySanitizer infrastructure Greg Kroah-Hartman
2021-12-16 10:12 ` Alexander Potapenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAG_fn=XOOoCQhEkN1oeOXUX99P+AQ+ApPiUQXPFxR6yeT-Tf=w@mail.gmail.com' \
--to=glider@google.com \
--cc=akpm@linux-foundation.org \
--cc=andreyknvl@google.com \
--cc=ard.biesheuvel@linaro.org \
--cc=arnd@arndb.de \
--cc=axboe@kernel.dk \
--cc=bp@alien8.de \
--cc=cl@linux.com \
--cc=dvyukov@google.com \
--cc=edumazet@google.com \
--cc=elver@google.com \
--cc=gor@linux.ibm.com \
--cc=gregkh@linuxfoundation.org \
--cc=hch@lst.de \
--cc=herbert@gondor.apana.org.au \
--cc=iamjoonsoo.kim@lge.com \
--cc=iii@linux.ibm.com \
--cc=keescook@chromium.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=mst@redhat.com \
--cc=penberg@kernel.org \
--cc=peterz@infradead.org \
--cc=pmladek@suse.com \
--cc=rientjes@google.com \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=vbabka@suse.cz \
--cc=vegard.nossum@oracle.com \
--cc=viro@zeniv.linux.org.uk \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).