Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220426164315.625149-1-glider@google.com> <20220426164315.625149-29-glider@google.com>
 <87a6c6y7mg.ffs@tglx> <CAG_fn=U7PPBmmkgxFcWFQUCqZitzMizr1e69D9f26sGGzeitLQ@mail.gmail.com>
 <87y1zjlhmj.ffs@tglx> <CAG_fn=XxAhBEBP2KJvahinbaxLAd1xvqTfRJdAu1Tk5r8=01jw@mail.gmail.com>
 <878rrfiqyr.ffs@tglx> <CAG_fn=XVchXCcOhFt+rP=vinRhkyrXJSP46cyvcZeHJWaDquGg@mail.gmail.com>
 <87k0ayhc43.ffs@tglx> <CAG_fn=UpcXMqJiZvho6_G3rjvjQA-3Ax6X8ONVO0D+4Pttc9dA@mail.gmail.com>
 <87h762h5c2.ffs@tglx> <CAG_fn=UroTgp0jt77X_E-b1DPJ+32Cye6dRL4DOZ8MRf+XSokg@mail.gmail.com>
 <871qx2r09k.ffs@tglx>
In-Reply-To: <871qx2r09k.ffs@tglx>
From:   Alexander Potapenko <glider@google.com>
Date:   Thu, 12 May 2022 14:24:29 +0200
Message-ID: <CAG_fn=VtQw1gL_UVONHi=OJakOuMa3wKfkzP0jWcuvGQEmV9Vw@mail.gmail.com>
Subject: Re: [PATCH v3 28/46] kmsan: entry: handle register passing from
 uninstrumented code
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrey Konovalov <andreyknvl@google.com>,
        Andy Lutomirski <luto@kernel.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>,
        Mark Rutland <mark.rutland@arm.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>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Vegard Nossum <vegard.nossum@oracle.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        kasan-dev <kasan-dev@googlegroups.com>,
        Linux Memory Management List <linux-mm@kvack.org>,
        Linux-Arch <linux-arch@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, May 9, 2022 at 9:09 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, May 09 2022 at 18:50, Alexander Potapenko wrote:
> > 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:
> >
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > static __always_inline bool kmsan_in_runtime(void)
> > {
> >   if ((hardirq_count() >> HARDIRQ_SHIFT) > 1)
> >     return true;
> >   return kmsan_get_context()->kmsan_in_runtime;
> > }
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > (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()=3D=3Dtrue),
> > kmsan_get_context() returns a per-task `struct *kmsan_ctx`.
> > If `in_task()=3D=3Dfalse` and `hardirq_count()>>HARDIRQ_SHIFT=3D=3D1`, =
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.
>
> But, that'd only > 1 when there is a nested interrupt, which is not the
> case. Interrupt handlers keep interrupts disabled. The last exception fro=
m
> that rule was some legacy IDE driver which is gone by now.

That's good to know, then we probably don't need this hardirq_count()
check anymore.

> So no, not a good explanation either.

After looking deeper I see that unpoisoning was indeed skipped because
kmsan_in_runtime() returned true, but I was wrong about the root
cause.
The problem was not caused by a nested hardirq, but rather by the fact
that the KMSAN hook in irqentry_enter() was called with in_task()=3D=3D1.

Roughly said, T0 was running some code in the task context, then it
started executing KMSAN instrumentation and entered the runtime by
setting current->kmsan_ctx.kmsan_in_runtime.
Then an IRQ kicked in and started calling
asm_sysvec_apic_timer_interrupt() =3D> sysvec_apic_timer_interrupt(regs)
=3D> irqentry_enter(regs) - but at that point in_task() was still true,
therefore kmsan_unpoison_memory() became a no-op.

As far as I can see, it is irq_enter_rcu() that makes in_task() return
0 by incrementing the preempt count in __irq_enter_raw(), so our
unpoisoning can only work if we perform it after we enter the IRQ
context.

I think the best that can be done here is (as suggested above) to
provide some kmsan_unpoison_pt_regs() function that will only be
called from the entry points and won't be doing reentrancy checks.
It should be safe, because unpoisoning boils down to calculating
shadow/origin addresses and calling memset() on them, no instrumented
code will be involved.

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.

Another way to deal with the problem is to not rely on in_task(), but
rather use some per-cpu counter in irqentry_enter()/irqentry_exit() to
figure out whether we are in IRQ code already.
However this is only possible irqentry_enter() itself guarantees that
the execution cannot be rescheduled to another CPU - is that the case?

> Thanks,
>
>         tglx
>
> --
> You received this message because you are subscribed to the Google Groups=
 "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an=
 email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgi=
d/kasan-dev/871qx2r09k.ffs%40tglx.


--=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.