Re: [PATCH v3 28/46] kmsan: entry: handle register passing from uninstrumented code

[Alexander Potapenko <glider@google.com>]

On Wed, Apr 27, 2022 at 3:32 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Tue, Apr 26 2022 at 18:42, Alexander Potapenko wrote:
>
> Can you please use 'entry:' as prefix. Slapping kmsan in front of
> everything does not really make sense.
Sure, will do.

> > Replace instrumentation_begin()       with instrumentation_begin_with_regs()
> > to let KMSAN handle the non-instrumented code and unpoison pt_regs
> > passed from the instrumented part.
>
> That should be:
>
>      from the non-instrumented part
> or
>      passed to the instrumented part
>
> right?

That should be "from the non-instrumented part", you are right.

> > --- a/kernel/entry/common.c
> > +++ b/kernel/entry/common.c
> > @@ -23,7 +23,7 @@ static __always_inline void __enter_from_user_mode(struct pt_regs *regs)
> >       CT_WARN_ON(ct_state() != CONTEXT_USER);
> >       user_exit_irqoff();
> >
> > -     instrumentation_begin();
> > +     instrumentation_begin_with_regs(regs);
>
> I can see what you are trying to do, but this will end up doing the same
> thing over and over. Let's just look at a syscall.
>
> __visible noinstr void do_syscall_64(struct pt_regs *regs, int nr)
> {
>         ...
>         nr = syscall_enter_from_user_mode(regs, nr)
>
>                 __enter_from_user_mode(regs)
>                         .....
>                         instrumentation_begin_with_regs(regs);
>                         ....
>
>                 instrumentation_begin_with_regs(regs);
>                 ....
>
>         instrumentation_begin_with_regs(regs);
>
>         if (!do_syscall_x64(regs, nr) && !do_syscall_x32(regs, nr) && nr != -1) {
>                 /* Invalid system call, but still a system call. */
>                 regs->ax = __x64_sys_ni_syscall(regs);
>         }
>
>         instrumentation_end();
>
>         syscall_exit_to_user_mode(regs);
>                 instrumentation_begin_with_regs(regs);
>                 __syscall_exit_to_user_mode_work(regs);
>         instrumentation_end();
>         __exit_to_user_mode();
>
> That means you memset state four times and unpoison regs four times. I'm
> not sure whether that's desired.

Regarding the regs, you are right. It should be enough to unpoison the
regs at idtentry prologue instead.
I tried that initially, but IIRC it required patching each of the
DEFINE_IDTENTRY_XXX macros, which already use instrumentation_begin().
This decision can probably be revisited.

As for the state, what we are doing here is still not enough, although
it appears to work.

Every time an instrumented function calls another function, it sets up
the metadata for the function arguments in the per-task struct
kmsan_context_state.
Similarly, every instrumented function expects its caller to put the
metadata into that structure.
Now, if a non-instrumented function (e.g. every `noinstr` function)
calls an instrumented one (which happens inside the
instrumentation_begin()/instrumentation_end() region), nobody sets up
the state for that instrumented function, so it may report false
positives when accessing its arguments, if there are leftover poisoned
values in the state.

To overcome this problem, ideally we need to wipe kmsan_context_state
every time a call from the non-instrumented function occurs.
But this cannot be done automatically exactly because we cannot
instrument the named function :)

We therefore apply an approximation, wiping the state at the point of
the first transition between instrumented and non-instrumented code.
Because poison values are generally rare, and instrumented regions
tend to be short, it is unlikely that further calls from the same
non-instrumented function will result in false positives.
Yet it is not completely impossible, so wiping the state for the
second/third etc. time won't hurt.

>
> instrumentation_begin()/end() are not really suitable IMO. They were
> added to allow objtool to validate that nothing escapes into
> instrumentable code unless annotated accordingly.

An alternative to this would be adding some extra code unpoisoning the
state to every non-instrumented function that contains an instrumented
region.
That code would have to precede the first instrumentation_begin()
anyway, so I thought it would be reasonable to piggyback on the
existing annotation.

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