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: <20180826112341.f77a528763e297cbc36058fa@kernel.org>
Date:   Sun, 26 Aug 2018 11:23:41 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     Nadav Amit <nadav.amit@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Jiri Kosina <jkosina@...e.cz>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Will Deacon <will.deacon@....com>,
        Benjamin Herrenschmidt <benh@....ibm.com>,
        Nick Piggin <npiggin@...il.com>,
        "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 Fri, 24 Aug 2018 21:23:26 -0700
Andy Lutomirski <luto@...nel.org> wrote:

> On Fri, Aug 24, 2018 at 7:29 PM,  <nadav.amit@...il.com> wrote:
> >
> >
> > 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.
> >
> >
> 
> This is almost certainly a bug, or even two bugs.  Bug 1:  why on
> Earth do we flush in __set_pte_vaddr()?  We should flush when
> *clearing* or when modifying an existing fixmap entry.  Right now, if
> we do text_poke() after boot, then the TLB entry will stick around and
> will be a nice exploit target.
> 
> Bug 2: what you're describing.  It's racy.
> 
> Couldn't text_poke() use kmap_atomic()?  Or, even better, just change CR3?

No, since kmap_atomic() is only for x86_32 and highmem support kernel.
In x86-64, it seems that returns just a page address. That is not
good for text_poke, since it needs to make a writable alias for RO
code page. Hmm, maybe, can we mimic copy_oldmem_page(), it uses ioremap_cache?

Thank you,

-- 
Masami Hiramatsu <mhiramat@...nel.org>