lkml.org 
[lkml]   [2022]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 28/46] kmsan: entry: handle register passing from uninstrumented code
On Thu, May 12, 2022 at 6:48 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Thu, May 12 2022 at 18:17, Thomas Gleixner wrote:
> > On Thu, May 12 2022 at 14:24, Alexander Potapenko wrote:
> >> We could try to figure out the places in idtentry code where normal
> >> kmsan_unpoison_memory() can be called in IRQ context, but as far as I
> >> can see it will depend on the type of the entry point.
> >
> > NMI is covered as it increments before it invokes the unpoison().
> >
> > Let me figure out why we increment the preempt count late for
> > interrupts. IIRC it's for symmetry reasons related to softirq processing
> > on return, but let me double check.
>
> It's even documented:
>
> https://www.kernel.org/doc/html/latest/core-api/entry.html#interrupts-and-regular-exceptions
>
> But who reads documentation? :)
>
> So, I think the simplest and least intrusive solution is to have special
> purpose unpoison functions. See the patch below for illustration.

This patch works well and I am going to adopt it for my series.
But the problem with occasional calls of instrumented functions from
noinstr still persists: if there is a noinstr function foo() and an
instrumented function bar() called from foo() with one or more
arguments, bar() must wipe its kmsan_context_state before using the
arguments.

I have a solution for this problem described in https://reviews.llvm.org/D126385
The plan is to pass __builtin_return_address(0) to
__msan_get_context_state_caller() at the beginning of each
instrumented function.
Then KMSAN runtime can check the passed return address and wipe the
context if it belongs to the .noinstr code section.

Alternatively, we could employ MSan's -fsanitize-memory-param-retval
flag, that will report supplying uninitialized parameters when calling
functions.
Doing so is currently allowed in the kernel, but Clang aggressively
applies the noundef attribute (see https://llvm.org/docs/LangRef.html)
to function arguments, which effectively makes passing uninit values
as function parameters an UB.
So if we make KMSAN detect such cases as well, we can ultimately get
rid of all cases when uninits are passed to functions.
As a result, kmsan_context_state will become unnecessary, because it
will never contain nonzero values.


> The reasons why I used specific ones:
>
> 1) User entry
>
> Whether that's a syscall or interrupt/exception does not
> matter. It's always on the task stack and your machinery cannot be
> running at that point because it came from user space.
>
> 2) Interrupt/exception/NMI entry kernel
>
> Those can nest into an already active context, so you really want
> to unpoison @regs.
>
> Also while regular interrupts cannot nest because of interrupts
> staying disabled, exceptions triggered in the interrupt handler and
> NMIs can nest.
>
> -> device interrupt()
> irqentry_enter(regs)
>
> -> NMI()
> irqentry_nmi_enter(regs)
>
> -> fault()
> irqentry_enter(regs)
>
> --> debug_exception()
> irqentry_nmi_enter(regs)
>
> Soft interrupt processing on return from interrupt makes it more
> interesting:
>
> interrupt()
> handler()
> do_softirq()
> local_irq_enable()
> interrupt()
> NMI
> ....
>
> And everytime you get a new @regs pointer to deal with.
>
> Wonderful, isn't it?
>
> Thanks,
>
> tglx
>

\
 
 \ /
  Last update: 2022-06-01 13:29    [W:0.158 / U:0.568 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site