i386: adjustment to segment fixup code

PATCH New Jan Beulich 1 participant 1 message Nov 15, 2007

Share & tools

Jan Beulich

author

2007-11-15 11:30 UTC #0 lore raw

PATCH 1/1 i386: adjustment to segment fixup code

Since SLES9SP4 is unfortunately expected to ship with a glibc that does use negative offsets into the TLS area, I looked more closely at the fixup code in Xen. Besides lacking support for SIB addressing and many general purpose instructions, I realized that the non-atomic updates of descriptor table entries provide a hole (albeit very small) for attacks on the hypervisor address space: Accessing negative offsets from a descriptor set up to have a base of e.g. 0xF0000000 (and its limit therefore truncated by Xen) would result in the low 16 bits of the limit in the descriptor being updated first (thus bumping the limit to the 4Gb boundary), and setting the expand down bit only in a subsequent memory access. Thus, if a guest tried hard enough it might be possible to load the %gs selector register at exactly the right point to access this partially updated descriptor. The kernel of that guest would then, by avoiding a reload of the selector register, be able to access Xen's private space.

Even worse - since the code in xen/arch/x86/x86_32/seg_fixup.c doesn't appear to be serialized in any way, under SMP there was a small potential for producing permanently corrupted descriptors.

Remains one question: What does BIGLOCK protect in do_update_descriptor()? If all it's about is to avoid racing updates, I would think that its use could be dropped with this patch applied.

Signed-off-by Jan Beulich <[email protected]>

xen/arch/x86/mm.c +1 -1

Index: 2007-11-13/xen/arch/x86/mm.c
===================================================================
@@ -3113,7 +3113,7 @@ long do_update_descriptor(u64 pa, u64 de
 
     /* All is good so make the update. */
     gdt_pent = map_domain_page(mfn);
-    memcpy(&gdt_pent[offset], &d, 8);
+    write_guest_descriptor(gdt_pent + offset, d);
     unmap_domain_page(gdt_pent);
 
     put_page_type(page);

xen/arch/x86/x86_32/seg_fixup.c +94 ~~-109~~

413 lines not on this page · raw message

xen/arch/x86/x86_emulate.c +1 -1

Index: 2007-11-13/xen/arch/x86/x86_emulate.c
===================================================================
@@ -2263,7 +2263,7 @@ x86_emulate(
     case 0x09: /* wbinvd */
         generate_exception_if(!mode_ring0(), EXC_GP);
         fail_if(ops->wbinvd == NULL);
-        if ( (rc = ops->wbinvd(ctxt)) != 0 )
+        if ( (rc = (ops->wbinvd)(ctxt)) != 0 )
             goto done;
         break;

xen/include/asm-x86/desc.h +28 -0

Index: 2007-11-13/xen/include/asm-x86/desc.h
===================================================================
@@ -137,6 +137,11 @@ struct desc_struct {
 
 #if defined(__x86_64__)
 
+#include <asm/uaccess.h>
+
+/* Use __put_user() just to ensure the compiler doesn't split the write. */
+#define write_guest_descriptor(pent, d) ((void)__put_user(d, pent))
+
 typedef struct {
     u64 a, b;
 } idt_entry_t;

@@ -168,6 +173,29 @@ do {                                    
 
 #elif defined(__i386__)
 
+#include <asm/system.h>
+
+static inline int read_guest_descriptor(struct desc_struct *pent,
+                                        struct desc_struct *pd)
+{
+    union { u64 raw; struct desc_struct d; } val = { 0 };
+    int rc = cmpxchg_user(pent, val.raw, val.raw);
+
+    *pd = val.d;
+    return rc;
+}
+
+static inline void write_guest_descriptor(struct desc_struct *pent,
+                                          struct desc_struct d)
+{
+    u64 cur = ((u64)pent->b << 32) | pent->a, old;
+
+    do
+    {
+        old = cmpxchg((u64 *)pent, cur, ((u64)d.b << 32) | d.a);
+    } while ( old ^ cur );
+}
+
 typedef struct desc_struct idt_entry_t;
 
 #define _set_gate(gate_addr,type,dpl,addr) \