2017-03-22 21:33:16

by Andy Lutomirski

[permalink] [raw]
Subject: [PATCH 0/7] Misc GDT fixes and a cleanup

Hi all-

This applies to tip:x86/mm. For ease of testing, the series is here, too:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/tag/?h=review_20170322_gdt_and_wp

This fixes a few issues, most of which appear to be rather old. For
whatever reason, Thomas' GDT series unearthed them. (And one is a
genuine bug in Thomas' code but, in his defense, he might have
cut-and-pasted it verbatim from the identical bug in the EFI code.)

The last three patches are cleanups I did while tracking these down.

Boris, any chance you could test this series on Xen? The 64-bit
case works for me, but I'm having issues testing on 32-bit right
now.

Ingo, the first patch should address your concerns from the earlier
version.

Andy Lutomirski (7):
selftests/x86/ldt_gdt_32: Work around a glibc sigaction bug
x86/gdt: Fix setup_fixmap_gdt() to use the correct PA
x86/efi/32: Fix EFI on systems where the percpu GDT is virtually
mapped
x86/boot/32: Defer resyncing initial_page_table until percpu is set up
x86/gdt: Get rid of the get_*_gdt_*_vaddr() helpers
x86/xen/gdt: Use X86_FEATURE_XENPV instead of globals for the GDT
fixup
x86/boot/32: Rewrite test_wp_bit()

arch/x86/include/asm/desc.h | 21 +++-------------
arch/x86/include/asm/processor.h | 2 --
arch/x86/kernel/cpu/common.c | 28 ++++++++++++---------
arch/x86/kernel/cpu/proc.c | 5 ++--
arch/x86/kernel/setup.c | 17 -------------
arch/x86/kernel/setup_percpu.c | 21 ++++++++++++++++
arch/x86/kvm/vmx.c | 4 +--
arch/x86/mm/init_32.c | 44 +++++++--------------------------
arch/x86/platform/efi/efi_32.c | 2 +-
arch/x86/xen/enlighten.c | 4 ---
tools/testing/selftests/x86/ldt_gdt.c | 46 +++++++++++++++++++++++++++++++++++
11 files changed, 101 insertions(+), 93 deletions(-)

--
2.9.3


2017-03-22 21:33:27

by Andy Lutomirski

[permalink] [raw]
Subject: [PATCH 2/7] x86/gdt: Fix setup_fixmap_gdt() to use the correct PA

__pa() cannot be used on percpu pointers because they may be
virtually mapped. Use per_cpu_ptr_to_phys() instead.

This fixes a boot crash on a some 32-bit configurations. I assume
this is related to which allocation strategy is chosen by the percpu
core.

Fixes: 69218e47994d x86: ("Remap GDT tables in the fixmap section")
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/include/asm/desc.h | 6 ++++++
arch/x86/kernel/cpu/common.c | 4 ++--
2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index ec05f9c1a62c..bde11696b893 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -98,6 +98,12 @@ static inline unsigned long get_current_gdt_ro_vaddr(void)
return (unsigned long)get_current_gdt_ro();
}

+/* Provide the physical address of the GDT page. */
+static inline phys_addr_t get_cpu_gdt_paddr(unsigned int cpu)
+{
+ return per_cpu_ptr_to_phys(get_cpu_gdt_rw(cpu));
+}
+
#ifdef CONFIG_X86_64

static inline void pack_gate(gate_desc *gate, unsigned type, unsigned long func,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index f8e22dbad86c..f6e20e2dbfa5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -461,8 +461,8 @@ pgprot_t pg_fixmap_gdt_flags = PAGE_KERNEL;
/* Setup the fixmap mapping only once per-processor */
static inline void setup_fixmap_gdt(int cpu)
{
- __set_fixmap(get_cpu_gdt_ro_index(cpu),
- __pa(get_cpu_gdt_rw(cpu)), pg_fixmap_gdt_flags);
+ __set_fixmap(get_cpu_gdt_ro_index(cpu), get_cpu_gdt_paddr(cpu),
+ pg_fixmap_gdt_flags);
}

/* Load the original GDT from the per-cpu structure */
--
2.9.3

2017-03-22 21:33:37

by Andy Lutomirski

[permalink] [raw]
Subject: [PATCH 1/7] selftests/x86/ldt_gdt_32: Work around a glibc sigaction bug

i386 glibc is buggy and calls the sigaction syscall incorrectly.
This is asymptomatic for normal programs, but it blows up on
programs that do evil things with segmentation. ldt_gdt an example
of such an evil program.

This doesn't appear to be a regression -- I think I just got lucky
with the uninitialized memory that glibc threw at the kernel when I
wrote the test.

This hackish fix manually issues sigaction(2) syscalls to undo the
damage. Without the fix, ldt_gdt_32 segfaults; with the fix, it
passes for me.

See https://sourceware.org/bugzilla/show_bug.cgi?id=21269

Cc: [email protected]
Signed-off-by: Andy Lutomirski <[email protected]>
---
tools/testing/selftests/x86/ldt_gdt.c | 46 +++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)

diff --git a/tools/testing/selftests/x86/ldt_gdt.c b/tools/testing/selftests/x86/ldt_gdt.c
index f6121612e769..b9a22f18566a 100644
--- a/tools/testing/selftests/x86/ldt_gdt.c
+++ b/tools/testing/selftests/x86/ldt_gdt.c
@@ -409,6 +409,51 @@ static void *threadproc(void *ctx)
}
}

+#ifdef __i386__
+
+#ifndef SA_RESTORE
+#define SA_RESTORER 0x04000000
+#endif
+
+/*
+ * The UAPI header calls this 'struct sigaction', which conflicts with
+ * glibc. Sigh.
+ */
+struct fake_ksigaction {
+ void *handler; /* the real type is nasty */
+ unsigned long sa_flags;
+ void (*sa_restorer)(void);
+ unsigned char sigset[8];
+};
+
+static void fix_sa_restorer(int sig)
+{
+ struct fake_ksigaction ksa;
+
+ if (syscall(SYS_rt_sigaction, sig, NULL, &ksa, 8) == 0) {
+ /*
+ * glibc has a nasty bug: it sometimes writes garbage to
+ * sa_restorer. This interacts quite badly with anything
+ * that fiddles with SS because it can trigger legacy
+ * stack switching. Patch it up. See:
+ *
+ * https://sourceware.org/bugzilla/show_bug.cgi?id=21269
+ */
+ if (!(ksa.sa_flags & SA_RESTORER) && ksa.sa_restorer) {
+ ksa.sa_restorer = NULL;
+ if (syscall(SYS_rt_sigaction, sig, &ksa, NULL,
+ sizeof(ksa.sigset)) != 0)
+ err(1, "rt_sigaction");
+ }
+ }
+}
+#else
+static void fix_sa_restorer(int sig)
+{
+ /* 64-bit glibc works fine. */
+}
+#endif
+
static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
int flags)
{
@@ -420,6 +465,7 @@ static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
if (sigaction(sig, &sa, 0))
err(1, "sigaction");

+ fix_sa_restorer(sig);
}

static jmp_buf jmpbuf;
--
2.9.3

2017-03-22 21:33:50

by Andy Lutomirski

[permalink] [raw]
Subject: [PATCH 3/7] x86/efi/32: Fix EFI on systems where the percpu GDT is virtually mapped

__pa on a percpu pointer is invalid. This bug appears to go *waaay*
back, and I guess it's just never been triggered.

Cc: Matt Fleming <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: [email protected]
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/platform/efi/efi_32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index 950071171436..3481268da3d0 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -68,7 +68,7 @@ pgd_t * __init efi_call_phys_prolog(void)
load_cr3(initial_page_table);
__flush_tlb_all();

- gdt_descr.address = __pa(get_cpu_gdt_rw(0));
+ gdt_descr.address = get_cpu_gdt_paddr(0);
gdt_descr.size = GDT_SIZE - 1;
load_gdt(&gdt_descr);

--
2.9.3

2017-03-22 21:34:00

by Andy Lutomirski

[permalink] [raw]
Subject: [PATCH 7/7] x86/boot/32: Rewrite test_wp_bit()

This code seems to be very old and has gotten only minor updates.
Nowadays we have a shiny function probe_kernel_write() that does
more or less exactly what we need. Use it.

While we're at it, remove cpuinfo_x86::wp_works_ok, since we panic
on kernels where wp doesn't work okay.

Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/include/asm/processor.h | 2 --
arch/x86/kernel/cpu/proc.c | 5 ++---
arch/x86/kernel/setup.c | 2 --
arch/x86/mm/init_32.c | 44 ++++++++--------------------------------
arch/x86/xen/enlighten.c | 1 -
5 files changed, 11 insertions(+), 43 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index edf42c4ac8c8..2c7695c9c684 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -90,8 +90,6 @@ struct cpuinfo_x86 {
__u8 x86_model;
__u8 x86_mask;
#ifdef CONFIG_X86_32
- char wp_works_ok; /* It doesn't on 386's */
-
/* Problems on some 486Dx4's and old 386's: */
char rfu;
char pad0;
diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 18ca99f2798b..6df621ae62a7 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -31,14 +31,13 @@ static void show_cpuinfo_misc(struct seq_file *m, struct cpuinfo_x86 *c)
"fpu\t\t: %s\n"
"fpu_exception\t: %s\n"
"cpuid level\t: %d\n"
- "wp\t\t: %s\n",
+ "wp\t\t: yes\n",
static_cpu_has_bug(X86_BUG_FDIV) ? "yes" : "no",
static_cpu_has_bug(X86_BUG_F00F) ? "yes" : "no",
static_cpu_has_bug(X86_BUG_COMA) ? "yes" : "no",
static_cpu_has(X86_FEATURE_FPU) ? "yes" : "no",
static_cpu_has(X86_FEATURE_FPU) ? "yes" : "no",
- c->cpuid_level,
- c->wp_works_ok ? "yes" : "no");
+ c->cpuid_level);
}
#else
static void show_cpuinfo_misc(struct seq_file *m, struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 56b1177155db..462b7c69443d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -175,11 +175,9 @@ static struct resource bss_resource = {
#ifdef CONFIG_X86_32
/* cpu data as detected by the assembly code in head.S */
struct cpuinfo_x86 new_cpu_data = {
- .wp_works_ok = -1,
};
/* common cpu data for all cpus */
struct cpuinfo_x86 boot_cpu_data __read_mostly = {
- .wp_works_ok = -1,
};
EXPORT_SYMBOL(boot_cpu_data);

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 5ed3c141bbd5..097089a5e4d5 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -56,8 +56,6 @@

unsigned long highstart_pfn, highend_pfn;

-static noinline int do_test_wp_bit(void);
-
bool __read_mostly __vmalloc_start_set = false;

/*
@@ -726,20 +724,21 @@ void __init paging_init(void)
*/
static void __init test_wp_bit(void)
{
+ char z = 0;
+
printk(KERN_INFO
"Checking if this processor honours the WP bit even in supervisor mode...");

- /* Any page-aligned address will do, the test is non-destructive */
- __set_fixmap(FIX_WP_TEST, __pa(&swapper_pg_dir), PAGE_KERNEL_RO);
- boot_cpu_data.wp_works_ok = do_test_wp_bit();
- clear_fixmap(FIX_WP_TEST);
+ __set_fixmap(FIX_WP_TEST, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);

- if (!boot_cpu_data.wp_works_ok) {
+ if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1) == 0) {
printk(KERN_CONT "No.\n");
panic("Linux doesn't support CPUs with broken WP.");
- } else {
- printk(KERN_CONT "Ok.\n");
}
+
+ clear_fixmap(FIX_WP_TEST);
+
+ printk(KERN_CONT "Ok.\n");
}

void __init mem_init(void)
@@ -821,8 +820,7 @@ void __init mem_init(void)
BUG_ON(VMALLOC_START >= VMALLOC_END);
BUG_ON((unsigned long)high_memory > VMALLOC_START);

- if (boot_cpu_data.wp_works_ok < 0)
- test_wp_bit();
+ test_wp_bit();
}

#ifdef CONFIG_MEMORY_HOTPLUG
@@ -850,30 +848,6 @@ int arch_remove_memory(u64 start, u64 size)
#endif
#endif

-/*
- * This function cannot be __init, since exceptions don't work in that
- * section. Put this after the callers, so that it cannot be inlined.
- */
-static noinline int do_test_wp_bit(void)
-{
- char tmp_reg;
- int flag;
-
- __asm__ __volatile__(
- " movb %0, %1 \n"
- "1: movb %1, %0 \n"
- " xorl %2, %2 \n"
- "2: \n"
- _ASM_EXTABLE(1b,2b)
- :"=m" (*(char *)fix_to_virt(FIX_WP_TEST)),
- "=q" (tmp_reg),
- "=r" (flag)
- :"2" (1)
- :"memory");
-
- return flag;
-}
-
int kernel_set_to_readonly __read_mostly;

void set_kernel_text_rw(void)
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 4951fcf95143..6efa0cc425a2 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1595,7 +1595,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
/* set up basic CPUID stuff */
cpu_detect(&new_cpu_data);
set_cpu_cap(&new_cpu_data, X86_FEATURE_FPU);
- new_cpu_data.wp_works_ok = 1;
new_cpu_data.x86_capability[CPUID_1_EDX] = cpuid_edx(1);
#endif

--
2.9.3

2017-03-22 21:34:11

by Andy Lutomirski

[permalink] [raw]
Subject: [PATCH 6/7] x86/xen/gdt: Use X86_FEATURE_XENPV instead of globals for the GDT fixup

Xen imposes special requirements on the GDT. Rather than using a
global variable for the pgprot, just use an explicit special case
for Xen -- this makes it clearer what's going on. It also debloats
64-bit kernels very slightly.

Cc: Boris Ostrovsky <[email protected]>
Cc: Juergen Gross <[email protected]>
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/include/asm/desc.h | 1 -
arch/x86/kernel/cpu/common.c | 28 +++++++++++++++++-----------
arch/x86/xen/enlighten.c | 3 ---
3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 17cb46e8a184..d0a21b12dd58 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -39,7 +39,6 @@ extern struct desc_ptr idt_descr;
extern gate_desc idt_table[];
extern const struct desc_ptr debug_idt_descr;
extern gate_desc debug_idt_table[];
-extern pgprot_t pg_fixmap_gdt_flags;

struct gdt_page {
struct desc_struct gdt[GDT_ENTRIES];
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index f6e20e2dbfa5..8ee32119144d 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -448,21 +448,27 @@ void load_percpu_segment(int cpu)
load_stack_canary_segment();
}

-/*
- * On 64-bit the GDT remapping is read-only.
- * A global is used for Xen to change the default when required.
- */
+/* Setup the fixmap mapping only once per-processor */
+static inline void setup_fixmap_gdt(int cpu)
+{
#ifdef CONFIG_X86_64
-pgprot_t pg_fixmap_gdt_flags = PAGE_KERNEL_RO;
+ /* On 64-bit systems, we use a read-only fixmap GDT. */
+ pgprot_t prot = PAGE_KERNEL_RO;
#else
-pgprot_t pg_fixmap_gdt_flags = PAGE_KERNEL;
+ /*
+ * On native 32-bit systems, the GDT cannot be read-only because
+ * our double fault handler uses a task gate, and entering through
+ * a task gate needs to change an available TSS to busy. If the GDT
+ * is read-only, that will triple fault.
+ *
+ * On Xen PV, the GDT must be read-only because the hypervisor requires
+ * it.
+ */
+ pgprot_t prot = boot_cpu_has(X86_FEATURE_XENPV) ?
+ PAGE_KERNEL_RO : PAGE_KERNEL;
#endif

-/* Setup the fixmap mapping only once per-processor */
-static inline void setup_fixmap_gdt(int cpu)
-{
- __set_fixmap(get_cpu_gdt_ro_index(cpu), get_cpu_gdt_paddr(cpu),
- pg_fixmap_gdt_flags);
+ __set_fixmap(get_cpu_gdt_ro_index(cpu), get_cpu_gdt_paddr(cpu), prot);
}

/* Load the original GDT from the per-cpu structure */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 08faa61de5f7..4951fcf95143 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1545,9 +1545,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
*/
xen_initial_gdt = &per_cpu(gdt_page, 0);

- /* GDT can only be remapped RO */
- pg_fixmap_gdt_flags = PAGE_KERNEL_RO;
-
xen_smp_init();

#ifdef CONFIG_ACPI_NUMA
--
2.9.3

2017-03-22 21:34:23

by Andy Lutomirski

[permalink] [raw]
Subject: [PATCH 5/7] x86/gdt: Get rid of the get_*_gdt_*_vaddr() helpers

There's a single caller that is only there because it's passing a
pointer into a function (vmcs_writel()) that takes an unsigned long.
Let's just cast it in place rather than having a bunch of trivial
helpers.

Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/include/asm/desc.h | 20 --------------------
arch/x86/kvm/vmx.c | 4 ++--
2 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index bde11696b893..17cb46e8a184 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -53,22 +53,12 @@ static inline struct desc_struct *get_cpu_gdt_rw(unsigned int cpu)
return per_cpu(gdt_page, cpu).gdt;
}

-static inline unsigned long get_cpu_gdt_rw_vaddr(unsigned int cpu)
-{
- return (unsigned long)get_cpu_gdt_rw(cpu);
-}
-
/* Provide the current original GDT */
static inline struct desc_struct *get_current_gdt_rw(void)
{
return this_cpu_ptr(&gdt_page)->gdt;
}

-static inline unsigned long get_current_gdt_rw_vaddr(void)
-{
- return (unsigned long)get_current_gdt_rw();
-}
-
/* Get the fixmap index for a specific processor */
static inline unsigned int get_cpu_gdt_ro_index(int cpu)
{
@@ -82,22 +72,12 @@ static inline struct desc_struct *get_cpu_gdt_ro(int cpu)
return (struct desc_struct *)__fix_to_virt(idx);
}

-static inline unsigned long get_cpu_gdt_ro_vaddr(int cpu)
-{
- return (unsigned long)get_cpu_gdt_ro(cpu);
-}
-
/* Provide the current read-only GDT */
static inline struct desc_struct *get_current_gdt_ro(void)
{
return get_cpu_gdt_ro(smp_processor_id());
}

-static inline unsigned long get_current_gdt_ro_vaddr(void)
-{
- return (unsigned long)get_current_gdt_ro();
-}
-
/* Provide the physical address of the GDT page. */
static inline phys_addr_t get_cpu_gdt_paddr(unsigned int cpu)
{
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 596a76d82b11..3acde663dc58 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2264,7 +2264,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
}

if (!already_loaded) {
- unsigned long gdt = get_current_gdt_ro_vaddr();
+ void *gdt = get_current_gdt_ro();
unsigned long sysenter_esp;

kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
@@ -2275,7 +2275,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
*/
vmcs_writel(HOST_TR_BASE,
(unsigned long)this_cpu_ptr(&cpu_tss));
- vmcs_writel(HOST_GDTR_BASE, gdt); /* 22.2.4 */
+ vmcs_writel(HOST_GDTR_BASE, (unsigned long)gdt); /* 22.2.4 */

/*
* VM exits change the host TR limit to 0x67 after a VM
--
2.9.3

2017-03-22 21:34:54

by Andy Lutomirski

[permalink] [raw]
Subject: [PATCH 4/7] x86/boot/32: Defer resyncing initial_page_table until percpu is set up

The x86 smpboot trampoline expects initial_page_table to have the
GDT mapped. If the GDT ends up in a virtually mapped percpu page,
then it won't be in the page tables at all until percpu areas are
set up. The result will be a triple fault the first time that the
CPU attempts to access the GDT after LGDT loads the percpu GDT.

This appears to be an old bug, but somehow the GDT fixmap rework
is triggering it. This seems to have something to do with the
memory layout.

Cc: Matt Fleming <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: [email protected]
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/kernel/setup.c | 15 ---------------
arch/x86/kernel/setup_percpu.c | 21 +++++++++++++++++++++
2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 4bf0c8926a1c..56b1177155db 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1226,21 +1226,6 @@ void __init setup_arch(char **cmdline_p)

kasan_init();

-#ifdef CONFIG_X86_32
- /* sync back kernel address range */
- clone_pgd_range(initial_page_table + KERNEL_PGD_BOUNDARY,
- swapper_pg_dir + KERNEL_PGD_BOUNDARY,
- KERNEL_PGD_PTRS);
-
- /*
- * sync back low identity map too. It is used for example
- * in the 32-bit EFI stub.
- */
- clone_pgd_range(initial_page_table,
- swapper_pg_dir + KERNEL_PGD_BOUNDARY,
- min(KERNEL_PGD_PTRS, KERNEL_PGD_BOUNDARY));
-#endif
-
tboot_probe();

map_vsyscall();
diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index 11338b0b3ad2..bb1e8cc0bc84 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -288,4 +288,25 @@ void __init setup_per_cpu_areas(void)

/* Setup cpu initialized, callin, callout masks */
setup_cpu_local_masks();
+
+#ifdef CONFIG_X86_32
+ /*
+ * Sync back kernel address range. We want to make sure that
+ * all kernel mappings, including percpu mappings, are available
+ * in the smpboot asm. We can't reliably pick up percpu
+ * mappings using vmalloc_fault(), because exception dispatch
+ * needs percpu data.
+ */
+ clone_pgd_range(initial_page_table + KERNEL_PGD_BOUNDARY,
+ swapper_pg_dir + KERNEL_PGD_BOUNDARY,
+ KERNEL_PGD_PTRS);
+
+ /*
+ * sync back low identity map too. It is used for example
+ * in the 32-bit EFI stub.
+ */
+ clone_pgd_range(initial_page_table,
+ swapper_pg_dir + KERNEL_PGD_BOUNDARY,
+ min(KERNEL_PGD_PTRS, KERNEL_PGD_BOUNDARY));
+#endif
}
--
2.9.3

2017-03-23 07:31:54

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 0/7] Misc GDT fixes and a cleanup


* Andy Lutomirski <[email protected]> wrote:

> Hi all-
>
> This applies to tip:x86/mm. For ease of testing, the series is here, too:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/tag/?h=review_20170322_gdt_and_wp
>
> This fixes a few issues, most of which appear to be rather old. For
> whatever reason, Thomas' GDT series unearthed them. (And one is a
> genuine bug in Thomas' code but, in his defense, he might have
> cut-and-pasted it verbatim from the identical bug in the EFI code.)
>
> The last three patches are cleanups I did while tracking these down.
>
> Boris, any chance you could test this series on Xen? The 64-bit
> case works for me, but I'm having issues testing on 32-bit right
> now.
>
> Ingo, the first patch should address your concerns from the earlier
> version.
>
> Andy Lutomirski (7):
> selftests/x86/ldt_gdt_32: Work around a glibc sigaction bug
> x86/gdt: Fix setup_fixmap_gdt() to use the correct PA
> x86/efi/32: Fix EFI on systems where the percpu GDT is virtually
> mapped
> x86/boot/32: Defer resyncing initial_page_table until percpu is set up
> x86/gdt: Get rid of the get_*_gdt_*_vaddr() helpers
> x86/xen/gdt: Use X86_FEATURE_XENPV instead of globals for the GDT
> fixup
> x86/boot/32: Rewrite test_wp_bit()

Ok, looks mostly good to me and I've applied the first 6 patches to tip:x86/mm and
will push them out if everything tests out fine.

Regarding patch #7: could you please split the last patch into two, and rebase the
wp_works_ok removal on the very latest tip:x86/mm tree? There's some pending
changes in tip:x86/process that conflict badly, so I've merged it into tip:x86/mm
for a conflict-free base. That patch is better split in two anyway, as it does two
only marginally related things.

Thanks,

Ingo

Subject: [tip:x86/mm] selftests/x86/ldt_gdt_32: Work around a glibc sigaction() bug

Commit-ID: 65973dd3fd31151823f4b8c289eebbb3fb7e6bc0
Gitweb: http://git.kernel.org/tip/65973dd3fd31151823f4b8c289eebbb3fb7e6bc0
Author: Andy Lutomirski <[email protected]>
AuthorDate: Wed, 22 Mar 2017 14:32:29 -0700
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 23 Mar 2017 08:25:07 +0100

selftests/x86/ldt_gdt_32: Work around a glibc sigaction() bug

i386 glibc is buggy and calls the sigaction syscall incorrectly.

This is asymptomatic for normal programs, but it blows up on
programs that do evil things with segmentation. The ldt_gdt
self-test is an example of such an evil program.

This doesn't appear to be a regression -- I think I just got lucky
with the uninitialized memory that glibc threw at the kernel when I
wrote the test.

This hackish fix manually issues sigaction(2) syscalls to undo the
damage. Without the fix, ldt_gdt_32 segfaults; with the fix, it
passes for me.

See: https://sourceware.org/bugzilla/show_bug.cgi?id=21269

Signed-off-by: Andy Lutomirski <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Garnier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/aaab0f9f93c9af25396f01232608c163a760a668.1490218061.git.luto@kernel.org
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/testing/selftests/x86/ldt_gdt.c | 46 +++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)

diff --git a/tools/testing/selftests/x86/ldt_gdt.c b/tools/testing/selftests/x86/ldt_gdt.c
index f612161..b9a22f1 100644
--- a/tools/testing/selftests/x86/ldt_gdt.c
+++ b/tools/testing/selftests/x86/ldt_gdt.c
@@ -409,6 +409,51 @@ static void *threadproc(void *ctx)
}
}

+#ifdef __i386__
+
+#ifndef SA_RESTORE
+#define SA_RESTORER 0x04000000
+#endif
+
+/*
+ * The UAPI header calls this 'struct sigaction', which conflicts with
+ * glibc. Sigh.
+ */
+struct fake_ksigaction {
+ void *handler; /* the real type is nasty */
+ unsigned long sa_flags;
+ void (*sa_restorer)(void);
+ unsigned char sigset[8];
+};
+
+static void fix_sa_restorer(int sig)
+{
+ struct fake_ksigaction ksa;
+
+ if (syscall(SYS_rt_sigaction, sig, NULL, &ksa, 8) == 0) {
+ /*
+ * glibc has a nasty bug: it sometimes writes garbage to
+ * sa_restorer. This interacts quite badly with anything
+ * that fiddles with SS because it can trigger legacy
+ * stack switching. Patch it up. See:
+ *
+ * https://sourceware.org/bugzilla/show_bug.cgi?id=21269
+ */
+ if (!(ksa.sa_flags & SA_RESTORER) && ksa.sa_restorer) {
+ ksa.sa_restorer = NULL;
+ if (syscall(SYS_rt_sigaction, sig, &ksa, NULL,
+ sizeof(ksa.sigset)) != 0)
+ err(1, "rt_sigaction");
+ }
+ }
+}
+#else
+static void fix_sa_restorer(int sig)
+{
+ /* 64-bit glibc works fine. */
+}
+#endif
+
static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
int flags)
{
@@ -420,6 +465,7 @@ static void sethandler(int sig, void (*handler)(int, siginfo_t *, void *),
if (sigaction(sig, &sa, 0))
err(1, "sigaction");

+ fix_sa_restorer(sig);
}

static jmp_buf jmpbuf;

Subject: [tip:x86/mm] x86/gdt: Fix setup_fixmap_gdt() to use the correct PA

Commit-ID: aa4ea675528f3fa11e9663e5a32f55a81c34dcac
Gitweb: http://git.kernel.org/tip/aa4ea675528f3fa11e9663e5a32f55a81c34dcac
Author: Andy Lutomirski <[email protected]>
AuthorDate: Wed, 22 Mar 2017 14:32:30 -0700
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 23 Mar 2017 08:25:07 +0100

x86/gdt: Fix setup_fixmap_gdt() to use the correct PA

__pa() cannot be used on percpu pointers because they may be
virtually mapped. Use per_cpu_ptr_to_phys() instead.

This fixes a boot crash on a some 32-bit configurations. I assume
this is related to which allocation strategy is chosen by the percpu
core.

Signed-off-by: Andy Lutomirski <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Garnier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Fixes: 69218e47994d x86: ("Remap GDT tables in the fixmap section")
Link: http://lkml.kernel.org/r/22e0069c29fba31998f193201e359eebfdac4960.1490218061.git.luto@kernel.org
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/desc.h | 6 ++++++
arch/x86/kernel/cpu/common.c | 4 ++--
2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index ec05f9c..bde1169 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -98,6 +98,12 @@ static inline unsigned long get_current_gdt_ro_vaddr(void)
return (unsigned long)get_current_gdt_ro();
}

+/* Provide the physical address of the GDT page. */
+static inline phys_addr_t get_cpu_gdt_paddr(unsigned int cpu)
+{
+ return per_cpu_ptr_to_phys(get_cpu_gdt_rw(cpu));
+}
+
#ifdef CONFIG_X86_64

static inline void pack_gate(gate_desc *gate, unsigned type, unsigned long func,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index f8e22db..f6e20e2 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -461,8 +461,8 @@ pgprot_t pg_fixmap_gdt_flags = PAGE_KERNEL;
/* Setup the fixmap mapping only once per-processor */
static inline void setup_fixmap_gdt(int cpu)
{
- __set_fixmap(get_cpu_gdt_ro_index(cpu),
- __pa(get_cpu_gdt_rw(cpu)), pg_fixmap_gdt_flags);
+ __set_fixmap(get_cpu_gdt_ro_index(cpu), get_cpu_gdt_paddr(cpu),
+ pg_fixmap_gdt_flags);
}

/* Load the original GDT from the per-cpu structure */

Subject: [tip:x86/mm] x86/efi/32: Fix EFI on systems where the per-cpu GDT is virtually mapped

Commit-ID: 3fa1cabbc3b61224ef33d3ca4a1a96998529bc68
Gitweb: http://git.kernel.org/tip/3fa1cabbc3b61224ef33d3ca4a1a96998529bc68
Author: Andy Lutomirski <[email protected]>
AuthorDate: Wed, 22 Mar 2017 14:32:31 -0700
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 23 Mar 2017 08:25:07 +0100

x86/efi/32: Fix EFI on systems where the per-cpu GDT is virtually mapped

__pa() on a per-cpu pointer is invalid. This bug appears to go *waaay*
back, and I guess it's just never been triggered.

Signed-off-by: Andy Lutomirski <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Garnier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/5ba1d3ffca85e1a5b3ac99265ebe55df4cf0dbe4.1490218061.git.luto@kernel.org
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/platform/efi/efi_32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index 9500711..3481268 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -68,7 +68,7 @@ pgd_t * __init efi_call_phys_prolog(void)
load_cr3(initial_page_table);
__flush_tlb_all();

- gdt_descr.address = __pa(get_cpu_gdt_rw(0));
+ gdt_descr.address = get_cpu_gdt_paddr(0);
gdt_descr.size = GDT_SIZE - 1;
load_gdt(&gdt_descr);


Subject: [tip:x86/mm] x86/boot/32: Defer resyncing initial_page_table until per-cpu is set up

Commit-ID: 23b2a4ddebdd17fad265b4bb77256c2e4ec37dee
Gitweb: http://git.kernel.org/tip/23b2a4ddebdd17fad265b4bb77256c2e4ec37dee
Author: Andy Lutomirski <[email protected]>
AuthorDate: Wed, 22 Mar 2017 14:32:32 -0700
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 23 Mar 2017 08:25:08 +0100

x86/boot/32: Defer resyncing initial_page_table until per-cpu is set up

The x86 smpboot trampoline expects initial_page_table to have the
GDT mapped. If the GDT ends up in a virtually mapped per-cpu page,
then it won't be in the page tables at all until perc-pu areas are
set up. The result will be a triple fault the first time that the
CPU attempts to access the GDT after LGDT loads the perc-pu GDT.

This appears to be an old bug, but somehow the GDT fixmap rework
is triggering it. This seems to have something to do with the
memory layout.

Signed-off-by: Andy Lutomirski <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Garnier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/a553264a5972c6a86f9b5caac237470a0c74a720.1490218061.git.luto@kernel.org
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/setup.c | 15 ---------------
arch/x86/kernel/setup_percpu.c | 21 +++++++++++++++++++++
2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 4bf0c89..56b1177 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1226,21 +1226,6 @@ void __init setup_arch(char **cmdline_p)

kasan_init();

-#ifdef CONFIG_X86_32
- /* sync back kernel address range */
- clone_pgd_range(initial_page_table + KERNEL_PGD_BOUNDARY,
- swapper_pg_dir + KERNEL_PGD_BOUNDARY,
- KERNEL_PGD_PTRS);
-
- /*
- * sync back low identity map too. It is used for example
- * in the 32-bit EFI stub.
- */
- clone_pgd_range(initial_page_table,
- swapper_pg_dir + KERNEL_PGD_BOUNDARY,
- min(KERNEL_PGD_PTRS, KERNEL_PGD_BOUNDARY));
-#endif
-
tboot_probe();

map_vsyscall();
diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index 11338b0..bb1e8cc 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -288,4 +288,25 @@ void __init setup_per_cpu_areas(void)

/* Setup cpu initialized, callin, callout masks */
setup_cpu_local_masks();
+
+#ifdef CONFIG_X86_32
+ /*
+ * Sync back kernel address range. We want to make sure that
+ * all kernel mappings, including percpu mappings, are available
+ * in the smpboot asm. We can't reliably pick up percpu
+ * mappings using vmalloc_fault(), because exception dispatch
+ * needs percpu data.
+ */
+ clone_pgd_range(initial_page_table + KERNEL_PGD_BOUNDARY,
+ swapper_pg_dir + KERNEL_PGD_BOUNDARY,
+ KERNEL_PGD_PTRS);
+
+ /*
+ * sync back low identity map too. It is used for example
+ * in the 32-bit EFI stub.
+ */
+ clone_pgd_range(initial_page_table,
+ swapper_pg_dir + KERNEL_PGD_BOUNDARY,
+ min(KERNEL_PGD_PTRS, KERNEL_PGD_BOUNDARY));
+#endif
}

Subject: [tip:x86/mm] x86/gdt: Get rid of the get_*_gdt_*_vaddr() helpers

Commit-ID: 59c58ceb29d0f030eddb36a3a9dbadcc499786a6
Gitweb: http://git.kernel.org/tip/59c58ceb29d0f030eddb36a3a9dbadcc499786a6
Author: Andy Lutomirski <[email protected]>
AuthorDate: Wed, 22 Mar 2017 14:32:33 -0700
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 23 Mar 2017 08:25:08 +0100

x86/gdt: Get rid of the get_*_gdt_*_vaddr() helpers

There's a single caller that is only there because it's passing a
pointer into a function (vmcs_writel()) that takes an unsigned long.
Let's just cast it in place rather than having a bunch of trivial
helpers.

Signed-off-by: Andy Lutomirski <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Garnier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/46108fb35e1699252b1b6a85039303ff562c9836.1490218061.git.luto@kernel.org
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/desc.h | 20 --------------------
arch/x86/kvm/vmx.c | 4 ++--
2 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index bde1169..17cb46e 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -53,22 +53,12 @@ static inline struct desc_struct *get_cpu_gdt_rw(unsigned int cpu)
return per_cpu(gdt_page, cpu).gdt;
}

-static inline unsigned long get_cpu_gdt_rw_vaddr(unsigned int cpu)
-{
- return (unsigned long)get_cpu_gdt_rw(cpu);
-}
-
/* Provide the current original GDT */
static inline struct desc_struct *get_current_gdt_rw(void)
{
return this_cpu_ptr(&gdt_page)->gdt;
}

-static inline unsigned long get_current_gdt_rw_vaddr(void)
-{
- return (unsigned long)get_current_gdt_rw();
-}
-
/* Get the fixmap index for a specific processor */
static inline unsigned int get_cpu_gdt_ro_index(int cpu)
{
@@ -82,22 +72,12 @@ static inline struct desc_struct *get_cpu_gdt_ro(int cpu)
return (struct desc_struct *)__fix_to_virt(idx);
}

-static inline unsigned long get_cpu_gdt_ro_vaddr(int cpu)
-{
- return (unsigned long)get_cpu_gdt_ro(cpu);
-}
-
/* Provide the current read-only GDT */
static inline struct desc_struct *get_current_gdt_ro(void)
{
return get_cpu_gdt_ro(smp_processor_id());
}

-static inline unsigned long get_current_gdt_ro_vaddr(void)
-{
- return (unsigned long)get_current_gdt_ro();
-}
-
/* Provide the physical address of the GDT page. */
static inline phys_addr_t get_cpu_gdt_paddr(unsigned int cpu)
{
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 596a76d..3acde66 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2264,7 +2264,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
}

if (!already_loaded) {
- unsigned long gdt = get_current_gdt_ro_vaddr();
+ void *gdt = get_current_gdt_ro();
unsigned long sysenter_esp;

kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
@@ -2275,7 +2275,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
*/
vmcs_writel(HOST_TR_BASE,
(unsigned long)this_cpu_ptr(&cpu_tss));
- vmcs_writel(HOST_GDTR_BASE, gdt); /* 22.2.4 */
+ vmcs_writel(HOST_GDTR_BASE, (unsigned long)gdt); /* 22.2.4 */

/*
* VM exits change the host TR limit to 0x67 after a VM

Subject: [tip:x86/mm] x86/xen/gdt: Use X86_FEATURE_XENPV instead of globals for the GDT fixup

Commit-ID: b23adb7d3f7d1d7cce03db9704de67a99ceeda38
Gitweb: http://git.kernel.org/tip/b23adb7d3f7d1d7cce03db9704de67a99ceeda38
Author: Andy Lutomirski <[email protected]>
AuthorDate: Wed, 22 Mar 2017 14:32:34 -0700
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 23 Mar 2017 08:25:08 +0100

x86/xen/gdt: Use X86_FEATURE_XENPV instead of globals for the GDT fixup

Xen imposes special requirements on the GDT. Rather than using a
global variable for the pgprot, just use an explicit special case
for Xen -- this makes it clearer what's going on. It also debloats
64-bit kernels very slightly.

Signed-off-by: Andy Lutomirski <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Garnier <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/e9ea96abbfd6a8c87753849171bb5987ecfeb523.1490218061.git.luto@kernel.org
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/desc.h | 1 -
arch/x86/kernel/cpu/common.c | 28 +++++++++++++++++-----------
arch/x86/xen/enlighten.c | 3 ---
3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 17cb46e..d0a21b1 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -39,7 +39,6 @@ extern struct desc_ptr idt_descr;
extern gate_desc idt_table[];
extern const struct desc_ptr debug_idt_descr;
extern gate_desc debug_idt_table[];
-extern pgprot_t pg_fixmap_gdt_flags;

struct gdt_page {
struct desc_struct gdt[GDT_ENTRIES];
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index f6e20e2..8ee3211 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -448,21 +448,27 @@ void load_percpu_segment(int cpu)
load_stack_canary_segment();
}

-/*
- * On 64-bit the GDT remapping is read-only.
- * A global is used for Xen to change the default when required.
- */
+/* Setup the fixmap mapping only once per-processor */
+static inline void setup_fixmap_gdt(int cpu)
+{
#ifdef CONFIG_X86_64
-pgprot_t pg_fixmap_gdt_flags = PAGE_KERNEL_RO;
+ /* On 64-bit systems, we use a read-only fixmap GDT. */
+ pgprot_t prot = PAGE_KERNEL_RO;
#else
-pgprot_t pg_fixmap_gdt_flags = PAGE_KERNEL;
+ /*
+ * On native 32-bit systems, the GDT cannot be read-only because
+ * our double fault handler uses a task gate, and entering through
+ * a task gate needs to change an available TSS to busy. If the GDT
+ * is read-only, that will triple fault.
+ *
+ * On Xen PV, the GDT must be read-only because the hypervisor requires
+ * it.
+ */
+ pgprot_t prot = boot_cpu_has(X86_FEATURE_XENPV) ?
+ PAGE_KERNEL_RO : PAGE_KERNEL;
#endif

-/* Setup the fixmap mapping only once per-processor */
-static inline void setup_fixmap_gdt(int cpu)
-{
- __set_fixmap(get_cpu_gdt_ro_index(cpu), get_cpu_gdt_paddr(cpu),
- pg_fixmap_gdt_flags);
+ __set_fixmap(get_cpu_gdt_ro_index(cpu), get_cpu_gdt_paddr(cpu), prot);
}

/* Load the original GDT from the per-cpu structure */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 08faa61..4951fcf 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1545,9 +1545,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
*/
xen_initial_gdt = &per_cpu(gdt_page, 0);

- /* GDT can only be remapped RO */
- pg_fixmap_gdt_flags = PAGE_KERNEL_RO;
-
xen_smp_init();

#ifdef CONFIG_ACPI_NUMA

2017-03-23 12:19:24

by Boris Ostrovsky

[permalink] [raw]
Subject: Re: [PATCH 0/7] Misc GDT fixes and a cleanup

On 03/22/2017 05:32 PM, Andy Lutomirski wrote:
> Hi all-
>
> This applies to tip:x86/mm. For ease of testing, the series is here, too:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/tag/?h=review_20170322_gdt_and_wp
>
> This fixes a few issues, most of which appear to be rather old. For
> whatever reason, Thomas' GDT series unearthed them. (And one is a
> genuine bug in Thomas' code but, in his defense, he might have
> cut-and-pasted it verbatim from the identical bug in the EFI code.)
>
> The last three patches are cleanups I did while tracking these down.
>
> Boris, any chance you could test this series on Xen? The 64-bit
> case works for me, but I'm having issues testing on 32-bit right
> now.

Yes, this is all good. Tests passed.

-boris