[Xen-devel] [PATCH RFC] x86/32on64: don't modify guest descriptors without need

["Jan Beulich" <JBeulich@xxxxxxxx>]

There are two cases: The obvious one is that system gates with type 0
shouldn't have what might be their DPL altered - such descriptors can't
be used anyway without incurring a #GP, and hence adjusting its DPL is
only risking to confuse the guest.

The less obvious (and potentially controversial) part is that of a call
gate with DPL < selector.RPL: Such gates can't be used either without
the hardware raising #GP, but the specific case of DPL=0 gets handled
in emulate_gate_op(). The purpose of the change is to avoid adjusting
a gate a second time when it did get adjusted already here, on the
assumption that guest OSes wouldn't normally install unusable gates
(i.e. we'd normally see DPL >= selector.RPL) and still have a way of
installing an unusable gate (by specifying a non-zero DPL right away).

Aforementioned second time adjustments of gates are a problem for
environments where
- per-CPU GDTs get cloned from the boot CPU's after the boot CPU had
  already installed its GDT (e.g. Linux),
- individual descriptors get installed via hypercall prior to actually
  making the respective page a descriptor one (this could alternatively
  be taken care of by making do_update_descriptor() call
  check_descriptor() only when modifying a PGT_seg_desc page),
i.e. the hypervisor gets presented an already adjusted descriptor a
second time.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
RFC reason: As mentioned above the change in behavior here might be
            controversial. The main question here appears to be whether
            this is viewed as having the potential for undermining
            guest OS security.

--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1085,12 +1085,11 @@ int check_descriptor(const struct domain
 
     /* A not-present descriptor will always fault, so is safe. */
     if ( !(b & _SEGMENT_P) ) 
-        goto good;
+        return 1;
 
     /* Check and fix up the DPL. */
     dpl = (b >> 13) & 3;
     __fixup_guest_selector(dom, dpl);
-    b = (b & ~_SEGMENT_DPL) | (dpl << 13);
 
     /* All code and data segments are okay. No base/limit checking. */
     if ( (b & _SEGMENT_S) )
@@ -1127,14 +1126,23 @@ int check_descriptor(const struct domain
 
     /* Invalid type 0 is harmless. It is used for 2nd half of a call gate. */
     if ( (b & _SEGMENT_TYPE) == 0x000 )
-        goto good;
+        return 1;
 
     /* Everything but a call gate is discarded here. */
     if ( (b & _SEGMENT_TYPE) != 0xc00 )
         goto bad;
 
-    /* Validate the target code selector. */
+    /*
+     * The gate's DPL being less than the gate selector's RPL is harmless too,
+     * as any access to such a gate will cause #GP. This in particular avoids
+     * modifying (see below) a gate a second time after it had its DPL forced
+     * to zero for emulation.
+     */
     cs = a >> 16;
+    if ( ((b >> 13) & 3) < (cs & 3) )
+        return 1;
+
+    /* Validate the target code selector. */
     if ( !guest_gate_selector_okay(dom, cs) )
         goto bad;
     /*
@@ -1147,8 +1155,8 @@ int check_descriptor(const struct domain
      * there are only 64-bit ones.
      * Store the original DPL in the selector's RPL field.
      */
-    b &= ~_SEGMENT_DPL;
     cs = (cs & ~3) | dpl;
+    dpl = 0;
     a = (a & 0xffffU) | (cs << 16);
 
     /* Reserved bits must be zero. */
@@ -1157,7 +1165,7 @@ int check_descriptor(const struct domain
         
  good:
     d->a = a;
-    d->b = b;
+    d->b = (b & ~_SEGMENT_DPL) | (dpl << 13);
     return 1;
  bad:
     return 0;

Attachment: x86-32on64-gate-op-reuse.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel