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

[PATCH v3 0/12] 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 implemented Julien's review feedback including:
      * added the GUEST_BUG_ON patch to the end to replace the BUG_ONs
        due to invalid h/w state, which gets more useful debug if that
        occurs.
      * handled CNTP_CVAL_EL0.

Ian.

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

[PATCH 01/12] 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 02/12] xen: arm: handle accesses to CNTP_CVAL_EL0

[Ian Campbell]

All OSes we have run on top of Xen use CNTP_TVAL_EL0 but for
completeness we really should handle CVAL too.

In vtimer_emulate_cp64 pull the propagation of the 64-bit result into
two 32-bit registers out of the switch to avoid duplicating for every
register. We also need to initialise x now since previously the only
register implemented register was R/O.

While adding HSR_SYSREG_CNTP_CVAL_EL0 also move
HSR_SYSREG_CNTP_CTL_EL0 so it is sorted correctly.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v3: New patch.

This should definitately be backported.
---
 xen/arch/arm/traps.c          |    2 ++
 xen/arch/arm/vtimer.c         |   49 +++++++++++++++++++++++++++++++++++------
 xen/include/asm-arm/sysregs.h |    3 ++-
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 50d67aa..9e4a60f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1670,6 +1670,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,
     switch ( hsr.bits & HSR_CP64_REGS_MASK )
     {
     case HSR_CPREG64(CNTPCT):
+    case HSR_CPREG64(CNTP_CVAL):
         if ( !vtimer_emulate(regs, hsr) )
         {
             dprintk(XENLOG_ERR,
@@ -1855,6 +1856,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
         break;
     case HSR_SYSREG_CNTP_CTL_EL0:
     case HSR_SYSREG_CNTP_TVAL_EL0:
+    case HSR_SYSREG_CNTP_CVAL_EL0:
         if ( !vtimer_emulate(regs, hsr) )
         {
             dprintk(XENLOG_ERR,
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 4427ae5..0c67f20 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -202,6 +202,31 @@ static void vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r, int read)
     }
 }
 
+static int vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_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 = ns_to_ticks(v->arch.phys_timer.cval);
+    }
+    else
+    {
+        v->arch.phys_timer.cval = ticks_to_ns(*r);
+        if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
+        {
+            v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
+            set_timer(&v->arch.phys_timer.timer,
+                      v->arch.phys_timer.cval +
+                      v->domain->arch.phys_timer_base.offset);
+        }
+    }
+    return 1;
+}
 static int vtimer_cntpct(struct cpu_user_regs *regs, uint64_t *r, int read)
 {
     struct vcpu *v = current;
@@ -253,7 +278,7 @@ static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
     struct hsr_cp64 cp64 = hsr.cp64;
     uint32_t *r1 = (uint32_t *)select_user_reg(regs, cp64.reg1);
     uint32_t *r2 = (uint32_t *)select_user_reg(regs, cp64.reg2);
-    uint64_t x;
+    uint64_t x = (uint64_t)(*r1) | ((uint64_t)(*r2) << 32);
 
     if ( cp64.read )
         perfc_incr(vtimer_cp64_reads);
@@ -265,17 +290,24 @@ static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
     case HSR_CPREG64(CNTPCT):
         if (!vtimer_cntpct(regs, &x, cp64.read))
             return 0;
+        break;
 
-        if ( cp64.read )
-        {
-            *r1 = (uint32_t)(x & 0xffffffff);
-            *r2 = (uint32_t)(x >> 32);
-        }
-        return 1;
+    case HSR_CPREG64(CNTP_CVAL):
+        if ( !vtimer_cntp_cval(regs, &x, cp64.read) )
+            return 0;
+        break;
 
     default:
         return 0;
     }
+
+    if ( cp64.read )
+    {
+        *r1 = (uint32_t)(x & 0xffffffff);
+        *r2 = (uint32_t)(x >> 32);
+    }
+
+    return 1;
 }
 
 #ifdef CONFIG_ARM_64
@@ -303,6 +335,9 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
             *x = r;
         return 1;
 
+    case HSR_SYSREG_CNTP_CVAL_EL0:
+        return vtimer_cntp_cval(regs, x, sysreg.read);
+
     case HSR_SYSREG_CNTPCT_EL0:
         return vtimer_cntpct(regs, x, sysreg.read);
 
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index 169b7ac..df8e070 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -100,8 +100,9 @@
 #define HSR_SYSREG_PMOVSSET_EL0   HSR_SYSREG(3,3,c9,c14,3)
 
 #define HSR_SYSREG_CNTPCT_EL0     HSR_SYSREG(3,3,c14,c0,0)
-#define HSR_SYSREG_CNTP_CTL_EL0   HSR_SYSREG(3,3,c14,c2,1)
 #define HSR_SYSREG_CNTP_TVAL_EL0  HSR_SYSREG(3,3,c14,c2,0)
+#define HSR_SYSREG_CNTP_CTL_EL0   HSR_SYSREG(3,3,c14,c2,1)
+#define HSR_SYSREG_CNTP_CVAL_EL0  HSR_SYSREG(3,3,c14,c2,2)
 
 /*
  * GIC System register assembly aliases picked from kernel
-- 
1.7.10.4

Re: [PATCH 02/12] xen: arm: handle accesses to CNTP_CVAL_EL0

[Julien Grall]

Hi Ian,

On 25/03/15 14:22, Ian Campbell wrote:
> All OSes we have run on top of Xen use CNTP_TVAL_EL0 but for
> completeness we really should handle CVAL too.
> 
> In vtimer_emulate_cp64 pull the propagation of the 64-bit result into
> two 32-bit registers out of the switch to avoid duplicating for every
> register. We also need to initialise x now since previously the only
> register implemented register was R/O.
> 
> While adding HSR_SYSREG_CNTP_CVAL_EL0 also move
> HSR_SYSREG_CNTP_CTL_EL0 so it is sorted correctly.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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

Regards,

-- 
Julien Grall

Re: [PATCH 02/12] xen: arm: handle accesses to CNTP_CVAL_EL0

[Julien Grall]

On 25/03/15 14:22, Ian Campbell wrote:
> +static int vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r, int read)
> +{
> +    struct vcpu *v = current;
> +
> +    if ( psr_mode_is_user(regs) &&
> +         !(READ_SYSREG(CNTKCTL_EL1) & CNTKCTL_EL1_EL0PTEN) )

Sorry, I didn't notice it on my previous review.

CNTKCTL_EL1_EL0PTEN and psr_mode_is_user are only defined in
respectively patch #6 and #4.

Can you invert the patches to avoid build breakage during bisection?

Regards,

-- 
Julien Grall

Re: [PATCH 02/12] xen: arm: handle accesses to CNTP_CVAL_EL0

[Ian Campbell]

On Wed, 2015-03-25 at 18:32 +0000, Julien Grall wrote:
> On 25/03/15 14:22, Ian Campbell wrote:
> > +static int vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r, int read)
> > +{
> > +    struct vcpu *v = current;
> > +
> > +    if ( psr_mode_is_user(regs) &&
> > +         !(READ_SYSREG(CNTKCTL_EL1) & CNTKCTL_EL1_EL0PTEN) )
> 
> Sorry, I didn't notice it on my previous review.
> 
> CNTKCTL_EL1_EL0PTEN and psr_mode_is_user are only defined in
> respectively patch #6 and #4.

Well spotted.

> Can you invert the patches to avoid build breakage during bisection?

I was hoping to be able to backport this, so I think I will move the
definitions of CNTKCTL_* here and either decide to backport the psr_mdoe
refactoring or see if I can avoid using it in this patch.
 
Ian.

Re: [PATCH 02/12] xen: arm: handle accesses to CNTP_CVAL_EL0

[Ian Campbell]

On Thu, 2015-03-26 at 10:59 +0000, Ian Campbell wrote:
> On Wed, 2015-03-25 at 18:32 +0000, Julien Grall wrote:
> > On 25/03/15 14:22, Ian Campbell wrote:
> > > +static int vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r, int read)
> > > +{
> > > +    struct vcpu *v = current;
> > > +
> > > +    if ( psr_mode_is_user(regs) &&
> > > +         !(READ_SYSREG(CNTKCTL_EL1) & CNTKCTL_EL1_EL0PTEN) )
> > 
> > Sorry, I didn't notice it on my previous review.
> > 
> > CNTKCTL_EL1_EL0PTEN and psr_mode_is_user are only defined in
> > respectively patch #6 and #4.
> 
> Well spotted.
> 
> > Can you invert the patches to avoid build breakage during bisection?
> 
> I was hoping to be able to backport this, so I think I will move the
> definitions of CNTKCTL_* here and either decide to backport the psr_mdoe
> refactoring or see if I can avoid using it in this patch.

In the end #6 (xen: arm: correctly handle vtimer traps from userspace)
should probably be backported too and the psr_mode_is_user one is simple
enough it's easiest to just take it as a precursor.

So I've reordered.

Ian.

[PATCH 03/12] xen: arm: Use ARMv8 names for CNTHCTL_EL2 bits

[Ian Campbell]

Rather than using the v8 register names and the v7 bit names, which
makes things needlessly difficult when reading.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v3: New patch.
---
 xen/arch/arm/time.c             |    2 +-
 xen/include/asm-arm/processor.h |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 352e25e..ce6d3fd 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -230,7 +230,7 @@ void __cpuinit init_timer_interrupt(void)
     /* Sensible defaults */
     WRITE_SYSREG64(0, CNTVOFF_EL2);     /* No VM-specific offset */
     /* Do not let the VMs program the physical timer, only read the physical counter */
-    WRITE_SYSREG32(CNTHCTL_PA, CNTHCTL_EL2);
+    WRITE_SYSREG32(CNTHCTL_EL2_EL1PCTEN, CNTHCTL_EL2);
     WRITE_SYSREG32(0, CNTP_CTL_EL0);    /* Physical timer disabled */
     WRITE_SYSREG32(0, CNTHP_CTL_EL2);   /* Hypervisor's timer disabled */
     isb();
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index fcd26fb..570d2a1 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -555,8 +555,8 @@ union hsr {
 #define FSC_LL_MASK    (_AC(0x03,U)<<0)
 
 /* Time counter hypervisor control register */
-#define CNTHCTL_PA      (1u<<0)  /* Kernel/user access to physical counter */
-#define CNTHCTL_TA      (1u<<1)  /* Kernel/user access to CNTP timer */
+#define CNTHCTL_EL2_EL1PCTEN (1u<<0) /* Kernel/user access to physical counter */
+#define CNTHCTL_EL2_EL1PCEN  (1u<<1) /* Kernel/user access to CNTP timer regs */
 
 /* Timer control registers */
 #define CNTx_CTL_ENABLE   (1u<<0)  /* Enable timer */
-- 
1.7.10.4

Re: [PATCH 03/12] xen: arm: Use ARMv8 names for CNTHCTL_EL2 bits

[Julien Grall]

Hi Ian,

On 25/03/15 14:22, Ian Campbell wrote:
> Rather than using the v8 register names and the v7 bit names, which
> makes things needlessly difficult when reading.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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

Regards,

-- 
Julien Grall

[PATCH 04/12] 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 9e4a60f..83abafa 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 05/12] xen: arm: Handle 32-bit EL0 on 64-bit EL1 when advancing PC after trap

[Ian Campbell]

Fix a coding style issue while in the area.

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 83abafa..4eda561 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 06/12] 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).

Since HSR_EC_CP15_64 cannot be taken from a guest in AArch64 mode
except due to a hardware bug switch the associated check to a BUG_ON
(which will be switched to something more appropriate in a subsequent
patch)

Fix a coding style issue in HSR_CPREG64(CNTPCT) while touching similar
code.

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 |    6 ++++++
 3 files changed, 38 insertions(+), 34 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 4eda561..c5ffcc5 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;
     }
@@ -1665,11 +1662,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,
     case HSR_CPREG64(CNTPCT):
     case HSR_CPREG64(CNTP_CVAL):
         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:
         {
@@ -1683,6 +1676,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;
         }
@@ -1851,11 +1845,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
     case HSR_SYSREG_CNTP_TVAL_EL0:
     case HSR_SYSREG_CNTP_CVAL_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) )
@@ -1874,8 +1864,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",
@@ -1888,7 +1878,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;
         }
     }
@@ -2064,8 +2055,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 0c67f20..5aca65e 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -154,9 +154,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;
@@ -176,13 +181,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 )
@@ -200,6 +210,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_cntp_cval(struct cpu_user_regs *regs, uint64_t *r, int read)
@@ -235,6 +246,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;
@@ -261,12 +275,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;
@@ -288,7 +300,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;
         break;
 
@@ -325,12 +337,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;
@@ -353,17 +367,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 570d2a1..5ccf61f 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -558,6 +558,12 @@ union hsr {
 #define CNTHCTL_EL2_EL1PCTEN (1u<<0) /* Kernel/user access to physical counter */
 #define CNTHCTL_EL2_EL1PCEN  (1u<<1) /* Kernel/user access to CNTP timer regs */
 
+/* Time counter kernel control register */
+#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 */
+
 /* Timer control registers */
 #define CNTx_CTL_ENABLE   (1u<<0)  /* Enable timer */
 #define CNTx_CTL_MASK     (1u<<1)  /* Mask IRQ */
-- 
1.7.10.4

Re: [PATCH 06/12] xen: arm: correctly handle vtimer traps from userspace

[Julien Grall]

Hi Ian,

On 25/03/15 14:22, Ian Campbell wrote:
> -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;
> +

Would it make sense to create a macro for this check? The code is pretty
much the same on every function except the field to check
(EL0PTEN/EL0PCTEN).

Either way:

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

Regards,

-- 
Julien Grall

Re: [PATCH 06/12] xen: arm: correctly handle vtimer traps from userspace

[Ian Campbell]

On Wed, 2015-03-25 at 18:41 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 25/03/15 14:22, Ian Campbell wrote:
> > -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;
> > +
> 
> Would it make sense to create a macro for this check? The code is pretty
> much the same on every function except the field to check
> (EL0PTEN/EL0PCTEN).

It might, I'll have a go.

> 
> Either way:
> 
> Reviewed-by:  Julien Grall <julien.grall@linaro.org>
> 
> Regards,
> 

[PATCH 07/12] 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 c5ffcc5..e49ae79 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:
@@ -2049,8 +2072,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

Re: [PATCH 07/12] xen: arm: Handle CP15 register traps from userspace

[Julien Grall]

Hi Ian,

On 25/03/15 14:22, Ian Campbell wrote:
> 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.

For PMINTENSET and PMINTENCLR the spec (ARMv8 DDI0487A rev d) says:

"If MDCR_EL2.TPM==1, Non-secure accesses to this register will trap from
EL1 and EL0 to EL2."

As we set to 1 MDCR_EL1.TPM, EL0 access will trap to Xen. So I think we
should replace the BUG_ON to injected a exception.

Reading more the spec only ACTLR access from EL0 will trap to EL1. All
access from EL0 to the others registers in the list above will trap to EL2.

Although, the ARMv7 spec seems to say to only valid access will be trapped.

Regards,

-- 
Julien Grall

Re: [PATCH 07/12] xen: arm: Handle CP15 register traps from userspace

[Ian Campbell]

On Wed, 2015-03-25 at 18:59 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 25/03/15 14:22, Ian Campbell wrote:
> > 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.
> 
> For PMINTENSET and PMINTENCLR the spec (ARMv8 DDI0487A rev d) says:
> 
> "If MDCR_EL2.TPM==1, Non-secure accesses to this register will trap from
> EL1 and EL0 to EL2."

Yes, it appears you are right. It is rather strange that an EL1 register
should trap to EL2 when accessed from EL0, but it does indeed seem to
say that.

I suspect this is an errata in the spec, but I suppose we should take it
at its word.

> As we set to 1 MDCR_EL1.TPM, EL0 access will trap to Xen. So I think we
> should replace the BUG_ON to injected a exception.

Yes.

> Reading more the spec only ACTLR access from EL0 will trap to EL1. All
> access from EL0 to the others registers in the list above will trap to EL2.
> 
> Although, the ARMv7 spec seems to say to only valid access will be trapped.

[PATCH 08/12] 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>
---
v3: Add comment to DBGDSCRINT case.
---
 xen/arch/arm/traps.c |   38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index e49ae79..909e880 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1722,10 +1722,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
@@ -1738,15 +1740,28 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
         break;
 
     case HSR_CPREG32(DBGDSCRINT):
+        /*
+         * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
+         * is set to 0, which we emulated below.
+         */
+        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):
@@ -1754,13 +1769,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",
@@ -1769,6 +1793,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;
     }
@@ -2082,8 +2107,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

Re: [PATCH 08/12] xen: arm: Handle CP14 32-bit register accesses from userspace

[Julien Grall]

Hi Ian,

On 25/03/15 14:22, 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.

This register is WO. Actually the implementation is right, just the
commit message.

Other than that:

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

Regards,

-- 
Julien Grall

[PATCH 09/12] 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 909e880..a65b32a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1852,11 +1852,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:
@@ -1868,16 +1897,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 */
@@ -1885,8 +1914,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:
@@ -1910,7 +1940,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;
@@ -2153,8 +2182,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 df8e070..3b9ddd3 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

Re: [PATCH 09/12] xen: arm: correctly handle sysreg accesses from userspace

[Julien Grall]

Hi Ian,

On 25/03/15 14:22, 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.

It's not true on all *_EL1 registers. See PMINTENSET_EL1 for instance.

> PMUSERENR_EL0 and MDCCSR_EL0 are R/O to EL0.

Might be worth to explain that we forgot to implement MDCCSR_EL0 before.

> 
> 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.

The ARMv8 spec is confusing on this point

Let's take PMCR_EL0 for instance. The PMUSERENR_EL0.EN trapping is not
explained.

Although, PMCR is trapped when PMUSERENR_EL0.EN == 0.

Do you have a paragraph on the spec which clearly explain the behavior?

Regards,

-- 
Julien Grall

Re: [PATCH 09/12] xen: arm: correctly handle sysreg accesses from userspace

[Ian Campbell]

On Wed, 2015-03-25 at 19:22 +0000, Julien Grall wrote:
> Do you have a paragraph on the spec which clearly explain the
> behavior?

Not clearly enough :-(

I think to be on the safe side I'm going to make most of these
occurrences in this series inject a trap to EL1, unless I can find a
non-ambiguous statement for a given register.

Ian.

[PATCH 10/12] 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>
---
v3: Reintroduce accidentally dropped undef injection from smc64 case.
---
 xen/arch/arm/traps.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a65b32a..9e7337f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2141,23 +2141,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);
@@ -2168,6 +2167,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 )
@@ -2178,6 +2178,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));
         perfc_incr(trap_smc64);
         inject_undef64_exception(regs, hsr.len);
         break;
@@ -2204,7 +2205,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

Re: [PATCH 10/12] xen: arm: handle remaining traps from userspace

[Julien Grall]

Hi Ian,

On 25/03/15 14:22, Ian Campbell wrote:
> 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>

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

Regards,

-- 
Julien Grall

[PATCH 11/12] 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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 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 9e7337f..8fec036 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2095,18 +2095,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

[PATCH 12/12] xen: arm: Dump guest state when invalid trap state is detected

[Ian Campbell]

By adding GUEST_BUG_ON locally to traps.c.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v3: New patch
---
 xen/arch/arm/traps.c |   58 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8fec036..b4f654a 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
 }
 
+/*
+ * GUEST_BUG_ON 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).
+ *
+ * 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.
+ *
+ * GUEST_BUG_ON *MUST* *NOT* be used to check for guest controllable state!
+ *
+ * Compared with regular BUG_ON it dumps the guest vcpu state instead
+ * of Xen's state.
+ */
+#define guest_bug_on_failed(p)                          \
+do {                                                    \
+    show_execution_state(guest_cpu_user_regs());        \
+    panic("Guest Bug: %pv: '%s', line %d, file %s\n",   \
+          current, p, __LINE__, __FILE__);              \
+} while (0)
+#define GUEST_BUG_ON(p) \
+    do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0)
+
 #ifdef CONFIG_ARM_32
 static int debug_stack_lines = 20;
 #define stack_words_per_line 8
@@ -1565,7 +1589,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));
+        GUEST_BUG_ON(psr_mode_is_user(regs));
         if ( !cp32.read )
         {
             dprintk(XENLOG_ERR,
@@ -1575,7 +1599,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));
+        GUEST_BUG_ON(psr_mode_is_user(regs));
         if ( !cp32.read )
         {
             dprintk(XENLOG_ERR,
@@ -1585,7 +1609,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));
+        GUEST_BUG_ON(psr_mode_is_user(regs));
         if ( cp32.read )
         {
             dprintk(XENLOG_ERR,
@@ -1604,7 +1628,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));
+        GUEST_BUG_ON(psr_mode_is_user(regs));
         if ( cp32.read )
            *r = v->arch.actlr;
         break;
@@ -1626,7 +1650,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
     case HSR_CPREG32(PMINTENSET):
     case HSR_CPREG32(PMINTENCLR):
         /* EL1 only */
-        BUG_ON(psr_mode_is_user(regs));
+        GUEST_BUG_ON(psr_mode_is_user(regs));
         goto cp15_32_raz_wi;
 
     case HSR_CPREG32(PMCR):
@@ -1867,7 +1891,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
     /* - Double Lock Register */
     case HSR_SYSREG_OSDLR_EL1:
         /* EL1 only */
-        BUG_ON(psr_mode_is_user(regs));
+        GUEST_BUG_ON(psr_mode_is_user(regs));
         goto sysreg_raz_wi;
 
     case HSR_SYSREG_PMUSERENR_EL0:
@@ -1914,7 +1938,7 @@ 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));
+        GUEST_BUG_ON(psr_mode_is_user(regs));
         if ( hsr.sysreg.read )
             goto undef_sysreg;
         /* else: write ignored */
@@ -2114,37 +2138,37 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         advance_pc(regs, hsr);
         break;
     case HSR_EC_CP15_32:
-        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp15_32);
         do_cp15_32(regs, hsr);
         break;
     case HSR_EC_CP15_64:
-        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp15_64);
         do_cp15_64(regs, hsr);
         break;
     case HSR_EC_CP14_32:
-        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp14_32);
         do_cp14_32(regs, hsr);
         break;
     case HSR_EC_CP14_DBG:
-        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp14_dbg);
         do_cp14_dbg(regs, hsr);
         break;
     case HSR_EC_CP:
-        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_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));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc32);
         inject_undef_exception(regs, hsr.len);
         break;
     case HSR_EC_HVC32:
-        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
             return do_debug_trap(regs, hsr.iss & 0x00ff);
@@ -2155,7 +2179,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));
+        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_hvc64);
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
@@ -2166,12 +2190,12 @@ 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));
+        GUEST_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));
+        GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_sysreg);
         do_sysreg(regs, hsr);
         break;
-- 
1.7.10.4

Re: [PATCH 12/12] xen: arm: Dump guest state when invalid trap state is detected

[Julien Grall]

Hi Ian,

On 25/03/15 14:22, Ian Campbell wrote:
> By adding GUEST_BUG_ON locally to traps.c.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall