[Xen-changelog] [xen staging-4.10] x86/PV: check GDT/LDT limits during emulation

[patchbot@xxxxxxx]

commit 0824bc6af1414208a65822a60046ceb9e3e72c24
Author:     Jan Beulich <jbeulich@xxxxxxxx>
AuthorDate: Mon Nov 4 14:46:09 2019 +0100
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Mon Nov 4 14:46:09 2019 +0100

    x86/PV: check GDT/LDT limits during emulation
    
    Accesses beyond the LDT limit originating from emulation would trigger
    the ASSERT() in pv_map_ldt_shadow_page(). On production builds such
    accesses would cause an attempt to promote the touched page (offset from
    the present LDT base address) to a segment descriptor one. If this
    happens to succeed, guest user mode would be able to elevate its
    privileges to that of the guest kernel. This is particularly easy when
    there's no LDT at all, in which case the LDT base stored internally to
    Xen is simply zero.
    
    Also adjust the ASSERT() that was triggering: It was off by one to
    begin with, and for production builds we also better use
    ASSERT_UNREACHABLE() instead with suitable recovery code afterwards.
    
    This is XSA-298.
    
    Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
    Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    master commit: 93021cbe880a8013691a48d0febef8ed7d3e3ebd
    master date: 2019-10-31 16:08:16 +0100
---
 xen/arch/x86/pv/emul-gate-op.c | 10 ++++++++--
 xen/arch/x86/pv/emulate.c      |  9 ++++++++-
 xen/arch/x86/pv/mm.c           |  8 ++++++--
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/pv/emul-gate-op.c b/xen/arch/x86/pv/emul-gate-op.c
index 0f89c91dff..884d27ed80 100644
--- a/xen/arch/x86/pv/emul-gate-op.c
+++ b/xen/arch/x86/pv/emul-gate-op.c
@@ -60,7 +60,13 @@ static int read_gate_descriptor(unsigned int gate_sel,
         (!(gate_sel & 4) ? GDT_VIRT_START(v) : LDT_VIRT_START(v))
         + (gate_sel >> 3);
     if ( (gate_sel < 4) ||
-         ((gate_sel >= FIRST_RESERVED_GDT_BYTE) && !(gate_sel & 4)) ||
+         /*
+          * We're interested in call gates only, which occupy a single
+          * seg_desc_t for 32-bit and a consecutive pair of them for 64-bit.
+          */
+         ((gate_sel >> 3) + !is_pv_32bit_vcpu(v) >=
+          (gate_sel & 4 ? v->arch.pv_vcpu.ldt_ents
+                        : v->arch.pv_vcpu.gdt_ents)) ||
          __get_user(desc, pdesc) )
         return 0;
 
@@ -79,7 +85,7 @@ static int read_gate_descriptor(unsigned int gate_sel,
     if ( !is_pv_32bit_vcpu(v) )
     {
         if ( (*ar & 0x1f00) != 0x0c00 ||
-             (gate_sel >= FIRST_RESERVED_GDT_BYTE - 8 && !(gate_sel & 4)) ||
+             /* Limit check done above already. */
              __get_user(desc, pdesc + 1) ||
              (desc.b & 0x1f00) )
             return 0;
diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
index 5750c7699b..d9b232024b 100644
--- a/xen/arch/x86/pv/emulate.c
+++ b/xen/arch/x86/pv/emulate.c
@@ -31,7 +31,14 @@ int pv_emul_read_descriptor(unsigned int sel, const struct 
vcpu *v,
 {
     struct desc_struct desc;
 
-    if ( sel < 4)
+    if ( sel < 4 ||
+         /*
+          * Don't apply the GDT limit here, as the selector may be a Xen
+          * provided one. __get_user() will fail (without taking further
+          * action) for ones falling in the gap between guest populated
+          * and Xen ones.
+          */
+         ((sel & 4) && (sel >> 3) >= v->arch.pv_vcpu.ldt_ents) )
         desc.b = desc.a = 0;
     else if ( __get_user(desc,
                          (const struct desc_struct *)(!(sel & 4)
diff --git a/xen/arch/x86/pv/mm.c b/xen/arch/x86/pv/mm.c
index 8d7a4fd85f..57490e18b0 100644
--- a/xen/arch/x86/pv/mm.c
+++ b/xen/arch/x86/pv/mm.c
@@ -98,12 +98,16 @@ bool pv_map_ldt_shadow_page(unsigned int offset)
     BUG_ON(unlikely(in_irq()));
 
     /*
-     * Hardware limit checking should guarantee this property.  NB. This is
+     * Prior limit checking should guarantee this property.  NB. This is
      * safe as updates to the LDT can only be made by MMUEXT_SET_LDT to the
      * current vcpu, and vcpu_reset() will block until this vcpu has been
      * descheduled before continuing.
      */
-    ASSERT((offset >> 3) <= curr->arch.pv_vcpu.ldt_ents);
+    if ( unlikely((offset >> 3) >= curr->arch.pv_vcpu.ldt_ents) )
+    {
+        ASSERT_UNREACHABLE();
+        return false;
+    }
 
     if ( is_pv_32bit_domain(currd) )
         linear = (uint32_t)linear;
--
generated by git-patchbot for /home/xen/git/xen.git#staging-4.10

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/xen-changelog