x86: modify_ldt improvement, test, and config option

modify_ldt has questionable locking and does not synchronize threads. Improve it: redesign the locking and synchronize all threads' LDTs using an IPI on all modifications.

This will dramatically slow down modify_ldt in multithreaded programs, but there shouldn't be any multithreaded programs that care about modify_ldt's performance in the first place.

Cc [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

---
  arch/x86/include/asm/desc.h        |  15 ---
  arch/x86/include/asm/mmu.h         |   3 +-
  arch/x86/include/asm/mmu_context.h |  48 ++++++-
  arch/x86/kernel/cpu/common.c       |   4 +-
  arch/x86/kernel/cpu/perf_event.c   |  12 +-
  arch/x86/kernel/ldt.c              | 247 +++++++++++++++++++------------------
  arch/x86/kernel/process_64.c       |   4 +-
  arch/x86/kernel/step.c             |   6 +-
  arch/x86/power/cpu.c               |   3 +-
  9 files changed, 192 insertions(+), 150 deletions(-)

arch/x86/include/asm/desc.h +0 -15

index a0bf89fd2647..4e10d73cf018 100644
@@ -280,21 +280,6 @@ static inline void clear_LDT(void)
  	set_ldt(NULL, 0);
  }
 
-/*
- * load one particular LDT into the current CPU
- */
-static inline void load_LDT_nolock(mm_context_t *pc)
-{
-	set_ldt(pc->ldt, pc->size);
-}
-
-static inline void load_LDT(mm_context_t *pc)
-{
-	preempt_disable();
-	load_LDT_nolock(pc);
-	preempt_enable();
-}
-
  static inline unsigned long get_desc_base(const struct desc_struct *desc)
  {
  	return (unsigned)(desc->base0 | ((desc->base1) << 16) | ((desc->base2) << 24));

arch/x86/include/asm/mmu.h +1 -2

index 09b9620a73b4..364d27481a52 100644
@@ -9,8 +9,7 @@
   * we put the segment information here.
   */
  typedef struct {
-	void *ldt;
-	int size;
+	struct ldt_struct *ldt;
 
  #ifdef CONFIG_X86_64
  	/* True if mm supports a task running in 32 bit compatibility mode. */

arch/x86/include/asm/mmu_context.h +0 -0

index 5e8daee7c5c9..1ff121fbf366 100644
@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct *mm) {}
  #endif

arch/x86/include/asm/mmu_context.h +35 -5

+
+static inline void load_mm_ldt(struct mm_struct *mm)
+{
+	struct ldt_struct *ldt;
+	DEBUG_LOCKS_WARN_ON(!irqs_disabled());
+
+	/* lockless_dereference synchronizes with smp_store_release */
+	ldt = lockless_dereference(mm->context.ldt);
+
+	/*
+	 * Any change to mm->context.ldt is followed by an IPI to all
+	 * CPUs with the mm active.  The LDT will not be freed until
+	 * after the IPI is handled by all such CPUs.  This means that,
+	 * if the ldt_struct changes before we return, the values we see
+	 * will be safe, and the new values will be loaded before we run
+	 * any user code.
+	 *
+	 * NB: don't try to convert this to use RCU without extreme care.
+	 * We would still need IRQs off, because we don't want to change
+	 * the local LDT after an IPI loaded a newer value than the one
+	 * that we can see.
+	 */
+
+	if (unlikely(ldt))
+		set_ldt(ldt->entries, ldt->size);
+	else
+		clear_LDT();
+}
+
+/*
   * Used for LDT copy/destruction.
   */
  int init_new_context(struct task_struct *tsk, struct mm_struct *mm);

@@ -78,12 +116,12 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
  		 * was called and then modify_ldt changed
  		 * prev->context.ldt but suppressed an IPI to this CPU.
  		 * In this case, prev->context.ldt != NULL, because we
-		 * never free an LDT while the mm still exists.  That
-		 * means that next->context.ldt != prev->context.ldt,
-		 * because mms never share an LDT.
+		 * never set context.ldt to NULL while the mm still
+		 * exists.  That means that next->context.ldt !=
+		 * prev->context.ldt, because mms never share an LDT.
  		 */
  		if (unlikely(prev->context.ldt != next->context.ldt))
-			load_LDT_nolock(&next->context);
+			load_mm_ldt(next);
  	}
  #ifdef CONFIG_SMP
  	  else {

@@ -106,7 +144,7 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
  			load_cr3(next->pgd);
  			trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
  			load_mm_cr4(next);
-			load_LDT_nolock(&next->context);
+			load_mm_ldt(next);
  		}
  	}
  #endif

arch/x86/kernel/cpu/common.c +2 -2

index 922c5e0cea4c..cb9e5df42dd2 100644
@@ -1410,7 +1410,7 @@ void cpu_init(void)
  	load_sp0(t, &current->thread);
  	set_tss_desc(cpu, t);
  	load_TR_desc();
-	load_LDT(&init_mm.context);
+	load_mm_ldt(&init_mm);
 
  	clear_all_debug_regs();
  	dbg_restore_debug_regs();

@@ -1459,7 +1459,7 @@ void cpu_init(void)
  	load_sp0(t, thread);
  	set_tss_desc(cpu, t);
  	load_TR_desc();
-	load_LDT(&init_mm.context);
+	load_mm_ldt(&init_mm);
 
  	t->x86_tss.io_bitmap_base = offsetof(struct tss_struct, io_bitmap);
 

arch/x86/kernel/cpu/perf_event.c +8 -4

index 3658de47900f..9469dfa55607 100644
@@ -2179,21 +2179,25 @@ static unsigned long get_segment_base(unsigned int segment)
  	int idx = segment >> 3;
 
  	if ((segment & SEGMENT_TI_MASK) == SEGMENT_LDT) {
+		struct ldt_struct *ldt;
+
  		if (idx > LDT_ENTRIES)
  			return 0;
 
-		if (idx > current->active_mm->context.size)
+		/* IRQs are off, so this synchronizes with smp_store_release */
+		ldt = lockless_dereference(current->active_mm->context.ldt);
+		if (!ldt || idx > ldt->size)
  			return 0;
 
-		desc = current->active_mm->context.ldt;
+		desc = &ldt->entries[idx];
  	} else {
  		if (idx > GDT_ENTRIES)
  			return 0;
 
-		desc = raw_cpu_ptr(gdt_page.gdt);
+		desc = raw_cpu_ptr(gdt_page.gdt) + idx;
  	}
 
-	return get_desc_base(desc + idx);
+	return get_desc_base(desc);
  }
 
  #ifdef CONFIG_COMPAT

arch/x86/kernel/ldt.c +130 -117

index c37886d759cc..d65e6ec90338 100644
@@ -20,82 +20,70 @@
  #include <asm/mmu_context.h>
  #include <asm/syscalls.h>
 
-#ifdef CONFIG_SMP
  static void flush_ldt(void *current_mm)
  {
-	if (current->active_mm == current_mm)
-		load_LDT(&current->active_mm->context);
+	if (current->active_mm == current_mm) {
+		/* context.lock is held for us, so we don't need any locking. */
+		mm_context_t *pc = &current->active_mm->context;
+		set_ldt(pc->ldt->entries, pc->ldt->size);
+	}
  }
-#endif
 
-static int alloc_ldt(mm_context_t *pc, int mincount, int reload)
+static struct ldt_struct *alloc_ldt_struct(int size)
  {
-	void *oldldt, *newldt;
-	int oldsize;
+	struct ldt_struct *new_ldt;
+	int alloc_size;
 
-	if (mincount <= pc->size)
-		return 0;
-	oldsize = pc->size;
-	mincount = (mincount + (PAGE_SIZE / LDT_ENTRY_SIZE - 1)) &
-			(~(PAGE_SIZE / LDT_ENTRY_SIZE - 1));
-	if (mincount * LDT_ENTRY_SIZE > PAGE_SIZE)
-		newldt = vmalloc(mincount * LDT_ENTRY_SIZE);
+	if (size > LDT_ENTRIES)
+		return NULL;
+
+	BUILD_BUG_ON(LDT_ENTRY_SIZE != sizeof(struct desc_struct));
+	alloc_size = sizeof(struct ldt_struct) + size * LDT_ENTRY_SIZE;
+
+	if (alloc_size > PAGE_SIZE)
+		new_ldt = vmalloc(alloc_size);
  	else
-		newldt = (void *)__get_free_page(GFP_KERNEL);
+		new_ldt = (void *)__get_free_page(GFP_KERNEL);
+	if (!new_ldt)
+		return NULL;
 
-	if (!newldt)
-		return -ENOMEM;
+	new_ldt->size = size;
+	paravirt_alloc_ldt(new_ldt->entries, size);
+	return new_ldt;
+}
 
-	if (oldsize)
-		memcpy(newldt, pc->ldt, oldsize * LDT_ENTRY_SIZE);
-	oldldt = pc->ldt;
-	memset(newldt + oldsize * LDT_ENTRY_SIZE, 0,
-	       (mincount - oldsize) * LDT_ENTRY_SIZE);
+static void install_ldt(struct mm_struct *current_mm,
+			struct ldt_struct *ldt)
+{
+	/* context.lock is held */
+	preempt_disable();
 
-	paravirt_alloc_ldt(newldt, mincount);
+	/* Synchronizes with lockless_dereference in load_mm_ldt. */
+	smp_store_release(&current_mm->context.ldt, ldt);
 
-#ifdef CONFIG_X86_64
-	/* CHECKME: Do we really need this ? */
-	wmb();
-#endif
-	pc->ldt = newldt;
-	wmb();
-	pc->size = mincount;
-	wmb();
+	/* Activate for this CPU. */
+	flush_ldt(current->mm);
 
-	if (reload) {
  #ifdef CONFIG_SMP
-		preempt_disable();
-		load_LDT(pc);
-		if (!cpumask_equal(mm_cpumask(current->mm),
-				   cpumask_of(smp_processor_id())))
-			smp_call_function(flush_ldt, current->mm, 1);
-		preempt_enable();
-#else
-		load_LDT(pc);
+	/* Synchronize with other CPUs. */
+	if (!cpumask_equal(mm_cpumask(current_mm),
+			   cpumask_of(smp_processor_id())))
+		smp_call_function(flush_ldt, current_mm, 1);
  #endif
-	}
-	if (oldsize) {
-		paravirt_free_ldt(oldldt, oldsize);
-		if (oldsize * LDT_ENTRY_SIZE > PAGE_SIZE)
-			vfree(oldldt);
-		else
-			put_page(virt_to_page(oldldt));
-	}
-	return 0;
+	preempt_enable();
  }
 
-static inline int copy_ldt(mm_context_t *new, mm_context_t *old)
+static void free_ldt_struct(struct ldt_struct *ldt)
  {
-	int err = alloc_ldt(new, old->size, 0);
-	int i;
-
-	if (err < 0)
-		return err;
-
-	for (i = 0; i < old->size; i++)
-		write_ldt_entry(new->ldt, i, old->ldt + i * LDT_ENTRY_SIZE);
-	return 0;
+	if (unlikely(ldt)) {
+		int alloc_size = sizeof(struct ldt_struct) +
+			ldt->size * LDT_ENTRY_SIZE;
+		paravirt_free_ldt(ldt->entries, ldt->size);
+		if (alloc_size > PAGE_SIZE)
+			vfree(ldt);
+		else
+			put_page(virt_to_page(ldt));
+	}
  }
 
  /*

@@ -106,15 +94,35 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
  {
  	struct mm_struct *old_mm;
  	int retval = 0;
+	int i;
 
  	mutex_init(&mm->context.lock);
-	mm->context.size = 0;
  	old_mm = current->mm;
-	if (old_mm && old_mm->context.size > 0) {
-		mutex_lock(&old_mm->context.lock);
-		retval = copy_ldt(&mm->context, &old_mm->context);
-		mutex_unlock(&old_mm->context.lock);
+	if (!old_mm) {
+		mm->context.ldt = NULL;
+		return 0;
  	}
+
+	mutex_lock(&old_mm->context.lock);
+	if (old_mm->context.ldt) {
+		struct ldt_struct *new_ldt =
+			alloc_ldt_struct(old_mm->context.ldt->size);
+		if (!new_ldt) {
+			retval = -ENOMEM;
+			goto out_unlock;
+		}
+
+		for (i = 0; i < old_mm->context.ldt->size; i++)
+			write_ldt_entry(new_ldt->entries, i,
+					&old_mm->context.ldt->entries[i]);
+
+		mm->context.ldt = new_ldt;
+	} else {
+		mm->context.ldt = NULL;
+	}
+
+out_unlock:
+	mutex_unlock(&old_mm->context.lock);
  	return retval;
  }
 

@@ -125,53 +133,47 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
   */
  void destroy_context(struct mm_struct *mm)
  {
-	if (mm->context.size) {
-#ifdef CONFIG_X86_32
-		/* CHECKME: Can this ever happen ? */
-		if (mm == current->active_mm)
-			clear_LDT();
-#endif
-		paravirt_free_ldt(mm->context.ldt, mm->context.size);
-		if (mm->context.size * LDT_ENTRY_SIZE > PAGE_SIZE)
-			vfree(mm->context.ldt);
-		else
-			put_page(virt_to_page(mm->context.ldt));
-		mm->context.size = 0;
-	}
+	free_ldt_struct(mm->context.ldt);
+	mm->context.ldt = NULL;
  }
 
  static int read_ldt(void __user *ptr, unsigned long bytecount)
  {
-	int err;
+	int retval;
  	unsigned long size;
  	struct mm_struct *mm = current->mm;
 
-	if (!mm->context.size)
-		return 0;
+	mutex_lock(&mm->context.lock);
+
+	if (!mm->context.ldt) {
+		retval = 0;
+		goto out_unlock;
+	}
+
  	if (bytecount > LDT_ENTRY_SIZE * LDT_ENTRIES)
  		bytecount = LDT_ENTRY_SIZE * LDT_ENTRIES;
 
-	mutex_lock(&mm->context.lock);
-	size = mm->context.size * LDT_ENTRY_SIZE;
+	size = mm->context.ldt->size * LDT_ENTRY_SIZE;
  	if (size > bytecount)
  		size = bytecount;
 
-	err = 0;
-	if (copy_to_user(ptr, mm->context.ldt, size))
-		err = -EFAULT;
-	mutex_unlock(&mm->context.lock);
-	if (err < 0)
-		goto error_return;
+	if (copy_to_user(ptr, mm->context.ldt->entries, size)) {
+		retval = -EFAULT;
+		goto out_unlock;
+	}
+
  	if (size != bytecount) {
-		/* zero-fill the rest */
+		/* Zero-fill the rest and pretend we read bytecount bytes. */
  		if (clear_user(ptr + size, bytecount - size) != 0) {
-			err = -EFAULT;
-			goto error_return;
+			retval = -EFAULT;
+			goto out_unlock;
  		}
  	}
-	return bytecount;
-error_return:
-	return err;
+	retval = bytecount;
+
+out_unlock:
+	mutex_unlock(&mm->context.lock);
+	return retval;
  }
 
  static int read_default_ldt(void __user *ptr, unsigned long bytecount)

@@ -189,12 +191,16 @@ static int read_default_ldt(void __user *ptr, unsigned long bytecount)
  	return bytecount;
  }
 
+static struct desc_struct blank_desc;
+
  static int write_ldt(void __user *ptr, unsigned long bytecount, int oldmode)
  {
  	struct mm_struct *mm = current->mm;
  	struct desc_struct ldt;
-	int error;
+	int error, i;
  	struct user_desc ldt_info;
+	int oldsize, newsize;
+	struct ldt_struct *new_ldt, *old_ldt;
 
  	error = -EINVAL;
  	if (bytecount != sizeof(ldt_info))

@@ -213,34 +219,41 @@ static int write_ldt(void __user *ptr, unsigned long bytecount, int oldmode)
  			goto out;
  	}
 
-	mutex_lock(&mm->context.lock);
-	if (ldt_info.entry_number >= mm->context.size) {
-		error = alloc_ldt(&current->mm->context,
-				  ldt_info.entry_number + 1, 1);
-		if (error < 0)
-			goto out_unlock;
-	}
-
-	/* Allow LDTs to be cleared by the user. */
-	if (ldt_info.base_addr == 0 && ldt_info.limit == 0) {
-		if (oldmode || LDT_empty(&ldt_info)) {
-			memset(&ldt, 0, sizeof(ldt));
-			goto install;
+	if ((oldmode && ldt_info.base_addr == 0 && ldt_info.limit == 0) ||
+	    LDT_empty(&ldt_info)) {
+		/* The user wants to clear the entry. */
+		memset(&ldt, 0, sizeof(ldt));
+	} else {
+		if (!IS_ENABLED(CONFIG_X86_16BIT) && !ldt_info.seg_32bit) {
+			error = -EINVAL;
+			goto out;
  		}
+
+		fill_ldt(&ldt, &ldt_info);
+		if (oldmode)
+			ldt.avl = 0;
  	}
 
-	if (!IS_ENABLED(CONFIG_X86_16BIT) && !ldt_info.seg_32bit) {
-		error = -EINVAL;
+	mutex_lock(&mm->context.lock);
+
+	old_ldt = mm->context.ldt;
+	oldsize = old_ldt ? old_ldt->size : 0;
+	newsize = max((int)(ldt_info.entry_number + 1), oldsize);
+
+	error = -ENOMEM;
+	new_ldt = alloc_ldt_struct(newsize);
+	if (!new_ldt)
  		goto out_unlock;
-	}
 
-	fill_ldt(&ldt, &ldt_info);
-	if (oldmode)
-		ldt.avl = 0;
+	for (i = 0; i < oldsize; i++)
+		write_ldt_entry(new_ldt->entries, i,
+				&old_ldt->entries[i]);
+	for (i = oldsize; i < newsize; i++)
+		write_ldt_entry(new_ldt->entries, i, &blank_desc);
+	write_ldt_entry(new_ldt->entries, ldt_info.entry_number, &ldt);
 
-	/* Install the new entry ...  */
-install:
-	write_ldt_entry(mm->context.ldt, ldt_info.entry_number, &ldt);
+	install_ldt(mm, new_ldt);
+	free_ldt_struct(old_ldt);
  	error = 0;
 
  out_unlock:

arch/x86/kernel/process_64.c +2 -2

index 71d7849a07f7..f6b916387590 100644
@@ -121,11 +121,11 @@ void __show_regs(struct pt_regs *regs, int all)
  void release_thread(struct task_struct *dead_task)
  {
  	if (dead_task->mm) {
-		if (dead_task->mm->context.size) {
+		if (dead_task->mm->context.ldt) {
  			pr_warn("WARNING: dead process %s still has LDT? <%p/%d>\n",
  				dead_task->comm,
  				dead_task->mm->context.ldt,
-				dead_task->mm->context.size);
+				dead_task->mm->context.ldt->size);
  			BUG();
  		}
  	}

arch/x86/kernel/step.c +4 -2

index 9b4d51d0c0d0..6273324186ac 100644
@@ -5,6 +5,7 @@
  #include <linux/mm.h>
  #include <linux/ptrace.h>
  #include <asm/desc.h>
+#include <asm/mmu_context.h>
 
  unsigned long convert_ip_to_linear(struct task_struct *child, struct pt_regs *regs)
  {

@@ -30,10 +31,11 @@ unsigned long convert_ip_to_linear(struct task_struct *child, struct pt_regs *re
  		seg &= ~7UL;
 
  		mutex_lock(&child->mm->context.lock);
-		if (unlikely((seg >> 3) >= child->mm->context.size))
+		if (unlikely(!child->mm->context.ldt ||
+			     (seg >> 3) >= child->mm->context.ldt->size))
  			addr = -1L; /* bogus selector, access would fault */
  		else {
-			desc = child->mm->context.ldt + seg;
+			desc = &child->mm->context.ldt->entries[seg];
  			base = get_desc_base(desc);
 
  			/* 16-bit code segment? */

arch/x86/power/cpu.c +2 -1

index 0d7dd1f5ac36..9ab52791fed5 100644
@@ -22,6 +22,7 @@
  #include <asm/fpu/internal.h>
  #include <asm/debugreg.h>
  #include <asm/cpu.h>
+#include <asm/mmu_context.h>
 
  #ifdef CONFIG_X86_32
  __visible unsigned long saved_context_ebx;

@@ -153,7 +154,7 @@ static void fix_processor_context(void)
  	syscall_init();				/* This sets MSR_*STAR and related */
  #endif
  	load_TR_desc();				/* This does ltr */
-	load_LDT(&current->active_mm->context);	/* This does lldt */
+	load_mm_ldt(current->active_mm);	/* This does lldt */
 
  	fpu__resume_cpu();
  }

modify_ldt has questionable locking and does not synchronize threads. Improve it: redesign the locking and synchronize all threads' LDTs using an IPI on all modifications.

This will dramatically slow down modify_ldt in multithreaded programs, but there shouldn't be any multithreaded programs that care about modify_ldt's performance in the first place.

Cc [email protected]

Signed-off-by Andy Lutomirski <[email protected]>

---
  arch/x86/include/asm/desc.h        |  15 ---
  arch/x86/include/asm/mmu.h         |   3 +-
  arch/x86/include/asm/mmu_context.h |  48 ++++++-
  arch/x86/kernel/cpu/common.c       |   4 +-
  arch/x86/kernel/cpu/perf_event.c   |  12 +-
  arch/x86/kernel/ldt.c              | 247 +++++++++++++++++++------------------
  arch/x86/kernel/process_64.c       |   4 +-
  arch/x86/kernel/step.c             |   6 +-
  arch/x86/power/cpu.c               |   3 +-
  9 files changed, 192 insertions(+), 150 deletions(-)

arch/x86/include/asm/desc.h +0 -15

index a0bf89fd2647..4e10d73cf018 100644
@@ -280,21 +280,6 @@ static inline void clear_LDT(void)
  	set_ldt(NULL, 0);
  }
 
-/*
- * load one particular LDT into the current CPU
- */
-static inline void load_LDT_nolock(mm_context_t *pc)
-{
-	set_ldt(pc->ldt, pc->size);
-}
-
-static inline void load_LDT(mm_context_t *pc)
-{
-	preempt_disable();
-	load_LDT_nolock(pc);
-	preempt_enable();
-}
-
  static inline unsigned long get_desc_base(const struct desc_struct *desc)
  {
  	return (unsigned)(desc->base0 | ((desc->base1) << 16) | ((desc->base2) << 24));

arch/x86/include/asm/mmu.h +1 -2

index 09b9620a73b4..364d27481a52 100644
@@ -9,8 +9,7 @@
   * we put the segment information here.
   */
  typedef struct {
-	void *ldt;
-	int size;
+	struct ldt_struct *ldt;
 
  #ifdef CONFIG_X86_64
  	/* True if mm supports a task running in 32 bit compatibility mode. */

arch/x86/include/asm/mmu_context.h +0 -0

index 5e8daee7c5c9..1ff121fbf366 100644
@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct *mm) {}
  #endif

arch/x86/include/asm/mmu_context.h +35 -5

+
+static inline void load_mm_ldt(struct mm_struct *mm)
+{
+	struct ldt_struct *ldt;
+	DEBUG_LOCKS_WARN_ON(!irqs_disabled());
+
+	/* lockless_dereference synchronizes with smp_store_release */
+	ldt = lockless_dereference(mm->context.ldt);
+
+	/*
+	 * Any change to mm->context.ldt is followed by an IPI to all
+	 * CPUs with the mm active.  The LDT will not be freed until
+	 * after the IPI is handled by all such CPUs.  This means that,
+	 * if the ldt_struct changes before we return, the values we see
+	 * will be safe, and the new values will be loaded before we run
+	 * any user code.
+	 *
+	 * NB: don't try to convert this to use RCU without extreme care.
+	 * We would still need IRQs off, because we don't want to change
+	 * the local LDT after an IPI loaded a newer value than the one
+	 * that we can see.
+	 */
+
+	if (unlikely(ldt))
+		set_ldt(ldt->entries, ldt->size);
+	else
+		clear_LDT();
+}
+
+/*
   * Used for LDT copy/destruction.
   */
  int init_new_context(struct task_struct *tsk, struct mm_struct *mm);

@@ -78,12 +116,12 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
  		 * was called and then modify_ldt changed
  		 * prev->context.ldt but suppressed an IPI to this CPU.
  		 * In this case, prev->context.ldt != NULL, because we
-		 * never free an LDT while the mm still exists.  That
-		 * means that next->context.ldt != prev->context.ldt,
-		 * because mms never share an LDT.
+		 * never set context.ldt to NULL while the mm still
+		 * exists.  That means that next->context.ldt !=
+		 * prev->context.ldt, because mms never share an LDT.
  		 */
  		if (unlikely(prev->context.ldt != next->context.ldt))
-			load_LDT_nolock(&next->context);
+			load_mm_ldt(next);
  	}
  #ifdef CONFIG_SMP
  	  else {

@@ -106,7 +144,7 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
  			load_cr3(next->pgd);
  			trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
  			load_mm_cr4(next);
-			load_LDT_nolock(&next->context);
+			load_mm_ldt(next);
  		}
  	}
  #endif

arch/x86/kernel/cpu/common.c +2 -2

index 922c5e0cea4c..cb9e5df42dd2 100644
@@ -1410,7 +1410,7 @@ void cpu_init(void)
  	load_sp0(t, &current->thread);
  	set_tss_desc(cpu, t);
  	load_TR_desc();
-	load_LDT(&init_mm.context);
+	load_mm_ldt(&init_mm);
 
  	clear_all_debug_regs();
  	dbg_restore_debug_regs();

@@ -1459,7 +1459,7 @@ void cpu_init(void)
  	load_sp0(t, thread);
  	set_tss_desc(cpu, t);
  	load_TR_desc();
-	load_LDT(&init_mm.context);
+	load_mm_ldt(&init_mm);
 
  	t->x86_tss.io_bitmap_base = offsetof(struct tss_struct, io_bitmap);
 

arch/x86/kernel/cpu/perf_event.c +8 -4

index 3658de47900f..9469dfa55607 100644
@@ -2179,21 +2179,25 @@ static unsigned long get_segment_base(unsigned int segment)
  	int idx = segment >> 3;
 
  	if ((segment & SEGMENT_TI_MASK) == SEGMENT_LDT) {
+		struct ldt_struct *ldt;
+
  		if (idx > LDT_ENTRIES)
  			return 0;
 
-		if (idx > current->active_mm->context.size)
+		/* IRQs are off, so this synchronizes with smp_store_release */
+		ldt = lockless_dereference(current->active_mm->context.ldt);
+		if (!ldt || idx > ldt->size)
  			return 0;
 
-		desc = current->active_mm->context.ldt;
+		desc = &ldt->entries[idx];
  	} else {
  		if (idx > GDT_ENTRIES)
  			return 0;
 
-		desc = raw_cpu_ptr(gdt_page.gdt);
+		desc = raw_cpu_ptr(gdt_page.gdt) + idx;
  	}
 
-	return get_desc_base(desc + idx);
+	return get_desc_base(desc);
  }
 
  #ifdef CONFIG_COMPAT

arch/x86/kernel/ldt.c +130 -117

index c37886d759cc..d65e6ec90338 100644
@@ -20,82 +20,70 @@
  #include <asm/mmu_context.h>
  #include <asm/syscalls.h>
 
-#ifdef CONFIG_SMP
  static void flush_ldt(void *current_mm)
  {
-	if (current->active_mm == current_mm)
-		load_LDT(&current->active_mm->context);
+	if (current->active_mm == current_mm) {
+		/* context.lock is held for us, so we don't need any locking. */
+		mm_context_t *pc = &current->active_mm->context;
+		set_ldt(pc->ldt->entries, pc->ldt->size);
+	}
  }
-#endif
 
-static int alloc_ldt(mm_context_t *pc, int mincount, int reload)
+static struct ldt_struct *alloc_ldt_struct(int size)
  {
-	void *oldldt, *newldt;
-	int oldsize;
+	struct ldt_struct *new_ldt;
+	int alloc_size;
 
-	if (mincount <= pc->size)
-		return 0;
-	oldsize = pc->size;
-	mincount = (mincount + (PAGE_SIZE / LDT_ENTRY_SIZE - 1)) &
-			(~(PAGE_SIZE / LDT_ENTRY_SIZE - 1));
-	if (mincount * LDT_ENTRY_SIZE > PAGE_SIZE)
-		newldt = vmalloc(mincount * LDT_ENTRY_SIZE);
+	if (size > LDT_ENTRIES)
+		return NULL;
+
+	BUILD_BUG_ON(LDT_ENTRY_SIZE != sizeof(struct desc_struct));
+	alloc_size = sizeof(struct ldt_struct) + size * LDT_ENTRY_SIZE;
+
+	if (alloc_size > PAGE_SIZE)
+		new_ldt = vmalloc(alloc_size);
  	else
-		newldt = (void *)__get_free_page(GFP_KERNEL);
+		new_ldt = (void *)__get_free_page(GFP_KERNEL);
+	if (!new_ldt)
+		return NULL;
 
-	if (!newldt)
-		return -ENOMEM;
+	new_ldt->size = size;
+	paravirt_alloc_ldt(new_ldt->entries, size);
+	return new_ldt;
+}
 
-	if (oldsize)
-		memcpy(newldt, pc->ldt, oldsize * LDT_ENTRY_SIZE);
-	oldldt = pc->ldt;
-	memset(newldt + oldsize * LDT_ENTRY_SIZE, 0,
-	       (mincount - oldsize) * LDT_ENTRY_SIZE);
+static void install_ldt(struct mm_struct *current_mm,
+			struct ldt_struct *ldt)
+{
+	/* context.lock is held */
+	preempt_disable();
 
-	paravirt_alloc_ldt(newldt, mincount);
+	/* Synchronizes with lockless_dereference in load_mm_ldt. */
+	smp_store_release(&current_mm->context.ldt, ldt);
 
-#ifdef CONFIG_X86_64
-	/* CHECKME: Do we really need this ? */
-	wmb();
-#endif
-	pc->ldt = newldt;
-	wmb();
-	pc->size = mincount;
-	wmb();
+	/* Activate for this CPU. */
+	flush_ldt(current->mm);
 
-	if (reload) {
  #ifdef CONFIG_SMP
-		preempt_disable();
-		load_LDT(pc);
-		if (!cpumask_equal(mm_cpumask(current->mm),
-				   cpumask_of(smp_processor_id())))
-			smp_call_function(flush_ldt, current->mm, 1);
-		preempt_enable();
-#else
-		load_LDT(pc);
+	/* Synchronize with other CPUs. */
+	if (!cpumask_equal(mm_cpumask(current_mm),
+			   cpumask_of(smp_processor_id())))
+		smp_call_function(flush_ldt, current_mm, 1);
  #endif
-	}
-	if (oldsize) {
-		paravirt_free_ldt(oldldt, oldsize);
-		if (oldsize * LDT_ENTRY_SIZE > PAGE_SIZE)
-			vfree(oldldt);
-		else
-			put_page(virt_to_page(oldldt));
-	}
-	return 0;
+	preempt_enable();
  }
 
-static inline int copy_ldt(mm_context_t *new, mm_context_t *old)
+static void free_ldt_struct(struct ldt_struct *ldt)
  {
-	int err = alloc_ldt(new, old->size, 0);
-	int i;
-
-	if (err < 0)
-		return err;
-
-	for (i = 0; i < old->size; i++)
-		write_ldt_entry(new->ldt, i, old->ldt + i * LDT_ENTRY_SIZE);
-	return 0;
+	if (unlikely(ldt)) {
+		int alloc_size = sizeof(struct ldt_struct) +
+			ldt->size * LDT_ENTRY_SIZE;
+		paravirt_free_ldt(ldt->entries, ldt->size);
+		if (alloc_size > PAGE_SIZE)
+			vfree(ldt);
+		else
+			put_page(virt_to_page(ldt));
+	}
  }
 
  /*

@@ -106,15 +94,35 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
  {
  	struct mm_struct *old_mm;
  	int retval = 0;
+	int i;
 
  	mutex_init(&mm->context.lock);
-	mm->context.size = 0;
  	old_mm = current->mm;
-	if (old_mm && old_mm->context.size > 0) {
-		mutex_lock(&old_mm->context.lock);
-		retval = copy_ldt(&mm->context, &old_mm->context);
-		mutex_unlock(&old_mm->context.lock);
+	if (!old_mm) {
+		mm->context.ldt = NULL;
+		return 0;
  	}
+
+	mutex_lock(&old_mm->context.lock);
+	if (old_mm->context.ldt) {
+		struct ldt_struct *new_ldt =
+			alloc_ldt_struct(old_mm->context.ldt->size);
+		if (!new_ldt) {
+			retval = -ENOMEM;
+			goto out_unlock;
+		}
+
+		for (i = 0; i < old_mm->context.ldt->size; i++)
+			write_ldt_entry(new_ldt->entries, i,
+					&old_mm->context.ldt->entries[i]);
+
+		mm->context.ldt = new_ldt;
+	} else {
+		mm->context.ldt = NULL;
+	}
+
+out_unlock:
+	mutex_unlock(&old_mm->context.lock);
  	return retval;
  }
 

@@ -125,53 +133,47 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
   */
  void destroy_context(struct mm_struct *mm)
  {
-	if (mm->context.size) {
-#ifdef CONFIG_X86_32
-		/* CHECKME: Can this ever happen ? */
-		if (mm == current->active_mm)
-			clear_LDT();
-#endif
-		paravirt_free_ldt(mm->context.ldt, mm->context.size);
-		if (mm->context.size * LDT_ENTRY_SIZE > PAGE_SIZE)
-			vfree(mm->context.ldt);
-		else
-			put_page(virt_to_page(mm->context.ldt));
-		mm->context.size = 0;
-	}
+	free_ldt_struct(mm->context.ldt);
+	mm->context.ldt = NULL;
  }
 
  static int read_ldt(void __user *ptr, unsigned long bytecount)
  {
-	int err;
+	int retval;
  	unsigned long size;
  	struct mm_struct *mm = current->mm;
 
-	if (!mm->context.size)
-		return 0;
+	mutex_lock(&mm->context.lock);
+
+	if (!mm->context.ldt) {
+		retval = 0;
+		goto out_unlock;
+	}
+
  	if (bytecount > LDT_ENTRY_SIZE * LDT_ENTRIES)
  		bytecount = LDT_ENTRY_SIZE * LDT_ENTRIES;
 
-	mutex_lock(&mm->context.lock);
-	size = mm->context.size * LDT_ENTRY_SIZE;
+	size = mm->context.ldt->size * LDT_ENTRY_SIZE;
  	if (size > bytecount)
  		size = bytecount;
 
-	err = 0;
-	if (copy_to_user(ptr, mm->context.ldt, size))
-		err = -EFAULT;
-	mutex_unlock(&mm->context.lock);
-	if (err < 0)
-		goto error_return;
+	if (copy_to_user(ptr, mm->context.ldt->entries, size)) {
+		retval = -EFAULT;
+		goto out_unlock;
+	}
+
  	if (size != bytecount) {
-		/* zero-fill the rest */
+		/* Zero-fill the rest and pretend we read bytecount bytes. */
  		if (clear_user(ptr + size, bytecount - size) != 0) {
-			err = -EFAULT;
-			goto error_return;
+			retval = -EFAULT;
+			goto out_unlock;
  		}
  	}
-	return bytecount;
-error_return:
-	return err;
+	retval = bytecount;
+
+out_unlock:
+	mutex_unlock(&mm->context.lock);
+	return retval;
  }
 
  static int read_default_ldt(void __user *ptr, unsigned long bytecount)

@@ -189,12 +191,16 @@ static int read_default_ldt(void __user *ptr, unsigned long bytecount)
  	return bytecount;
  }
 
+static struct desc_struct blank_desc;
+
  static int write_ldt(void __user *ptr, unsigned long bytecount, int oldmode)
  {
  	struct mm_struct *mm = current->mm;
  	struct desc_struct ldt;
-	int error;
+	int error, i;
  	struct user_desc ldt_info;
+	int oldsize, newsize;
+	struct ldt_struct *new_ldt, *old_ldt;
 
  	error = -EINVAL;
  	if (bytecount != sizeof(ldt_info))

@@ -213,34 +219,41 @@ static int write_ldt(void __user *ptr, unsigned long bytecount, int oldmode)
  			goto out;
  	}
 
-	mutex_lock(&mm->context.lock);
-	if (ldt_info.entry_number >= mm->context.size) {
-		error = alloc_ldt(&current->mm->context,
-				  ldt_info.entry_number + 1, 1);
-		if (error < 0)
-			goto out_unlock;
-	}
-
-	/* Allow LDTs to be cleared by the user. */
-	if (ldt_info.base_addr == 0 && ldt_info.limit == 0) {
-		if (oldmode || LDT_empty(&ldt_info)) {
-			memset(&ldt, 0, sizeof(ldt));
-			goto install;
+	if ((oldmode && ldt_info.base_addr == 0 && ldt_info.limit == 0) ||
+	    LDT_empty(&ldt_info)) {
+		/* The user wants to clear the entry. */
+		memset(&ldt, 0, sizeof(ldt));
+	} else {
+		if (!IS_ENABLED(CONFIG_X86_16BIT) && !ldt_info.seg_32bit) {
+			error = -EINVAL;
+			goto out;
  		}
+
+		fill_ldt(&ldt, &ldt_info);
+		if (oldmode)
+			ldt.avl = 0;
  	}
 
-	if (!IS_ENABLED(CONFIG_X86_16BIT) && !ldt_info.seg_32bit) {
-		error = -EINVAL;
+	mutex_lock(&mm->context.lock);
+
+	old_ldt = mm->context.ldt;
+	oldsize = old_ldt ? old_ldt->size : 0;
+	newsize = max((int)(ldt_info.entry_number + 1), oldsize);
+
+	error = -ENOMEM;
+	new_ldt = alloc_ldt_struct(newsize);
+	if (!new_ldt)
  		goto out_unlock;
-	}
 
-	fill_ldt(&ldt, &ldt_info);
-	if (oldmode)
-		ldt.avl = 0;
+	for (i = 0; i < oldsize; i++)
+		write_ldt_entry(new_ldt->entries, i,
+				&old_ldt->entries[i]);
+	for (i = oldsize; i < newsize; i++)
+		write_ldt_entry(new_ldt->entries, i, &blank_desc);
+	write_ldt_entry(new_ldt->entries, ldt_info.entry_number, &ldt);
 
-	/* Install the new entry ...  */
-install:
-	write_ldt_entry(mm->context.ldt, ldt_info.entry_number, &ldt);
+	install_ldt(mm, new_ldt);
+	free_ldt_struct(old_ldt);
  	error = 0;
 
  out_unlock:

arch/x86/kernel/process_64.c +2 -2

index 71d7849a07f7..f6b916387590 100644
@@ -121,11 +121,11 @@ void __show_regs(struct pt_regs *regs, int all)
  void release_thread(struct task_struct *dead_task)
  {
  	if (dead_task->mm) {
-		if (dead_task->mm->context.size) {
+		if (dead_task->mm->context.ldt) {
  			pr_warn("WARNING: dead process %s still has LDT? <%p/%d>\n",
  				dead_task->comm,
  				dead_task->mm->context.ldt,
-				dead_task->mm->context.size);
+				dead_task->mm->context.ldt->size);
  			BUG();
  		}
  	}

arch/x86/kernel/step.c +4 -2

index 9b4d51d0c0d0..6273324186ac 100644
@@ -5,6 +5,7 @@
  #include <linux/mm.h>
  #include <linux/ptrace.h>
  #include <asm/desc.h>
+#include <asm/mmu_context.h>
 
  unsigned long convert_ip_to_linear(struct task_struct *child, struct pt_regs *regs)
  {

@@ -30,10 +31,11 @@ unsigned long convert_ip_to_linear(struct task_struct *child, struct pt_regs *re
  		seg &= ~7UL;
 
  		mutex_lock(&child->mm->context.lock);
-		if (unlikely((seg >> 3) >= child->mm->context.size))
+		if (unlikely(!child->mm->context.ldt ||
+			     (seg >> 3) >= child->mm->context.ldt->size))
  			addr = -1L; /* bogus selector, access would fault */
  		else {
-			desc = child->mm->context.ldt + seg;
+			desc = &child->mm->context.ldt->entries[seg];
  			base = get_desc_base(desc);
 
  			/* 16-bit code segment? */

arch/x86/power/cpu.c +2 -1

index 0d7dd1f5ac36..9ab52791fed5 100644
@@ -22,6 +22,7 @@
  #include <asm/fpu/internal.h>
  #include <asm/debugreg.h>
  #include <asm/cpu.h>
+#include <asm/mmu_context.h>
 
  #ifdef CONFIG_X86_32
  __visible unsigned long saved_context_ebx;

@@ -153,7 +154,7 @@ static void fix_processor_context(void)
  	syscall_init();				/* This sets MSR_*STAR and related */
  #endif
  	load_TR_desc();				/* This does ltr */
-	load_LDT(&current->active_mm->context);	/* This does lldt */
+	load_mm_ldt(current->active_mm);	/* This does lldt */
 
  	fpu__resume_cpu();
  }

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Jan, Andrew?

Quoted 20 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

I think I have a solution, but I doubt it is going to be very popular.

* Make a new paravirt hook for allocation of ldt_struct, so the paravirt backend can choose an alignment if needed
* Make absolutely certain that __pad has the value 0 (so size and __pad combined don't look like a present descriptor)
* Never hand selector 0x0008 to unsuspecting users.

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

Quoted 20 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

I think I have a solution, but I doubt it is going to be very popular.

* Make a new paravirt hook for allocation of ldt_struct, so the paravirt backend can choose an alignment if needed
* Make absolutely certain that __pad has the value 0 (so size and __pad combined don't look like a present descriptor)
* Never hand selector 0x0008 to unsuspecting users.

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

Quoted 33 lines · Andrew Cooper

Quoted 19 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

Quoted 6 lines

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

Quoted 8 lines

I think I have a solution, but I doubt it is going to be very popular.

* Make a new paravirt hook for allocation of ldt_struct, so the paravirt backend can choose an alignment if needed
* Make absolutely certain that __pad has the value 0 (so size and __pad combined don't look like a present descriptor)
* Never hand selector 0x0008 to unsuspecting users.

Yuck.

Quoted 11 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Quoted 3 lines

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

Quoted 33 lines · Andrew Cooper

Quoted 19 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

Quoted 6 lines

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

Quoted 8 lines

I think I have a solution, but I doubt it is going to be very popular.

* Make a new paravirt hook for allocation of ldt_struct, so the paravirt backend can choose an alignment if needed
* Make absolutely certain that __pad has the value 0 (so size and __pad combined don't look like a present descriptor)
* Never hand selector 0x0008 to unsuspecting users.

Yuck.

Quoted 11 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Quoted 3 lines

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

Quoted 41 lines · Andy Lutomirski

Quoted 39 lines · Andrew Cooper

Quoted 19 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Quoted 2 lines

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

The address which Xen receives needs to be aligned.

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

Quoted 10 lines

Quoted 9 lines

I think I have a solution, but I doubt it is going to be very popular.

* Make a new paravirt hook for allocation of ldt_struct, so the paravirt backend can choose an alignment if needed
* Make absolutely certain that __pad has the value 0 (so size and __pad combined don't look like a present descriptor)
* Never hand selector 0x0008 to unsuspecting users.

Yuck.

I actually meant 0x0004, but yes. Very much yuck.

Quoted 14 lines

Quoted 14 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

Quoted 3 lines

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

I suspect two separate allocations would be the better solution, as it means that the size field doesn't need to be subject to funny page permissions.

Quoted 41 lines · Andy Lutomirski

Quoted 39 lines · Andrew Cooper

Quoted 19 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Quoted 2 lines

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

The address which Xen receives needs to be aligned.

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

Quoted 10 lines

Quoted 9 lines

I think I have a solution, but I doubt it is going to be very popular.

* Make a new paravirt hook for allocation of ldt_struct, so the paravirt backend can choose an alignment if needed
* Make absolutely certain that __pad has the value 0 (so size and __pad combined don't look like a present descriptor)
* Never hand selector 0x0008 to unsuspecting users.

Yuck.

I actually meant 0x0004, but yes. Very much yuck.

Quoted 14 lines

Quoted 14 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

Quoted 3 lines

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

I suspect two separate allocations would be the better solution, as it means that the size field doesn't need to be subject to funny page permissions.

Quoted 43 lines · Andrew Cooper

Quoted 42 lines · Andy Lutomirski

Quoted 32 lines · Andrew Cooper

Quoted 18 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

Quoted 6 lines

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

Quoted 4 lines

The address which Xen receives needs to be aligned.

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

xen_alloc_ldt is just fiddling with protection though, I think. Isn't it xen_set_ldt that's the meat? We could easily pass xen_alloc_ldt a pointer to the ldt_struct.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

Quoted 6 lines

Quoted 3 lines

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

I suspect two separate allocations would be the better solution, as it means that the size field doesn't need to be subject to funny page permissions.

True. OTOH we never write to the size field after allocating the thing.

Quoted 43 lines · Andrew Cooper

Quoted 42 lines · Andy Lutomirski

Quoted 32 lines · Andrew Cooper

Quoted 18 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

Quoted 6 lines

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

Quoted 4 lines

The address which Xen receives needs to be aligned.

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

xen_alloc_ldt is just fiddling with protection though, I think. Isn't it xen_set_ldt that's the meat? We could easily pass xen_alloc_ldt a pointer to the ldt_struct.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

Quoted 6 lines

Quoted 3 lines

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

I suspect two separate allocations would be the better solution, as it means that the size field doesn't need to be subject to funny page permissions.

True. OTOH we never write to the size field after allocating the thing.

Quoted 45 lines · Andy Lutomirski

Quoted 43 lines · Andrew Cooper

Quoted 42 lines · Andy Lutomirski

Quoted 32 lines · Andrew Cooper

Quoted 18 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

Quoted 6 lines

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

Quoted 7 lines

Quoted 4 lines

The address which Xen receives needs to be aligned.

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

xen_alloc_ldt is just fiddling with protection though, I think. Isn't it xen_set_ldt that's the meat? We could easily pass xen_alloc_ldt a pointer to the ldt_struct.

So it is. It is the linear_addr in xen_set_ldt() which Xen currently audits to be page aligned.

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 10 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Quoted 3 lines

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

Quoted 4 lines

Quoted 3 lines

I suspect two separate allocations would be the better solution, as it means that the size field doesn't need to be subject to funny page permissions.

True. OTOH we never write to the size field after allocating the thing.

Right, but even reading it is going to cause problems if one of the paravirt ops can't re-establish ro mappings.

~Andrew

Quoted 45 lines · Andy Lutomirski

Quoted 43 lines · Andrew Cooper

Quoted 42 lines · Andy Lutomirski

Quoted 32 lines · Andrew Cooper

Quoted 18 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

  #endif
    /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

Quoted 6 lines

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

Quoted 7 lines

Quoted 4 lines

The address which Xen receives needs to be aligned.

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

xen_alloc_ldt is just fiddling with protection though, I think. Isn't it xen_set_ldt that's the meat? We could easily pass xen_alloc_ldt a pointer to the ldt_struct.

So it is. It is the linear_addr in xen_set_ldt() which Xen currently audits to be page aligned.

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 10 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Quoted 3 lines

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

Quoted 4 lines

Quoted 3 lines

I suspect two separate allocations would be the better solution, as it means that the size field doesn't need to be subject to funny page permissions.

True. OTOH we never write to the size field after allocating the thing.

Right, but even reading it is going to cause problems if one of the paravirt ops can't re-establish ro mappings.

~Andrew

Quoted 46 lines · Andy Lutomirski

Quoted 44 lines · Andrew Cooper

Quoted 33 lines · Andy Lutomirski

Quoted 31 lines · Andrew Cooper

Quoted 17 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

   #endif
     /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

Quoted 8 lines

Quoted 6 lines

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

The address which Xen receives needs to be aligned.

Quoted 5 lines

Quoted 2 lines

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

xen_alloc_ldt is just fiddling with protection though, I think. Isn't it xen_set_ldt that's the meat? We could easily pass xen_alloc_ldt a pointer to the ldt_struct.

So it is. It is the linear_addr in xen_set_ldt() which Xen currently audits to be page aligned.

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 10 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Quoted 3 lines

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

Quoted 4 lines

Quoted 3 lines

I suspect two separate allocations would be the better solution, as it means that the size field doesn't need to be subject to funny page permissions.

True. OTOH we never write to the size field after allocating the thing.

Right, but even reading it is going to cause problems if one of the paravirt ops can't re-establish ro mappings.

~Andrew

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 46 lines · Andy Lutomirski

Quoted 44 lines · Andrew Cooper

Quoted 33 lines · Andy Lutomirski

Quoted 31 lines · Andrew Cooper

Quoted 17 lines · Boris Ostrovsky

Quoted 4 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +0 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}

arch/x86/include/asm/mmu_context.h static inline void load_mm_cr4(struct mm_struct · line 44

Quoted 10 lines

arch/x86/include/asm/mmu_context.h +8 -0

   #endif
     /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

Quoted 8 lines

Quoted 6 lines

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

The address which Xen receives needs to be aligned.

Quoted 5 lines

Quoted 2 lines

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

xen_alloc_ldt is just fiddling with protection though, I think. Isn't it xen_set_ldt that's the meat? We could easily pass xen_alloc_ldt a pointer to the ldt_struct.

So it is. It is the linear_addr in xen_set_ldt() which Xen currently audits to be page aligned.

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 10 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in question (although frankly it is getting late so you will want to double check all of this).

Quoted 3 lines

Anyhow, this looks like an issue which should be fixed up with slightly more PVOps, rather than enforcing a Xen view of the world on native Linux.

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

Quoted 4 lines

Quoted 3 lines

I suspect two separate allocations would be the better solution, as it means that the size field doesn't need to be subject to funny page permissions.

True. OTOH we never write to the size field after allocating the thing.

Right, but even reading it is going to cause problems if one of the paravirt ops can't re-establish ro mappings.

~Andrew

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 65 lines · Andrew Cooper

Quoted 63 lines · Andy Lutomirski

Quoted 59 lines · Andrew Cooper

Quoted 45 lines · Andy Lutomirski

Quoted 42 lines · Andrew Cooper

Quoted 21 lines · Boris Ostrovsky

Quoted 11 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +1 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}
   #endif
     /*
+ * ldt_structs can be allocated, used, and freed, but they are

never
+ * modified while live.
+ */

arch/x86/include/asm/mmu_context.h

Quoted 5 lines

arch/x86/include/asm/mmu_context.h +5 -0

+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual
region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

Quoted 7 lines

Quoted 4 lines

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given
the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

The address which Xen receives needs to be aligned.

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

Quoted 27 lines

Quoted 3 lines

xen_alloc_ldt is just fiddling with protection though, I think. Isn't it xen_set_ldt that's the meat? We could easily pass xen_alloc_ldt a pointer to the ldt_struct.

So it is. It is the linear_addr in xen_set_ldt() which Xen currently audits to be page aligned.

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 12 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in
question (although frankly it is getting late so you will want to double
check all of this).

Quoted 5 lines

Anyhow, this looks like an issue which should be fixed up with slightly
more PVOps, rather than enforcing a Xen view of the world on native Linux.

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

Why not wrap mm_context_t's ldt and size into a struct (just like ldt_struct but without __pad) and have a single allocation of ldt?

I.e.

struct ldt_struct {
    int size;
    struct desc_struct *entries;
}

Quoted 65 lines · Andrew Cooper

Quoted 63 lines · Andy Lutomirski

Quoted 59 lines · Andrew Cooper

Quoted 45 lines · Andy Lutomirski

Quoted 42 lines · Andrew Cooper

Quoted 21 lines · Boris Ostrovsky

Quoted 11 lines · Andy Lutomirski

arch/x86/include/asm/mmu_context.h +1 -0

@@ -34,6 +34,44 @@ static inline void load_mm_cr4(struct mm_struct
 *mm) {}
   #endif
     /*
+ * ldt_structs can be allocated, used, and freed, but they are

never
+ * modified while live.
+ */

arch/x86/include/asm/mmu_context.h

Quoted 5 lines

arch/x86/include/asm/mmu_context.h +5 -0

+struct ldt_struct {
+    int size;
+    int __pad;    /* keep the descriptors naturally aligned. */
+    struct desc_struct entries[];
+};

This breaks Xen which expects LDT to be page-aligned. Not sure why.

Quoted 1 line

Jan, Andrew?

PV guests are not permitted to have writeable mappings to the frames making up the GDT and LDT, so it cannot make unaudited changes to loadable descriptors. In particular, for a 32bit PV guest, it is only the segment limit which protects Xen from the ring1 guest kernel.

A lot of this code hasn't been touched in years, and it certainly predates me. The alignment requirement appears to come from the virtual
region Xen uses to map the guests GDT and LDT. Strict alignment is required for the GDT so Xen's descriptors starting at 0xe0xx are correct, but the LDT alignment seems to be a side effect of similar codepaths.

For an LDT smaller than 8192 entries, I can't see any specific reason for enforcing alignment, other than "that's the way it has always been".

Quoted 7 lines

Quoted 4 lines

However, the guest would still have to relinquish write access to all frames which make up the LDT, which looks to be a bit of an issue given
the snippet above.

Does the LDT itself need to be aligned or just the address passed to paravirt_alloc_ldt?

The address which Xen receives needs to be aligned.

It looks like xen_alloc_ldt() blindly assumes that the desc_struct *ldt it is passed is page aligned, and passes it straight through.

Quoted 27 lines

Quoted 3 lines

xen_alloc_ldt is just fiddling with protection though, I think. Isn't it xen_set_ldt that's the meat? We could easily pass xen_alloc_ldt a pointer to the ldt_struct.

So it is. It is the linear_addr in xen_set_ldt() which Xen currently audits to be page aligned.

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 12 lines

This will allow ldt_struct itself to be page aligned, and for the size field to sit across the base/limit field of what would logically be selector 0x0008 There would be some issues accessing size. To load frames as an LDT, a guest must drop all refs to the page so that its type may be changed from writeable to segdesc. After that, an update_descriptor hypercall can be used to change size, and I believe the guest may subsequently recreate read-only mappings to the frames in
question (although frankly it is getting late so you will want to double
check all of this).

Quoted 5 lines

Anyhow, this looks like an issue which should be fixed up with slightly
more PVOps, rather than enforcing a Xen view of the world on native Linux.

I could presumably make the allocation the other way around so the size is at the end. I could even use two separate allocations if needed.

Why not wrap mm_context_t's ldt and size into a struct (just like ldt_struct but without __pad) and have a single allocation of ldt?

I.e.

struct ldt_struct {
    int size;
    struct desc_struct *entries;
}

modify_ldt has questionable locking and does not synchronize threads. Improve it: redesign the locking and synchronize all threads' LDTs using an IPI on all modifications.

Quoted 3 lines · Andy Lutomirski

modify_ldt has questionable locking and does not synchronize threads. Improve it: redesign the locking and synchronize all threads' LDTs using an IPI on all modifications.

What does this fix? I can see sending an IPI if the LDT is reallocated, but on every update seems unnecessary.

--
Brian Gerst

Quoted 5 lines · Brian Gerst

Quoted 3 lines · Andy Lutomirski

modify_ldt has questionable locking and does not synchronize threads. Improve it: redesign the locking and synchronize all threads' LDTs using an IPI on all modifications.

Quoted 3 lines

What does this fix? I can see sending an IPI if the LDT is reallocated, but on every update seems unnecessary.

It prevents nastiness in which you're in user mode with an impossible CS or SS, resulting in potentially interesting artifacts in interrupts, NMIs, etc.

Quoted 10 lines · Andy Lutomirski

Quoted 8 lines · Brian Gerst

Quoted 3 lines · Andy Lutomirski

modify_ldt has questionable locking and does not synchronize threads. Improve it: redesign the locking and synchronize all threads' LDTs using an IPI on all modifications.

What does this fix? I can see sending an IPI if the LDT is reallocated, but on every update seems unnecessary.

Quoted 3 lines

It prevents nastiness in which you're in user mode with an impossible CS or SS, resulting in potentially interesting artifacts in interrupts, NMIs, etc.

By impossible, do you mean a partially updated descriptor when the interrupt occurs? Would making sure that the descriptor is atomically updated (using set_64bit()) fix that?

The modify_ldt syscall exposes a large attack surface and is unnecessary for modern userspace. Make it optional.

Quoted 2 lines · Andy Lutomirski

The modify_ldt syscall exposes a large attack surface and is unnecessary for modern userspace. Make it optional.

Since this a "default y" option I think we need to make the implications of this a bit clearer.

Do we know what userspace would break?

Maybe add a WARN_ONCE() in a stub syscall?

The modify_ldt syscall exposes a large attack surface and is unnecessary for modern userspace. Make it optional.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                   | 17 +++++++++++++++++
 arch/x86/include/asm/mmu.h         |  2 ++
 arch/x86/include/asm/mmu_context.h | 31 +++++++++++++++++++++++--------
 arch/x86/kernel/Makefile           |  3 ++-
 arch/x86/kernel/cpu/perf_event.c   |  4 ++++
 arch/x86/kernel/process_64.c       |  2 ++
 arch/x86/kernel/step.c             |  2 ++
 kernel/sys_ni.c                    |  1 +
 8 files changed, 53 insertions(+), 9 deletions(-)

arch/x86/Kconfig +7 -0

index 55bced17dc95..a7ff3980bd65 100644
@@ -1009,6 +1009,7 @@ config VM86
 config X86_16BIT
        bool "Enable support for 16-bit segments" if EXPERT
        default y
+       depends on MODIFY_LDT_SYSCALL
        ---help---
          This option is required by programs like Wine to run 16-bit
          protected mode legacy code on x86 processors.  Disabling

@@ -2047,6 +2048,22 @@ config CMDLINE_OVERRIDE
          This is used to work around broken boot loaders.  This should
          be set to 'N' under normal conditions.
 
+config MODIFY_LDT_SYSCALL
+       bool "Enable the LDT (local descriptor table)" if EXPERT
+       default y
+       ---help---
+         Linux can allow user programs to install a per-process x86
+        Local Descriptor Table (LDT) using the modify_ldt(2) system

Quoted 42 lines · Andy Lutomirski

The modify_ldt syscall exposes a large attack surface and is unnecessary for modern userspace. Make it optional.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                   | 17 +++++++++++++++++
 arch/x86/include/asm/mmu.h         |  2 ++
 arch/x86/include/asm/mmu_context.h | 31 +++++++++++++++++++++++--------
 arch/x86/kernel/Makefile           |  3 ++-
 arch/x86/kernel/cpu/perf_event.c   |  4 ++++
 arch/x86/kernel/process_64.c       |  2 ++
 arch/x86/kernel/step.c             |  2 ++
 kernel/sys_ni.c                    |  1 +
 8 files changed, 53 insertions(+), 9 deletions(-)

arch/x86/Kconfig +12 -0

index 55bced17dc95..a7ff3980bd65 100644
@@ -1009,6 +1009,7 @@ config VM86
 config X86_16BIT
        bool "Enable support for 16-bit segments" if EXPERT
        default y
+       depends on MODIFY_LDT_SYSCALL
        ---help---
          This option is required by programs like Wine to run 16-bit
          protected mode legacy code on x86 processors.  Disabling

@@ -2047,6 +2048,22 @@ config CMDLINE_OVERRIDE
          This is used to work around broken boot loaders.  This should
          be set to 'N' under normal conditions.
 
+config MODIFY_LDT_SYSCALL
+       bool "Enable the LDT (local descriptor table)" if EXPERT
+       default y
+       ---help---
+         Linux can allow user programs to install a per-process x86
+        Local Descriptor Table (LDT) using the modify_ldt(2) system
+        call.  This is required to run 16-bit or segmented code such as
+        DOSEMU or some Wine programs.  It is also used by some very old
+        threading libraries.
+
+        Enabling this feature adds a small amount of overhead to

arch/x86/Kconfig config CMDLINE_OVERRIDE · line 2066

Quoted 5 lines

arch/x86/Kconfig +5 -0

+        context switches and increases the low-level kernel attack
+        surface.  Disabling it removes the modify_ldt(2) system call.
+
+        Saying 'N' here may make sense for embedded or server kernels.
+

I believe Wine still uses the LDT for thread-local data, even for 32 and 64-bit programs. This is separate from the Linux runtime TLS.

Quoted 43 lines · Brian Gerst

Quoted 42 lines · Andy Lutomirski

The modify_ldt syscall exposes a large attack surface and is unnecessary for modern userspace. Make it optional.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                   | 17 +++++++++++++++++
 arch/x86/include/asm/mmu.h         |  2 ++
 arch/x86/include/asm/mmu_context.h | 31 +++++++++++++++++++++++--------
 arch/x86/kernel/Makefile           |  3 ++-
 arch/x86/kernel/cpu/perf_event.c   |  4 ++++
 arch/x86/kernel/process_64.c       |  2 ++
 arch/x86/kernel/step.c             |  2 ++
 kernel/sys_ni.c                    |  1 +
 8 files changed, 53 insertions(+), 9 deletions(-)

arch/x86/Kconfig +12 -0

index 55bced17dc95..a7ff3980bd65 100644
@@ -1009,6 +1009,7 @@ config VM86
 config X86_16BIT
        bool "Enable support for 16-bit segments" if EXPERT
        default y
+       depends on MODIFY_LDT_SYSCALL
        ---help---
          This option is required by programs like Wine to run 16-bit
          protected mode legacy code on x86 processors.  Disabling

@@ -2047,6 +2048,22 @@ config CMDLINE_OVERRIDE
          This is used to work around broken boot loaders.  This should
          be set to 'N' under normal conditions.
 
+config MODIFY_LDT_SYSCALL
+       bool "Enable the LDT (local descriptor table)" if EXPERT
+       default y
+       ---help---
+         Linux can allow user programs to install a per-process x86
+        Local Descriptor Table (LDT) using the modify_ldt(2) system
+        call.  This is required to run 16-bit or segmented code such as
+        DOSEMU or some Wine programs.  It is also used by some very old
+        threading libraries.
+
+        Enabling this feature adds a small amount of overhead to

Quoted 9 lines

arch/x86/Kconfig config CMDLINE_OVERRIDE · line 2066

Quoted 5 lines

arch/x86/Kconfig +5 -0

+        context switches and increases the low-level kernel attack
+        surface.  Disabling it removes the modify_ldt(2) system call.
+
+        Saying 'N' here may make sense for embedded or server kernels.
+

I believe Wine still uses the LDT for thread-local data, even for 32 and 64-bit programs. This is separate from the Linux runtime TLS.

Really? I thought the whole reason we had three set_thread_area slots was for Wine.

Quoted 44 lines · Brian Gerst

Quoted 43 lines · Andy Lutomirski

The modify_ldt syscall exposes a large attack surface and is unnecessary for modern userspace. Make it optional.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                   | 17 +++++++++++++++++
 arch/x86/include/asm/mmu.h         |  2 ++
 arch/x86/include/asm/mmu_context.h | 31 +++++++++++++++++++++++--------
 arch/x86/kernel/Makefile           |  3 ++-
 arch/x86/kernel/cpu/perf_event.c   |  4 ++++
 arch/x86/kernel/process_64.c       |  2 ++
 arch/x86/kernel/step.c             |  2 ++
 kernel/sys_ni.c                    |  1 +
 8 files changed, 53 insertions(+), 9 deletions(-)

arch/x86/Kconfig +12 -0

index 55bced17dc95..a7ff3980bd65 100644
@@ -1009,6 +1009,7 @@ config VM86
 config X86_16BIT
        bool "Enable support for 16-bit segments" if EXPERT
        default y
+       depends on MODIFY_LDT_SYSCALL
        ---help---
          This option is required by programs like Wine to run 16-bit
          protected mode legacy code on x86 processors.  Disabling

@@ -2047,6 +2048,22 @@ config CMDLINE_OVERRIDE
          This is used to work around broken boot loaders.  This should
          be set to 'N' under normal conditions.
 
+config MODIFY_LDT_SYSCALL
+       bool "Enable the LDT (local descriptor table)" if EXPERT
+       default y
+       ---help---
+         Linux can allow user programs to install a per-process x86
+        Local Descriptor Table (LDT) using the modify_ldt(2) system
+        call.  This is required to run 16-bit or segmented code such as
+        DOSEMU or some Wine programs.  It is also used by some very old
+        threading libraries.
+
+        Enabling this feature adds a small amount of overhead to

Quoted 9 lines

arch/x86/Kconfig config CMDLINE_OVERRIDE · line 2066

Quoted 5 lines

arch/x86/Kconfig +5 -0

+        context switches and increases the low-level kernel attack
+        surface.  Disabling it removes the modify_ldt(2) system call.
+
+        Saying 'N' here may make sense for embedded or server kernels.
+

I believe Wine still uses the LDT for thread-local data, even for 32 and 64-bit programs. This is separate from the Linux runtime TLS.

Really? I thought the whole reason we had three set_thread_area slots was for Wine.

Quoted 55 lines · Andy Lutomirski

Quoted 44 lines · Brian Gerst

Quoted 43 lines · Andy Lutomirski

The modify_ldt syscall exposes a large attack surface and is unnecessary for modern userspace. Make it optional.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                   | 17 +++++++++++++++++
 arch/x86/include/asm/mmu.h         |  2 ++
 arch/x86/include/asm/mmu_context.h | 31 +++++++++++++++++++++++--------
 arch/x86/kernel/Makefile           |  3 ++-
 arch/x86/kernel/cpu/perf_event.c   |  4 ++++
 arch/x86/kernel/process_64.c       |  2 ++
 arch/x86/kernel/step.c             |  2 ++
 kernel/sys_ni.c                    |  1 +
 8 files changed, 53 insertions(+), 9 deletions(-)

arch/x86/Kconfig +12 -0

index 55bced17dc95..a7ff3980bd65 100644
@@ -1009,6 +1009,7 @@ config VM86
 config X86_16BIT
        bool "Enable support for 16-bit segments" if EXPERT
        default y
+       depends on MODIFY_LDT_SYSCALL
        ---help---
          This option is required by programs like Wine to run 16-bit
          protected mode legacy code on x86 processors.  Disabling

@@ -2047,6 +2048,22 @@ config CMDLINE_OVERRIDE
          This is used to work around broken boot loaders.  This should
          be set to 'N' under normal conditions.
 
+config MODIFY_LDT_SYSCALL
+       bool "Enable the LDT (local descriptor table)" if EXPERT
+       default y
+       ---help---
+         Linux can allow user programs to install a per-process x86
+        Local Descriptor Table (LDT) using the modify_ldt(2) system
+        call.  This is required to run 16-bit or segmented code such as
+        DOSEMU or some Wine programs.  It is also used by some very old
+        threading libraries.
+
+        Enabling this feature adds a small amount of overhead to

Quoted 9 lines

arch/x86/Kconfig config CMDLINE_OVERRIDE · line 2066

Quoted 5 lines

arch/x86/Kconfig +5 -0

+        context switches and increases the low-level kernel attack
+        surface.  Disabling it removes the modify_ldt(2) system call.
+
+        Saying 'N' here may make sense for embedded or server kernels.
+

I believe Wine still uses the LDT for thread-local data, even for 32 and 64-bit programs. This is separate from the Linux runtime TLS.

Quoted 2 lines

Really? I thought the whole reason we had three set_thread_area slots was for Wine.

Too bad we have to guess, if only we had the Wine source code under a nicely accessible Git archive or so to check?

git clone git://source.winehq.org/git/wine.git

;-)

Quoted 64 lines · Ingo Molnar

Quoted 55 lines · Andy Lutomirski

Quoted 44 lines · Brian Gerst

Quoted 43 lines · Andy Lutomirski

The modify_ldt syscall exposes a large attack surface and is unnecessary for modern userspace. Make it optional.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                   | 17 +++++++++++++++++
 arch/x86/include/asm/mmu.h         |  2 ++
 arch/x86/include/asm/mmu_context.h | 31 +++++++++++++++++++++++--------
 arch/x86/kernel/Makefile           |  3 ++-
 arch/x86/kernel/cpu/perf_event.c   |  4 ++++
 arch/x86/kernel/process_64.c       |  2 ++
 arch/x86/kernel/step.c             |  2 ++
 kernel/sys_ni.c                    |  1 +
 8 files changed, 53 insertions(+), 9 deletions(-)

arch/x86/Kconfig +12 -0

index 55bced17dc95..a7ff3980bd65 100644
@@ -1009,6 +1009,7 @@ config VM86
 config X86_16BIT
        bool "Enable support for 16-bit segments" if EXPERT
        default y
+       depends on MODIFY_LDT_SYSCALL
        ---help---
          This option is required by programs like Wine to run 16-bit
          protected mode legacy code on x86 processors.  Disabling

@@ -2047,6 +2048,22 @@ config CMDLINE_OVERRIDE
          This is used to work around broken boot loaders.  This should
          be set to 'N' under normal conditions.
 
+config MODIFY_LDT_SYSCALL
+       bool "Enable the LDT (local descriptor table)" if EXPERT
+       default y
+       ---help---
+         Linux can allow user programs to install a per-process x86
+        Local Descriptor Table (LDT) using the modify_ldt(2) system
+        call.  This is required to run 16-bit or segmented code such as
+        DOSEMU or some Wine programs.  It is also used by some very old
+        threading libraries.
+
+        Enabling this feature adds a small amount of overhead to

Quoted 9 lines

arch/x86/Kconfig config CMDLINE_OVERRIDE · line 2066

Quoted 5 lines

arch/x86/Kconfig +5 -0

+        context switches and increases the low-level kernel attack
+        surface.  Disabling it removes the modify_ldt(2) system call.
+
+        Saying 'N' here may make sense for embedded or server kernels.
+

I believe Wine still uses the LDT for thread-local data, even for 32 and 64-bit programs. This is separate from the Linux runtime TLS.

Quoted 2 lines

Really? I thought the whole reason we had three set_thread_area slots was for Wine.

Too bad we have to guess, if only we had the Wine source code under a nicely accessible Git archive or so to check?

Quoted 3 lines

git clone git://source.winehq.org/git/wine.git

;-)

You don't say?

It appears that Wine uses set_thread_area with a fallback to modify_ldt for 32-bit binaries and arch_prctl for 64-bit.

Quoted 65 lines · Andy Lutomirski

Quoted 64 lines · Ingo Molnar

Quoted 55 lines · Andy Lutomirski

Quoted 44 lines · Brian Gerst

Quoted 43 lines · Andy Lutomirski

The modify_ldt syscall exposes a large attack surface and is unnecessary for modern userspace. Make it optional.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                   | 17 +++++++++++++++++
 arch/x86/include/asm/mmu.h         |  2 ++
 arch/x86/include/asm/mmu_context.h | 31 +++++++++++++++++++++++--------
 arch/x86/kernel/Makefile           |  3 ++-
 arch/x86/kernel/cpu/perf_event.c   |  4 ++++
 arch/x86/kernel/process_64.c       |  2 ++
 arch/x86/kernel/step.c             |  2 ++
 kernel/sys_ni.c                    |  1 +
 8 files changed, 53 insertions(+), 9 deletions(-)

arch/x86/Kconfig +12 -0

index 55bced17dc95..a7ff3980bd65 100644
@@ -1009,6 +1009,7 @@ config VM86
 config X86_16BIT
        bool "Enable support for 16-bit segments" if EXPERT
        default y
+       depends on MODIFY_LDT_SYSCALL
        ---help---
          This option is required by programs like Wine to run 16-bit
          protected mode legacy code on x86 processors.  Disabling

@@ -2047,6 +2048,22 @@ config CMDLINE_OVERRIDE
          This is used to work around broken boot loaders.  This should
          be set to 'N' under normal conditions.
 
+config MODIFY_LDT_SYSCALL
+       bool "Enable the LDT (local descriptor table)" if EXPERT
+       default y
+       ---help---
+         Linux can allow user programs to install a per-process x86
+        Local Descriptor Table (LDT) using the modify_ldt(2) system
+        call.  This is required to run 16-bit or segmented code such as
+        DOSEMU or some Wine programs.  It is also used by some very old
+        threading libraries.
+
+        Enabling this feature adds a small amount of overhead to

Quoted 9 lines

arch/x86/Kconfig config CMDLINE_OVERRIDE · line 2066

Quoted 5 lines

arch/x86/Kconfig +5 -0

+        context switches and increases the low-level kernel attack
+        surface.  Disabling it removes the modify_ldt(2) system call.
+
+        Saying 'N' here may make sense for embedded or server kernels.
+

I believe Wine still uses the LDT for thread-local data, even for 32 and 64-bit programs. This is separate from the Linux runtime TLS.

Quoted 2 lines

Really? I thought the whole reason we had three set_thread_area slots was for Wine.

Too bad we have to guess, if only we had the Wine source code under a nicely accessible Git archive or so to check?

Quoted 8 lines

Quoted 3 lines

git clone git://source.winehq.org/git/wine.git

;-)

You don't say?

It appears that Wine uses set_thread_area with a fallback to modify_ldt for 32-bit binaries and arch_prctl for 64-bit.

I also think it uses modify_ldt() unconditionally for 16-bit APIs:

triton:~/wine> git grep wine_ldt_set_entry

dlls/krnl386.exe16/dosvm.c:        wine_ldt_set_entry( sel, &entry );
dlls/krnl386.exe16/int31.c:        wine_ldt_set_entry( sel, &entry );
dlls/krnl386.exe16/int31.c:            wine_ldt_set_entry( BX_reg(context), entry );
dlls/krnl386.exe16/selector.c:            if (wine_ldt_set_entry( sel + (i << __AHSHIFT), &entry ) < 0)
dlls/krnl386.exe16/selector.c:        wine_ldt_set_entry( newsel + (i << __AHSHIFT), &entry );
dlls/krnl386.exe16/selector.c:        if (wine_ldt_set_entry( sel + (i << __AHSHIFT), &entry ) < 0) return FALSE;
dlls/krnl386.exe16/selector.c:    wine_ldt_set_entry( selDst, &entry );
dlls/krnl386.exe16/selector.c:    if (wine_ldt_set_entry( newsel, &entry ) >= 0) return newsel;
dlls/krnl386.exe16/selector.c:    if (wine_ldt_set_entry( newsel, &entry ) >= 0) return newsel;
dlls/krnl386.exe16/selector.c:    wine_ldt_set_entry( SELECTOROF(ptr), &entry );
dlls/krnl386.exe16/selector.c:    if (wine_ldt_set_entry( sel, &entry ) < 0) sel = 0;
dlls/krnl386.exe16/selector.c:    if (wine_ldt_set_entry( sel, &entry ) < 0) sel = 0;
dlls/krnl386.exe16/selector.c:        wine_ldt_set_entry( sel, &entry );
dlls/krnl386.exe16/wowthunk.c:    wine_ldt_set_entry( codesel, &entry );
dlls/user.exe16/message.c:        wine_ldt_set_entry( thunk_selector, &entry );

include/wine/library.h:extern int wine_ldt_set_entry( unsigned short sel, const LDT_ENTRY *entry );
libs/wine/ldt.c: * wine_ldt_set_entry
libs/wine/ldt.c:int wine_ldt_set_entry( unsigned short sel, const LDT_ENTRY *entry )
libs/wine/wine.map: wine_ldt_set_entry;

So the situation looks mostly encouraging, IMHO.

Plus an 'strace -fc' output of a modern, Wine driven Windows game would probably tell us pretty definitely whether there's anything particularly performance sensitive about modify_ldt().

Quoted 27 lines

Quoted 21 lines

Quoted 11 lines

Quoted 11 lines

arch/x86/Kconfig +11 -0

+config MODIFY_LDT_SYSCALL
+       bool "Enable the LDT (local descriptor table)" if EXPERT
+       default y
+       ---help---
+         Linux can allow user programs to install a per-process x86
+        Local Descriptor Table (LDT) using the modify_ldt(2) system
+        call.  This is required to run 16-bit or segmented code such as
+        DOSEMU or some Wine programs.  It is also used by some very old
+        threading libraries.
+
+        Enabling this feature adds a small amount of overhead to

Quoted 9 lines

arch/x86/Kconfig config CMDLINE_OVERRIDE · line 2066

Quoted 5 lines

arch/x86/Kconfig +5 -0

+        context switches and increases the low-level kernel attack
+        surface.  Disabling it removes the modify_ldt(2) system call.
+
+        Saying 'N' here may make sense for embedded or server kernels.
+

I believe Wine still uses the LDT for thread-local data, even for 32 and 64-bit programs. This is separate from the Linux runtime TLS.

Quoted 2 lines

Really? I thought the whole reason we had three set_thread_area slots was for Wine.

Too bad we have to guess, if only we had the Wine source code under a nicely accessible Git archive or so to check?

Quoted 3 lines

git clone git://source.winehq.org/git/wine.git

;-)

You don't say?

It appears that Wine uses set_thread_area with a fallback to modify_ldt for 32-bit binaries and arch_prctl for 64-bit.

tools/testing/selftests/x86/ldt_gdt.c new file +4 -0

new file mode 100644
index 000000000000..6f6699f0351a
@@ -0,0 +1,492 @@
+/*
+ * ldt_gdt.c - Test cases for LDT and GDT access
+ * Copyright (c) 2015 Andrew Lutomirski
+ */

Quoted 17 lines · Andy Lutomirski

tools/testing/selftests/x86/ldt_gdt.c new file +9 -0

new file mode 100644
index 000000000000..6f6699f0351a
@@ -0,0 +1,492 @@
+/*
+ * ldt_gdt.c - Test cases for LDT and GDT access
+ * Copyright (c) 2015 Andrew Lutomirski
+ */
+
+#define _GNU_SOURCE
+#include <err.h>
+#include <stdio.h>
+#include <stdint.h>

tools/testing/selftests/x86/ldt_gdt.c line 14

Quoted 5 lines

tools/testing/selftests/x86/ldt_gdt.c new file +5 -0

+#include <signal.h>
+#include <setjmp.h>
+#include <string.h>
+#include <errno.h>
+#include <xmmintrin.h>

Is xmmintrin.h necessary? It breaks 32-bit build with

This tests general modify_ldt behavior (only writes, so far) as well as synchronous updates via IPI. It fails on old kernels.

I called this ldt_gdt because I'll add set_thread_area tests to it at some point.

Hi Andy,

Quoted 5 lines · Andy Lutomirski

This tests general modify_ldt behavior (only writes, so far) as well as synchronous updates via IPI. It fails on old kernels.

I called this ldt_gdt because I'll add set_thread_area tests to it at some point.

Quick feedback : at two places you have this :

Review

Quoted 2 lines

(quoted diff) +2 -0

+     } else if (errno == ENOSYS) {
+             printf("[OK]\tmodify_ldt is returned -ENOSYS\n");

=> s/is //

Please add stdlib.h to avoid this warning I'm getting on 32-bit :

ldt_gdt.c:286:4: warning: incompatible implicit declaration of built-in function 'exit' [enabled by default]

And I had to remove xmmintrinsic as suggested by Boris as well.

FWIW here's what I'm getting here on 4.1.2 without CONFIG_X86_16BIT (where nmi_espfix failed), the output is the same for a 32- and a 64-bit process :

[OK]    LDT entry 0 has AR 0x0040FA00 and limit 0x0000000A
[OK]    LDT entry 0 has AR 0x00C0FA00 and limit 0x0000AFFF
[OK]    LDT entry 1 is invalid
[OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
[OK]    LDT entry 1 is invalid
[OK]    LDT entry 2 has AR 0x00C0FA00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D0FA00 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07A00 and limit 0x0000AFFF
[OK]    modify_ldt rejected 16 bit segment
[OK]    LDT entry 2 has AR 0x00D07200 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07000 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00D07400 and limit 0x0000AFFF
[OK]    LDT entry 2 has AR 0x00507600 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507E00 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507C00 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507A00 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
[OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
[RUN]   Test fork
[OK]    LDT entry 2 has AR 0x00507800 and limit 0x0000000A
[OK]    LDT entry 1 is invalid
[OK]    Child succeeded
[OK]    modify_ldt failure 22
[OK]    modify_ldt rejected 16 bit segment
[OK]    modify_ldt rejected 16 bit segment
[OK]    modify_ldt rejected 16 bit segment

[OK]    modify_ldt rejected 16 bit segment
[OK]    modify_ldt rejected 16 bit segment
[OK]    modify_ldt rejected 16 bit segment
[OK]    LDT entry 0 is invalid
[OK]    LDT entry 0 has AR 0x0040F200 and limit 0x00000000
[OK]    LDT entry 0 is invalid
[RUN]   Cross-CPU LDT invalidation
[FAIL]  5 of 5 iterations failed

Thanks,
Willy