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


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  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
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.

Cheers,
Willy


___
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 Kees Cook
On Mon, Jul 27, 2015 at 10:29 PM, Andy Lutomirski  wrote:
> The modify_ldt syscall exposes a large attack surface and is
> unnecessary for modern userspace.  Make it optional.
>
> Signed-off-by: Andy Lutomirski 

Reviewed-by: Kees Cook 

> ---
>  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,
>