Date:   Mon, 9 May 2022 18:50:45 +0200
From:   Alexander Potapenko <glider@...gle.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Arnd Bergmann <arnd@...db.de>, Borislav Petkov <bp@...en8.de>,
        Christoph Hellwig <hch@....de>,
        Christoph Lameter <cl@...ux.com>,
        David Rientjes <rientjes@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Ilya Leoshkevich <iii@...ux.ibm.com>,
        Ingo Molnar <mingo@...hat.com>, Jens Axboe <axboe@...nel.dk>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Kees Cook <keescook@...omium.org>,
        Marco Elver <elver@...gle.com>,
        Mark Rutland <mark.rutland@....com>,
        Matthew Wilcox <willy@...radead.org>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Pekka Enberg <penberg@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Vegard Nossum <vegard.nossum@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        kasan-dev <kasan-dev@...glegroups.com>,
        Linux Memory Management List <linux-mm@...ck.org>,
        Linux-Arch <linux-arch@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 28/46] kmsan: entry: handle register passing from
 uninstrumented code

> The callchain is:
>
>   asm_sysvec_apic_timer_interrupt               <- ASM entry in gate
>      sysvec_apic_timer_interrupt(regs)          <- noinstr C entry point
>         irqentry_enter(regs)                    <- unpoisons @reg
>         __sysvec_apic_timer_interrupt(regs)     <- the actual handler
>            set_irq_regs(regs)                   <- stores regs
>            local_apic_timer_interrupt()
>              ...
>              tick_handler()                     <- One of the 4 variants
>                 regs = get_irq_regs();          <- retrieves regs
>                 update_process_times(user_tick = user_mode(regs))
>                    account_process_tick(user_tick)
>                       irqtime_account_process_tick(user_tick)
> line 382:                } else if { user_tick }   <- KMSAN complains
>
> I'm even more confused now.

Ok, I think I know what's going on.

Indeed, calling kmsan_unpoison_memory() in irqentry_enter() was
supposed to be enough, but we have code in kmsan_unpoison_memory() (as
well as other runtime functions) that checks for kmsan_in_runtime()
and bails out to prevent potential recursion if KMSAN code starts
calling itself.

kmsan_in_runtime() is implemented as follows:

==============================================
static __always_inline bool kmsan_in_runtime(void)
{
  if ((hardirq_count() >> HARDIRQ_SHIFT) > 1)
    return true;
  return kmsan_get_context()->kmsan_in_runtime;
}
==============================================
(see the code here:
https://lore.kernel.org/lkml/20220426164315.625149-13-glider@google.com/#Z31mm:kmsan:kmsan.h)

If we are running in the task context (in_task()==true),
kmsan_get_context() returns a per-task `struct *kmsan_ctx`.
If `in_task()==false` and `hardirq_count()>>HARDIRQ_SHIFT==1`, it
returns a per-CPU one.
Otherwise kmsan_in_runtime() is considered true to avoid dealing with
nested interrupts.

So in the case when `hardirq_count()>>HARDIRQ_SHIFT` is greater than
1, kmsan_in_runtime() becomes a no-op, which leads to false positives.

The solution I currently have in mind is to provide a specialized
version of kmsan_unpoison_memory() for entry.c, which will not perform
the reentrancy checks.

> > I guess handling those will require wrapping every interrupt gate into
> > a function that performs register unpoisoning?
>
> No, guessing does not help here.
>
> The gates point to the ASM entry point, which then invokes the C entry
> point. All C entry points use a DEFINE_IDTENTRY variant.

Thanks for the explanation. I previously thought there were two
different entry points, one with asm_ and one without, that ended up
calling the same code.

> Some of the DEFINE_IDTENTRY_* C entry points are not doing anything in
> the macro, but the C function either invokes irqentry_enter() or
> irqentry_nmi_enter() open coded _before_ invoking any instrumentable
> function. So the unpoisoning of @regs in these functions should tell
> KMSAN that @regs or something derived from @regs are not some random
> uninitialized values.
>
> There should be no difference between unpoisoning @regs in
> irqentry_enter() or in set_irq_regs(), right?
>
> If so, then the problem is definitely _not_ the idt entry code.
>
> > By the way, if it helps, I think we don't necessarily have to call
> > kmsan_unpoison_memory() from within the
> > instrumentation_begin()/instrumentation_end() region?
> > We could move the call to the beginning of irqentry_enter(), removing
> > unnecessary duplication.
>
> We could, but then you need to mark unpoison_memory() noinstr too and you
> have to add the unpoison into the syscall code. No win and irrelevant to
> the problem at hand.

Makes sense, thank you!

> Thanks,
>
>         tglx
>
>


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise
erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes
weiter, löschen Sie alle Kopien und Anhänge 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.

Powered by blists - more mailing lists