arm64: use non-global mappings for UEFI runtime regions

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

From digging into the way the ASID allocator works, I believe this is

---
 arch/arm64/include/asm/mmu_context.h |  2 +-
 arch/arm64/kernel/efi.c              | 14 +++++---------
 2 files changed, 6 insertions(+), 10 deletions(-)

arch/arm64/include/asm/mmu_context.h +1 -1

index c0e87898ba96..24165784b803 100644
@@ -101,7 +101,7 @@ static inline void cpu_set_default_tcr_t0sz(void)
 #define destroy_context(mm)		do { } while(0)
 void check_and_switch_context(struct mm_struct *mm, unsigned int cpu);
 
-#define init_new_context(tsk,mm)	({ atomic64_set(&mm->context.id, 0); 0; })
+#define init_new_context(tsk,mm)	({ atomic64_set(&(mm)->context.id, 0); 0; })
 
 /*
  * This is called when "tsk" is about to enter lazy TLB mode.

arch/arm64/kernel/efi.c +5 -9

index de46b50f4cdf..fc5508e0df57 100644
@@ -224,6 +224,8 @@ static bool __init efi_virtmap_init(void)
 {
 	efi_memory_desc_t *md;
 
+	init_new_context(NULL, &efi_mm);
+
 	for_each_efi_memory_desc(&memmap, md) {
 		u64 paddr, npages, size;
 		pgprot_t prot;

@@ -254,7 +256,8 @@ static bool __init efi_virtmap_init(void)
 		else
 			prot = PAGE_KERNEL;
 
-		create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
+		create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size,
+				   __pgprot(pgprot_val(prot) | PTE_NG));
 	}
 	return true;
 }

@@ -329,14 +332,7 @@ core_initcall(arm64_dmi_init);
 
 static void efi_set_pgd(struct mm_struct *mm)
 {
-	if (mm == &init_mm)
-		cpu_set_reserved_ttbr0();
-	else
-		cpu_switch_mm(mm->pgd, mm);
-
-	local_flush_tlb_all();
-	if (icache_is_aivivt())
-		__local_flush_icache_all();
+	switch_mm(NULL, mm, NULL);
 }
 
 void efi_virtmap_load(void)

Signature · 1 line

1.9.1

Quoted 6 lines · Ard Biesheuvel

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

Wow, annoying that we missed that.

Quoted 4 lines

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

From digging into the way the ASID allocator works, I believe this is correct. FWIW:

Reviewed-by Mark Rutland <[email protected]>

For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something.

Will, are you aware of anything that could bite us here?

Quoted 26 lines · Mark Rutland

Quoted 6 lines · Ard Biesheuvel

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

Wow, annoying that we missed that.

Quoted 4 lines

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

From digging into the way the ASID allocator works, I believe this is correct. FWIW:

Reviewed-by Mark Rutland <[email protected]>

For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something.

Quoted 1 line

Will, are you aware of anything that could bite us here?

Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled?

Quoted 27 lines · Will Deacon

Quoted 26 lines · Mark Rutland

Quoted 6 lines · Ard Biesheuvel

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

Wow, annoying that we missed that.

Quoted 4 lines

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

From digging into the way the ASID allocator works, I believe this is correct. FWIW:

Reviewed-by Mark Rutland <[email protected]>

For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something.

Quoted 4 lines

Quoted 1 line

Will, are you aware of anything that could bite us here?

Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled?

Unfortuantely, it looks like we can guarantee interrupts are _disabled_.

Every function in drivers/firmware/efi/runtime-wrappers.c which uses efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a spin_lock_irq{save,restore} pair. Those appear to be the only uses of efi_call_virt.

Quoted 33 lines · Mark Rutland

Quoted 27 lines · Will Deacon

Quoted 26 lines · Mark Rutland

Quoted 6 lines · Ard Biesheuvel

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

Wow, annoying that we missed that.

Quoted 4 lines

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

From digging into the way the ASID allocator works, I believe this is correct. FWIW:

Reviewed-by Mark Rutland <[email protected]>

For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something.

Quoted 4 lines

Quoted 1 line

Will, are you aware of anything that could bite us here?

Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled?

Quoted 7 lines

Unfortuantely, it looks like we can guarantee interrupts are _disabled_.

Every function in drivers/firmware/efi/runtime-wrappers.c which uses efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a spin_lock_irq{save,restore} pair. Those appear to be the only uses of efi_call_virt.

There is actually no need from the UEFI pov to invoke the UEFI runtime services with interrupts disabled, this is simply an implementation detail of the kernel support, and I think it is primarily for x86 (but I have to dig up the old thread for the details)

And even if we stick with spin_lock_irqsave(), we could refactor the runtime wrappers to perform the mm switch outside of them.

Quoted 7 lines · Mark Rutland

Quoted 5 lines · Will Deacon

Quoted 1 line · Mark Rutland

Will, are you aware of anything that could bite us here?

Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled?

Quoted 7 lines

Unfortuantely, it looks like we can guarantee interrupts are _disabled_.

Every function in drivers/firmware/efi/runtime-wrappers.c which uses efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a spin_lock_irq{save,restore} pair. Those appear to be the only uses of efi_call_virt.

There is actually no need from the UEFI pov to invoke the UEFI runtime services with interrupts disabled, this is simply an implementation detail of the kernel support, and I think it is primarily for x86 (but I have to dig up the old thread for the details)

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something.

Will, are you aware of anything that could bite us here?

Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled?

Quoted 7 lines

Unfortuantely, it looks like we can guarantee interrupts are _disabled_.

Every function in drivers/firmware/efi/runtime-wrappers.c which uses efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a spin_lock_irq{save,restore} pair. Those appear to be the only uses of efi_call_virt.

There is actually no need from the UEFI pov to invoke the UEFI runtime services with interrupts disabled, this is simply an implementation detail of the kernel support, and I think it is primarily for x86 (but I have to dig up the old thread for the details)

And even if we stick with spin_lock_irqsave(), we could refactor the runtime wrappers to perform the mm switch outside of them.

Quoted 19 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something.

Will, are you aware of anything that could bite us here?

Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled?

Quoted 7 lines

Unfortuantely, it looks like we can guarantee interrupts are _disabled_.

Every function in drivers/firmware/efi/runtime-wrappers.c which uses efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a spin_lock_irq{save,restore} pair. Those appear to be the only uses of efi_call_virt.

Quoted 7 lines

There is actually no need from the UEFI pov to invoke the UEFI runtime services with interrupts disabled, this is simply an implementation detail of the kernel support, and I think it is primarily for x86 (but I have to dig up the old thread for the details)

And even if we stick with spin_lock_irqsave(), we could refactor the runtime wrappers to perform the mm switch outside of them.

Ok.

I'm only thinking about stable here.

In the context of a stable backport, I think the simplest thing to do is always go via the resesrved ttbr0 to perform the TLB flush, and hand-code the save/restore of the active mm's TTBR0_EL1 value rather than going through cpu_switch_mm (which I believe we can't call with interrupts disabled).

It doesn't look like it's easy to stash the value given efi_virtmap_{load,unload} are separate functions, and I don't think we can just restore from current->active_mm in case there was a concurrent rollover on another CPU.

Quoted 29 lines · Mark Rutland

Quoted 19 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something.

Will, are you aware of anything that could bite us here?

Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled?

Quoted 7 lines

Unfortuantely, it looks like we can guarantee interrupts are _disabled_.

Every function in drivers/firmware/efi/runtime-wrappers.c which uses efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a spin_lock_irq{save,restore} pair. Those appear to be the only uses of efi_call_virt.

Quoted 7 lines

There is actually no need from the UEFI pov to invoke the UEFI runtime services with interrupts disabled, this is simply an implementation detail of the kernel support, and I think it is primarily for x86 (but I have to dig up the old thread for the details)

And even if we stick with spin_lock_irqsave(), we could refactor the runtime wrappers to perform the mm switch outside of them.

Ok.

Quoted 8 lines

I'm only thinking about stable here.

In the context of a stable backport, I think the simplest thing to do is always go via the resesrved ttbr0 to perform the TLB flush, and hand-code the save/restore of the active mm's TTBR0_EL1 value rather than going through cpu_switch_mm (which I believe we can't call with interrupts disabled).

I think calling cpu_switch_mm() should be fine. It does set the ASID to #0 (which shouldn't matter since we only create global mappings) but does not invoke any of the logic regarding ASID assignment or rollover.

Quoted 5 lines

It doesn't look like it's easy to stash the value given efi_virtmap_{load,unload} are separate functions, and I don't think we can just restore from current->active_mm in case there was a concurrent rollover on another CPU.

OK, so the fact that we switch to the UEFI page tables and back with interrupts disabled is keeping us safe here, and whether we use global mappings or not is completely irrelevant.

In any case, I am going to check with Matt if we can leave interrupts enabled during the UEFI Runtime Services (since the execution time is not bounded by the spec), but I will keep this in mind in case anyone tries to backport that.

Quoted 26 lines · Mark Rutland

Quoted 6 lines · Ard Biesheuvel

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

Wow, annoying that we missed that.

Quoted 4 lines

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

From digging into the way the ASID allocator works, I believe this is correct. FWIW:

Reviewed-by Mark Rutland <[email protected]>

For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something.

Quoted 1 line

Will, are you aware of anything that could bite us here?

Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled? Also, the old rollover code seems to rely on current->active_mm being the thing to switch to, so an incoming rollover might break things if we're running with the efi_mm.

Quoted 27 lines · Will Deacon

Quoted 26 lines · Mark Rutland

Quoted 6 lines · Ard Biesheuvel

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

Wow, annoying that we missed that.

Quoted 4 lines

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

From digging into the way the ASID allocator works, I believe this is correct. FWIW:

Reviewed-by Mark Rutland <[email protected]>

For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something.

Quoted 7 lines

Quoted 1 line

Will, are you aware of anything that could bite us here?

Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled? Also, the old rollover code seems to rely on current->active_mm being the thing to switch to, so an incoming rollover might break things if we're running with the efi_mm.

OK, but not the current rollover code, right? I.e., if we go with this approach (after fixing the interupts issue), it will carry over to ARM as well?

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/mmu_context.h |  2 +-
 arch/arm64/kernel/efi.c              | 14 +++++---------
 2 files changed, 6 insertions(+), 10 deletions(-)

arch/arm64/include/asm/mmu_context.h +1 -1

index c0e87898ba96..24165784b803 100644
@@ -101,7 +101,7 @@ static inline void cpu_set_default_tcr_t0sz(void)
 #define destroy_context(mm)		do { } while(0)
 void check_and_switch_context(struct mm_struct *mm, unsigned int cpu);
 
-#define init_new_context(tsk,mm)	({ atomic64_set(&mm->context.id, 0); 0; })
+#define init_new_context(tsk,mm)	({ atomic64_set(&(mm)->context.id, 0); 0; })
 
 /*
  * This is called when "tsk" is about to enter lazy TLB mode.

arch/arm64/kernel/efi.c +5 -9

index de46b50f4cdf..fc5508e0df57 100644
@@ -224,6 +224,8 @@ static bool __init efi_virtmap_init(void)
 {
 	efi_memory_desc_t *md;
 
+	init_new_context(NULL, &efi_mm);
+
 	for_each_efi_memory_desc(&memmap, md) {
 		u64 paddr, npages, size;
 		pgprot_t prot;

@@ -254,7 +256,8 @@ static bool __init efi_virtmap_init(void)
 		else
 			prot = PAGE_KERNEL;
 
-		create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
+		create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size,
+				   __pgprot(pgprot_val(prot) | PTE_NG));
 	}
 	return true;
 }

@@ -329,14 +332,7 @@ core_initcall(arm64_dmi_init);
 
 static void efi_set_pgd(struct mm_struct *mm)
 {
-	if (mm == &init_mm)
-		cpu_set_reserved_ttbr0();
-	else
-		cpu_switch_mm(mm->pgd, mm);
-
-	local_flush_tlb_all();
-	if (icache_is_aivivt())
-		__local_flush_icache_all();
+	switch_mm(NULL, mm, NULL);
 }
 
 void efi_virtmap_load(void)

Signature · 1 line

1.9.1

Quoted 7 lines · Ard Biesheuvel

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

Quoted 8 lines

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/mmu_context.h |  2 +-
 arch/arm64/kernel/efi.c              | 14 +++++---------
 2 files changed, 6 insertions(+), 10 deletions(-)

Acked-by Will Deacon <[email protected]>

Please do *not* tag this for stable! ;)

Quoted 55 lines

arch/arm64/include/asm/mmu_context.h +1 -1

index c0e87898ba96..24165784b803 100644
@@ -101,7 +101,7 @@ static inline void cpu_set_default_tcr_t0sz(void)
 #define destroy_context(mm)          do { } while(0)
 void check_and_switch_context(struct mm_struct *mm, unsigned int cpu);
 
-#define init_new_context(tsk,mm)     ({ atomic64_set(&mm->context.id, 0); 0; })
+#define init_new_context(tsk,mm)     ({ atomic64_set(&(mm)->context.id, 0); 0; })
 
 /*
  * This is called when "tsk" is about to enter lazy TLB mode.

arch/arm64/kernel/efi.c +5 -10

index de46b50f4cdf..fc5508e0df57 100644
@@ -224,6 +224,8 @@ static bool __init efi_virtmap_init(void)
 {
      efi_memory_desc_t *md;
 
+     init_new_context(NULL, &efi_mm);
+
      for_each_efi_memory_desc(&memmap, md) {
              u64 paddr, npages, size;
              pgprot_t prot;

@@ -254,7 +256,8 @@ static bool __init efi_virtmap_init(void)
              else
                      prot = PAGE_KERNEL;
 
-             create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
+             create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size,
+                                __pgprot(pgprot_val(prot) | PTE_NG));
      }
      return true;
 }

@@ -329,14 +332,7 @@ core_initcall(arm64_dmi_init);
 
 static void efi_set_pgd(struct mm_struct *mm)
 {
-     if (mm == &init_mm)
-             cpu_set_reserved_ttbr0();
-     else
-             cpu_switch_mm(mm->pgd, mm);
-
-     local_flush_tlb_all();
-     if (icache_is_aivivt())
-             __local_flush_icache_all();
+     switch_mm(NULL, mm, NULL);
 }
 
 void efi_virtmap_load(void)
--

1.9.1

Quoted 17 lines · Will Deacon

Quoted 7 lines · Ard Biesheuvel

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

Quoted 8 lines

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/mmu_context.h |  2 +-
 arch/arm64/kernel/efi.c              | 14 +++++---------
 2 files changed, 6 insertions(+), 10 deletions(-)

Quoted 4 lines

Acked-by Will Deacon <[email protected]>

Please do *not* tag this for stable! ;)

OK, thanks for clarifying.

So for stable, should we keep the global mappings and do something like this instead?

"""

       cpu_set_reserved_ttbr0();

       local_flush_tlb_all();
       if (icache_is_aivivt())
               __local_flush_icache_all();

       if (mm != &init_mm)
               cpu_switch_mm(mm->pgd, mm);

"""

As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries.

So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/mmu_context.h |  2 +-
 arch/arm64/kernel/efi.c              | 14 +++++---------
 2 files changed, 6 insertions(+), 10 deletions(-)