Message-ID: <CANpmjNOqSQ38DZxunagMLdBi8gjRN=14+FFXPhc+9SsUk+FiXQ@mail.gmail.com>
Date:   Tue, 2 Jun 2020 19:51:40 +0200
From:   Marco Elver <elver@...gle.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Dmitry Vyukov <dvyukov@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        syzbot <syzbot+dc1fa714cb070b184db5@...kaller.appspotmail.com>,
        LKML <linux-kernel@...r.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Oleg Nesterov <oleg@...hat.com>,
        kasan-dev <kasan-dev@...glegroups.com>
Subject: Re: PANIC: double fault in fixup_bad_iret

You were a bit faster with the other patches ;-) I was still
experimenting the the patches, but let me briefly respond here.

On Tue, 2 Jun 2020 at 11:41, Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Mon, Jun 01, 2020 at 02:40:31PM +0200, Marco Elver wrote:
> > I think Peter wanted to send a patch to add __no_kcsan to noinstr:
> > https://lkml.kernel.org/r/20200529170755.GN706495@hirez.programming.kicks-ass.net
> >
> > In the same patch we can add __no_sanitize_address to noinstr. But:
> >
> > - We're missing a definition for __no_sanitize_undefined and
> > __no_sanitize_coverage.
>
> Do those function attributes actually work? Because the last time I
> played with some of that I didn't.

__no_sanitize_coverage won't work, because neither compiler has an
attribute to disable coverage instrumentation. I'll try and add this
to compilers, but KCOV_INSTRUMENT := n is in the right places right
now it seems. More on that in the patch adding this.

> Specifically: unmarked __always_inline functions must not generate
> instrumentation when they're inlined into a __no_*san function.
>
> (and that fails to build on some GCC versions, and I think fails to
> actually work on the rest of them, but I'd have to double check)

We'll probably need to bump the required compiler version if anybody
still attempts to use these old compilers with sanitizers. The precise
versions of compilers and what mixes with what is a bit of a
nightmare. For now I'd just say, let's add the attributes, and see
where that gets us. Surely it won't be more broken than before. ;-)

> > - We still need the above blanket no-instrument for x86 because of
> > GCC. We could guard it with "ifdef CONFIG_CC_IS_GCC".
>
> Right; so all of GCC is broken vs that function attribute stuff? Any
> plans of getting that fixed? Do we have GCC that care?
>
> Does the GCC plugin approach sound like a viable alternative
> implementation of all this?

I don't think it's realistic to maintain a GCC plugin like that
indefinitely. We can investigate, but it's not a quick fix.

> Anyway, we can make it:
>
> KASAN := SANITIZER_HAS_FUNCTION_ATTRIBUTES
>
> or something, and only make that 'y' when the compiler is sane.

We have all attributes except __no_sanitize_coverage. GCC <= 7 has
problems with __always_inline, so we may just have to bump the
required compiler or emit a warning.

> > Not sure what the best strategy is to minimize patch conflicts. For
> > now I could send just the patches to add missing definitions. If you'd
> > like me to send all patches (including modifying 'noinstr'), let me
> > know.
>
> If you're going to do patches anyway, might as well do that :-)

I was stuck on trying to find ways to emulate __no_sanitize_coverage
(with no success), and then agonizing which patches to send in which
sequence. ;-) You made that decision by sending the KCSAN noinstr
series first, so let me respond to that with what I think we can add
for KASAN and UBSAN at least.

Thanks,
-- Marco

Powered by blists - more mailing lists