[PATCH v2 0/9] xen: arm: reenable support for 32-bit userspace running in 64-bit guest.

[PATCH v2 0/9] xen: arm: reenable support for 32-bit userspace running in 64-bit guest.

[Ian Campbell]

XSA-102/CVE-2014-5147[0] concerned a crash when trapping from 32-bit
userspace in a 64-bit guest. Part of that security patch was c0020e09970
"xen: arm: Handle traps from 32-bit userspace on 64-bit kernel as undef
fix" which turned the exploitable crash into a #undef to the guest (so
as to kill the process but not the host) as a workaround for the issue.

However while this prevented the exploit it did not make 32-bit
userspaces which were prone to triggering the issue actually work.

This series consists of some patches which I originally wrote for
XSA-102 to fix the issue properly before it was determined that those
fixes were too invasive by far for a security update. At the end of the
series is a new patch which removes the XSA-102 workaround since all
problematic traps should now be handled.

Since these were originally intended to be the security fix they have
had a fair bit of scrutiny already in private . However since there is
now a risk of reintroducing XSA-102 I would appreciate a pretty thorough
second pair of eyes on it this time around.

I've tested this with a local utility which tries to access the various
cp and system registers from both 32- and 64-bit processes and checks
that they either work or give the expected traps. Since this tool is
effectively an exploit for XSA-102 I'm not sharing here but if you ask
nicely and appear to be wearing the correct colour hat I might share it
with you (it's not terribly impressive, so don't get too excited).

Since last time I've redone the v/ptimer emulation to be correct instead
of removing it. Actually removing depends on the "xen: arm: context
switch vtimer PPI state." patch, which is going to to take a bit longer.
I also implemented Julien's review feedback.

Ian.

[0] http://xenbits.xen.org/xsa/advisory-102.html

[PATCH v2 1/9] xen: arm: Correct PMXEV cp register definitions

[Ian Campbell]

p15,0,c9,c13,1 is PMXEVTYPER not PMXEVCNTR.
p15,0,c9,c13,2 is PMXEVCNTR not PMXEVCNR.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/traps.c         |    2 +-
 xen/include/asm-arm/cpregs.h |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ad046e8..50d67aa 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1633,8 +1633,8 @@ static void do_cp15_32(struct cpu_user_regs *regs,
     case HSR_CPREG32(PMCEID0):
     case HSR_CPREG32(PMCEID1):
     case HSR_CPREG32(PMCCNTR):
+    case HSR_CPREG32(PMXEVTYPER):
     case HSR_CPREG32(PMXEVCNTR):
-    case HSR_CPREG32(PMXEVCNR):
     case HSR_CPREG32(PMUSERENR):
     case HSR_CPREG32(PMINTENSET):
     case HSR_CPREG32(PMINTENCLR):
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index f1100c8..afe9148 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -222,8 +222,8 @@
 #define PMCEID0         p15,0,c9,c12,6  /* Perf. Mon. Common Event Identification register 0 */
 #define PMCEID1         p15,0,c9,c12,7  /* Perf. Mon. Common Event Identification register 1 */
 #define PMCCNTR         p15,0,c9,c13,0  /* Perf. Mon. Cycle Count Register */
-#define PMXEVCNTR       p15,0,c9,c13,1  /* Perf. Mon. Event Type Select Register */
-#define PMXEVCNR        p15,0,c9,c13,2  /* Perf. Mon. Event Count Register */
+#define PMXEVTYPER      p15,0,c9,c13,1  /* Perf. Mon. Event Type Select Register */
+#define PMXEVCNTR       p15,0,c9,c13,2  /* Perf. Mon. Event Count Register */
 #define PMUSERENR       p15,0,c9,c14,0  /* Perf. Mon. User Enable Register */
 #define PMINTENSET      p15,0,c9,c14,1  /* Perf. Mon. Interrupt Enable Set Register */
 #define PMINTENCLR      p15,0,c9,c14,2  /* Perf. Mon. Interrupt Enable Clear Register */
-- 
1.7.10.4

[PATCH v2 2/9] xen: arm: Factor out psr_mode_is_user

[Ian Campbell]

This embodies the logic on arm64 that userspace can be either 32-bit
or 64-bit. It will be used in other places shortly.

Note that the logic differs slightly because the original (in
inject_abt64_exception) knew that the kernel was 64-bit and could
therefore assume that any 32-bit mode was userspace. Instead the
refactored code explicitly checks for usr mode.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/traps.c       |    9 +--------
 xen/include/asm-arm/regs.h |    8 ++++++++
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 50d67aa..c5f65db 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -455,14 +455,7 @@ static void inject_abt64_exception(struct cpu_user_regs *regs,
         .len = instr_len,
     };
 
-    /*
-     * Trap may have been taken from EL0, which might be in AArch32
-     * mode (PSR_MODE_BIT set), or in AArch64 mode (PSR_MODE_EL0t).
-     *
-     * Since we know the kernel must be 64-bit any trap from a 32-bit
-     * mode must have been from EL0.
-     */
-    if ( psr_mode_is_32bit(regs->cpsr) || psr_mode(regs->cpsr,PSR_MODE_EL0t) )
+    if ( psr_mode_is_user(regs) )
         esr.ec = prefetch
             ? HSR_EC_INSTR_ABORT_LOWER_EL : HSR_EC_DATA_ABORT_LOWER_EL;
     else
diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
index 0951857..56d53d6 100644
--- a/xen/include/asm-arm/regs.h
+++ b/xen/include/asm-arm/regs.h
@@ -24,9 +24,17 @@
 
 #ifdef CONFIG_ARM_32
 #define hyp_mode(r)     psr_mode((r)->cpsr,PSR_MODE_HYP)
+#define psr_mode_is_user(r) usr_mode(r)
 #else
 #define hyp_mode(r)     (psr_mode((r)->cpsr,PSR_MODE_EL2h) || \
                          psr_mode((r)->cpsr,PSR_MODE_EL2t))
+
+/*
+ * Trap may have been taken from EL0, which might be in AArch32 usr
+ * mode, or in AArch64 mode (PSR_MODE_EL0t).
+ */
+#define psr_mode_is_user(r) \
+    (psr_mode((r)->cpsr,PSR_MODE_EL0t) || usr_mode(r))
 #endif
 
 #define guest_mode(r)                                                         \
-- 
1.7.10.4

[PATCH v2 3/9] xen: arm: Handle 32-bit EL0 on 64-bit EL1 when advancing PC after trap

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/traps.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index c5f65db..be65862 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1487,7 +1487,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)
     {
         unsigned long it;
 
-        BUG_ON( !is_32bit_domain(current->domain) || !(cpsr&PSR_THUMB) );
+        BUG_ON( !psr_mode_is_32bit(regs->cpsr) || !(cpsr&PSR_THUMB) );
 
         it = ( (cpsr >> (10-2)) & 0xfc) | ((cpsr >> 25) & 0x3 );
 
@@ -1496,7 +1496,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)
             return 1;
 
         /* The cond for this instruction works out as the top 4 bits. */
-        cond =  ( it >> 4 );
+        cond = ( it >> 4 );
     }
 
     cpsr_cond = cpsr >> 28;
@@ -1514,10 +1514,10 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
     unsigned long itbits, cond, cpsr = regs->cpsr;
 
     /* PSR_IT_MASK bits can only be set for 32-bit processors in Thumb mode. */
-    BUG_ON( (!is_32bit_domain(current->domain)||!(cpsr&PSR_THUMB))
+    BUG_ON( (!psr_mode_is_32bit(cpsr)||!(cpsr&PSR_THUMB))
             && (cpsr&PSR_IT_MASK) );
 
-    if ( is_32bit_domain(current->domain) && (cpsr&PSR_IT_MASK) )
+    if ( cpsr&PSR_IT_MASK )
     {
         /* The ITSTATE[7:0] block is contained in CPSR[15:10],CPSR[26:25]
          *
-- 
1.7.10.4

[PATCH v2 4/9] xen: arm: correctly handle vtimer traps from userspace

[Ian Campbell]

Previously 32-bit userspace on 32-bit kernel and 64-bit userspace on 64-bit
kernel could access these registers irrespective of whether the kernel had
configured them to be allowed to. To fix this:

 - Userspace access to CNTP_CTL_EL0 and CNTP_TVAL_EL0 should be gated on
   CNTKCTL_EL1.EL0PTEN.
 - Userspace access to CNTPCT_EL0 should be gated on CNTKCTL_EL1.EL0PCTEN.

When we do not handle an access we now silently inject an undef even in debug
mode since the debugging messages are not helpful (we have handled the access,
by explicitly choosing not to).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Replaces "xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef"
---
 xen/arch/arm/traps.c            |   28 +++++++++-------------------
 xen/arch/arm/vtimer.c           |   38 +++++++++++++++++++++++---------------
 xen/include/asm-arm/processor.h |    5 +++++
 3 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index be65862..354490d 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1598,11 +1598,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
     case HSR_CPREG32(CNTP_CTL):
     case HSR_CPREG32(CNTP_TVAL):
         if ( !vtimer_emulate(regs, hsr) )
-        {
-            dprintk(XENLOG_ERR,
-                    "failed emulation of 32-bit vtimer CP register access\n");
-            domain_crash_synchronous();
-        }
+            goto undef_cp15_32;
         break;
     case HSR_CPREG32(ACTLR):
         if ( cp32.read )
@@ -1645,6 +1641,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#x\n",
                  hsr.bits & HSR_CP32_REGS_MASK);
 #endif
+ undef_cp15_32:
         inject_undef_exception(regs, hsr.len);
         return;
     }
@@ -1664,11 +1661,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,
     {
     case HSR_CPREG64(CNTPCT):
         if ( !vtimer_emulate(regs, hsr) )
-        {
-            dprintk(XENLOG_ERR,
-                    "failed emulation of 64-bit vtimer CP register access\n");
-            domain_crash_synchronous();
-        }
+            goto undef_cp15_64;
         break;
     default:
         {
@@ -1682,6 +1675,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,
             gdprintk(XENLOG_ERR, "unhandled 64-bit CP15 access %#x\n",
                      hsr.bits & HSR_CP64_REGS_MASK);
 #endif
+ undef_cp15_64:
             inject_undef_exception(regs, hsr.len);
             return;
         }
@@ -1849,11 +1843,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
     case HSR_SYSREG_CNTP_CTL_EL0:
     case HSR_SYSREG_CNTP_TVAL_EL0:
         if ( !vtimer_emulate(regs, hsr) )
-        {
-            dprintk(XENLOG_ERR,
-                    "failed emulation of 64-bit vtimer sysreg access\n");
-            domain_crash_synchronous();
-        }
+            goto undef_sysreg;
         break;
     case HSR_SYSREG_ICC_SGI1R_EL1:
         if ( !vgic_emulate(regs, hsr) )
@@ -1872,8 +1862,8 @@ static void do_sysreg(struct cpu_user_regs *regs,
     default:
  bad_sysreg:
         {
-            struct hsr_sysreg sysreg = hsr.sysreg;
 #ifndef NDEBUG
+            struct hsr_sysreg sysreg = hsr.sysreg;
 
             gdprintk(XENLOG_ERR,
                      "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
@@ -1886,7 +1876,8 @@ static void do_sysreg(struct cpu_user_regs *regs,
             gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access %#x\n",
                      hsr.bits & HSR_SYSREG_REGS_MASK);
 #endif
-            inject_undef_exception(regs, sysreg.len);
+ undef_sysreg:
+            inject_undef_exception(regs, hsr.len);
             return;
         }
     }
@@ -2062,8 +2053,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_cp15_32(regs, hsr);
         break;
     case HSR_EC_CP15_64:
-        if ( !is_32bit_domain(current->domain) )
-            goto bad_trap;
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp15_64);
         do_cp15_64(regs, hsr);
         break;
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 848e2c6..a914a0a 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -129,9 +129,14 @@ int virt_timer_restore(struct vcpu *v)
     return 0;
 }
 
-static void vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, int read)
+static int vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, int read)
 {
     struct vcpu *v = current;
+
+    if ( psr_mode_is_user(regs) &&
+         !(READ_SYSREG(CNTKCTL_EL1) & CNTKCTL_EL1_EL0PTEN) )
+        return 0;
+
     if ( read )
     {
         *r = v->arch.phys_timer.ctl;
@@ -151,13 +156,18 @@ static void vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t *r, int read)
         else
             stop_timer(&v->arch.phys_timer.timer);
     }
+    return 1;
 }
 
-static void vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r, int read)
+static int vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r, int read)
 {
     struct vcpu *v = current;
     s_time_t now;
 
+    if ( psr_mode_is_user(regs) &&
+         !(READ_SYSREG(CNTKCTL_EL1) & CNTKCTL_EL1_EL0PTEN) )
+        return 0;
+
     now = NOW() - v->domain->arch.phys_timer_base.offset;
 
     if ( read )
@@ -175,6 +185,7 @@ static void vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r, int read)
                       v->domain->arch.phys_timer_base.offset);
         }
     }
+    return 1;
 }
 
 static int vtimer_cntpct(struct cpu_user_regs *regs, uint64_t *r, int read)
@@ -185,6 +196,9 @@ static int vtimer_cntpct(struct cpu_user_regs *regs, uint64_t *r, int read)
 
     if ( read )
     {
+        if ( psr_mode_is_user(regs) &&
+             !(READ_SYSREG(CNTKCTL_EL1) & CNTKCTL_EL1_EL0PCTEN) )
+            return 0;
         now = NOW() - v->domain->arch.phys_timer_base.offset;
         ticks = ns_to_ticks(now);
         *r = ticks;
@@ -211,12 +225,10 @@ static int vtimer_emulate_cp32(struct cpu_user_regs *regs, union hsr hsr)
     switch ( hsr.bits & HSR_CP32_REGS_MASK )
     {
     case HSR_CPREG32(CNTP_CTL):
-        vtimer_cntp_ctl(regs, r, cp32.read);
-        return 1;
+        return vtimer_cntp_ctl(regs, r, cp32.read);
 
     case HSR_CPREG32(CNTP_TVAL):
-        vtimer_cntp_tval(regs, r, cp32.read);
-        return 1;
+        return vtimer_cntp_tval(regs, r, cp32.read);
 
     default:
         return 0;
@@ -238,7 +250,7 @@ static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
     switch ( hsr.bits & HSR_CP64_REGS_MASK )
     {
     case HSR_CPREG64(CNTPCT):
-        if (!vtimer_cntpct(regs, &x, cp64.read))
+        if ( !vtimer_cntpct(regs, &x, cp64.read) )
             return 0;
 
         if ( cp64.read )
@@ -268,12 +280,14 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
     case HSR_SYSREG_CNTP_CTL_EL0:
-        vtimer_cntp_ctl(regs, &r, sysreg.read);
+        if ( !vtimer_cntp_ctl(regs, &r, sysreg.read) )
+            return 0;
         if ( sysreg.read )
             *x = r;
         return 1;
     case HSR_SYSREG_CNTP_TVAL_EL0:
-        vtimer_cntp_tval(regs, &r, sysreg.read);
+        if ( !vtimer_cntp_tval(regs, &r, sysreg.read) )
+            return 0;
         if ( sysreg.read )
             *x = r;
         return 1;
@@ -293,17 +307,11 @@ int vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr)
 
     switch (hsr.ec) {
     case HSR_EC_CP15_32:
-        if ( !is_32bit_domain(current->domain) )
-            return 0;
         return vtimer_emulate_cp32(regs, hsr);
     case HSR_EC_CP15_64:
-        if ( !is_32bit_domain(current->domain) )
-            return 0;
         return vtimer_emulate_cp64(regs, hsr);
 #ifdef CONFIG_ARM_64
     case HSR_EC_SYSREG:
-        if ( is_32bit_domain(current->domain) )
-            return 0;
         return vtimer_emulate_sysreg(regs, hsr);
 #endif
     default:
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index fcd26fb..ebbe804 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -563,6 +563,11 @@ union hsr {
 #define CNTx_CTL_MASK     (1u<<1)  /* Mask IRQ */
 #define CNTx_CTL_PENDING  (1u<<2)  /* IRQ pending */
 
+#define CNTKCTL_EL1_EL0PCTEN (1u<<0) /* Expose phys counters to EL0 */
+#define CNTKCTL_EL1_EL0VCTEN (1u<<1) /* Expose virt counters to EL0 */
+#define CNTKCTL_EL1_EL0VTEN  (1u<<8) /* Expose virt timer registers to EL0 */
+#define CNTKCTL_EL1_EL0PTEN  (1u<<9) /* Expose phys timer registers to EL0 */
+
 /* Exception Vector offsets */
 /* ... ARM32 */
 #define VECTOR32_RST  0
-- 
1.7.10.4

[PATCH v2 5/9] xen: arm: Handle CP15 register traps from userspace

[Ian Campbell]

Previously userspace access to PM* would have been incorrectly (but benignly)
implemented as RAZ/WI when running on a 32-bit kernel and would cause a
hypervisor exception (host crash) when running a 64-bit kernel (this was
already solved via the fix to XSA-102).

CLIDR, CCSIDR, DCCISW, ACTLR, PMINTENSET, PMINTENCLR are EL1 only, attempts to
access from EL0 will trap to EL1 not to us, hence BUG_ON is appropriate now.

PMUSERENR is R/O at EL0 and we implement as RAZ/WI at EL1 as before.

The remaining PM* registers are accessible to EL0 only if PMUSERENR_EL0.EN is
set, since we emulate this as RAZ/WI the bit is never set so we inject a trap
on attempted access. We weren't previously handling PMCCNTR.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c |   32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 354490d..6045396 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1565,6 +1565,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
     switch ( hsr.bits & HSR_CP32_REGS_MASK )
     {
     case HSR_CPREG32(CLIDR):
+        BUG_ON(psr_mode_is_user(regs));
         if ( !cp32.read )
         {
             dprintk(XENLOG_ERR,
@@ -1574,6 +1575,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         *r = READ_SYSREG32(CLIDR_EL1);
         break;
     case HSR_CPREG32(CCSIDR):
+        BUG_ON(psr_mode_is_user(regs));
         if ( !cp32.read )
         {
             dprintk(XENLOG_ERR,
@@ -1583,6 +1585,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         *r = READ_SYSREG32(CCSIDR_EL1);
         break;
     case HSR_CPREG32(DCCISW):
+        BUG_ON(psr_mode_is_user(regs));
         if ( cp32.read )
         {
             dprintk(XENLOG_ERR,
@@ -1601,6 +1604,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
             goto undef_cp15_32;
         break;
     case HSR_CPREG32(ACTLR):
+        BUG_ON(psr_mode_is_user(regs));
         if ( cp32.read )
            *r = v->arch.actlr;
         break;
@@ -1613,6 +1617,18 @@ static void do_cp15_32(struct cpu_user_regs *regs,
      * always support PMCCNTR (the cyle counter): we just RAZ/WI for all
      * PM register, which doesn't crash the kernel at least
      */
+    case HSR_CPREG32(PMUSERENR):
+        /* RO at EL0. RAZ/WI at EL1 */
+        if ( psr_mode_is_user(regs) && !hsr.cp32.read )
+            goto undef_cp15_32;
+        goto cp15_32_raz_wi;
+
+    case HSR_CPREG32(PMINTENSET):
+    case HSR_CPREG32(PMINTENCLR):
+        /* EL1 only */
+        BUG_ON(psr_mode_is_user(regs));
+        goto cp15_32_raz_wi;
+
     case HSR_CPREG32(PMCR):
     case HSR_CPREG32(PMCNTENSET):
     case HSR_CPREG32(PMCNTENCLR):
@@ -1624,12 +1640,19 @@ static void do_cp15_32(struct cpu_user_regs *regs,
     case HSR_CPREG32(PMCCNTR):
     case HSR_CPREG32(PMXEVTYPER):
     case HSR_CPREG32(PMXEVCNTR):
-    case HSR_CPREG32(PMUSERENR):
-    case HSR_CPREG32(PMINTENSET):
-    case HSR_CPREG32(PMINTENCLR):
     case HSR_CPREG32(PMOVSSET):
+        /*
+         * Accessible at EL0 only if PMUSERENR_EL0.EN is set. We
+         * emulate that register as 0 above.
+         */
+        if ( psr_mode_is_user(regs) )
+            goto undef_cp15_32;
+        /* Fall thru */
+
+ cp15_32_raz_wi:
         if ( cp32.read )
             *r = 0;
+        /* else: write ignored */
         break;
 
     default:
@@ -2047,8 +2070,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         advance_pc(regs, hsr);
         break;
     case HSR_EC_CP15_32:
-        if ( !is_32bit_domain(current->domain) )
-            goto bad_trap;
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp15_32);
         do_cp15_32(regs, hsr);
         break;
-- 
1.7.10.4

[PATCH v2 6/9] xen: arm: Handle CP14 32-bit register accesses from userspace

[Ian Campbell]

Accesses to these from 32-bit userspace would cause a hypervisor exception
(host crash) when running a 64-bit kernel, which is worked around by the fix to
XSA-102. On 32-bit kernels they would be implemented as RAZ/WI which is
incorrect but harmless.

Update as follows:
 - DBGDSCRINT should be R/O.
 - DBGDSCREXT should be EL1 only.
 - DBGOSLAR is RO and EL1 only.
 - DBGVCR, DBGB[VC]R*, DBGW[VC]R*, and DBGOSDLR are EL1 only.

DBGDIDR and DBGDSCRINT are accessible from EL0 if DBGDSCRext.UDCCdis. Since we
emulate that as RAZ/WI we allow access.

When we do not allow an access we now silently inject an undef even in debug
mode since the debugging messages are not helpful (we have handled the access,
by explicitly choosing not to).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c |   34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6045396..18f8332 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1721,10 +1721,12 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
     switch ( hsr.bits & HSR_CP32_REGS_MASK )
     {
     case HSR_CPREG32(DBGDIDR):
-
-        /* Read-only register */
+        /*
+         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
+         * is set to 0, which we emulated below.
+         */
         if ( !cp32.read )
-            goto bad_cp;
+            goto undef_cp14_32;
 
         /* Implement the minimum requirements:
          *  - Number of watchpoints: 1
@@ -1737,15 +1739,24 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
         break;
 
     case HSR_CPREG32(DBGDSCRINT):
+        if ( !cp32.read )
+            goto undef_cp14_32;
+
+        *r = 0;
+        break;
+
     case HSR_CPREG32(DBGDSCREXT):
+        if ( usr_mode(regs) )
+            goto undef_cp14_32;
+
         /* Implement debug status and control register as RAZ/WI.
          * The OS won't use Hardware debug if MDBGen not set
          */
         if ( cp32.read )
            *r = 0;
         break;
+
     case HSR_CPREG32(DBGVCR):
-    case HSR_CPREG32(DBGOSLAR):
     case HSR_CPREG32(DBGBVR0):
     case HSR_CPREG32(DBGBCR0):
     case HSR_CPREG32(DBGWVR0):
@@ -1753,13 +1764,22 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
     case HSR_CPREG32(DBGBVR1):
     case HSR_CPREG32(DBGBCR1):
     case HSR_CPREG32(DBGOSDLR):
+        if ( usr_mode(regs) )
+            goto undef_cp14_32;
         /* RAZ/WI */
         if ( cp32.read )
             *r = 0;
         break;
 
+    case HSR_CPREG32(DBGOSLAR):
+        if ( usr_mode(regs) )
+            goto undef_cp14_32;
+        /* WO */
+        if ( cp32.read )
+            goto undef_cp14_32;
+        /* else: ignore */
+        break;
     default:
-bad_cp:
 #ifndef NDEBUG
         gdprintk(XENLOG_ERR,
                  "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1768,6 +1788,7 @@ bad_cp:
         gdprintk(XENLOG_ERR, "unhandled 32-bit cp14 access %#x\n",
                  hsr.bits & HSR_CP32_REGS_MASK);
 #endif
+undef_cp14_32:
         inject_undef_exception(regs, hsr.len);
         return;
     }
@@ -2080,8 +2101,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_cp15_64(regs, hsr);
         break;
     case HSR_EC_CP14_32:
-        if ( !is_32bit_domain(current->domain) )
-            goto bad_trap;
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp14_32);
         do_cp14_32(regs, hsr);
         break;
-- 
1.7.10.4

[PATCH v2 7/9] xen: arm: correctly handle sysreg accesses from userspace

[Ian Campbell]

Previously we implemented all registers as RAZ/WI even if they
shouldn't be accessible to userspace.

Accesses to the *_EL1 registers from EL0 are trapped to EL1 by the
hardware, so add a BUG_ON. Likewise accesses from 32-bit EL1 cannot
happen.

PMUSERENR_EL0 and MDCCSR_EL0 are R/O to EL0.

Other PM*_EL0 registers are accessible at EL0 only if
PMUSERENR_EL0.EN is set, since we emulate that as RAZ/WI we know that
bit cannot be set.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c          |   54 +++++++++++++++++++++++++++++++----------
 xen/include/asm-arm/sysregs.h |    1 +
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 18f8332..2c2e6f0 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1847,11 +1847,40 @@ static void do_sysreg(struct cpu_user_regs *regs,
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
     /* RAZ/WI registers: */
+
     /*  - Debug */
     case HSR_SYSREG_MDSCR_EL1:
     /*  - Perf monitors */
     case HSR_SYSREG_PMINTENSET_EL1:
     case HSR_SYSREG_PMINTENCLR_EL1:
+    /* - Breakpoints */
+    HSR_SYSREG_DBG_CASES(DBGBVR):
+    HSR_SYSREG_DBG_CASES(DBGBCR):
+    /* - Watchpoints */
+    HSR_SYSREG_DBG_CASES(DBGWVR):
+    HSR_SYSREG_DBG_CASES(DBGWCR):
+    /* - Double Lock Register */
+    case HSR_SYSREG_OSDLR_EL1:
+        /* EL1 only */
+        BUG_ON(psr_mode_is_user(regs));
+        goto sysreg_raz_wi;
+
+    case HSR_SYSREG_PMUSERENR_EL0:
+        /* RO at EL0. RAZ/WI at EL1 */
+        if ( psr_mode_is_user(regs) && !hsr.sysreg.read )
+            goto undef_sysreg;
+        goto sysreg_raz_wi;
+
+    case HSR_SYSREG_MDCCSR_EL0:
+        /*
+         * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that
+         * register as RAZ/WI above. So RO at both EL0 and EL1.
+         */
+        if ( !hsr.sysreg.read )
+            goto undef_sysreg;
+
+        *x = 0;
+        break;
     case HSR_SYSREG_PMCR_EL0:
     case HSR_SYSREG_PMCNTENSET_EL0:
     case HSR_SYSREG_PMCNTENCLR_EL0:
@@ -1863,16 +1892,16 @@ static void do_sysreg(struct cpu_user_regs *regs,
     case HSR_SYSREG_PMCCNTR_EL0:
     case HSR_SYSREG_PMXEVTYPER_EL0:
     case HSR_SYSREG_PMXEVCNTR_EL0:
-    case HSR_SYSREG_PMUSERENR_EL0:
     case HSR_SYSREG_PMOVSSET_EL0:
-    /* - Breakpoints */
-    HSR_SYSREG_DBG_CASES(DBGBVR):
-    HSR_SYSREG_DBG_CASES(DBGBCR):
-    /* - Watchpoints */
-    HSR_SYSREG_DBG_CASES(DBGWVR):
-    HSR_SYSREG_DBG_CASES(DBGWCR):
-    /* - Double Lock Register */
-    case HSR_SYSREG_OSDLR_EL1:
+        /*
+         * Accessible at EL0 only if PMUSERENR_EL0.EN is set. We
+         * emulate that register as 0 above.
+         */
+        if ( psr_mode_is_user(regs) )
+            goto undef_sysreg;
+        /* Fall thru */
+
+ sysreg_raz_wi:
         if ( hsr.sysreg.read )
             *x = 0;
         /* else: write ignored */
@@ -1880,8 +1909,9 @@ static void do_sysreg(struct cpu_user_regs *regs,
 
     /* Write only, Write ignore registers: */
     case HSR_SYSREG_OSLAR_EL1:
+        BUG_ON(psr_mode_is_user(regs));
         if ( hsr.sysreg.read )
-            goto bad_sysreg;
+            goto undef_sysreg;
         /* else: write ignored */
         break;
     case HSR_SYSREG_CNTP_CTL_EL0:
@@ -1904,7 +1934,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
                 "Emulation of sysreg ICC_SGI0R_EL1/ASGI1R_EL1 not supported\n");
         inject_undef64_exception(regs, hsr.len);
     default:
- bad_sysreg:
         {
 #ifndef NDEBUG
             struct hsr_sysreg sysreg = hsr.sysreg;
@@ -2147,8 +2176,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         inject_undef64_exception(regs, hsr.len);
         break;
     case HSR_EC_SYSREG:
-        if ( is_32bit_domain(current->domain) )
-            goto bad_trap;
+        BUG_ON(psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_sysreg);
         do_sysreg(regs, hsr);
         break;
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index 169b7ac..d9d8dd5 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -43,6 +43,7 @@
 #define HSR_SYSREG_MDSCR_EL1      HSR_SYSREG(2,0,c0,c2,2)
 #define HSR_SYSREG_OSLAR_EL1      HSR_SYSREG(2,0,c1,c0,4)
 #define HSR_SYSREG_OSDLR_EL1      HSR_SYSREG(2,0,c1,c3,4)
+#define HSR_SYSREG_MDCCSR_EL0     HSR_SYSREG(2,3,c0,c1,0)
 
 #define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
 #define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
-- 
1.7.10.4

[PATCH v2 8/9] xen: arm: handle remaining traps from userspace

[Ian Campbell]

CP14 dbg and general CP register access are both handled with
unconditional injection of #undef from their respective handlers, so
allow these even from 32-bit userspace on a 64-bit kernel.

SMC32 and HVC32 should only come from a guest in AArch32 mode and
SMC64 and HVC64 should only come from a guest in AArch64 mode. Add
appropriate BUG_ONs to all cases.

After this bad_trap is no longer used.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2c2e6f0..b6bed6d 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2135,23 +2135,22 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_cp14_32(regs, hsr);
         break;
     case HSR_EC_CP14_DBG:
-        if ( !is_32bit_domain(current->domain) )
-            goto bad_trap;
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp14_dbg);
         do_cp14_dbg(regs, hsr);
         break;
     case HSR_EC_CP:
-        if ( !is_32bit_domain(current->domain) )
-            goto bad_trap;
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp);
         do_cp(regs, hsr);
         break;
     case HSR_EC_SMC32:
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc32);
-        inject_undef32_exception(regs);
+        inject_undef_exception(regs, hsr.len);
         break;
     case HSR_EC_HVC32:
-        perfc_incr(trap_hvc32);
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
             return do_debug_trap(regs, hsr.iss & 0x00ff);
@@ -2162,6 +2161,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         break;
 #ifdef CONFIG_ARM_64
     case HSR_EC_HVC64:
+        BUG_ON(psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_hvc64);
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
@@ -2172,8 +2172,8 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_trap_hypercall(regs, &regs->x16, hsr.iss);
         break;
     case HSR_EC_SMC64:
+        BUG_ON(psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc64);
-        inject_undef64_exception(regs, hsr.len);
         break;
     case HSR_EC_SYSREG:
         BUG_ON(psr_mode_is_32bit(regs->cpsr));
@@ -2198,7 +2198,6 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 #endif
 
     default:
- bad_trap:
         printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
                hsr.bits, hsr.ec, hsr.len, hsr.iss);
         do_unexpected_trap("Hypervisor", regs);
-- 
1.7.10.4

[PATCH v2 9/9] xen: arm: Allow traps from 32 bit userspace on 64 bit hypervisors again

[Ian Campbell]

This removes the unconditional #undef injected in response to such traps which
was added by the fixes to CVE-2014-5147 / XSA-102 in c0020e099702 "xen: arm:
Handle traps from 32-bit userspace on 64-bit kernel as undef", we now handle
such traps correctly.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c |   12 ------------
 1 file changed, 12 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index b6bed6d..474747e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2089,18 +2089,6 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 
     enter_hypervisor_head(regs);
 
-    /*
-     * We currently do not handle 32-bit userspace on 64-bit kernels
-     * correctly (See XSA-102). Until that is resolved we treat any
-     * trap from 32-bit userspace on 64-bit kernel as undefined.
-     */
-    if ( !hyp_mode(regs) && is_64bit_domain(current->domain) &&
-         psr_mode_is_32bit(regs->cpsr) )
-    {
-        inject_undef_exception(regs, hsr.len);
-        return;
-    }
-
     switch (hsr.ec) {
     case HSR_EC_WFI_WFE:
         if ( !check_conditional_instr(regs, hsr) )
-- 
1.7.10.4

Re: [PATCH v2 3/9] xen: arm: Handle 32-bit EL0 on 64-bit EL1 when advancing PC after trap

[Julien Grall]

Hi Ian,

On 10/02/2015 12:45, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
> ---
>   xen/arch/arm/traps.c |    8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index c5f65db..be65862 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1487,7 +1487,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)
>       {
>           unsigned long it;
>
> -        BUG_ON( !is_32bit_domain(current->domain) || !(cpsr&PSR_THUMB) );
> +        BUG_ON( !psr_mode_is_32bit(regs->cpsr) || !(cpsr&PSR_THUMB) );
>
>           it = ( (cpsr >> (10-2)) & 0xfc) | ((cpsr >> 25) & 0x3 );
>
> @@ -1496,7 +1496,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)
>               return 1;
>
>           /* The cond for this instruction works out as the top 4 bits. */
> -        cond =  ( it >> 4 );
> +        cond = ( it >> 4 );

I haven't spot it in the previous review. This seems to be only a coding 
change. Could you specify it in the commit message?

Regards,

-- 
Julien Grall

Re: [PATCH v2 3/9] xen: arm: Handle 32-bit EL0 on 64-bit EL1 when advancing PC after trap

[Ian Campbell]

On Tue, 2015-02-10 at 13:44 +0800, Julien Grall wrote:
> >           /* The cond for this instruction works out as the top 4 bits. */
> > -        cond =  ( it >> 4 );
> > +        cond = ( it >> 4 );
> 
> I haven't spot it in the previous review. This seems to be only a coding 
> change. Could you specify it in the commit message?

Sigh, this level of pedantry isn't really helpful IMHO.

Ian.

Re: [PATCH v2 4/9] xen: arm: correctly handle vtimer traps from userspace

[Julien Grall]

Hi Ian,

On 10/02/2015 12:45, Ian Campbell wrote:
> Previously 32-bit userspace on 32-bit kernel and 64-bit userspace on 64-bit
> kernel could access these registers irrespective of whether the kernel had
> configured them to be allowed to. To fix this:
>
>   - Userspace access to CNTP_CTL_EL0 and CNTP_TVAL_EL0 should be gated on
>     CNTKCTL_EL1.EL0PTEN.

Should not we take care of CNTP_CVAL_EL0 too? It seems that we don't 
even trap it for now...

[..]

> @@ -2062,8 +2053,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>           do_cp15_32(regs, hsr);
>           break;
>       case HSR_EC_CP15_64:
> -        if ( !is_32bit_domain(current->domain) )
> -            goto bad_trap;
> +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));

You should mention the change from if ( .... ) goto bad_trap to BUG_ON( 
... ) in the commit message.

Although, I think the debug message in bad_trap is useful to keep. It 
may be handy to have the HSR and the guest stack trace printed if Xen 
hit the condition.

[..]

> @@ -238,7 +250,7 @@ static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
>       switch ( hsr.bits & HSR_CP64_REGS_MASK )
>       {
>       case HSR_CPREG64(CNTPCT):
> -        if (!vtimer_cntpct(regs, &x, cp64.read))
> +        if ( !vtimer_cntpct(regs, &x, cp64.read) )

I would mention the coding style change in the commit message.

Regards,

-- 
Julien Grall

Re: [PATCH v2 5/9] xen: arm: Handle CP15 register traps from userspace

[Julien Grall]

Hi Ian,

On 10/02/15 04:45, Ian Campbell wrote:
>      default:
> @@ -2047,8 +2070,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>          advance_pc(regs, hsr);
>          break;
>      case HSR_EC_CP15_32:
> -        if ( !is_32bit_domain(current->domain) )
> -            goto bad_trap;
> +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));

It's a bit strange that on the previous patch (#5) you fixed CP15_64 but
not CP15_32. If I'm not mistaken you need both in-order to make the
things correctly work.

So, I would invert the 2 patches.

Regards,

-- 
Julien Grall

Re: [PATCH v2 6/9] xen: arm: Handle CP14 32-bit register accesses from userspace

[Julien Grall]

Hi Ian,

On 10/02/15 04:45, Ian Campbell wrote:
> Accesses to these from 32-bit userspace would cause a hypervisor exception
> (host crash) when running a 64-bit kernel, which is worked around by the fix to
> XSA-102. On 32-bit kernels they would be implemented as RAZ/WI which is
> incorrect but harmless.
> 
> Update as follows:
>  - DBGDSCRINT should be R/O.
>  - DBGDSCREXT should be EL1 only.
>  - DBGOSLAR is RO and EL1 only.
>  - DBGVCR, DBGB[VC]R*, DBGW[VC]R*, and DBGOSDLR are EL1 only.
> 
> DBGDIDR and DBGDSCRINT are accessible from EL0 if DBGDSCRext.UDCCdis. Since we
> emulate that as RAZ/WI we allow access.
> 
> When we do not allow an access we now silently inject an undef even in debug
> mode since the debugging messages are not helpful (we have handled the access,
> by explicitly choosing not to).
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/traps.c |   34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 6045396..18f8332 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1721,10 +1721,12 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
>      switch ( hsr.bits & HSR_CP32_REGS_MASK )
>      {
>      case HSR_CPREG32(DBGDIDR):
> -
> -        /* Read-only register */
> +        /*
> +         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> +         * is set to 0, which we emulated below.
> +         */
>          if ( !cp32.read )
> -            goto bad_cp;
> +            goto undef_cp14_32;
>  
>          /* Implement the minimum requirements:
>           *  - Number of watchpoints: 1
> @@ -1737,15 +1739,24 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
>          break;
>  
>      case HSR_CPREG32(DBGDSCRINT):
> +        if ( !cp32.read )
> +            goto undef_cp14_32;
> +
> +        *r = 0;
> +        break;
> +

The comment explaining why this field is RAZ/WI is useful. I would add
it again here.

Regards,


-- 
Julien Grall

Re: [PATCH v2 7/9] xen: arm: correctly handle sysreg accesses from userspace

[Julien Grall]

Hi Ian,

On 10/02/15 04:45, Ian Campbell wrote:
> Previously we implemented all registers as RAZ/WI even if they
> shouldn't be accessible to userspace.
> 
> Accesses to the *_EL1 registers from EL0 are trapped to EL1 by the
> hardware, so add a BUG_ON. Likewise accesses from 32-bit EL1 cannot
> happen.
> 
> PMUSERENR_EL0 and MDCCSR_EL0 are R/O to EL0.
> 
> Other PM*_EL0 registers are accessible at EL0 only if
> PMUSERENR_EL0.EN is set, since we emulate that as RAZ/WI we know that
> bit cannot be set.

You assume that PMUSERENR_EL0.EN is always set to 0 during Xen boot.
Actually it's not the case and from the spec, it may be possible to have
it in an unkwnown state.

Regards,

-- 
Julien Grall

Re: [PATCH v2 8/9] xen: arm: handle remaining traps from userspace

[Julien Grall]

Hi Ian,

On 10/02/15 04:45, Ian Campbell wrote:
>      case HSR_EC_SMC64:
> +        BUG_ON(psr_mode_is_32bit(regs->cpsr));
>          perfc_incr(trap_smc64);
> -        inject_undef64_exception(regs, hsr.len);

I don't understand why you drop the #undef injection. Is it a mistake?

Regards,

-- 
Julien Grall

Re: [PATCH v2 9/9] xen: arm: Allow traps from 32 bit userspace on 64 bit hypervisors again

[Julien Grall]

Hi Ian,

On 10/02/15 04:45, Ian Campbell wrote:
> This removes the unconditional #undef injected in response to such traps which
> was added by the fixes to CVE-2014-5147 / XSA-102 in c0020e099702 "xen: arm:
> Handle traps from 32-bit userspace on 64-bit kernel as undef", we now handle
> such traps correctly.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Assuming that all the traps are correctly handled:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH v2 4/9] xen: arm: correctly handle vtimer traps from userspace

[Ian Campbell]

On Tue, 2015-02-10 at 14:41 +0800, Julien Grall wrote:
> Hi Ian,
> 
> On 10/02/2015 12:45, Ian Campbell wrote:
> > Previously 32-bit userspace on 32-bit kernel and 64-bit userspace on 64-bit
> > kernel could access these registers irrespective of whether the kernel had
> > configured them to be allowed to. To fix this:
> >
> >   - Userspace access to CNTP_CTL_EL0 and CNTP_TVAL_EL0 should be gated on
> >     CNTKCTL_EL1.EL0PTEN.
> 
> Should not we take care of CNTP_CVAL_EL0 too? It seems that we don't 
> even trap it for now...

We should, I'll prepend such a patch to the series, since it should be
backported.

As it happens Linux doesn't allow user access to this register (due to
the settings in CNTKCTL), so it traps to EL1 in h/w and we never see it.
But if an OS were to expose the phys timer to userspace for some reason
then we would trap and inject undef, which is a bit mean of us.

> 
> [..]
> 
> > @@ -2062,8 +2053,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >           do_cp15_32(regs, hsr);
> >           break;
> >       case HSR_EC_CP15_64:
> > -        if ( !is_32bit_domain(current->domain) )
> > -            goto bad_trap;
> > +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> 
> You should mention the change from if ( .... ) goto bad_trap to BUG_ON( 
> ... ) in the commit message.

OK.

> Although, I think the debug message in bad_trap is useful to keep. It 
> may be handy to have the HSR and the guest stack trace printed if Xen 
> hit the condition.

Doesn't BUG_ON include all that? It should really.

Ian.

Re: [PATCH v2 5/9] xen: arm: Handle CP15 register traps from userspace

[Ian Campbell]

On Tue, 2015-02-17 at 15:07 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 10/02/15 04:45, Ian Campbell wrote:
> >      default:
> > @@ -2047,8 +2070,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >          advance_pc(regs, hsr);
> >          break;
> >      case HSR_EC_CP15_32:
> > -        if ( !is_32bit_domain(current->domain) )
> > -            goto bad_trap;
> > +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> 
> It's a bit strange that on the previous patch (#5) you fixed CP15_64 but
> not CP15_32. If I'm not mistaken you need both in-order to make the
> things correctly work.

The previous patch (I think you meant #4) made the CP15_64 handlers
correct (since as it happens they are all vtimer related), while it is
only at this point that the cp15_32 registers are all finally updated.

> So, I would invert the 2 patches.

I'm not sure the ordering matters, none of this will be active until the
final patch removes the top-level check. IOW I think the patches make as
much sense in either order and this is the one it has ended up in.

Ian.

Re: [PATCH v2 7/9] xen: arm: correctly handle sysreg accesses from userspace

[Ian Campbell]

On Tue, 2015-02-17 at 15:25 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 10/02/15 04:45, Ian Campbell wrote:
> > Previously we implemented all registers as RAZ/WI even if they
> > shouldn't be accessible to userspace.
> > 
> > Accesses to the *_EL1 registers from EL0 are trapped to EL1 by the
> > hardware, so add a BUG_ON. Likewise accesses from 32-bit EL1 cannot
> > happen.
> > 
> > PMUSERENR_EL0 and MDCCSR_EL0 are R/O to EL0.
> > 
> > Other PM*_EL0 registers are accessible at EL0 only if
> > PMUSERENR_EL0.EN is set, since we emulate that as RAZ/WI we know that
> > bit cannot be set.
> 
> You assume that PMUSERENR_EL0.EN is always set to 0 during Xen boot.
> Actually it's not the case and from the spec, it may be possible to have
> it in an unkwnown state.

The real PMUSERENR_EL0 is never seen/touched by EL1/EL0 since we trap
all accesses (via MDCR_EL2.TPM) and emulate as RAZ, so the value of the
real PMUSERENR_EL0 is never used (since MDCR_EL2 trumps it).

Ian.

Re: [PATCH v2 8/9] xen: arm: handle remaining traps from userspace

[Ian Campbell]

On Tue, 2015-02-17 at 15:28 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 10/02/15 04:45, Ian Campbell wrote:
> >      case HSR_EC_SMC64:
> > +        BUG_ON(psr_mode_is_32bit(regs->cpsr));
> >          perfc_incr(trap_smc64);
> > -        inject_undef64_exception(regs, hsr.len);
> 
> I don't understand why you drop the #undef injection. Is it a mistake?

Yes, a rebase-o. Fixed.

Ian.

Re: [PATCH v2 4/9] xen: arm: correctly handle vtimer traps from userspace

[Julien Grall]

On 19/02/15 12:10, Ian Campbell wrote:
> On Tue, 2015-02-10 at 14:41 +0800, Julien Grall wrote:
>> Hi Ian,
>>
>> On 10/02/2015 12:45, Ian Campbell wrote:
>>> Previously 32-bit userspace on 32-bit kernel and 64-bit userspace on 64-bit
>>> kernel could access these registers irrespective of whether the kernel had
>>> configured them to be allowed to. To fix this:
>>>
>>>   - Userspace access to CNTP_CTL_EL0 and CNTP_TVAL_EL0 should be gated on
>>>     CNTKCTL_EL1.EL0PTEN.
>>
>> Should not we take care of CNTP_CVAL_EL0 too? It seems that we don't 
>> even trap it for now...
> 
> We should, I'll prepend such a patch to the series, since it should be
> backported.
> 
> As it happens Linux doesn't allow user access to this register (due to
> the settings in CNTKCTL), so it traps to EL1 in h/w and we never see it.
> But if an OS were to expose the phys timer to userspace for some reason
> then we would trap and inject undef, which is a bit mean of us.

Good, thanks!

>>
>> [..]
>>
>>> @@ -2062,8 +2053,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>>>           do_cp15_32(regs, hsr);
>>>           break;
>>>       case HSR_EC_CP15_64:
>>> -        if ( !is_32bit_domain(current->domain) )
>>> -            goto bad_trap;
>>> +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
>>
>> You should mention the change from if ( .... ) goto bad_trap to BUG_ON( 
>> ... ) in the commit message.
> 
> OK.
> 
>> Although, I think the debug message in bad_trap is useful to keep. It 
>> may be handy to have the HSR and the guest stack trace printed if Xen 
>> hit the condition.
> 
> Doesn't BUG_ON include all that? It should really.

Not really BUG_ON will jump into the exception mode and therefore print
the HSR of the exception (breakpoint for ARM64 and undef for ARM32).

Also, BUG_ON doesn't print the stack trace of the guest but only the
hypervisor one. The latter is not really useful in the current case.

Regards,

-- 
Julien Grall

Re: [PATCH v2 5/9] xen: arm: Handle CP15 register traps from userspace

[Julien Grall]

On 19/02/15 12:15, Ian Campbell wrote:
> On Tue, 2015-02-17 at 15:07 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 10/02/15 04:45, Ian Campbell wrote:
>>>      default:
>>> @@ -2047,8 +2070,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>>>          advance_pc(regs, hsr);
>>>          break;
>>>      case HSR_EC_CP15_32:
>>> -        if ( !is_32bit_domain(current->domain) )
>>> -            goto bad_trap;
>>> +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
>>
>> It's a bit strange that on the previous patch (#5) you fixed CP15_64 but
>> not CP15_32. If I'm not mistaken you need both in-order to make the
>> things correctly work.
> 
> The previous patch (I think you meant #4) made the CP15_64 handlers
> correct (since as it happens they are all vtimer related), while it is
> only at this point that the cp15_32 registers are all finally updated.

>> So, I would invert the 2 patches.
> 
> I'm not sure the ordering matters, none of this will be active until the
> final patch removes the top-level check. IOW I think the patches make as
> much sense in either order and this is the one it has ended up in.

It doesn't make sense to correct the both cp15_* on different patches.

As it would never be possible to run 32 bit userspace on 64 bit guest.
It doesn't harm to move the two in the same patch. It would be more logic.

Regards,

-- 
Julien Grall

Re: [PATCH v2 7/9] xen: arm: correctly handle sysreg accesses from userspace

[Julien Grall]

On 19/02/15 12:23, Ian Campbell wrote:
> On Tue, 2015-02-17 at 15:25 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 10/02/15 04:45, Ian Campbell wrote:
>>> Previously we implemented all registers as RAZ/WI even if they
>>> shouldn't be accessible to userspace.
>>>
>>> Accesses to the *_EL1 registers from EL0 are trapped to EL1 by the
>>> hardware, so add a BUG_ON. Likewise accesses from 32-bit EL1 cannot
>>> happen.
>>>
>>> PMUSERENR_EL0 and MDCCSR_EL0 are R/O to EL0.
>>>
>>> Other PM*_EL0 registers are accessible at EL0 only if
>>> PMUSERENR_EL0.EN is set, since we emulate that as RAZ/WI we know that
>>> bit cannot be set.
>>
>> You assume that PMUSERENR_EL0.EN is always set to 0 during Xen boot.
>> Actually it's not the case and from the spec, it may be possible to have
>> it in an unkwnown state.
> 
> The real PMUSERENR_EL0 is never seen/touched by EL1/EL0 since we trap
> all accesses (via MDCR_EL2.TPM) and emulate as RAZ, so the value of the
> real PMUSERENR_EL0 is never used (since MDCR_EL2 trumps it).

Hmmm... right sorry for the noise.

Regards,


-- 
Julien Grall

Re: [PATCH v2 5/9] xen: arm: Handle CP15 register traps from userspace

[Ian Campbell]

On Thu, 2015-02-19 at 14:53 +0000, Julien Grall wrote:
> On 19/02/15 12:15, Ian Campbell wrote:
> > On Tue, 2015-02-17 at 15:07 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 10/02/15 04:45, Ian Campbell wrote:
> >>>      default:
> >>> @@ -2047,8 +2070,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >>>          advance_pc(regs, hsr);
> >>>          break;
> >>>      case HSR_EC_CP15_32:
> >>> -        if ( !is_32bit_domain(current->domain) )
> >>> -            goto bad_trap;
> >>> +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> >>
> >> It's a bit strange that on the previous patch (#5) you fixed CP15_64 but
> >> not CP15_32. If I'm not mistaken you need both in-order to make the
> >> things correctly work.
> > 
> > The previous patch (I think you meant #4) made the CP15_64 handlers
> > correct (since as it happens they are all vtimer related), while it is
> > only at this point that the cp15_32 registers are all finally updated.
> 
> >> So, I would invert the 2 patches.
> > 
> > I'm not sure the ordering matters, none of this will be active until the
> > final patch removes the top-level check. IOW I think the patches make as
> > much sense in either order and this is the one it has ended up in.
> 
> It doesn't make sense to correct the both cp15_* on different patches.

Sure it does, it breaks things down into more logical chunks by
separating the vtimer stuff into its own patch, which is a completely
reasonable way to do things (even if not the only way).

> As it would never be possible to run 32 bit userspace on 64 bit guest.
> It doesn't harm to move the two in the same patch. It would be more logic.

If I was just abut to write these patches from scratch maybe I would do
that, but they exist now and are reasonable in the form they are in. I'm
afraid I'm not going to rework them for what amounts to bikeshedding
reasons.

Ian.

Re: [PATCH v2 4/9] xen: arm: correctly handle vtimer traps from userspace

[Ian Campbell]

On Thu, 2015-02-19 at 14:42 +0000, Julien Grall wrote:
> >>
> >> [..]
> >>
> >>> @@ -2062,8 +2053,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >>>           do_cp15_32(regs, hsr);
> >>>           break;
> >>>       case HSR_EC_CP15_64:
> >>> -        if ( !is_32bit_domain(current->domain) )
> >>> -            goto bad_trap;
> >>> +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> >>
> >> You should mention the change from if ( .... ) goto bad_trap to BUG_ON( 
> >> ... ) in the commit message.
> > 
> > OK.
> > 
> >> Although, I think the debug message in bad_trap is useful to keep. It 
> >> may be handy to have the HSR and the guest stack trace printed if Xen 
> >> hit the condition.
> > 
> > Doesn't BUG_ON include all that? It should really.
> 
> Not really BUG_ON will jump into the exception mode and therefore print
> the HSR of the exception (breakpoint for ARM64 and undef for ARM32).

Hrm, good point.

Rather than reintroducing the goto idiom what about some form of
noreturn panic helper for checking for sane h/w state (since these
failures are really of the "buggy hardware" variety) e.g.
ASSERT_GUEST_STATE(is_32bit_domain(...)) which would dump the guest
state and then panic?

Since these scenarios really indicate some sort of h/w fault I'm
considering making them debug=y only, as indicated by the use of
ASSERT_FOO.

Ian.

Re: [PATCH v2 4/9] xen: arm: correctly handle vtimer traps from userspace

[Ian Campbell]

On Thu, 2015-02-19 at 15:13 +0000, Ian Campbell wrote:
> On Thu, 2015-02-19 at 14:42 +0000, Julien Grall wrote:
> > >> Although, I think the debug message in bad_trap is useful to keep. It 
> > >> may be handy to have the HSR and the guest stack trace printed if Xen 
> > >> hit the condition.
> > > 
> > > Doesn't BUG_ON include all that? It should really.
> > 
> > Not really BUG_ON will jump into the exception mode and therefore print
> > the HSR of the exception (breakpoint for ARM64 and undef for ARM32).
> 
> Hrm, good point.
> 
> Rather than reintroducing the goto idiom what about some form of
> noreturn panic helper for checking for sane h/w state (since these
> failures are really of the "buggy hardware" variety) e.g.
> ASSERT_GUEST_STATE(is_32bit_domain(...)) which would dump the guest
> state and then panic?

Here is the sort of thing I was thinking about (only converted one
BUG_ON so far as an example, there are more candidates).

Jan, would this be useful for x86 do you think, i.e. would you like me
to put it in lib.h with regular ASSERT? (Although making it more widely
available concerns me due to the pretty huge caveat in its use).

Should it be on for debug=n too? (In which case it might want to become
GUEST_BUG_ON or similar). The argument for doing so is that it would
reduce the impact of potential security issues arising from h/w bugs (or
spec misunderstandings), in which case I would add to the comment:

 * The intention is to limit the damage such h/w bugs (or spec
 * misunderstandings) can do by turning them into Denial of Service
 * attacks instead of e.g. information leaks or privilege escalations.

It's possible that in some cases the DoS might be worse than the actual
issue, i.e. this might turn a fairly minor info leak into a DoS for
example. Making it debug=y only helps here with catching the issues, but
doesn't protect end users (who run debug=n), but doesn't have this risk
of the cure being worse than the disease.

Ian.

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8fec036..198e3b8 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -63,6 +63,30 @@ static inline void check_stack_alignment_constraints(void) {
 #endif
 }
 
+/*
+ * GASSERT is intended for checking that the guest state has not been
+ * corrupted in hardware and/or that the hardware behaves as we
+ * believe it should (i.e. that certain traps can only occur when the
+ * guest is in a particular mode).
+ *
+ * GASSERT *MUST* *NOT* be used to check for guest controllable state!
+ *
+ * Compared with regular ASSERT it dumps the guest vcpu state instead
+ * of Xen's state.
+ */
+#ifndef NDEBUG
+#define gassert_failed(p)                                               \
+do {                                                                    \
+    show_execution_state(guest_cpu_user_regs());                        \
+    panic("%pv: Guest assertion '%s' failed, line %d, file %s\n", p ,   \
+          current,__LINE__, __FILE__);                                  \
+} while (0)
+#define GASSERT(p) \
+    do { if ( unlikely(!(p)) ) gassert_failed(#p); } while (0)
+#else
+#define GASSERT(p) do { if ( 0 && (p) ); } while (0)
+#endif
+
 #ifdef CONFIG_ARM_32
 static int debug_stack_lines = 20;
 #define stack_words_per_line 8
@@ -2166,7 +2194,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_trap_hypercall(regs, &regs->x16, hsr.iss);
         break;
     case HSR_EC_SMC64:
-        BUG_ON(psr_mode_is_32bit(regs->cpsr));
+        GASSERT(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc64);
         inject_undef64_exception(regs, hsr.len);
         break;

Re: [PATCH v2 4/9] xen: arm: correctly handle vtimer traps from userspace

[Julien Grall]

On 25/02/15 14:32, Ian Campbell wrote:
> On Thu, 2015-02-19 at 15:13 +0000, Ian Campbell wrote:
>> On Thu, 2015-02-19 at 14:42 +0000, Julien Grall wrote:
>>>>> Although, I think the debug message in bad_trap is useful to keep. It 
>>>>> may be handy to have the HSR and the guest stack trace printed if Xen 
>>>>> hit the condition.
>>>>
>>>> Doesn't BUG_ON include all that? It should really.
>>>
>>> Not really BUG_ON will jump into the exception mode and therefore print
>>> the HSR of the exception (breakpoint for ARM64 and undef for ARM32).
>>
>> Hrm, good point.
>>
>> Rather than reintroducing the goto idiom what about some form of
>> noreturn panic helper for checking for sane h/w state (since these
>> failures are really of the "buggy hardware" variety) e.g.
>> ASSERT_GUEST_STATE(is_32bit_domain(...)) which would dump the guest
>> state and then panic?
> 
> Here is the sort of thing I was thinking about (only converted one
> BUG_ON so far as an example, there are more candidates).
> 
> Jan, would this be useful for x86 do you think, i.e. would you like me
> to put it in lib.h with regular ASSERT? (Although making it more widely
> available concerns me due to the pretty huge caveat in its use).
> 
> Should it be on for debug=n too? (In which case it might want to become
> GUEST_BUG_ON or similar). The argument for doing so is that it would
> reduce the impact of potential security issues arising from h/w bugs (or
> spec misunderstandings), in which case I would add to the comment:

People may run a binary provided by a distribution on buggy hardware. So
the GUEST_BUG_ON seems better.

Although, I'm wondering what is the overhead of having a check at each
traps? I guess none.

Regards,

-- 
Julien Grall