[xen master] x86/pv: Correct the auditing of guest breakpoint addresses

[patchbot@xxxxxxx]

commit dc9d9aa62ddeb14abd5672690d30789829f58f7e
Author:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
AuthorDate: Tue Sep 19 12:13:50 2023 +0100
Commit:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Wed Oct 11 06:36:50 2023 +0100

    x86/pv: Correct the auditing of guest breakpoint addresses
    
    The use of access_ok() is buggy, because it permits access to the compat
    translation area.  64bit PV guests don't use the XLAT area, but on AMD
    hardware, the DBEXT feature allows a breakpoint to match up to a 4G aligned
    region, allowing the breakpoint to reach outside of the XLAT area.
    
    Prior to c/s cda16c1bb223 ("x86: mirror compat argument translation area for
    32-bit PV"), the live GDT was within 4G of the XLAT area.
    
    All together, this allowed a malicious 64bit PV guest on AMD hardware to 
place
    a breakpoint over the live GDT, and trigger a #DB livelock (CVE-2015-8104).
    
    Introduce breakpoint_addr_ok() and explain why __addr_ok() happens to be an
    appropriate check in this case.
    
    For Xen 4.14 and later, this is a latent bug because the XLAT area has moved
    to be on its own with nothing interesting adjacent.  For Xen 4.13 and older 
on
    AMD hardware, this fixes a PV-trigger-able DoS.
    
    This is part of XSA-444 / CVE-2023-34328.
    
    Fixes: 65e355490817 ("x86/PV: support data breakpoint extension registers")
    Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
    Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
---
 xen/arch/x86/domain.c               |  2 +-
 xen/arch/x86/include/asm/debugreg.h | 19 +++++++++++++++++++
 xen/arch/x86/pv/misc-hypercalls.c   |  2 +-
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8d3d52034a..d05ee0da55 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1088,7 +1088,7 @@ int arch_set_info_guest(
     if ( is_pv_domain(d) )
     {
         for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
-            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
+            if ( !breakpoint_addr_ok(c(debugreg[i])) )
                 return -EINVAL;
         /*
          * Prior to Xen 4.11, dr5 was used to hold the emulated-only
diff --git a/xen/arch/x86/include/asm/debugreg.h 
b/xen/arch/x86/include/asm/debugreg.h
index 39ba312b84..b6454cc04e 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -76,6 +76,25 @@
     __val;                                                  \
 })
 
+/*
+ * Architecturally, %dr{0..3} can have any arbitrary value.  However, Xen
+ * can't allow the guest to breakpoint the Xen address range, so we limit the
+ * guest to the lower canonical half, or above the Xen range in the higher
+ * canonical half.
+ *
+ * Breakpoint lengths are specified to mask the low order address bits,
+ * meaning all breakpoints are naturally aligned.  With %dr7, the widest
+ * breakpoint is 8 bytes.  With DBEXT, the widest breakpoint is 4G.  Both of
+ * the Xen boundaries have >4G alignment.
+ *
+ * In principle we should account for HYPERVISOR_COMPAT_VIRT_START(d), but
+ * 64bit Xen has never enforced this for compat guests, and there's no problem
+ * (to Xen) if the guest breakpoints it's alias of the M2P.  Skipping this
+ * aspect simplifies the logic, and causes us not to reject a migrating guest
+ * which operated fine on prior versions of Xen.
+ */
+#define breakpoint_addr_ok(a) __addr_ok(a)
+
 struct vcpu;
 long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
 void activate_debugregs(const struct vcpu *);
diff --git a/xen/arch/x86/pv/misc-hypercalls.c 
b/xen/arch/x86/pv/misc-hypercalls.c
index 99f5028128..b529f00ea1 100644
--- a/xen/arch/x86/pv/misc-hypercalls.c
+++ b/xen/arch/x86/pv/misc-hypercalls.c
@@ -61,7 +61,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned 
long value)
     switch ( reg )
     {
     case 0 ... 3:
-        if ( !access_ok(value, sizeof(long)) )
+        if ( !breakpoint_addr_ok(value) )
             return -EPERM;
 
         v->arch.dr[reg] = value;
--
generated by git-patchbot for /home/xen/git/xen.git#master