x86/pti: Fix various fallout

The error path in write_ldt() frees the already installed LDT memory instead of the newly allocated which cannot be installed.

Fixes f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")

Reported-by Mathieu Desnoyers <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/kernel/ldt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The error path in write_ldt() tries to free old_ldt instead of the newly allocated new_ldt resulting in a memory leak. It also misses to clean up a half populated LDT pagetable, which is not a leak as it gets cleaned up when the process exits.

Free both the potentially half populated LDT pagetable and the newly allocated LDT struct. This can be done unconditionally because once a LDT is mapped subsequent maps will succeed because the PTE page is already populated and the two LDTs fit into that single page.

Fixes f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")

Reported-by Mathieu Desnoyers <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
arch/x86/kernel/ldt.c | 8 +++++++-

1 file changed, 7 insertions(+), 1 deletion(-)

arch/x86/kernel/ldt.c +2 -0

+        free_ldt_pgtables(mm);
+        free_ldt_struct(new_ldt);
       goto out_unlock;
   }
 

smpboot_setup_warm_reset_vector() and smpboot_restore_warm_reset_vector() invoke local_flush_tlb() for no obvious reason.

Digging in history revealed that the original code in the 2.1 aera added those because the code manipulated a swapper_pg_dir pagetable entry. The pagetable manipulation was removed long ago in the 2.3 timeframe, but the tlb flush invocations stayed around forever.

Remove them along with the pointless pr_debugs which come from the same 2.1 change.

Reported-by Dominik Brodowski <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/kernel/smpboot.c |    9 ---------
 1 file changed, 9 deletions(-)

arch/x86/kernel/smpboot.c +0 -4

@@ -128,14 +128,10 @@ static inline void smpboot_setup_warm_re
 	spin_lock_irqsave(&rtc_lock, flags);
 	CMOS_WRITE(0xa, 0xf);
 	spin_unlock_irqrestore(&rtc_lock, flags);
-	local_flush_tlb();
-	pr_debug("1.\n");
 	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
 							start_eip >> 4;
-	pr_debug("2.\n");
 	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
 							start_eip & 0xf;
-	pr_debug("3.\n");
 }
 
 static inline void smpboot_restore_warm_reset_vector(void)

@@ -143,11 +139,6 @@ static inline void smpboot_restore_warm_
 	unsigned long flags;

The preempt_disable/enable() pair in __native_flush_tlb() was added in commit 5cf0791da5c1 ("x86/mm: Disable preemption during CR3 read+write") to protect the UP variant of flush_tlb_mm_range().

That preempt_disable/enable() pair should have been added to the UP variant of flush_tlb_mm_range() instead.

The UP variant was removed with commit ce4a4e565f52 ("x86/mm: Remove the UP asm/tlbflush.h code, always use the (formerly) SMP code"), but the preempt_disable/enable() pair stayed around.

The latest change to __native_flush_tlb() in commit 6fd166aae78c ("x86/mm:
Use/Fix PCID to optimize user/kernel switches") added an access to a per cpu variable outside the preempt disabled regions which makes no sense at all. __native_flush_tlb() must always be called with at least preemption disabled.

Remove the preempt_disable/enable() pair and add a WARN_ON_ONCE() to catch bad callers independent of the smp_processor_id() debugging.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/include/asm/tlbflush.h |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

arch/x86/include/asm/tlbflush.h +2 -4

@@ -345,15 +345,17 @@ static inline void invalidate_user_asid(
  */
 static inline void __native_flush_tlb(void)
 {
-	invalidate_user_asid(this_cpu_read(cpu_tlbstate.loaded_mm_asid));
 	/*
-	 * If current->mm == NULL then we borrow a mm which may change
-	 * during a task switch and therefore we must not be preempted
-	 * while we write CR3 back:
+	 * Preemption or interrupts must be disabled to protect the access
+	 * to the per cpu variable and to prevent being preempted between

The small series fixes the recent fallout of the x86/pti merge:

- Remove the stale local_flush_tlb() invocations from the CPU hotplug code

- Remove the stale preempt_disable/enable() pair from __native_flush_tlb()

- Fix a bogus free in the write_ldt() error path

Linus, I suspect -rc6 is imminent, and it would be nice to at least have the LDT error path fix in. I'll send you these fixes tomorrow, but feel free to pick it up from email if you wanted to release -rc6 today.

Quoted 6 lines · Ingo Molnar

Linus, I suspect -rc6 is imminent, and it would be nice to at least have the LDT error path fix in. I'll send you these fixes tomorrow, but feel free to pick it up from email if you wanted to release -rc6 today.

I'll do rc6 tomorrow probably around this time (early afternoon PST). So I think I should be ok just waiting for your pull request.

Thanks,

Linus