Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1712434 > unrolled thread

[PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3 Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> - 2017-08-15 21:30 +0200
    Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Andy Lutomirski <luto@kernel.org> - 2017-08-15 23:50 +0200
      Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> - 2017-08-16 02:30 +0200
        Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Andy Lutomirski <luto@kernel.org> - 2017-08-16 02:50 +0200
      Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-08-16 11:40 +0200
        Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Mark Rutland <mark.rutland@arm.com> - 2017-08-16 12:00 +0200
          Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Will Deacon <will.deacon@arm.com> - 2017-08-16 12:10 +0200
            Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Mark Rutland <mark.rutland@arm.com> - 2017-08-16 13:10 +0200
              Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Matt Fleming <matt@codeblueprint.co.uk> - 2017-08-16 15:00 +0200
                Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Andy Lutomirski <luto@kernel.org> - 2017-08-16 18:20 +0200
                  Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Mark Rutland <mark.rutland@arm.com> - 2017-08-16 19:20 +0200
                    Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Will Deacon <will.deacon@arm.com> - 2017-08-17 12:40 +0200
                      Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Andy Lutomirski <luto@kernel.org> - 2017-08-17 18:00 +0200
                        Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Peter Zijlstra <peterz@infradead.org> - 2017-08-21 12:40 +0200
                          Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3 Andy Lutomirski <luto@amacapital.net> - 2017-08-21 16:00 +0200
                            Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Peter Zijlstra <peterz@infradead.org> - 2017-08-21 16:20 +0200
                              Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3 Andy Lutomirski <luto@amacapital.net> - 2017-08-21 17:30 +0200
                                Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Peter Zijlstra <peterz@infradead.org> - 2017-08-21 18:00 +0200
                                  Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-08-21 18:10 +0200
                                Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> - 2017-08-24 01:00 +0200
                            Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Peter Zijlstra <peterz@infradead.org> - 2017-08-21 19:30 +0200
      Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually  twiddling with cr3 Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> - 2017-08-25 04:50 +0200

Page 1 of 2  [1] 2  Next page →

#1712434 — [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

Use helper function (efi_switch_mm()) to switch to/from efi_mm. We
switch to efi_mm before calling
1. efi_set_virtual_address_map() and
2. Invoking any efi_runtime_service()

Likewise, we need to switch back to previous mm (mm context stolen by
efi_mm) after the above calls return successfully. We can use
efi_switch_mm() only with x86_64 kernel and "efi=old_map" disabled
because, x86_32 and efi=old_map doesn't use efi_pgd, rather they use
swapper_pg_dir.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
---
 arch/x86/include/asm/efi.h           | 30 +++++++++++++-------------
 arch/x86/platform/efi/efi_64.c       | 41 ++++++++++++++++++++++++------------
 arch/x86/platform/efi/efi_thunk_64.S |  2 +-
 3 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 2f77bcefe6b4..aa38b546e842 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -1,10 +1,14 @@
 #ifndef _ASM_X86_EFI_H
 #define _ASM_X86_EFI_H
 
+#include <linux/sched/mm.h>
+#include <linux/sched/task.h>
+
 #include <asm/fpu/api.h>
 #include <asm/pgtable.h>
 #include <asm/processor-flags.h>
 #include <asm/tlb.h>
+#include <asm/mmu_context.h>
 
 /*
  * We map the EFI regions needed for runtime services non-contiguously,
@@ -57,14 +61,14 @@ extern u64 asmlinkage efi_call(void *fp, ...);
 #define efi_call_phys(f, args...)		efi_call((f), args)
 
 /*
- * Scratch space used for switching the pagetable in the EFI stub
+ * struct efi_scratch - Scratch space used while switching to/from efi_mm
+ * @phys_stack:	stack used during EFI Mixed Mode
+ * @prev_mm:		store/restore stolen mm_struct while switching
+ * 			to/from efi_mm
  */
 struct efi_scratch {
-	u64	r15;
-	u64	prev_cr3;
-	pgd_t	*efi_pgt;
-	bool	use_pgd;
-	u64	phys_stack;
+	u64			phys_stack;
+	struct mm_struct 	*prev_mm;
 } __packed;
 
 #define arch_efi_call_virt_setup()					\
@@ -73,11 +77,8 @@ struct efi_scratch {
 	preempt_disable();						\
 	__kernel_fpu_begin();						\
 									\
-	if (efi_scratch.use_pgd) {					\
-		efi_scratch.prev_cr3 = read_cr3();			\
-		write_cr3((unsigned long)efi_scratch.efi_pgt);		\
-		__flush_tlb_all();					\
-	}								\
+	if (!efi_enabled(EFI_OLD_MEMMAP))				\
+		efi_switch_mm(&efi_mm);					\
 })
 
 #define arch_efi_call_virt(p, f, args...)				\
@@ -85,10 +86,8 @@ struct efi_scratch {
 
 #define arch_efi_call_virt_teardown()					\
 ({									\
-	if (efi_scratch.use_pgd) {					\
-		write_cr3(efi_scratch.prev_cr3);			\
-		__flush_tlb_all();					\
-	}								\
+	if (!efi_enabled(EFI_OLD_MEMMAP))				\
+		efi_switch_mm(efi_scratch.prev_mm);			\
 									\
 	__kernel_fpu_end();						\
 	preempt_enable();						\
@@ -130,6 +129,7 @@ extern void __init efi_dump_pagetable(void);
 extern void __init efi_apply_memmap_quirks(void);
 extern int __init efi_reuse_config(u64 tables, int nr_tables);
 extern void efi_delete_dummy_variable(void);
+extern void efi_switch_mm(struct mm_struct *mm);
 
 struct efi_setup_data {
 	u64 fw_vendor;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 0bb98c35e178..3be94480c1ce 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -80,9 +80,8 @@ pgd_t * __init efi_call_phys_prolog(void)
 	int n_pgds, i, j;
 
 	if (!efi_enabled(EFI_OLD_MEMMAP)) {
-		save_pgd = (pgd_t *)read_cr3();
-		write_cr3((unsigned long)efi_scratch.efi_pgt);
-		goto out;
+		efi_switch_mm(&efi_mm);
+		return NULL;
 	}
 
 	early_code_mapping_set_exec(1);
@@ -152,8 +151,7 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
 	pud_t *pud;
 
 	if (!efi_enabled(EFI_OLD_MEMMAP)) {
-		write_cr3((unsigned long)save_pgd);
-		__flush_tlb_all();
+		efi_switch_mm(efi_scratch.prev_mm);
 		return;
 	}
 
@@ -336,8 +334,6 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	if (efi_enabled(EFI_OLD_MEMMAP))
 		return 0;
 
-	efi_scratch.efi_pgt = (pgd_t *)__pa(pgd);
-
 	/*
 	 * It can happen that the physical address of new_memmap lands in memory
 	 * which is not mapped in the EFI page table. Therefore we need to go
@@ -350,8 +346,6 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 		return 1;
 	}
 
-	efi_scratch.use_pgd = true;
-
 	/*
 	 * Certain firmware versions are way too sentimential and still believe
 	 * they are exclusive and unquestionable owners of the first physical page,
@@ -596,6 +590,28 @@ void __init efi_dump_pagetable(void)
 #endif
 }
 
+/*
+ * Makes the calling kernel thread switch to/from efi_mm context
+ * Can be used from SetVirtualAddressMap() or during efi runtime calls
+ * (Note: This routine is heavily inspired from use_mm)
+ */
+void efi_switch_mm(struct mm_struct *mm)
+{
+       struct task_struct *tsk = current;
+
+       task_lock(tsk);
+       efi_scratch.prev_mm = tsk->active_mm;
+       if (efi_scratch.prev_mm != mm) {
+               mmgrab(mm);
+               tsk->active_mm = mm;
+       }
+       switch_mm(efi_scratch.prev_mm, mm, NULL);
+       task_unlock(tsk);
+
+       if (efi_scratch.prev_mm != mm)
+               mmdrop(efi_scratch.prev_mm);
+}
+
 #ifdef CONFIG_EFI_MIXED
 extern efi_status_t efi64_thunk(u32, ...);
 
@@ -649,16 +665,13 @@ efi_status_t efi_thunk_set_virtual_address_map(
 	efi_sync_low_kernel_mappings();
 	local_irq_save(flags);
 
-	efi_scratch.prev_cr3 = read_cr3();
-	write_cr3((unsigned long)efi_scratch.efi_pgt);
-	__flush_tlb_all();
+	efi_switch_mm(&efi_mm);
 
 	func = (u32)(unsigned long)phys_set_virtual_address_map;
 	status = efi64_thunk(func, memory_map_size, descriptor_size,
 			     descriptor_version, virtual_map);
 
-	write_cr3(efi_scratch.prev_cr3);
-	__flush_tlb_all();
+	efi_switch_mm(efi_scratch.prev_mm);
 	local_irq_restore(flags);
 
 	return status;
diff --git a/arch/x86/platform/efi/efi_thunk_64.S b/arch/x86/platform/efi/efi_thunk_64.S
index ff85d28c50f2..5cdc72ebbc82 100644
--- a/arch/x86/platform/efi/efi_thunk_64.S
+++ b/arch/x86/platform/efi/efi_thunk_64.S
@@ -32,7 +32,7 @@ ENTRY(efi64_thunk)
 	 * Switch to 1:1 mapped 32-bit stack pointer.
 	 */
 	movq	%rsp, efi_saved_sp(%rip)
-	movq	efi_scratch+25(%rip), %rsp
+	movq	efi_scratch(%rip), %rsp
 
 	/*
 	 * Calculate the physical address of the kernel text.
-- 
2.1.4

[toc] | [next] | [standalone]

#1712490 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
<sai.praneeth.prakhya@intel.com> wrote:
> +/*
> + * Makes the calling kernel thread switch to/from efi_mm context
> + * Can be used from SetVirtualAddressMap() or during efi runtime calls
> + * (Note: This routine is heavily inspired from use_mm)
> + */
> +void efi_switch_mm(struct mm_struct *mm)
> +{
> +       struct task_struct *tsk = current;
> +
> +       task_lock(tsk);
> +       efi_scratch.prev_mm = tsk->active_mm;
> +       if (efi_scratch.prev_mm != mm) {
> +               mmgrab(mm);
> +               tsk->active_mm = mm;
> +       }
> +       switch_mm(efi_scratch.prev_mm, mm, NULL);
> +       task_unlock(tsk);
> +
> +       if (efi_scratch.prev_mm != mm)
> +               mmdrop(efi_scratch.prev_mm);

I'm confused.  You're mmdropping an mm that you are still keeping a
pointer to.  This is also a bit confusing in the case where you do
efi_switch_mm(efi_scratch.prev_mm).

This whole manipulation seems fairly dangerous to me for another
reason -- you're taking a user thread (I think) and swapping out its
mm to something that the user in question should *not* have access to.
What if a perf interrupt happens while you're in the alternate mm?
What if you segfault and dump core?  Should we maybe just have a flag
that says "this cpu is using a funny mm", assert that the flag is
clear when scheduling, and teach perf, coredumps, etc not to touch
user memory when the flag is set?

Admittedly, the latter problem may well have existed even before these patches.

> +}
> +



>  #ifdef CONFIG_EFI_MIXED
>  extern efi_status_t efi64_thunk(u32, ...);
>
> @@ -649,16 +665,13 @@ efi_status_t efi_thunk_set_virtual_address_map(
>         efi_sync_low_kernel_mappings();
>         local_irq_save(flags);
>
> -       efi_scratch.prev_cr3 = read_cr3();
> -       write_cr3((unsigned long)efi_scratch.efi_pgt);
> -       __flush_tlb_all();
> +       efi_switch_mm(&efi_mm);
>
>         func = (u32)(unsigned long)phys_set_virtual_address_map;
>         status = efi64_thunk(func, memory_map_size, descriptor_size,
>                              descriptor_version, virtual_map);
>
> -       write_cr3(efi_scratch.prev_cr3);
> -       __flush_tlb_all();
> +       efi_switch_mm(efi_scratch.prev_mm);
>         local_irq_restore(flags);
>
>         return status;

[toc] | [prev] | [next] | [standalone]

#1712546 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Tue, 2017-08-15 at 14:46 -0700, Andy Lutomirski wrote:
> On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
> <sai.praneeth.prakhya@intel.com> wrote:
> > +/*
> > + * Makes the calling kernel thread switch to/from efi_mm context
> > + * Can be used from SetVirtualAddressMap() or during efi runtime calls
> > + * (Note: This routine is heavily inspired from use_mm)
> > + */
> > +void efi_switch_mm(struct mm_struct *mm)
> > +{
> > +       struct task_struct *tsk = current;
> > +
> > +       task_lock(tsk);
> > +       efi_scratch.prev_mm = tsk->active_mm;
> > +       if (efi_scratch.prev_mm != mm) {
> > +               mmgrab(mm);
> > +               tsk->active_mm = mm;
> > +       }
> > +       switch_mm(efi_scratch.prev_mm, mm, NULL);
> > +       task_unlock(tsk);
> > +
> > +       if (efi_scratch.prev_mm != mm)
> > +               mmdrop(efi_scratch.prev_mm);
> 

Thanks for the quick review Andy,

> I'm confused.  You're mmdropping an mm that you are still keeping a
> pointer to.  This is also a bit confusing in the case where you do
> efi_switch_mm(efi_scratch.prev_mm).
> 

This makes sense, I will look into it.

> This whole manipulation seems fairly dangerous to me for another
> reason -- you're taking a user thread (I think) and swapping out its
> mm to something that the user in question should *not* have access to.

We are switching to efi_mm from user mm_struct because
EFI_RUNTIME_SERVICES like efi_set_variable()/efi_get_variable() are
accessible only through efi_pgd. The user thread calls ioctl() which in
turn calls efi_call() and thus efi_switch_mm(). So, I think, the user
still does not have direct access to EFI_RUNTIME_SERVICES memory regions
but accesses them through sys call.

> What if a perf interrupt happens while you're in the alternate mm?

Since we are disabling/enabling interrupts around switching, I think we
are safe. We do these in following functions
phys_efi_set_virtual_address_map()
efi_thunk_set_virtual_address_map()
efi_call_virt_pointer()

> What if you segfault and dump core?

We could seg fault only if firmware touches regions which it shouldn't.
i.e. Firmware touching regions outside EFI_RUNTIME_SERVICES (this is a
UEFI Spec violation). So, in this case of buggy firmware, we panic (this
is an existing problem). We also map EFI_BOOT_TIME_SERVICES into efi_pgd
because we know some buggy firmware touches these regions.

>  Should we maybe just have a flag
> that says "this cpu is using a funny mm", assert that the flag is
> clear when scheduling, and teach perf, coredumps, etc not to touch
> user memory when the flag is set?
> 
> Admittedly, the latter problem may well have existed even before these patches.

Please let me know if you think otherwise.

Matt,
Please feel free to correct my understanding.

Regards,
Sai

[toc] | [prev] | [next] | [standalone]

#1712558 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Tue, Aug 15, 2017 at 5:23 PM, Sai Praneeth Prakhya
<sai.praneeth.prakhya@intel.com> wrote:
> On Tue, 2017-08-15 at 14:46 -0700, Andy Lutomirski wrote:
>> On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
>> <sai.praneeth.prakhya@intel.com> wrote:
>> > +/*
>> > + * Makes the calling kernel thread switch to/from efi_mm context
>> > + * Can be used from SetVirtualAddressMap() or during efi runtime calls
>> > + * (Note: This routine is heavily inspired from use_mm)
>> > + */
>> > +void efi_switch_mm(struct mm_struct *mm)
>> > +{
>> > +       struct task_struct *tsk = current;
>> > +
>> > +       task_lock(tsk);
>> > +       efi_scratch.prev_mm = tsk->active_mm;
>> > +       if (efi_scratch.prev_mm != mm) {
>> > +               mmgrab(mm);
>> > +               tsk->active_mm = mm;
>> > +       }
>> > +       switch_mm(efi_scratch.prev_mm, mm, NULL);
>> > +       task_unlock(tsk);
>> > +
>> > +       if (efi_scratch.prev_mm != mm)
>> > +               mmdrop(efi_scratch.prev_mm);
>>
>
> Thanks for the quick review Andy,
>
>> I'm confused.  You're mmdropping an mm that you are still keeping a
>> pointer to.  This is also a bit confusing in the case where you do
>> efi_switch_mm(efi_scratch.prev_mm).
>>
>
> This makes sense, I will look into it.
>
>> This whole manipulation seems fairly dangerous to me for another
>> reason -- you're taking a user thread (I think) and swapping out its
>> mm to something that the user in question should *not* have access to.
>
> We are switching to efi_mm from user mm_struct because
> EFI_RUNTIME_SERVICES like efi_set_variable()/efi_get_variable() are
> accessible only through efi_pgd. The user thread calls ioctl() which in
> turn calls efi_call() and thus efi_switch_mm(). So, I think, the user
> still does not have direct access to EFI_RUNTIME_SERVICES memory regions
> but accesses them through sys call.
>
>> What if a perf interrupt happens while you're in the alternate mm?
>
> Since we are disabling/enabling interrupts around switching, I think we
> are safe. We do these in following functions
> phys_efi_set_virtual_address_map()
> efi_thunk_set_virtual_address_map()
> efi_call_virt_pointer()

perf uses NMI, so this doesn't help.

Perhaps the sequence could look like this:

local_irq_disable();
current->active_mm = efi_mm;
switch_to();

...

switch_to(back to old mm);
current->active_mm = old mm;

and make perf know that current->active_mm != current->mm means that
user memory is off limits.

[toc] | [prev] | [next] | [standalone]

#1712801 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

(+ Mark, Will)

On 15 August 2017 at 22:46, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
> <sai.praneeth.prakhya@intel.com> wrote:
>> +/*
>> + * Makes the calling kernel thread switch to/from efi_mm context
>> + * Can be used from SetVirtualAddressMap() or during efi runtime calls
>> + * (Note: This routine is heavily inspired from use_mm)
>> + */
>> +void efi_switch_mm(struct mm_struct *mm)
>> +{
>> +       struct task_struct *tsk = current;
>> +
>> +       task_lock(tsk);
>> +       efi_scratch.prev_mm = tsk->active_mm;
>> +       if (efi_scratch.prev_mm != mm) {
>> +               mmgrab(mm);
>> +               tsk->active_mm = mm;
>> +       }
>> +       switch_mm(efi_scratch.prev_mm, mm, NULL);
>> +       task_unlock(tsk);
>> +
>> +       if (efi_scratch.prev_mm != mm)
>> +               mmdrop(efi_scratch.prev_mm);
>
> I'm confused.  You're mmdropping an mm that you are still keeping a
> pointer to.  This is also a bit confusing in the case where you do
> efi_switch_mm(efi_scratch.prev_mm).
>
> This whole manipulation seems fairly dangerous to me for another
> reason -- you're taking a user thread (I think) and swapping out its
> mm to something that the user in question should *not* have access to.
> What if a perf interrupt happens while you're in the alternate mm?
> What if you segfault and dump core?  Should we maybe just have a flag
> that says "this cpu is using a funny mm", assert that the flag is
> clear when scheduling, and teach perf, coredumps, etc not to touch
> user memory when the flag is set?
>

It appears we may have introduced this exact issue on arm64 and ARM by
starting to run the UEFI runtime services with interrupts enabled.
(perf does not use NMI on ARM, so the issue did not exist beforehand)

Mark, Will, any thoughts?

[toc] | [prev] | [next] | [standalone]

#1712818 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Wed, Aug 16, 2017 at 10:31:12AM +0100, Ard Biesheuvel wrote:
> (+ Mark, Will)
> 
> On 15 August 2017 at 22:46, Andy Lutomirski <luto@kernel.org> wrote:
> > On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
> > <sai.praneeth.prakhya@intel.com> wrote:
> >> +/*
> >> + * Makes the calling kernel thread switch to/from efi_mm context
> >> + * Can be used from SetVirtualAddressMap() or during efi runtime calls
> >> + * (Note: This routine is heavily inspired from use_mm)
> >> + */
> >> +void efi_switch_mm(struct mm_struct *mm)
> >> +{
> >> +       struct task_struct *tsk = current;
> >> +
> >> +       task_lock(tsk);
> >> +       efi_scratch.prev_mm = tsk->active_mm;
> >> +       if (efi_scratch.prev_mm != mm) {
> >> +               mmgrab(mm);
> >> +               tsk->active_mm = mm;
> >> +       }
> >> +       switch_mm(efi_scratch.prev_mm, mm, NULL);
> >> +       task_unlock(tsk);
> >> +
> >> +       if (efi_scratch.prev_mm != mm)
> >> +               mmdrop(efi_scratch.prev_mm);
> >
> > I'm confused.  You're mmdropping an mm that you are still keeping a
> > pointer to.  This is also a bit confusing in the case where you do
> > efi_switch_mm(efi_scratch.prev_mm).
> >
> > This whole manipulation seems fairly dangerous to me for another
> > reason -- you're taking a user thread (I think) and swapping out its
> > mm to something that the user in question should *not* have access to.
> > What if a perf interrupt happens while you're in the alternate mm?
> > What if you segfault and dump core?  Should we maybe just have a flag
> > that says "this cpu is using a funny mm", assert that the flag is
> > clear when scheduling, and teach perf, coredumps, etc not to touch
> > user memory when the flag is set?
> 
> It appears we may have introduced this exact issue on arm64 and ARM by
> starting to run the UEFI runtime services with interrupts enabled.
> (perf does not use NMI on ARM, so the issue did not exist beforehand)
> 
> Mark, Will, any thoughts?

Yup, I can cause perf to take samples from the EFI FW code, so that's
less than ideal.

The "funny mm" flag sounds like a good idea to me, though given recent
pain with sampling in the case of skid, I don't know exactly what we
should do if/when we take an overflow interrupt while in EFI.

Mark.

[toc] | [prev] | [next] | [standalone]

#1712821 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Wed, Aug 16, 2017 at 10:53:38AM +0100, Mark Rutland wrote:
> On Wed, Aug 16, 2017 at 10:31:12AM +0100, Ard Biesheuvel wrote:
> > (+ Mark, Will)
> > 
> > On 15 August 2017 at 22:46, Andy Lutomirski <luto@kernel.org> wrote:
> > > On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
> > > <sai.praneeth.prakhya@intel.com> wrote:
> > >> +/*
> > >> + * Makes the calling kernel thread switch to/from efi_mm context
> > >> + * Can be used from SetVirtualAddressMap() or during efi runtime calls
> > >> + * (Note: This routine is heavily inspired from use_mm)
> > >> + */
> > >> +void efi_switch_mm(struct mm_struct *mm)
> > >> +{
> > >> +       struct task_struct *tsk = current;
> > >> +
> > >> +       task_lock(tsk);
> > >> +       efi_scratch.prev_mm = tsk->active_mm;
> > >> +       if (efi_scratch.prev_mm != mm) {
> > >> +               mmgrab(mm);
> > >> +               tsk->active_mm = mm;
> > >> +       }
> > >> +       switch_mm(efi_scratch.prev_mm, mm, NULL);
> > >> +       task_unlock(tsk);
> > >> +
> > >> +       if (efi_scratch.prev_mm != mm)
> > >> +               mmdrop(efi_scratch.prev_mm);
> > >
> > > I'm confused.  You're mmdropping an mm that you are still keeping a
> > > pointer to.  This is also a bit confusing in the case where you do
> > > efi_switch_mm(efi_scratch.prev_mm).
> > >
> > > This whole manipulation seems fairly dangerous to me for another
> > > reason -- you're taking a user thread (I think) and swapping out its
> > > mm to something that the user in question should *not* have access to.
> > > What if a perf interrupt happens while you're in the alternate mm?
> > > What if you segfault and dump core?  Should we maybe just have a flag
> > > that says "this cpu is using a funny mm", assert that the flag is
> > > clear when scheduling, and teach perf, coredumps, etc not to touch
> > > user memory when the flag is set?
> > 
> > It appears we may have introduced this exact issue on arm64 and ARM by
> > starting to run the UEFI runtime services with interrupts enabled.
> > (perf does not use NMI on ARM, so the issue did not exist beforehand)
> > 
> > Mark, Will, any thoughts?
> 
> Yup, I can cause perf to take samples from the EFI FW code, so that's
> less than ideal.

But that should only happen if you're profiling EL1, right, which needs
root privileges? (assuming the skid issue is solved -- not sure what
happened to those patches after they broke criu).

> The "funny mm" flag sounds like a good idea to me, though given recent
> pain with sampling in the case of skid, I don't know exactly what we
> should do if/when we take an overflow interrupt while in EFI.

I don't think special-casing perf interrupts is the right thing to do here.
If we're concerned about user-accesses being made off the back of interrupts
taken whilst in EFI, then we should probably either swizzle back in the
user page table on the IRQ path or postpone handling it until we're done
with the firmware. Having a flag feels a bit weird: would the uaccess
routines return -EFAULT if it's set?

Will

[toc] | [prev] | [next] | [standalone]

#1712838 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Wed, Aug 16, 2017 at 11:07:10AM +0100, Will Deacon wrote:
> On Wed, Aug 16, 2017 at 10:53:38AM +0100, Mark Rutland wrote:
> > On Wed, Aug 16, 2017 at 10:31:12AM +0100, Ard Biesheuvel wrote:
> > > (+ Mark, Will)
> > > 
> > > On 15 August 2017 at 22:46, Andy Lutomirski <luto@kernel.org> wrote:
> > > > On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
> > > > <sai.praneeth.prakhya@intel.com> wrote:
> > > >> +/*
> > > >> + * Makes the calling kernel thread switch to/from efi_mm context
> > > >> + * Can be used from SetVirtualAddressMap() or during efi runtime calls
> > > >> + * (Note: This routine is heavily inspired from use_mm)
> > > >> + */
> > > >> +void efi_switch_mm(struct mm_struct *mm)
> > > >> +{
> > > >> +       struct task_struct *tsk = current;
> > > >> +
> > > >> +       task_lock(tsk);
> > > >> +       efi_scratch.prev_mm = tsk->active_mm;
> > > >> +       if (efi_scratch.prev_mm != mm) {
> > > >> +               mmgrab(mm);
> > > >> +               tsk->active_mm = mm;
> > > >> +       }
> > > >> +       switch_mm(efi_scratch.prev_mm, mm, NULL);
> > > >> +       task_unlock(tsk);
> > > >> +
> > > >> +       if (efi_scratch.prev_mm != mm)
> > > >> +               mmdrop(efi_scratch.prev_mm);
> > > >
> > > > I'm confused.  You're mmdropping an mm that you are still keeping a
> > > > pointer to.  This is also a bit confusing in the case where you do
> > > > efi_switch_mm(efi_scratch.prev_mm).
> > > >
> > > > This whole manipulation seems fairly dangerous to me for another
> > > > reason -- you're taking a user thread (I think) and swapping out its
> > > > mm to something that the user in question should *not* have access to.
> > > > What if a perf interrupt happens while you're in the alternate mm?
> > > > What if you segfault and dump core?  Should we maybe just have a flag
> > > > that says "this cpu is using a funny mm", assert that the flag is
> > > > clear when scheduling, and teach perf, coredumps, etc not to touch
> > > > user memory when the flag is set?
> > > 
> > > It appears we may have introduced this exact issue on arm64 and ARM by
> > > starting to run the UEFI runtime services with interrupts enabled.
> > > (perf does not use NMI on ARM, so the issue did not exist beforehand)
> > > 
> > > Mark, Will, any thoughts?
> > 
> > Yup, I can cause perf to take samples from the EFI FW code, so that's
> > less than ideal.
> 
> But that should only happen if you're profiling EL1, right, which needs
> root privileges? (assuming the skid issue is solved -- not sure what
> happened to those patches after they broke criu).

I *think* that only needs perf_event_paranoid < 1, rather than root.

It's certianly not accessible by default to most users (e.g. my Ubuntu
fs sets this to 2, and IIRC Debian go to a much more stringent
non-upstream paranoid level).
 
> > The "funny mm" flag sounds like a good idea to me, though given recent
> > pain with sampling in the case of skid, I don't know exactly what we
> > should do if/when we take an overflow interrupt while in EFI.
> 
> I don't think special-casing perf interrupts is the right thing to do here.
> If we're concerned about user-accesses being made off the back of interrupts
> taken whilst in EFI, then we should probably either swizzle back in the
> user page table on the IRQ path or postpone handling it until we're done
> with the firmware.

Doing that for every IRQ feels odd, especially as the result would be
sampling something that wasn't executed, potentially repeatedly, giveing
bogus info.

> Having a flag feels a bit weird: would the uaccess routines return
> -EFAULT if it's set?

I'd expect we'd abort at a higher level, not taking any sample. i.e.
we'd have the core overflow handler check in_funny_mm(), and if so, skip
the sample, as with the skid case.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1712924 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Wed, 16 Aug, at 12:03:22PM, Mark Rutland wrote:
> 
> I'd expect we'd abort at a higher level, not taking any sample. i.e.
> we'd have the core overflow handler check in_funny_mm(), and if so, skip
> the sample, as with the skid case.

FYI, this is my preferred solution for x86 too.

[toc] | [prev] | [next] | [standalone]

#1713092 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Wed, Aug 16, 2017 at 5:57 AM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Wed, 16 Aug, at 12:03:22PM, Mark Rutland wrote:
>>
>> I'd expect we'd abort at a higher level, not taking any sample. i.e.
>> we'd have the core overflow handler check in_funny_mm(), and if so, skip
>> the sample, as with the skid case.
>
> FYI, this is my preferred solution for x86 too.

One option for the "funny mm" flag would be literally the condition
current->mm != current->active_mm.  I *think* this gets all the cases
right as long as efi_switch_mm is careful with its ordering and that
the arch switch_mm() code can handle the resulting ordering.  (x86's
can now, I think, or at least will be able to in 4.14 -- not sure
about other arches).

That being said, there's a totally different solution: run EFI
callbacks in a kernel thread.  This has other benefits: we could run
those callbacks in user mode some day, and doing *that* in a user
thread seems like a mistake.

--Andy

[toc] | [prev] | [next] | [standalone]

#1713150 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Wed, Aug 16, 2017 at 09:14:41AM -0700, Andy Lutomirski wrote:
> On Wed, Aug 16, 2017 at 5:57 AM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > On Wed, 16 Aug, at 12:03:22PM, Mark Rutland wrote:
> >>
> >> I'd expect we'd abort at a higher level, not taking any sample. i.e.
> >> we'd have the core overflow handler check in_funny_mm(), and if so, skip
> >> the sample, as with the skid case.
> >
> > FYI, this is my preferred solution for x86 too.
> 
> One option for the "funny mm" flag would be literally the condition
> current->mm != current->active_mm.  I *think* this gets all the cases
> right as long as efi_switch_mm is careful with its ordering and that
> the arch switch_mm() code can handle the resulting ordering.  (x86's
> can now, I think, or at least will be able to in 4.14 -- not sure
> about other arches).

For arm64 we'd have to rework things a bit to get the ordering right
(especially when we flip to/from the idmap), but otherwise this sounds sane to
me.

> That being said, there's a totally different solution: run EFI
> callbacks in a kernel thread.  This has other benefits: we could run
> those callbacks in user mode some day, and doing *that* in a user
> thread seems like a mistake.

I think that wouldn't work for CPU-bound perf events (which are not
ctx-switched with the task).

It might be desireable to do that anyway, though.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1713805 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Tue, Aug 15, 2017 at 11:35:41PM +0100, Mark Rutland wrote:
> On Wed, Aug 16, 2017 at 09:14:41AM -0700, Andy Lutomirski wrote:
> > On Wed, Aug 16, 2017 at 5:57 AM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > > On Wed, 16 Aug, at 12:03:22PM, Mark Rutland wrote:
> > >>
> > >> I'd expect we'd abort at a higher level, not taking any sample. i.e.
> > >> we'd have the core overflow handler check in_funny_mm(), and if so, skip
> > >> the sample, as with the skid case.
> > >
> > > FYI, this is my preferred solution for x86 too.
> > 
> > One option for the "funny mm" flag would be literally the condition
> > current->mm != current->active_mm.  I *think* this gets all the cases
> > right as long as efi_switch_mm is careful with its ordering and that
> > the arch switch_mm() code can handle the resulting ordering.  (x86's
> > can now, I think, or at least will be able to in 4.14 -- not sure
> > about other arches).
> 
> For arm64 we'd have to rework things a bit to get the ordering right
> (especially when we flip to/from the idmap), but otherwise this sounds sane to
> me.
> 
> > That being said, there's a totally different solution: run EFI
> > callbacks in a kernel thread.  This has other benefits: we could run
> > those callbacks in user mode some day, and doing *that* in a user
> > thread seems like a mistake.
> 
> I think that wouldn't work for CPU-bound perf events (which are not
> ctx-switched with the task).
> 
> It might be desireable to do that anyway, though.

I'm still concerned that we're treating perf specially here -- are we
absolutely sure that nobody else is going to attempt user accesses off the
back of an interrupt? If not, then I'd much prefer a solution that catches
anybody doing that with the EFI page table installed, rather than trying
to play whack-a-mole like this.

Will

[toc] | [prev] | [next] | [standalone]

#1714172 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Thu, Aug 17, 2017 at 3:35 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Aug 15, 2017 at 11:35:41PM +0100, Mark Rutland wrote:
>> On Wed, Aug 16, 2017 at 09:14:41AM -0700, Andy Lutomirski wrote:
>> > On Wed, Aug 16, 2017 at 5:57 AM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> > > On Wed, 16 Aug, at 12:03:22PM, Mark Rutland wrote:
>> > >>
>> > >> I'd expect we'd abort at a higher level, not taking any sample. i.e.
>> > >> we'd have the core overflow handler check in_funny_mm(), and if so, skip
>> > >> the sample, as with the skid case.
>> > >
>> > > FYI, this is my preferred solution for x86 too.
>> >
>> > One option for the "funny mm" flag would be literally the condition
>> > current->mm != current->active_mm.  I *think* this gets all the cases
>> > right as long as efi_switch_mm is careful with its ordering and that
>> > the arch switch_mm() code can handle the resulting ordering.  (x86's
>> > can now, I think, or at least will be able to in 4.14 -- not sure
>> > about other arches).
>>
>> For arm64 we'd have to rework things a bit to get the ordering right
>> (especially when we flip to/from the idmap), but otherwise this sounds sane to
>> me.
>>
>> > That being said, there's a totally different solution: run EFI
>> > callbacks in a kernel thread.  This has other benefits: we could run
>> > those callbacks in user mode some day, and doing *that* in a user
>> > thread seems like a mistake.
>>
>> I think that wouldn't work for CPU-bound perf events (which are not
>> ctx-switched with the task).
>>
>> It might be desireable to do that anyway, though.
>
> I'm still concerned that we're treating perf specially here -- are we
> absolutely sure that nobody else is going to attempt user accesses off the
> back of an interrupt?

Reasonably sure?  If nothing else, an interrupt taken while mmap_sem()
is held for write that tries to access user memory is asking for
serious trouble.  There are still a few callers of pagefault_disable()
and copy...inatomic(), though.

> If not, then I'd much prefer a solution that catches
> anybody doing that with the EFI page table installed, rather than trying
> to play whack-a-mole like this.

Using a kernel thread solves the problem for real.  Anything that
blindly accesses user memory in kernel thread context is terminally
broken no matter what.

>
> Will

[toc] | [prev] | [next] | [standalone]

#1716288 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Thu, Aug 17, 2017 at 08:52:38AM -0700, Andy Lutomirski wrote:
> On Thu, Aug 17, 2017 at 3:35 AM, Will Deacon <will.deacon@arm.com> wrote:

> > I'm still concerned that we're treating perf specially here -- are we
> > absolutely sure that nobody else is going to attempt user accesses off the
> > back of an interrupt?
> 
> Reasonably sure?  If nothing else, an interrupt taken while mmap_sem()
> is held for write that tries to access user memory is asking for
> serious trouble.  There are still a few callers of pagefault_disable()
> and copy...inatomic(), though.

I'm not immediately seeing how holding mmap_sem for writing is a
problem.

> > If not, then I'd much prefer a solution that catches
> > anybody doing that with the EFI page table installed, rather than trying
> > to play whack-a-mole like this.
> 
> Using a kernel thread solves the problem for real.  Anything that
> blindly accesses user memory in kernel thread context is terminally
> broken no matter what.

So perf-callchain doesn't do it 'blindly', it wants either:

 - user_mode(regs) true, or
 - task_pt_regs() set.

However I'm thinking that if the kernel thread has ->mm == &efi_mm, the
EFI code running could very well have user_mode(regs) being true.

intel_pmu_pebs_fixup() OTOH 'blindly' assumes that the LBR addresses are
accessible. It bails on error though. So while its careful, it does
attempt to access the 'user' mapping directly. Which should also trigger
with the EFI code.

And I'm not seeing anything particularly broken with either. The PEBS
fixup relies on the CPU having just executed the code, and if it could
fetch and execute the code, why shouldn't it be able to fetch and read?
(eXecute implies Read assumed). And like said, it if triggers a fault,
it bails, no worries.

It really doesn't care if the task is a kernel thread or not. Same for
the unwinder, if we get an interrupt register set that points into
'userspace' we try and unwind it.

[toc] | [prev] | [next] | [standalone]

#1716501

> On Aug 21, 2017, at 3:33 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> On Thu, Aug 17, 2017 at 08:52:38AM -0700, Andy Lutomirski wrote:
>> On Thu, Aug 17, 2017 at 3:35 AM, Will Deacon <will.deacon@arm.com> wrote:
> 
>>> I'm still concerned that we're treating perf specially here -- are we
>>> absolutely sure that nobody else is going to attempt user accesses off the
>>> back of an interrupt?
>> 
>> Reasonably sure?  If nothing else, an interrupt taken while mmap_sem()
>> is held for write that tries to access user memory is asking for
>> serious trouble.  There are still a few callers of pagefault_disable()
>> and copy...inatomic(), though.
> 
> I'm not immediately seeing how holding mmap_sem for writing is a
> problem.
> 
>>> If not, then I'd much prefer a solution that catches
>>> anybody doing that with the EFI page table installed, rather than trying
>>> to play whack-a-mole like this.
>> 
>> Using a kernel thread solves the problem for real.  Anything that
>> blindly accesses user memory in kernel thread context is terminally
>> broken no matter what.
> 
> So perf-callchain doesn't do it 'blindly', it wants either:
> 
> - user_mode(regs) true, or
> - task_pt_regs() set.
> 
> However I'm thinking that if the kernel thread has ->mm == &efi_mm, the
> EFI code running could very well have user_mode(regs) being true.
> 
> intel_pmu_pebs_fixup() OTOH 'blindly' assumes that the LBR addresses are
> accessible. It bails on error though. So while its careful, it does
> attempt to access the 'user' mapping directly. Which should also trigger
> with the EFI code.
> 
> And I'm not seeing anything particularly broken with either. The PEBS
> fixup relies on the CPU having just executed the code, and if it could
> fetch and execute the code, why shouldn't it be able to fetch and read?

There are two ways this could be a problem.  One is that u privileged user apps shouldn't be able to read from EFI memory.  The other is that, if EFI were to have IO memory mapped at a "user" address, perf could end up reading it.

> (eXecute implies Read assumed). And like said, it if triggers a fault,
> it bails, no worries.
> 
> It really doesn't care if the task is a kernel thread or not. Same for
> the unwinder, if we get an interrupt register set that points into
> 'userspace' we try and unwind it.

[toc] | [prev] | [next] | [standalone]

#1716536 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Mon, Aug 21, 2017 at 06:56:01AM -0700, Andy Lutomirski wrote:
> 
> 
> > On Aug 21, 2017, at 3:33 AM, Peter Zijlstra <peterz@infradead.org> wrote:

> >> 
> >> Using a kernel thread solves the problem for real.  Anything that
> >> blindly accesses user memory in kernel thread context is terminally
> >> broken no matter what.
> > 
> > So perf-callchain doesn't do it 'blindly', it wants either:
> > 
> > - user_mode(regs) true, or
> > - task_pt_regs() set.
> > 
> > However I'm thinking that if the kernel thread has ->mm == &efi_mm, the
> > EFI code running could very well have user_mode(regs) being true.
> > 
> > intel_pmu_pebs_fixup() OTOH 'blindly' assumes that the LBR addresses are
> > accessible. It bails on error though. So while its careful, it does
> > attempt to access the 'user' mapping directly. Which should also trigger
> > with the EFI code.
> > 
> > And I'm not seeing anything particularly broken with either. The PEBS
> > fixup relies on the CPU having just executed the code, and if it could
> > fetch and execute the code, why shouldn't it be able to fetch and read?
> 
> There are two ways this could be a problem.  One is that u privileged
> user apps shouldn't be able to read from EFI memory.

Ah, but only root can create per-cpu events or attach events to kernel
threads (with sensible paranoia levels).

> The other is that, if EFI were to have IO memory mapped at a "user"
> address, perf could end up reading it.

Ah, but in neither mode does perf assume much, the LBR follows branches
the CPU took and thus we _know_ there was code there, not MMIO. And the
stack unwind simply follows the stack up, although I suppose it could be
'tricked' into probing MMIO. We can certainly add an "->mm !=
->active_mm" escape clause to the unwind code.

Although I don't see how we're currently avoiding the same problem with
existing userspace unwinds, userspace can equally have MMIO mapped.

But neither will use pre-existing user addresses in the efi_mm I think.

[toc] | [prev] | [next] | [standalone]

#1716595

> On Aug 21, 2017, at 7:08 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> On Mon, Aug 21, 2017 at 06:56:01AM -0700, Andy Lutomirski wrote:
>> 
>> 
>>> On Aug 21, 2017, at 3:33 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
>>>> 
>>>> Using a kernel thread solves the problem for real.  Anything that
>>>> blindly accesses user memory in kernel thread context is terminally
>>>> broken no matter what.
>>> 
>>> So perf-callchain doesn't do it 'blindly', it wants either:
>>> 
>>> - user_mode(regs) true, or
>>> - task_pt_regs() set.
>>> 
>>> However I'm thinking that if the kernel thread has ->mm == &efi_mm, the
>>> EFI code running could very well have user_mode(regs) being true.
>>> 
>>> intel_pmu_pebs_fixup() OTOH 'blindly' assumes that the LBR addresses are
>>> accessible. It bails on error though. So while its careful, it does
>>> attempt to access the 'user' mapping directly. Which should also trigger
>>> with the EFI code.
>>> 
>>> And I'm not seeing anything particularly broken with either. The PEBS
>>> fixup relies on the CPU having just executed the code, and if it could
>>> fetch and execute the code, why shouldn't it be able to fetch and read?
>> 
>> There are two ways this could be a problem.  One is that u privileged
>> user apps shouldn't be able to read from EFI memory.
> 
> Ah, but only root can create per-cpu events or attach events to kernel
> threads (with sensible paranoia levels).

But this may not need to be percpu.  If a non root user can trigger, say, an EFI variable read in their own thread context, boom.

> 
>> The other is that, if EFI were to have IO memory mapped at a "user"
>> address, perf could end up reading it.
> 
> Ah, but in neither mode does perf assume much, the LBR follows branches
> the CPU took and thus we _know_ there was code there, not MMIO. And the
> stack unwind simply follows the stack up, although I suppose it could be
> 'tricked' into probing MMIO. We can certainly add an "->mm !=
> ->active_mm" escape clause to the unwind code.
> 
> Although I don't see how we're currently avoiding the same problem with
> existing userspace unwinds, userspace can equally have MMIO mapped.

But user space at least only has IO mapped to which the user program in question has rights.

> 
> But neither will use pre-existing user addresses in the efi_mm I think.

[toc] | [prev] | [next] | [standalone]

#1716643 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Mon, Aug 21, 2017 at 08:23:10AM -0700, Andy Lutomirski wrote:
> > Ah, but only root can create per-cpu events or attach events to kernel
> > threads (with sensible paranoia levels).
> 
> But this may not need to be percpu.  If a non root user can trigger, say, an EFI variable read in their own thread context, boom.

I was going by the proposed: "everything EFI in a kthread" model. But
yes, if that's not done, then you're quite right.

> > 
> >> The other is that, if EFI were to have IO memory mapped at a "user"
> >> address, perf could end up reading it.
> > 
> > Ah, but in neither mode does perf assume much, the LBR follows branches
> > the CPU took and thus we _know_ there was code there, not MMIO. And the
> > stack unwind simply follows the stack up, although I suppose it could be
> > 'tricked' into probing MMIO. We can certainly add an "->mm !=
> > ->active_mm" escape clause to the unwind code.
> > 
> > Although I don't see how we're currently avoiding the same problem with
> > existing userspace unwinds, userspace can equally have MMIO mapped.
> 
> But user space at least only has IO mapped to which the user program in question has rights.

Still, we should not mess it up just because we're trying to unwind
stacks.

[toc] | [prev] | [next] | [standalone]

#1716652 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On 21 August 2017 at 16:59, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Aug 21, 2017 at 08:23:10AM -0700, Andy Lutomirski wrote:
>> > Ah, but only root can create per-cpu events or attach events to kernel
>> > threads (with sensible paranoia levels).
>>
>> But this may not need to be percpu.  If a non root user can trigger, say, an EFI variable read in their own thread context, boom.
>
> I was going by the proposed: "everything EFI in a kthread" model. But
> yes, if that's not done, then you're quite right.
>

How does this work in cases where we need to call into UEFI from
non-process context? Or at least from a context where current != EFI's
kthread. We have EFI pstore code, for instance, that records panic
data. Should we make an exception for those?

I'm happy to have a stab at implementing the EFI kthread, but I'd like
to get some of these details clarified first.

[toc] | [prev] | [next] | [standalone]

#1718687 — Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

On Mon, 2017-08-21 at 08:23 -0700, Andy Lutomirski wrote:
> 
> > On Aug 21, 2017, at 7:08 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> >> On Mon, Aug 21, 2017 at 06:56:01AM -0700, Andy Lutomirski wrote:
> >> 
> >> 
> >>> On Aug 21, 2017, at 3:33 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> >>>> 
> >>>> Using a kernel thread solves the problem for real.  Anything that
> >>>> blindly accesses user memory in kernel thread context is terminally
> >>>> broken no matter what.
> >>> 
> >>> So perf-callchain doesn't do it 'blindly', it wants either:
> >>> 
> >>> - user_mode(regs) true, or
> >>> - task_pt_regs() set.
> >>> 
> >>> However I'm thinking that if the kernel thread has ->mm == &efi_mm, the
> >>> EFI code running could very well have user_mode(regs) being true.
> >>> 
> >>> intel_pmu_pebs_fixup() OTOH 'blindly' assumes that the LBR addresses are
> >>> accessible. It bails on error though. So while its careful, it does
> >>> attempt to access the 'user' mapping directly. Which should also trigger
> >>> with the EFI code.
> >>> 
> >>> And I'm not seeing anything particularly broken with either. The PEBS
> >>> fixup relies on the CPU having just executed the code, and if it could
> >>> fetch and execute the code, why shouldn't it be able to fetch and read?
> >> 
> >> There are two ways this could be a problem.  One is that u privileged
> >> user apps shouldn't be able to read from EFI memory.
> > 
> > Ah, but only root can create per-cpu events or attach events to kernel
> > threads (with sensible paranoia levels).
> 
> But this may not need to be percpu.  If a non root user can trigger, say, an EFI variable read in their own thread context, boom.
> 
+ Tony

Hi Andi,

I am trying to reproduce the issue that we are discussing and hence
tried an experiment like this:
A user process continuously reads efi variable by
"cat /sys/firmware/efi/efivars/Boot0000-8be4df61-93ca-11d2-aa0d-00e098032b8c" for specified time (Eg: 100 seconds) and simultaneously I ran "perf top" as root (which I suppose should trigger NMI's). I see that everything is fine, no lockups, no kernel crash, no warnings/errors in dmesg.

I see that perf top reports 50% of time is spent in efi function
(probably efi_get_variable()).
Overhead	Shared Object	Symbol
50%		[unknown]	[k] 0xfffffffeea967416

50% is max, on avg it's 35%.

I have tested this on two kernels v4.12 and v3.19. My machine has 8
cores and to stress test, I further offlined all cpus except cpu0.

Could you please let me know a way to reproduce the issue that we are
discussing here.
I think the issue we are concerned here is, when kernel is in efi
context and an NMI happens and if the NMI handler tries to access user
space, boom! we don't have user space in efi context. Am I right in
understanding the issue or is it something else?

Regards,
Sai

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web