Re: [Xen-devel] [PATCH 3.2 110/110] x86/ldt: Make modify_ldt synchronous

2015-08-11 Thread Ben Hutchings
On Mon, 2015-08-10 at 09:47 -0700, Andy Lutomirski wrote:
> On Mon, Aug 10, 2015 at 3:12 AM, Ben Hutchings  wrote:
> > 3.2.71-rc1 review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > From: Andy Lutomirski 
> > 
> > commit 37868fe113ff2ba814b3b4eb12df214df555f8dc upstream.
> 
> Unfortunately, this patch was slightly buggy.  The fixes are:
> 
> https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=x86/urgent&id=4809146b86c3d41ce588fdb767d021e2a80600dd
> 
> https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=x86/urgent&id=136d9d83c07c5e30ac49fc83b27e8c4842f108fc
> 
> Grr, making major changes like this in the middle of a release cycle
> isn't the best.

OK, I'll defer this to the next update.  Thanks.

Ben.

-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of comp.compilers



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel


Re: [Xen-devel] [PATCH 3.2 110/110] x86/ldt: Make modify_ldt synchronous

2015-08-10 Thread Andy Lutomirski
On Mon, Aug 10, 2015 at 3:12 AM, Ben Hutchings  wrote:
> 3.2.71-rc1 review patch.  If anyone has any objections, please let me know.
>
> --
>
> From: Andy Lutomirski 
>
> commit 37868fe113ff2ba814b3b4eb12df214df555f8dc upstream.

Unfortunately, this patch was slightly buggy.  The fixes are:

https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=x86/urgent&id=4809146b86c3d41ce588fdb767d021e2a80600dd

https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=x86/urgent&id=136d9d83c07c5e30ac49fc83b27e8c4842f108fc

Grr, making major changes like this in the middle of a release cycle
isn't the best.

--Andy

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


[Xen-devel] [PATCH 3.2 110/110] x86/ldt: Make modify_ldt synchronous

2015-08-10 Thread Ben Hutchings
3.2.71-rc1 review patch.  If anyone has any objections, please let me know.

--

From: Andy Lutomirski 

commit 37868fe113ff2ba814b3b4eb12df214df555f8dc upstream.

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.

This fixes some fallout from the CVE-2015-5157 fixes.

Signed-off-by: Andy Lutomirski 
Reviewed-by: Borislav Petkov 
Cc: Andrew Cooper 
Cc: Andy Lutomirski 
Cc: Boris Ostrovsky 
Cc: Borislav Petkov 
Cc: Brian Gerst 
Cc: Denys Vlasenko 
Cc: H. Peter Anvin 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Sasha Levin 
Cc: Steven Rostedt 
Cc: Thomas Gleixner 
Cc: secur...@kernel.org 
Cc: xen-devel 
Link: 
http://lkml.kernel.org/r/4c6978476782160600471bd865b318db34c7b628.1438291540.git.l...@kernel.org
Signed-off-by: Ingo Molnar 
[bwh: Backported to 3.2:
 - Adjust context
 - Drop comment changes in switch_mm()
 - Drop changes to get_segment_base() in arch/x86/kernel/cpu/perf_event.c
 - Open-code lockless_dereference(), smp_store_release(), on_each_cpu_mask()]
Signed-off-by: Ben Hutchings 
---
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -277,21 +277,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));
--- a/arch/x86/include/asm/mmu.h
+++ b/arch/x86/include/asm/mmu.h
@@ -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. */
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -16,6 +16,51 @@ static inline void paravirt_activate_mm(
 #endif /* !CONFIG_PARAVIRT */
 
 /*
+ * ldt_structs can be allocated, used, and freed, but they are never
+ * modified while live.
+ */
+struct ldt_struct {
+   /*
+* Xen requires page-aligned LDTs with special permissions.  This is
+* needed to prevent us from installing evil descriptors such as
+* call gates.  On native, we could merge the ldt_struct and LDT
+* allocations, but it's not worth trying to optimize.
+*/
+   struct desc_struct *entries;
+   int size;
+};
+
+static inline void load_mm_ldt(struct mm_struct *mm)
+{
+   struct ldt_struct *ldt;
+
+   /* smp_read_barrier_depends synchronizes with barrier in install_ldt */
+   ldt = ACCESS_ONCE(mm->context.ldt);
+   smp_read_barrier_depends();
+
+   /*
+* 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();
+
+   DEBUG_LOCKS_WARN_ON(preemptible());
+}
+
+/*
  * Used for LDT copy/destruction.
  */
 int init_new_context(struct task_struct *tsk, struct mm_struct *mm);
@@ -52,7 +97,7 @@ static inline void switch_mm(struct mm_s
 * load the LDT, if the LDT is different:
 */
if (unlikely(prev->context.ldt != next->context.ldt))
-   load_LDT_nolock(&next->context);
+   load_mm_ldt(next);
}
 #ifdef CONFIG_SMP
else {
@@ -65,7 +110,7 @@ static inline void switch_mm(struct mm_s
 * to make sure to use no freed page tables.
 */
load_cr3(next->pgd);
-   load_LDT_nolock(&next->context);
+   load_mm_ldt(next);
}
}
 #endif
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1225,7 +1225,7 @@ void __cpuinit cpu_init(void)