linux-kernel - Re: TLB flushes on fixmap changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFz1KYT7fRRG98wei24spiVg7u1Ec66piWY5359ykFmezw@mail.gmail.com>
Date:   Fri, 24 Aug 2018 17:58:43 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Nadav Amit <nadav.amit@...il.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Jiri Kosina <jkosina@...e.cz>,
        Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Will Deacon <will.deacon@....com>,
        Benjamin Herrenschmidt <benh@....ibm.com>,
        Nick Piggin <npiggin@...il.com>,
        Andrew Lutomirski <luto@...nel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Borislav Petkov <bp@...en8.de>,
        Rik van Riel <riel@...riel.com>,
        Jann Horn <jannh@...gle.com>,
        Adin Scannell <ascannell@...gle.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-mm <linux-mm@...ck.org>,
        David Miller <davem@...emloft.net>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Michael Ellerman <mpe@...erman.id.au>
Subject: Re: TLB flushes on fixmap changes

Adding a few people to the cc.

On Fri, Aug 24, 2018 at 1:24 PM Nadav Amit <nadav.amit@...il.com> wrote:
> >
> > Can you actually find something that changes the fixmaps after boot
> > (again, ignoring kmap)?
>
> At least the alternatives mechanism appears to do so.
>
> IIUC the following path is possible when adding a module:
>
>         jump_label_add_module()
>         ->__jump_label_update()
>         ->arch_jump_label_transform()
>         ->__jump_label_transform()
>         ->text_poke_bp()
>         ->text_poke()
>         ->set_fixmap()

Yeah, that looks a bit iffy.

But making the tlb flush global wouldn't help.  This is running on a
local core, and if there are other CPU's that can do this at the same
time, then they'd just fight about the same mapping.

Honestly, I think it's ok just because I *hope* this is all serialized
anyway (jump_label_lock? But what about other users of text_poke?).

But I'd be a lot happier about it if it either used an explicit lock
to make sure, or used per-cpu fixmap entries.

And the tlb flush is done *after* the address is used, which is bogus anyway.

> And a similar path can happen when static_key_enable/disable() is called.

Same comments.

How about replacing that

        local_irq_save(flags);
       ... do critical things here ...
        local_irq_restore(flags);

in text_poke() with

        static DEFINE_SPINLOCK(poke_lock);

        spin_lock_irqsave(&poke_lock, flags);
       ... do critical things here ...
        spin_unlock_irqrestore(&poke_lock, flags);

and moving the local_flush_tlb() to after the set_fixmaps, but before
the access through the virtual address.

But changing things to do a global tlb flush would just be wrong.

           Linus