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: <8E0D8C66-6F21-4890-8984-B6B3082D4CC5@gmail.com>
Date:   Fri, 24 Aug 2018 19:29:02 -0700
From:   nadav.amit@...il.com
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        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



On August 24, 2018 5:58:43 PM PDT, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
>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?).

The users should hold text_mutex.

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

My concern is that despite the lock, one core would do a speculative page walk and cache a translation that soon after would become stale.

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

It seems to me that it is intended to remove the mapping that might be a security issue. 

But anyhow, set_fixmap and clear_fixmap perform a local TLB flush, (in __set_pte_vaddr()) so locally things should be fine.

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

As I noted, I think that locking and local flushes as they are right now are fine (besides the redundant flush).

My concern is merely that speculative page walks on other cores would cache stale entries.



-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.