Messages in this thread | | | From | Andy Lutomirski <> | Date | Mon, 11 Dec 2017 10:40:13 -0800 | Subject | Re: [PATCH PTI v2 6/6] x86/pti: Put the LDT in its own PGD if PTI is on |
| |
On Mon, Dec 11, 2017 at 9:49 AM, Dave Hansen <dave.hansen@intel.com> wrote: > So, before this, > > On 12/10/2017 10:47 PM, Andy Lutomirski wrote: > ...> + if (unlikely(ldt)) { >> + if (static_cpu_has_bug(X86_BUG_CPU_SECURE_MODE_PTI)) { >> + if (WARN_ON_ONCE((unsigned long)ldt->slot > 1)) { >> + clear_LDT(); >> + return; >> + } > > I'm missing the purpose of the slots. Are you hoping to use those > eventually for randomization, but just punting on implementing it for now? > >> + >> + set_ldt(ldt_slot_va(ldt->slot), ldt->nr_entries); >> + } else { >> + set_ldt(ldt->entries, ldt->nr_entries); >> + } > > This seems like a much better place to point out why the aliasing exists > and what it is doing than the place it is actually commented. > > Maybe: > > /* > * ldt->entries is not mapped into the user page > * tables when page table isolation is enabled. > * Point the hardware to the alias we created. > */ > set_ldt(ldt_slot_va(ldt->slot), ... > } else { > /* > * Point the hardware at the normal kernel > * mapping when not isolated. > */ > set_ldt(ldt->entries, ldt->nr_entries); > } >
Good call.
>> /* >> - * User space process size. 47bits minus one guard page. The guard >> - * page is necessary on Intel CPUs: if a SYSCALL instruction is at >> - * the highest possible canonical userspace address, then that >> - * syscall will enter the kernel with a non-canonical return >> - * address, and SYSRET will explode dangerously. We avoid this >> - * particular problem by preventing anything from being mapped >> - * at the maximum canonical address. >> + * User space process size. This is the first address outside the user range. >> + * There are a few constraints that determine this: >> + * >> + * On Intel CPUs, if a SYSCALL instruction is at the highest canonical >> + * address, then that syscall will enter the kernel with a >> + * non-canonical return address, and SYSRET will explode dangerously. >> + * We avoid this particular problem by preventing anything executable >> + * from being mapped at the maximum canonical address. >> + * >> + * On AMD CPUs in the Ryzen family, there's a nasty bug in which the >> + * CPUs malfunction if they execute code from the highest canonical page. >> + * They'll speculate right off the end of the canonical space, and >> + * bad things happen. This is worked around in the same way as the >> + * Intel problem. >> + * >> + * With page table isolation enabled, we map the LDT in ... [stay tuned] >> */ >> #define TASK_SIZE_MAX ((1UL << __VIRTUAL_MASK_SHIFT) - PAGE_SIZE) >> >> diff --git a/arch/x86/kernel/ldt.c b/arch/x86/kernel/ldt.c >> index ae5615b03def..46ad333ed797 100644 >> --- a/arch/x86/kernel/ldt.c >> +++ b/arch/x86/kernel/ldt.c >> @@ -19,6 +19,7 @@ >> #include <linux/uaccess.h> >> >> #include <asm/ldt.h> >> +#include <asm/tlb.h> >> #include <asm/desc.h> >> #include <asm/mmu_context.h> >> #include <asm/syscalls.h> >> @@ -46,13 +47,12 @@ static void refresh_ldt_segments(void) >> static void flush_ldt(void *__mm) >> { >> struct mm_struct *mm = __mm; >> - mm_context_t *pc; >> >> if (this_cpu_read(cpu_tlbstate.loaded_mm) != mm) >> return; >> >> - pc = &mm->context; >> - set_ldt(pc->ldt->entries, pc->ldt->nr_entries); >> + __flush_tlb_all(); >> + load_mm_ldt(mm); > > Why the new TLB flush?
It was an attempt to debug a bug and I forgot to delete it.
>> @@ -90,9 +90,112 @@ static struct ldt_struct *alloc_ldt_struct(unsigned int num_entries) >> } >> >> new_ldt->nr_entries = num_entries; >> + new_ldt->slot = -1; >> return new_ldt; >> } > > This seems a bit silly to do given that 'slot' is an int and this patch > introduces warnings looking for positive values: > > if (WARN_ON_ONCE((unsigned long)ldt->slot > 1)) { > > Seems like a good idea to just have a single warning in there looking > for non-zero, probably covering the PTI and non-PTI cases (at least for > now until the slots get used).
The idea is to warn if we haven't mapped it yet.
> >> +/* >> + * If PTI is enabled, this maps the LDT into the kernelmode and >> + * usermode tables for the given mm. >> + * >> + * There is no corresponding unmap function. Even if the LDT is freed, we >> + * leave the PTEs around until the slot is reused or the mm is destroyed. >> + * This is harmless: the LDT is always in ordinary memory, and no one will >> + * access the freed slot. >> + * >> + * If we wanted to unmap freed LDTs, we'd also need to do a flush to make >> + * it useful, and the flush would slow down modify_ldt(). >> + */ >> +static int map_ldt_struct(struct mm_struct *mm, struct ldt_struct *ldt, int slot) >> +{ >> +#ifdef CONFIG_PAGE_TABLE_ISOLATION >> + spinlock_t *ptl; >> + bool is_vmalloc; >> + bool had_top_level_entry; >> + pgd_t *pgd; >> + int i; >> + >> + if (!static_cpu_has_bug(X86_BUG_CPU_SECURE_MODE_PTI)) >> + return 0; >> + >> + WARN_ON(ldt->slot != -1); > > Only allow mapping newly-allocated LDTs? > >> + /* >> + * Did we already have the top level entry allocated? We can't >> + * use pgd_none() for this because it doens't do anything on >> + * 4-level page table kernels. >> + */ >> + pgd = pgd_offset(mm, LDT_BASE_ADDR); >> + had_top_level_entry = (pgd->pgd != 0); >> + >> + is_vmalloc = is_vmalloc_addr(ldt->entries); >> + >> + for (i = 0; i * PAGE_SIZE < ldt->nr_entries * LDT_ENTRY_SIZE; i++) { >> + unsigned long offset = i << PAGE_SHIFT; >> + unsigned long va = (unsigned long)ldt_slot_va(slot) + offset; >> + const void *src = (char *)ldt->entries + offset; >> + unsigned long pfn = is_vmalloc ? vmalloc_to_pfn(src) : >> + page_to_pfn(virt_to_page(src)); >> + pte_t pte, *ptep; >> + >> + ptep = get_locked_pte(mm, va, &ptl); > > It's *probably* worth calling out that all the page table allocation > happens in there. I went looking for it in this patch and it took me a > few minutes to find it. > >> + if (!ptep) >> + return -ENOMEM; >> + pte = pfn_pte(pfn, __pgprot(__PAGE_KERNEL & ~_PAGE_GLOBAL)); > > This ~_PAGE_GLOBAL is for the same reason as all the other KPTI code, > right? BTW, does this function deserve to be in the LDT code or kpti? > >> + set_pte_at(mm, va, ptep, pte); >> + pte_unmap_unlock(ptep, ptl); >> + } > > Might want to fix up the set_pte_at() whitespace damage.
It's not damaged -- it lines up nicely if you look at the file instead of the patch :)
> >> + if (mm->context.ldt) { >> + /* >> + * We already had an LDT. The top-level entry should already >> + * have been allocated and synchronized with the usermode >> + * tables. >> + */ >> + WARN_ON(!had_top_level_entry); >> + if (static_cpu_has_bug(X86_BUG_CPU_SECURE_MODE_PTI)) >> + WARN_ON(!kernel_to_user_pgdp(pgd)->pgd); >> + } else { >> + /* >> + * This is the first time we're mapping an LDT for this process. >> + * Sync the pgd to the usermode tables. >> + */ >> + WARN_ON(had_top_level_entry); >> + if (static_cpu_has_bug(X86_BUG_CPU_SECURE_MODE_PTI)) { >> + WARN_ON(kernel_to_user_pgdp(pgd)->pgd); >> + set_pgd(kernel_to_user_pgdp(pgd), *pgd); >> + } >> + } >> + >> + flush_tlb_mm_range(mm, >> + (unsigned long)ldt_slot_va(slot), >> + (unsigned long)ldt_slot_va(slot) + LDT_SLOT_STRIDE, >> + 0); > > Why wait until here to flush? Isn't this primarily for the case where > set_pte_at() overwrote something?
I think it would be okay if we did it sooner, but CPUs are allowed to cache intermediate mappings, and we're changing the userspace tables above it.
>> + >> +static void free_ldt_pgtables(struct mm_struct *mm) >> +{ >> +#ifdef CONFIG_PAGE_TABLE_ISOLATION >> + struct mmu_gather tlb; >> + unsigned long start = LDT_BASE_ADDR; >> + unsigned long end = start + (1UL << PGDIR_SHIFT); >> + >> + if (!static_cpu_has_bug(X86_BUG_CPU_SECURE_MODE_PTI)) >> + return; >> + >> + tlb_gather_mmu(&tlb, mm, start, end); >> + free_pgd_range(&tlb, start, end, start, end); >> + tlb_finish_mmu(&tlb, start, end); >> +#endif >> +} > > Isn't this primarily called at exit()? Isn't it a bit of a shame we > can't combine this with the other exit()-time TLB flushing?
Yes. In fact, we don't really need that flush at all since the mm is totally dead. But the free_pgd_range() API is too dumb. And yes, we have the same issue in the normal mm/memory.c code. In general, exit handling is seriously overengineered.
> Also, from a high level, this does increase the overhead of KPTI in a > non-trivial way, right? It costs us three more page table pages per > process allocated at fork() and freed at exit() and a new TLB flush.
Yeah, but no one will care. modify_ldt() is used for DOSEMU, Wine, and really old 32-bit programs.
| |