[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

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

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/mmu_context.h |  2 +-
 arch/arm64/kernel/efi.c              | 14 +++++---------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index c0e87898ba96..24165784b803 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -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.
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index de46b50f4cdf..fc5508e0df57 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -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

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Mark Rutland]

On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
> 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.

> So instead, use non-global mappings, and perform the switch via the
> ordinary ASID-aware context switch routines.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

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?

Mark.

> ---
>  arch/arm64/include/asm/mmu_context.h |  2 +-
>  arch/arm64/kernel/efi.c              | 14 +++++---------
>  2 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> index c0e87898ba96..24165784b803 100644
> --- a/arch/arm64/include/asm/mmu_context.h
> +++ b/arch/arm64/include/asm/mmu_context.h
> @@ -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.
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index de46b50f4cdf..fc5508e0df57 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -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
> 

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Will Deacon]

On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote:
> On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
> > 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.
> 
> > So instead, use non-global mappings, and perform the switch via the
> > ordinary ASID-aware context switch routines.
> > 
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> From digging into the way the ASID allocator works, I believe this is
> correct. FWIW:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> 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? 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.

Will

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Mark Rutland]

On Tue, Nov 17, 2015 at 04:34:46PM +0000, Will Deacon wrote:
> On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote:
> > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
> > > 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.
> > 
> > > So instead, use non-global mappings, and perform the switch via the
> > > ordinary ASID-aware context switch routines.
> > > 
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > 
> > From digging into the way the ASID allocator works, I believe this is
> > correct. FWIW:
> > 
> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> > 
> > 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?

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.

Mark.

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Ard Biesheuvel]

On 17 November 2015 at 17:48, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Nov 17, 2015 at 04:34:46PM +0000, Will Deacon wrote:
>> On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote:
>> > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
>> > > 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.
>> >
>> > > So instead, use non-global mappings, and perform the switch via the
>> > > ordinary ASID-aware context switch routines.
>> > >
>> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >
>> > From digging into the way the ASID allocator works, I believe this is
>> > correct. FWIW:
>> >
>> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>> >
>> > 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?
>
> 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.

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Ard Biesheuvel]

On 17 November 2015 at 18:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 17 November 2015 at 17:48, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Tue, Nov 17, 2015 at 04:34:46PM +0000, Will Deacon wrote:
>>> On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote:
>>> > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
>>> > > 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.
>>> >
>>> > > So instead, use non-global mappings, and perform the switch via the
>>> > > ordinary ASID-aware context switch routines.
>>> > >
>>> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> >
>>> > From digging into the way the ASID allocator works, I believe this is
>>> > correct. FWIW:
>>> >
>>> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>>> >
>>> > 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?
>>
>> 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)
>

Thread is here:
http://marc.info/?l=linux-arm-kernel&m=140429592914544

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

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Will Deacon]

On Tue, Nov 17, 2015 at 06:00:36PM +0100, Ard Biesheuvel wrote:
> On 17 November 2015 at 17:48, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Nov 17, 2015 at 04:34:46PM +0000, Will Deacon wrote:
> >> On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote:
> >> > 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.
> >
> 
> 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)

So you have a double-edged sword here:

  - switch_mm must be called with interrupts enabled prior to -rc1,
    otherwise we play a song-and-dance with TIF_SWITCH_MM.

  - If you have interrupts enabled, you can receive a rollover IPI from
    another core, which means you switch to current->active_mm.

Will

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Mark Rutland]

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

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.

Mark.

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Ard Biesheuvel]

On 17 November 2015 at 18:17, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> > 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?
>> >
>> > 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.
>
> 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).
>

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.

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

-- 
Ard.

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Mark Rutland]

On Wed, Nov 18, 2015 at 07:42:27AM +0100, Ard Biesheuvel wrote:
> On 17 November 2015 at 18:17, Mark Rutland <mark.rutland@arm.com> wrote:
> >> >> > 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?
> >> >
> >> > 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.
> >
> > 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).
> >
> 
> 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.

Sorry, I got confused between switch_mm and cpu_switch_mm.

I was also worried about a concurrent rollover of the same mm's ASID
(e.g. in another thread) on another CPU, but I think that's fine. With
the old allocator the CPU handling the rollover would block until the
CPU doing the EFI call returned and enabled interrupts.

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

You are correct.

I'd misunderstood the old allocator. The CPU handling the rollover will
block until all other CPUs handled their IPI, so disabling interrupts
saves us from problems with concurrent rollover.

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

Sounds good; sorry for the noise!

Mark.

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Ard Biesheuvel]

On 17 November 2015 at 17:34, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote:
>> On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
>> > 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.
>>
>> > So instead, use non-global mappings, and perform the switch via the
>> > ordinary ASID-aware context switch routines.
>> >
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> From digging into the way the ASID allocator works, I believe this is
>> correct. FWIW:
>>
>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>>
>> 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? 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?

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Will Deacon]

On Tue, Nov 17, 2015 at 06:01:38PM +0100, Ard Biesheuvel wrote:
> On 17 November 2015 at 17:34, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote:
> >> On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
> >> > 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.
> >>
> >> > So instead, use non-global mappings, and perform the switch via the
> >> > ordinary ASID-aware context switch routines.
> >> >
> >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>
> >> From digging into the way the ASID allocator works, I believe this is
> >> correct. FWIW:
> >>
> >> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> >>
> >> 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? 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?

Sure, the current code (i.e. for anything after 4.3) looks fine. Let me
go back and ack that...

Will

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Will Deacon]

On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  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 <will.deacon@arm.com>

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

Will

> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> index c0e87898ba96..24165784b803 100644
> --- a/arch/arm64/include/asm/mmu_context.h
> +++ b/arch/arm64/include/asm/mmu_context.h
> @@ -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.
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index de46b50f4cdf..fc5508e0df57 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -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
> 

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Ard Biesheuvel]

On 17 November 2015 at 18:08, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
>> 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 <ard.biesheuvel@linaro.org>
>> ---
>>  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 <will.deacon@arm.com>
>
> 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);
"""



>> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
>> index c0e87898ba96..24165784b803 100644
>> --- a/arch/arm64/include/asm/mmu_context.h
>> +++ b/arch/arm64/include/asm/mmu_context.h
>> @@ -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.
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index de46b50f4cdf..fc5508e0df57 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -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
>>

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Will Deacon]

On Tue, Nov 17, 2015 at 06:11:56PM +0100, Ard Biesheuvel wrote:
> On 17 November 2015 at 18:08, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
> >> 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 <ard.biesheuvel@linaro.org>
> >> ---
> >>  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 <will.deacon@arm.com>
> >
> > 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);
> """

That looks good to me, but I think we'd want to give it a good testing
given that we're solving a problem that has been spotted by code inspection
as opposed to real failures on hardware. There's even an argument that
it's not worth doing anything for -stable.

Will

[RFC PATCH] arm64: use non-global mappings for UEFI runtime regions

[Catalin Marinas]

On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/mmu_context.h |  2 +-
>  arch/arm64/kernel/efi.c              | 14 +++++---------
>  2 files changed, 6 insertions(+), 10 deletions(-)

Patch applied. Thanks.

-- 
Catalin