Re: TLB flushes on fixmap changes

[Andy Lutomirski]

On Fri, Aug 24, 2018 at 7:29 PM,  <nadav.amit@xxxxxxxxx> wrote:
>
>
> On August 24, 2018 5:58:43 PM PDT, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>>Adding a few people to the cc.
>>
>>On Fri, Aug 24, 2018 at 1:24 PM Nadav Amit <nadav.amit@xxxxxxxxx>
>>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?