On Thu, May 12, 2022 at 6:48 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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 >
