[xen stable-4.8] x86: Refine checks in #DB handler for faulting conditions

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[xen stable-4.8] x86: Refine checks in #DB handler for faulting conditions

patchbot
commit 5fd28d27d3f7840e4c6724e67936fed57a638155
Author:     Andrew Cooper <[hidden email]>
AuthorDate: Thu Jun 28 11:22:30 2018 +0200
Commit:     Jan Beulich <[hidden email]>
CommitDate: Thu Jun 28 11:22:30 2018 +0200

    x86: Refine checks in #DB handler for faulting conditions
   
    One of the fix for XSA-260 (c/s 75d6828bc2 "x86/traps: Fix handling of #DB
    exceptions in hypervisor context") added some safety checks to help avoid
    livelocks of #DB faults.
   
    While a General Detect #DB exception does have fault semantics, hardware
    clears %dr7.gd on entry to the handler, meaning that it is actually safe to
    return to.  Furthermore, %dr6.gd is guest controlled and sticky (never cleared
    by hardware).  A malicious PV guest can therefore trigger the fatal_trap() and
    crash Xen.
   
    Instruction breakpoints are more tricky.  The breakpoint match bits in %dr6
    are not sticky, but the Intel manual warns that they may be set for
    non-enabled breakpoints, so add a breakpoint enabled check.
   
    Beyond that, because of the restriction on the linear addresses PV guests can
    set, and the fault (rather than trap) nature of instruction breakpoints
    (i.e. can't be deferred by a MovSS shadow), there should be no way to
    encounter an instruction breakpoint in Xen context.  However, for extra
    robustness, deal with this situation by clearing the breakpoint configuration,
    rather than crashing.
   
    This is XSA-265
   
    Signed-off-by: Andrew Cooper <[hidden email]>
    Reviewed-by: Jan Beulich <[hidden email]>
    master commit: 17bf51297220dcd74da29de99320b6b1c72d1fa5
    master date: 2018-06-28 09:04:20 +0200
---
 xen/arch/x86/traps.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e7f197d336..00f944070c 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -4072,6 +4072,13 @@ void do_debug(struct cpu_user_regs *regs)
 
     if ( !guest_mode(regs) )
     {
+        /*
+         * !!! WARNING !!!
+         *
+         * %dr6 is mostly guest controlled at this point.  Any decsions base
+         * on its value must be crosschecked with non-guest controlled state.
+         */
+
         if ( regs->eflags & X86_EFLAGS_TF )
         {
             /* In SYSENTER entry path we can't zap TF until EFLAGS is saved. */
@@ -4093,33 +4100,44 @@ void do_debug(struct cpu_user_regs *regs)
          * Check for fault conditions.  General Detect, and instruction
          * breakpoints are faults rather than traps, at which point attempting
          * to ignore and continue will result in a livelock.
+         *
+         * However, on entering the #DB handler, hardware clears %dr7.gd for
+         * us (as confirmed by the earlier %dr6 accesses succeeding), meaning
+         * that a real General Detect exception is restartable.
+         *
+         * PV guests are not permitted to point %dr{0..3} at Xen linear
+         * addresses, and Instruction Breakpoints (being faults) don't get
+         * delayed by a MovSS shadow, so we should never encounter one in
+         * hypervisor context.
+         *
+         * If however we do, safety measures need to be enacted.  Use a big
+         * hammer and clear all debug settings.
          */
-        if ( dr6 & DR_GENERAL_DETECT )
-        {
-            printk(XENLOG_ERR "Hit General Detect in Xen context\n");
-            fatal_trap(regs, 0);
-        }
-
         if ( dr6 & (DR_TRAP3 | DR_TRAP2 | DR_TRAP1 | DR_TRAP0) )
         {
-            unsigned int bp, dr7 = read_debugreg(7) >> DR_CONTROL_SHIFT;
+            unsigned int bp, dr7 = read_debugreg(7);
 
             for ( bp = 0; bp < 4; ++bp )
             {
                 if ( (dr6 & (1u << bp)) && /* Breakpoint triggered? */
-                     ((dr7 & (3u << (bp * DR_CONTROL_SIZE))) == 0) /* Insn? */ )
+                     (dr7 & (3u << (bp * DR_ENABLE_SIZE))) && /* Enabled? */
+                     ((dr7 & (3u << ((bp * DR_CONTROL_SIZE) + /* Insn? */
+                                     DR_CONTROL_SHIFT))) == DR_RW_EXECUTE) )
                 {
+                    ASSERT_UNREACHABLE();
+
                     printk(XENLOG_ERR
                            "Hit instruction breakpoint in Xen context\n");
-                    fatal_trap(regs, 0);
+                    write_debugreg(7, 0);
+                    break;
                 }
             }
         }
 
         /*
-         * Whatever caused this #DB should be a trap.  Note it and continue.
-         * Guests can trigger this in certain corner cases, so ensure the
-         * message is ratelimited.
+         * Whatever caused this #DB should be restartable by this point.  Note
+         * it and continue.  Guests can trigger this in certain corner cases,
+         * so ensure the message is ratelimited.
          */
         gprintk(XENLOG_WARNING,
                 "Hit #DB in Xen context: %04x:%p [%ps], stk %04x:%p, dr6 %lx\n",
--
generated by git-patchbot for /home/xen/git/xen.git#stable-4.8

_______________________________________________
Xen-changelog mailing list
[hidden email]
https://lists.xenproject.org/xen-changelog