Re: TLB flushes on fixmap changes

[nadav . amit]

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.



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