Re: [Xen-devel] [PATCH v5 4/4] x86/ldt: Make modify_ldt optional

2015-07-28 Thread Kees Cook 
On Mon, Jul 27, 2015 at 10:29 PM, Andy Lutomirski l...@kernel.org wrote:
 The modify_ldt syscall exposes a large attack surface and is
 unnecessary for modern userspace.  Make it optional.

 Signed-off-by: Andy Lutomirski l...@kernel.org

Reviewed-by: Kees Cook keesc...@chromium.org

 ---
  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(-)

 diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
 index b3a1a5d77d92..ede52be845db 100644
 --- a/arch/x86/Kconfig
 +++ b/arch/x86/Kconfig
 @@ -1015,6 +1015,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
 @@ -2053,6 +2054,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

nit: looks like a spaces vs tabs issue in the line above?

 +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
 +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.
 +
  source kernel/livepatch/Kconfig

  endmenu
 diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h
 index 364d27481a52..55234d5e7160 100644
 --- a/arch/x86/include/asm/mmu.h
 +++ b/arch/x86/include/asm/mmu.h
 @@ -9,7 +9,9 @@
   * we put the segment information here.
   */
  typedef struct {
 +#ifdef CONFIG_MODIFY_LDT_SYSCALL
 struct ldt_struct *ldt;
 +#endif

  #ifdef CONFIG_X86_64
 /* True if mm supports a task running in 32 bit compatibility mode. */
 diff --git a/arch/x86/include/asm/mmu_context.h 
 b/arch/x86/include/asm/mmu_context.h
 index 3fcff70c398e..08094eded318 100644
 --- a/arch/x86/include/asm/mmu_context.h
 +++ b/arch/x86/include/asm/mmu_context.h
 @@ -33,6 +33,7 @@ static inline void load_mm_cr4(struct mm_struct *mm)
  static inline void load_mm_cr4(struct mm_struct *mm) {}
  #endif

 +#ifdef CONFIG_MODIFY_LDT_SYSCALL
  /*
   * ldt_structs can be allocated, used, and freed, but they are never
   * modified while live.
 @@ -48,10 +49,24 @@ struct ldt_struct {
 int size;
  };

 +/*
 + * Used for LDT copy/destruction.
 + */
 +int init_new_context(struct task_struct *tsk, struct mm_struct *mm);
 +void destroy_context(struct mm_struct *mm);
 +#else  /* CONFIG_MODIFY_LDT_SYSCALL */
 +static inline int init_new_context(struct task_struct *tsk,
 +  struct mm_struct *mm)
 +{
 +   return 0;
 +}
 +static inline void destroy_context(struct mm_struct *mm) {}
 +#endif
 +
  static inline void load_mm_ldt(struct mm_struct *mm)
  {
 +#ifdef CONFIG_MODIFY_LDT_SYSCALL
 struct ldt_struct *ldt;
 -   DEBUG_LOCKS_WARN_ON(!irqs_disabled());

 /* lockless_dereference synchronizes with smp_store_release */
 ldt = lockless_dereference(mm-context.ldt);
 @@ -74,14 +89,12 @@ static inline void load_mm_ldt(struct mm_struct *mm)
 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);
 -void destroy_context(struct mm_struct *mm);
 +#else
 +   clear_LDT();
 +#endif

 +   DEBUG_LOCKS_WARN_ON(!irqs_disabled());
 +}

  static inline void enter_lazy_tlb(struct mm_struct *mm, struct task_struct 
 *tsk)
  {
 @@ -113,6 +126,7 @@ static inline void switch_mm(struct mm_struct *prev, 
 struct mm_struct *next,
 /* Load per-mm CR4 state */
 load_mm_cr4(next);

 +#ifdef CONFIG_MODIFY_LDT_SYSCALL
 /*
  * Load the LDT, if the LDT is different.
  *
 @@ -127,6 +141,7 @@ static inline void switch_mm(struct mm_struct *prev, 
 struct mm_struct *next,
  */
 if (unlikely(prev-context.ldt != next-context.ldt))

Re: [Xen-devel] [PATCH v5 4/4] x86/ldt: Make modify_ldt optional

2015-07-28 Thread Kees Cook 
On Tue, Jul 28, 2015 at 1:03 PM, Willy Tarreau w...@1wt.eu wrote:
 Hi Kees,

 On Tue, Jul 28, 2015 at 09:56:12AM -0700, Kees Cook wrote:
 I look forward to the runtime disabling patch. :)

 Did you get my response to your comments regarding the proposed patch ?

 I can rebase it and update it if needed, I just want to make sure
 everyone's on the same line regarding this.

Yeah, I'm fine with what you have. I'd still like a off until next
reboot, but I'll live. :)

-Kees

-- 
Kees Cook
Chrome OS Security

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 4/4] x86/ldt: Make modify_ldt optional

2015-07-28 Thread Willy Tarreau 
On Tue, Jul 28, 2015 at 01:42:20PM -0700, Kees Cook wrote:
 On Tue, Jul 28, 2015 at 1:03 PM, Willy Tarreau w...@1wt.eu wrote:
  Hi Kees,
 
  On Tue, Jul 28, 2015 at 09:56:12AM -0700, Kees Cook wrote:
  I look forward to the runtime disabling patch. :)
 
  Did you get my response to your comments regarding the proposed patch ?
 
  I can rebase it and update it if needed, I just want to make sure
  everyone's on the same line regarding this.
 
 Yeah, I'm fine with what you have. I'd still like a off until next
 reboot, but I'll live. :)

That's an improvement we can bring later with special value -1 which is
even less than zero.

Thanks,
Willy


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v5 4/4] x86/ldt: Make modify_ldt optional

2015-07-27 Thread 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 l...@kernel.org
---
 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(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b3a1a5d77d92..ede52be845db 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1015,6 +1015,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
@@ -2053,6 +2054,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
+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.
+
 source kernel/livepatch/Kconfig
 
 endmenu
diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h
index 364d27481a52..55234d5e7160 100644
--- a/arch/x86/include/asm/mmu.h
+++ b/arch/x86/include/asm/mmu.h
@@ -9,7 +9,9 @@
  * we put the segment information here.
  */
 typedef struct {
+#ifdef CONFIG_MODIFY_LDT_SYSCALL
struct ldt_struct *ldt;
+#endif
 
 #ifdef CONFIG_X86_64
/* True if mm supports a task running in 32 bit compatibility mode. */
diff --git a/arch/x86/include/asm/mmu_context.h 
b/arch/x86/include/asm/mmu_context.h
index 3fcff70c398e..08094eded318 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -33,6 +33,7 @@ static inline void load_mm_cr4(struct mm_struct *mm)
 static inline void load_mm_cr4(struct mm_struct *mm) {}
 #endif
 
+#ifdef CONFIG_MODIFY_LDT_SYSCALL
 /*
  * ldt_structs can be allocated, used, and freed, but they are never
  * modified while live.
@@ -48,10 +49,24 @@ struct ldt_struct {
int size;
 };
 
+/*
+ * Used for LDT copy/destruction.
+ */
+int init_new_context(struct task_struct *tsk, struct mm_struct *mm);
+void destroy_context(struct mm_struct *mm);
+#else  /* CONFIG_MODIFY_LDT_SYSCALL */
+static inline int init_new_context(struct task_struct *tsk,
+  struct mm_struct *mm)
+{
+   return 0;
+}
+static inline void destroy_context(struct mm_struct *mm) {}
+#endif
+
 static inline void load_mm_ldt(struct mm_struct *mm)
 {
+#ifdef CONFIG_MODIFY_LDT_SYSCALL
struct ldt_struct *ldt;
-   DEBUG_LOCKS_WARN_ON(!irqs_disabled());
 
/* lockless_dereference synchronizes with smp_store_release */
ldt = lockless_dereference(mm-context.ldt);
@@ -74,14 +89,12 @@ static inline void load_mm_ldt(struct mm_struct *mm)
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);
-void destroy_context(struct mm_struct *mm);
+#else
+   clear_LDT();
+#endif
 
+   DEBUG_LOCKS_WARN_ON(!irqs_disabled());
+}
 
 static inline void enter_lazy_tlb(struct mm_struct *mm, struct task_struct 
*tsk)
 {
@@ -113,6 +126,7 @@ static inline void switch_mm(struct mm_struct *prev, struct 
mm_struct *next,
/* Load per-mm CR4 state */
load_mm_cr4(next);
 
+#ifdef CONFIG_MODIFY_LDT_SYSCALL
/*
 * Load the LDT, if the LDT is different.
 *
@@ -127,6 +141,7 @@ static inline void switch_mm(struct mm_struct *prev, struct 
mm_struct *next,
 */
if (unlikely(prev-context.ldt != next-context.ldt))
load_mm_ldt(next);
+#endif
}
 #ifdef CONFIG_SMP
  else {
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 0f15af41bd80..2b507befcd3f 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -24,7 +24,8 @@ CFLAGS_irq.o :=