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

[Thomas Gleixner]

Alexander,

On Mon, May 02 2022 at 19:00, Alexander Potapenko wrote:
> On Wed, Apr 27, 2022 at 3:32 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> > --- 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().

Exactly 4 instances :)

> This decision can probably be revisited.

It has to be revisited because the whole thing is incomplete if this is
not addressed.

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

Understood. But if I understand you correctly:

> Similarly, every instrumented function expects its caller to put the
> metadata into that structure.

then

     instrumentation_begin();
     foo(fargs...);
     bar(bargs...);
     instrumentation_end();

is a source of potential false positives because the state is not
guaranteed to be correct, neither for foo() nor for bar(), even if you
wipe the state in instrumentation_begin(), right?

This approximation approach smells fishy and it's inevitably going to be
a constant source of 'add yet another kmsan annotation/fixup' patches,
which I'm not interested in at all.

As this needs compiler support anyway, then why not doing the obvious:

#define noinstr                                 \
        .... __kmsan_conditional

#define instrumentation_begin()                 \
        ..... __kmsan_cond_begin

#define instrumentation_end()                   \
        __kmsan_cond_end .......

and let the compiler stick whatever is required into that code section
between instrumentation_begin() and instrumentation_end()?

That's not violating any of the noinstr constraints at all. In fact we
allow _any_ instrumentation to be placed between this two points. We
have tracepoints there today.

We could also allow breakpoints, kprobes or whatever, but handling this
at that granularity level for a production kernel is just overkill and
the code in those instrumentable sections is usually not that
interesting as it's mostly function calls.

But if the compiler converts

     instrumentation_begin();
     foo(fargs...);
     bar(bargs...);
     instrumentation_end();

to

     instrumentation_begin();
     kmsan_instr_begin_magic();
     kmsan_magic(fargs...);
     foo(fargs...);
     kmsan_magic(bargs...);
     bar(bargs...);
     kmsan_instr_end_magic();
     instrumentation_end();

for the kmsan case and leaves anything outside of these sections alone,
then you have:

   - a minimal code change
   - the best possible coverage
   - the least false positive crap to chase and annotate

IOW, a solution which is solid and future proof.

I'm all for making use of advanced instrumentation, validation and
debugging features, but this mindset of 'make the code comply to what
the tool of today provides' is fundamentally wrong. Tools have to
provide value to the programmer and not the other way round.

Yes, it's more work on the tooling side, but the tooling side is mostly
a one time effort while chasing the false positives is a long term
nightmare.

Thanks,

        tglx