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

[PATCH v4 0/15] 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:
      * Handle any unexpected EL0 register traps by injecting #undef not
        by crashing since the docs really don't make it clear in all
        cases whether these are to be expected or not.
              * HSR.EC decoding still does assume e.g. that hvc32 can't
                come from a 64-bit guest, so GUEST_BUG_ON remains for
                that
      * Dropped handling of CLIDR, CCSIDR traps, since we don't actually
        request them (two less things to think about in the other
        patches)
      * No longer pretend to handle set/way operations, these are
        incompatible with virtualisation.
      * Various knock on effects from the above, I've retained most of
        the ack/review since it was mainly just adjusting the context.
      * Reordered a few things, i.e. some prerequisites for a patch
        which wanted backporting are now sooner and the switch to
        GUEST_BUG_ON is now before reenabling 32-bit userspace.
      * Bonus patch to vcpu_show_execution_state which I noticed while
        digging around.

Ian.

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

[PATCH v4 01/15] 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 v4 02/15] 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>
---
v4: moved earlier in series. This will need backporting as a basis for
other patches.
---
 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 v4 03/15] 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).

The usermode accessibility check is rather repetitive, so a helper
macro is introduced.

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>
---
This should be backported

v4: Moved earlier in series. Use a helper macro to check for allowed
accesses
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           |   43 +++++++++++++++++++++++++--------------
 xen/include/asm-arm/processor.h |    6 ++++++
 3 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index c5f65db..3d76cd1 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 4427ae5..38841ca 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -31,6 +31,14 @@
 extern s_time_t ticks_to_ns(uint64_t ticks);
 extern uint64_t ns_to_ticks(s_time_t ns);
 
+/*
+ * Check if regs is allowed access, user_gate is tail end of a
+ * CNTKCTL_EL1_ bit name which gates user access
+ */
+#define ACCESS_ALLOWED(regs, user_gate) \
+    ( !psr_mode_is_user(regs) || \
+      (READ_SYSREG(CNTKCTL_EL1) & CNTKCTL_EL1_##user_gate) )
+
 static void phys_timer_expired(void *data)
 {
     struct vtimer *t = data;
@@ -154,9 +162,13 @@ 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 ( !ACCESS_ALLOWED(regs, EL0PTEN) )
+        return 0;
+
     if ( read )
     {
         *r = v->arch.phys_timer.ctl;
@@ -176,13 +188,17 @@ 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 ( !ACCESS_ALLOWED(regs, EL0PTEN) )
+        return 0;
+
     now = NOW() - v->domain->arch.phys_timer_base.offset;
 
     if ( read )
@@ -200,6 +216,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)
@@ -210,6 +227,8 @@ static int vtimer_cntpct(struct cpu_user_regs *regs, uint64_t *r, int read)
 
     if ( read )
     {
+        if ( !ACCESS_ALLOWED(regs, EL0PCTEN) )
+            return 0;
         now = NOW() - v->domain->arch.phys_timer_base.offset;
         ticks = ns_to_ticks(now);
         *r = ticks;
@@ -236,12 +255,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;
@@ -263,7 +280,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 )
@@ -293,12 +310,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;
@@ -318,17 +337,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..b7e88a6 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -558,6 +558,12 @@ union hsr {
 #define CNTHCTL_PA      (1u<<0)  /* Kernel/user access to physical counter */
 #define CNTHCTL_TA      (1u<<1)  /* Kernel/user access to CNTP timer */
 
+/* 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 v4 03/15] xen: arm: correctly handle vtimer traps from userspace

[Julien Grall]

Hi Ian,

On 27/03/15 14:33, 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.
>  - 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).
> 
> The usermode accessibility check is rather repetitive, so a helper
> macro is introduced.
> 
> 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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

[PATCH v4 04/15] 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         |   48 +++++++++++++++++++++++++++++++++++------
 xen/include/asm-arm/sysregs.h |    3 ++-
 3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 3d76cd1..cc34a0a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1660,6 +1660,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) )
             goto undef_cp15_64;
         break;
@@ -1842,6 +1843,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) )
             goto undef_sysreg;
         break;
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 38841ca..7521628 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -219,6 +219,30 @@ static int vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r, int read)
     return 1;
 }
 
+static int vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r, int read)
+{
+    struct vcpu *v = current;
+
+    if ( !ACCESS_ALLOWED(regs, 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;
@@ -270,7 +294,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);
@@ -282,17 +306,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
@@ -322,6 +353,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 v4 04/15] xen: arm: handle accesses to CNTP_CVAL_EL0

[Julien Grall]

Hi Ian,

On 27/03/15 14:33, 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

[PATCH v4 05/15] 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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
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 b7e88a6..5ccf61f 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 */
 
 /* Time counter kernel control register */
 #define CNTKCTL_EL1_EL0PCTEN (1u<<0) /* Expose phys counters to EL0 */
-- 
1.7.10.4

[PATCH v4 06/15] 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 cc34a0a..c5ffcc5 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 v4 07/15] xen: arm: do not handle traps accessing CLIDR_EL1 or CCSIDR_EL1

[Ian Campbell]

They are trapped only with HCR_EL2.TID2 which we don't set, and in any
case we handled only for 32-bit.

One day we may want to trap and emulate these, but for now don't
bother with the dead code.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index c5ffcc5..f35b7c3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1564,24 +1564,6 @@ static void do_cp15_32(struct cpu_user_regs *regs,
 
     switch ( hsr.bits & HSR_CP32_REGS_MASK )
     {
-    case HSR_CPREG32(CLIDR):
-        if ( !cp32.read )
-        {
-            dprintk(XENLOG_ERR,
-                    "attempt to write to read-only register CLIDR\n");
-            domain_crash_synchronous();
-        }
-        *r = READ_SYSREG32(CLIDR_EL1);
-        break;
-    case HSR_CPREG32(CCSIDR):
-        if ( !cp32.read )
-        {
-            dprintk(XENLOG_ERR,
-                    "attempt to write to read-only register CCSIDR\n");
-            domain_crash_synchronous();
-        }
-        *r = READ_SYSREG32(CCSIDR_EL1);
-        break;
     case HSR_CPREG32(DCCISW):
         if ( cp32.read )
         {
-- 
1.7.10.4

Re: [PATCH v4 07/15] xen: arm: do not handle traps accessing CLIDR_EL1 or CCSIDR_EL1

[Julien Grall]

Hi Ian,

On 27/03/15 14:33, Ian Campbell wrote:
> They are trapped only with HCR_EL2.TID2 which we don't set, and in any
> case we handled only for 32-bit.
> 
> One day we may want to trap and emulate these, but for now don't
> bother with the dead code.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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

Regards,

-- 
Julien Grall

[PATCH v4 08/15] xen: arm: don't pretend to handle cache maintenance by set/way

[Ian Campbell]

We set HCR_EL2.TSW but only (sort of) handle 32-bit access to DCCISW
but not the other two registers, nor any 64-bit access. Add handlers
for all of these.

The existing handling of DCCISW was to simply propagate the action to
the host level. This can't really work on an SMP system (due to
speculation from other cores) and is even worse under virtualisation
(imagine a VCPU changing PCPU half way through a set/way loop).

Since set/way operations are not really usable under any hypervisor
don't even try, instead for writes log a guest debug message and
inject an under exception.

These registers are all WO, so continue to kill the guest on read from
EL1.

For accesses from EL0 just silently inject undef.

In do_sysreg promote the sysreg variable to the top level and use
throughout.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v4: New patch
---
 xen/arch/arm/traps.c          |   57 +++++++++++++++++++++++++++++++++--------
 xen/include/asm-arm/sysregs.h |    4 +++
 xen/include/public/arch-arm.h |    5 ++++
 3 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f35b7c3..10eb3e6 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1564,19 +1564,28 @@ static void do_cp15_32(struct cpu_user_regs *regs,
 
     switch ( hsr.bits & HSR_CP32_REGS_MASK )
     {
+    case HSR_CPREG32(DCISW):
+    case HSR_CPREG32(DCCSW):
     case HSR_CPREG32(DCCISW):
+        if ( psr_mode_is_user(regs) )
+            goto undef_cp15_32;
+
+        gdprintk(XENLOG_ERR,
+                 "Unhandled set/way operation "
+                 "p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister", "
+                 "rn = %"PRIx32"\n",
+                 cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2,
+                 regs->pc, *r);
+
         if ( cp32.read )
         {
             dprintk(XENLOG_ERR,
-                    "attempt to read from write-only register DCCISW\n");
+                    "attempt to read from write-only register\n");
             domain_crash_synchronous();
         }
-#ifdef CONFIG_ARM_32
-        WRITE_CP32(*r, DCCISW);
-#else
-        asm volatile("dc cisw, %0;" : : "r" (*r) : "memory");
-#endif
-        break;
+
+        goto undef_cp15_32;
+
     case HSR_CPREG32(CNTP_CTL):
     case HSR_CPREG32(CNTP_TVAL):
         if ( !vtimer_emulate(regs, hsr) )
@@ -1781,10 +1790,37 @@ static void do_cp(struct cpu_user_regs *regs, union hsr hsr)
 static void do_sysreg(struct cpu_user_regs *regs,
                       union hsr hsr)
 {
-    register_t *x = select_user_reg(regs, hsr.sysreg.reg);
+    struct hsr_sysreg sysreg = hsr.sysreg;
+    register_t *x = select_user_reg(regs, sysreg.reg);
 
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
+
+    /* Set/way registers */
+    case HSR_SYSREG_DCISW:
+    case HSR_SYSREG_DCCSW:
+    case HSR_SYSREG_DCCISW:
+        if ( psr_mode_is_user(regs) )
+            goto undef_sysreg;
+
+        gdprintk(XENLOG_ERR,
+                 "Unhandled set/way operation "
+                 "%d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister", "
+                 "xn = %"PRIx64"\n",
+                 sysreg.op0, sysreg.op1,
+                 sysreg.crn, sysreg.crm,
+                 sysreg.op2,
+                 sysreg.read ? "=>" : "<=",
+                 sysreg.reg, regs->pc, *x);
+        if ( sysreg.read )
+        {
+            dprintk(XENLOG_ERR,
+                    "attempt to read from write-only register\n");
+            domain_crash_synchronous();
+        }
+
+        goto undef_sysreg;
+
     /* RAZ/WI registers: */
     /*  - Debug */
     case HSR_SYSREG_MDSCR_EL1:
@@ -1812,14 +1848,14 @@ static void do_sysreg(struct cpu_user_regs *regs,
     HSR_SYSREG_DBG_CASES(DBGWCR):
     /* - Double Lock Register */
     case HSR_SYSREG_OSDLR_EL1:
-        if ( hsr.sysreg.read )
+        if ( sysreg.read )
             *x = 0;
         /* else: write ignored */
         break;
 
     /* Write only, Write ignore registers: */
     case HSR_SYSREG_OSLAR_EL1:
-        if ( hsr.sysreg.read )
+        if ( sysreg.read )
             goto bad_sysreg;
         /* else: write ignored */
         break;
@@ -1847,7 +1883,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
  bad_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",
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index df8e070..2e256cc 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -40,6 +40,10 @@
     ((__HSR_SYSREG_##crm) << HSR_SYSREG_CRM_SHIFT) | \
     ((__HSR_SYSREG_##op2) << HSR_SYSREG_OP2_SHIFT)
 
+#define HSR_SYSREG_DCISW          HSR_SYSREG(1,0,c7,c6,2)
+#define HSR_SYSREG_DCCSW          HSR_SYSREG(1,0,c7,c10,2)
+#define HSR_SYSREG_DCCISW         HSR_SYSREG(1,0,c7,c14,2)
+
 #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)
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index c2dcb66..cf3d6cc 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -161,6 +161,11 @@
  *
  * - The device tree Xen compatible node is fully described under Linux
  *   at Documentation/devicetree/bindings/arm/xen.txt.
+ *
+ * - Cache maintenaince operations by set/way ("dc isw|cisw|csw" and
+ *   the equivalent cp15 registers) are not available when running
+ *   under Xen and will result in an undefined instruction exception
+ *   delivered to the guest.
  */
 
 #define XEN_HYPERCALL_TAG   0XEA1
-- 
1.7.10.4

Re: [PATCH v4 08/15] xen: arm: don't pretend to handle cache maintenance by set/way

[Julien Grall]

Hi Ian,

On 27/03/15 14:33, Ian Campbell wrote:
> We set HCR_EL2.TSW but only (sort of) handle 32-bit access to DCCISW
> but not the other two registers, nor any 64-bit access. Add handlers
> for all of these.

We don't set HCR_EL2.TSW so DCCISW is not trapped.

> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index c2dcb66..cf3d6cc 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -161,6 +161,11 @@
>   *
>   * - The device tree Xen compatible node is fully described under Linux
>   *   at Documentation/devicetree/bindings/arm/xen.txt.
> + *
> + * - Cache maintenaince operations by set/way ("dc isw|cisw|csw" and
> + *   the equivalent cp15 registers) are not available when running
> + *   under Xen and will result in an undefined instruction exception
> + *   delivered to the guest.
>   */

set/way operations is used by Linux ARM32 in order to flush all the
cache. Injecting an undefined instruction would make guest unusable.

Regards,

-- 
Julien Grall

Re: [PATCH v4 08/15] xen: arm: don't pretend to handle cache maintenance by set/way

[Ian Campbell]

On Fri, 2015-03-27 at 16:36 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 27/03/15 14:33, Ian Campbell wrote:
> > We set HCR_EL2.TSW but only (sort of) handle 32-bit access to DCCISW
> > but not the other two registers, nor any 64-bit access. Add handlers
> > for all of these.
> 
> We don't set HCR_EL2.TSW so DCCISW is not trapped.

I was completely sure we did, but I was wrong.

> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index c2dcb66..cf3d6cc 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -161,6 +161,11 @@
> >   *
> >   * - The device tree Xen compatible node is fully described under Linux
> >   *   at Documentation/devicetree/bindings/arm/xen.txt.
> > + *
> > + * - Cache maintenaince operations by set/way ("dc isw|cisw|csw" and

I've just spotted a typo here, which I have fixed in my tree.

> > + *   the equivalent cp15 registers) are not available when running
> > + *   under Xen and will result in an undefined instruction exception
> > + *   delivered to the guest.
> >   */
> 
> set/way operations is used by Linux ARM32 in order to flush all the
> cache. Injecting an undefined instruction would make guest unusable.

Yes, that's a shame. AIUI operations by set/way aren't really very
useful under virt. See ARM v7 B1.14.4.

AIUI2 the reasons Linux does those flushes are due to bootloader's
dirtying of cachelines, which we take great care to avoid in our domain
builder, so I _think_ they probably don't need this under Xen, but have
no easy way to know that at the point they do them. Perhaps it would be
needed for a bootloader run under Xen to do this, I think that would
come under "things to fix when paravirtualaising a bootloader"

So I think we have a few options:

     1. Continue to not trap set/way operations. Guests will be able to
        flush the entire host cache by set/way. I don't think this is a
        good idea to keep allowing as a general principal.
     2. Trap set/way operations and do one of:
             1. Inject #undef
             2. Silently ignore
             3. Ignore with a debug level print of some sort
             4. Try to do some sort of useful operation.
                
I don't think #1 is a very good idea, and we've essentially ruled out
#2.1 (essentially this patch + enable the trap) here I think.

I've absolutely no idea what #2.4 might be.

So I think we are down to trap and ignore either with or without
logging.

Given the caveats with s/w under a hypervisor knowing about it would be
nice. The logging would be a few dozen (nr_sets*nr_ways) on each guest
boot, so would have to be a debug level log, but it wouldn't be terribly
spammy.

On the other hand, there is no way for a kernel to know it can not
bother with these, so those log messages will always be there and any
problematic uses won't be noticeable anyway.

So I am probably leaning towards #2.2

The KVM approach appears to be to flush the entire guest RAM space on
the first s/w operation and set HCR_EL2.TVM and then ignore all
subsequent s/w ops until caches are enabled, at which point they disable
HCR_EL2.TVM and go back to normal until the next s/w op. This catches
the actual expected use of s/w which is when enabling/disabling caches.

Something similar might work for us too actually. Maybe we could go with
#2.2 in the short term and plan to do the more complex thing later?

Ian.

Re: [PATCH v4 08/15] xen: arm: don't pretend to handle cache maintenance by set/way

[Julien Grall]

Hi Ian,

On 27/03/15 17:05, Ian Campbell wrote:
> On Fri, 2015-03-27 at 16:36 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 27/03/15 14:33, Ian Campbell wrote:
>>> We set HCR_EL2.TSW but only (sort of) handle 32-bit access to DCCISW
>>> but not the other two registers, nor any 64-bit access. Add handlers
>>> for all of these.
>>
>> We don't set HCR_EL2.TSW so DCCISW is not trapped.
> 
> I was completely sure we did, but I was wrong.
> 
>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>>> index c2dcb66..cf3d6cc 100644
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -161,6 +161,11 @@
>>>   *
>>>   * - The device tree Xen compatible node is fully described under Linux
>>>   *   at Documentation/devicetree/bindings/arm/xen.txt.
>>> + *
>>> + * - Cache maintenaince operations by set/way ("dc isw|cisw|csw" and
> 
> I've just spotted a typo here, which I have fixed in my tree.
> 
>>> + *   the equivalent cp15 registers) are not available when running
>>> + *   under Xen and will result in an undefined instruction exception
>>> + *   delivered to the guest.
>>>   */
>>
>> set/way operations is used by Linux ARM32 in order to flush all the
>> cache. Injecting an undefined instruction would make guest unusable.
> 
> Yes, that's a shame. AIUI operations by set/way aren't really very
> useful under virt. See ARM v7 B1.14.4.
> 
> AIUI2 the reasons Linux does those flushes are due to bootloader's
> dirtying of cachelines, which we take great care to avoid in our domain
> builder, so I _think_ they probably don't need this under Xen, but have
> no easy way to know that at the point they do them. Perhaps it would be
> needed for a bootloader run under Xen to do this, I think that would
> come under "things to fix when paravirtualaising a bootloader"

Technically we already support PV bootloader such as UEFI.

> 
> So I think we have a few options:
> 
>      1. Continue to not trap set/way operations. Guests will be able to
>         flush the entire host cache by set/way. I don't think this is a
>         good idea to keep allowing as a general principal.
>      2. Trap set/way operations and do one of:
>              1. Inject #undef
>              2. Silently ignore
>              3. Ignore with a debug level print of some sort
>              4. Try to do some sort of useful operation.
>                 
> I don't think #1 is a very good idea, and we've essentially ruled out
> #2.1 (essentially this patch + enable the trap) here I think.
> 
> I've absolutely no idea what #2.4 might be.
> 
> So I think we are down to trap and ignore either with or without
> logging.
> 
> Given the caveats with s/w under a hypervisor knowing about it would be
> nice. The logging would be a few dozen (nr_sets*nr_ways) on each guest
> boot, so would have to be a debug level log, but it wouldn't be terribly
> spammy.
> 
> On the other hand, there is no way for a kernel to know it can not
> bother with these, so those log messages will always be there and any
> problematic uses won't be noticeable anyway.
> 
> So I am probably leaning towards #2.2
> 
> The KVM approach appears to be to flush the entire guest RAM space on
> the first s/w operation and set HCR_EL2.TVM and then ignore all
> subsequent s/w ops until caches are enabled, at which point they disable
> HCR_EL2.TVM and go back to normal until the next s/w op. This catches
> the actual expected use of s/w which is when enabling/disabling caches.
> 
> Something similar might work for us too actually. Maybe we could go with
> #2.2 in the short term and plan to do the more complex thing later?

I'm ok with the #2.2 as a temporary solution. Although I think it should
be properly fixed for Xen 4.6.

Would it be worth to open a bug on the tracker and mark as a blocker?

Regards,

-- 
Julien Grall

Re: [PATCH v4 08/15] xen: arm: don't pretend to handle cache maintenance by set/way

[Ian Campbell]

create <5515870C.1080702@linaro.org>
title it proper handling of set/way cache maintenance instructions by guests on ARM
thanks

On Mon, 2015-03-30 at 13:17 +0100, Julien Grall wrote:
> Would it be worth to open a bug on the tracker and mark as a blocker?

Yes, done.

Ian.

Processed: Re: [PATCH v4 08/15] xen: arm: don't pretend to handle cache maintenance by set/way

[xen]

Processing commands for xen@bugs.xenproject.org:

> create <5515870C.1080702@linaro.org>
Created new bug #50 rooted at `<5515870C.1080702@linaro.org>'
Title: `Re: [Xen-devel] [PATCH v4 08/15] xen: arm: don't pretend to handle cache maintenance by set/way'
> title it proper handling of set/way cache maintenance instructions by guests on ARM
Set title for #50 to `proper handling of set/way cache maintenance instructions by guests on ARM'
> thanks
Finished processing.

Modified/created Bugs:
 - 50: http://bugs.xenproject.org/xen/bug/50 (new)

---
Xen Hypervisor Bug Tracker
See http://wiki.xen.org/wiki/Reporting_Bugs_against_Xen for information on reporting bugs
Contact xen-bugs-owner@bugs.xenproject.org with any infrastructure issues

[PATCH v4 09/15] 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).

PMINTENSET, PMINTENCLR are EL1 only, but it is not clear whether
attempts to access from EL0 will trap to EL1 or EL2, be conservative
and handle EL0 access with an undef injection.

ACTLR is EL1 only and the ARM ARM states that HCR_EL2.TACR causes
accesses from EL1 to trap. However remain conservative even here and
handle accesses from EL0 by injecting an undef injection.

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.

HSR_EC_CP15_32 should never be seen from a 64-bit guest, so BUG_ON if
that occurs.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v4:
- Handle the possibility of PMINTEN{SET,CLR}_EL1, ACTLR trapping from
  EL0 as well, by injecting an undef.
- No longer handle/mention CLIDR, CCSIDR, DCCISW.
---
 xen/arch/arm/traps.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 10eb3e6..0611fbe 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1592,6 +1592,8 @@ static void do_cp15_32(struct cpu_user_regs *regs,
             goto undef_cp15_32;
         break;
     case HSR_CPREG32(ACTLR):
+        if ( psr_mode_is_user(regs) )
+            goto undef_cp15_32;
         if ( cp32.read )
            *r = v->arch.actlr;
         break;
@@ -1604,6 +1606,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, however MDCR_EL2.TPM==1 means EL0 may trap here also. */
+        if ( psr_mode_is_user(regs) )
+            goto undef_cp15_32;
+        goto cp15_32_raz_wi;
     case HSR_CPREG32(PMCR):
     case HSR_CPREG32(PMCNTENSET):
     case HSR_CPREG32(PMCNTENCLR):
@@ -1615,12 +1629,17 @@ 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;
+ cp15_32_raz_wi:
         if ( cp32.read )
             *r = 0;
+        /* else: write ignored */
         break;
 
     default:
@@ -2066,8 +2085,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 v4 09/15] xen: arm: Handle CP15 register traps from userspace

[Julien Grall]

Hi Ian,

On 27/03/15 14:33, 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).
> 
> PMINTENSET, PMINTENCLR are EL1 only, but it is not clear whether
> attempts to access from EL0 will trap to EL1 or EL2, be conservative
> and handle EL0 access with an undef injection.
> 
> ACTLR is EL1 only and the ARM ARM states that HCR_EL2.TACR causes
> accesses from EL1 to trap. However remain conservative even here and
> handle accesses from EL0 by injecting an undef injection.
> 
> 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.
> 
> HSR_EC_CP15_32 should never be seen from a 64-bit guest, so BUG_ON if
> that occurs.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

[PATCH v4 10/15] 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 WO 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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
v4: Correct commit msg, DBGOSLAR is WO
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 0611fbe..6d82c72 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1709,10 +1709,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
@@ -1725,15 +1727,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):
@@ -1741,13 +1756,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",
@@ -1756,6 +1780,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;
     }
@@ -2095,8 +2120,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 v4 11/15] 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.

It is not entirely clear whether attempts to access *_EL1 registers
from EL0 will trap to EL1 or EL2, be conservative and treat as an
undef injection.

PMUSERENR_EL0 and MDCCSR_EL0 are R/O to EL0. MDCCSR_EL0 was previously
not handled at all.

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>
---
v4:
- Handle the possibility of EL1 trapping from EL0 as well, by
  injecting an undef, since the docs are not completely unambiguous
  about this possibility.
---
 xen/arch/arm/traps.c          |   55 +++++++++++++++++++++++++++++++----------
 xen/include/asm-arm/sysregs.h |    1 +
 2 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6d82c72..9a4deef 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1868,9 +1868,42 @@ static void do_sysreg(struct cpu_user_regs *regs,
     /* RAZ/WI registers: */
     /*  - Debug */
     case HSR_SYSREG_MDSCR_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:
     /*  - Perf monitors */
     case HSR_SYSREG_PMINTENSET_EL1:
     case HSR_SYSREG_PMINTENCLR_EL1:
+        /*
+         * Accessible from EL1 only, but if EL0 trap happens handle as
+         * undef.
+         */
+        if ( psr_mode_is_user(regs) )
+            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;
+
+    /* - Perf monitors */
+    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_PMCR_EL0:
     case HSR_SYSREG_PMCNTENSET_EL0:
     case HSR_SYSREG_PMCNTENCLR_EL0:
@@ -1882,16 +1915,14 @@ 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;
+ sysreg_raz_wi:
         if ( sysreg.read )
             *x = 0;
         /* else: write ignored */
@@ -1900,7 +1931,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
     /* Write only, Write ignore registers: */
     case HSR_SYSREG_OSLAR_EL1:
         if ( sysreg.read )
-            goto bad_sysreg;
+            goto undef_sysreg;
         /* else: write ignored */
         break;
     case HSR_SYSREG_CNTP_CTL_EL0:
@@ -1924,7 +1955,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
 
@@ -2166,8 +2196,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 2e256cc..2284fd7 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -47,6 +47,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 v4 11/15] xen: arm: correctly handle sysreg accesses from userspace

[Julien Grall]

Hi Ian,

On 27/03/15 14:33, Ian Campbell wrote:
> Previously we implemented all registers as RAZ/WI even if they
> shouldn't be accessible to userspace.
> 
> It is not entirely clear whether attempts to access *_EL1 registers
> from EL0 will trap to EL1 or EL2, be conservative and treat as an
> undef injection.
> 
> PMUSERENR_EL0 and MDCCSR_EL0 are R/O to EL0. MDCCSR_EL0 was previously
> not handled at all.
> 
> 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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

[PATCH v4 12/15] 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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
v3: Reintroduce accidentally dropped undef injection from smc64 case.
---
 xen/arch/arm/traps.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9a4deef..53062fe 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2155,22 +2155,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);
         break;
     case HSR_EC_HVC32:
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_hvc32);
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
@@ -2182,6 +2182,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 )
@@ -2192,6 +2193,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;
@@ -2218,7 +2220,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 v4 13/15] 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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
v4: This is now only used for HSR decode and no in the individual do_*
    which instead inject undef.
v3: New patch
---
 xen/arch/arm/traps.c |   44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 53062fe..1f52d27 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
@@ -2140,37 +2164,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_undef32_exception(regs);
         break;
     case HSR_EC_HVC32:
-        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_hvc32);
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
@@ -2182,7 +2206,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 )
@@ -2193,12 +2217,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

[PATCH v4 14/15] 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 1f52d27..04ade74 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2133,18 +2133,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 v4 15/15] xen: arm: always omit guest user stack in vcpu_show_execution_state

[Ian Campbell]

Using !usr_mode(regs) only catches arm32 usr mode and not arm64 user
mode, switch to psr_mode_is_user instead.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v4: New patch
---
 xen/arch/arm/traps.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 04ade74..45b4b78 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1056,7 +1056,7 @@ void vcpu_show_execution_state(struct vcpu *v)
     vcpu_pause(v); /* acceptably dangerous */
 
     vcpu_show_registers(v);
-    if ( !usr_mode(&v->arch.cpu_info->guest_cpu_user_regs) )
+    if ( !psr_mode_is_user(&v->arch.cpu_info->guest_cpu_user_regs) )
         show_guest_stack(v, &v->arch.cpu_info->guest_cpu_user_regs);
 
     vcpu_unpause(v);
-- 
1.7.10.4

Re: [PATCH v4 15/15] xen: arm: always omit guest user stack in vcpu_show_execution_state

[Julien Grall]

Hi Ian,

On 27/03/15 14:33, Ian Campbell wrote:
> Using !usr_mode(regs) only catches arm32 usr mode and not arm64 user
> mode, switch to psr_mode_is_user instead.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

It might be a good backport

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

Regards,

-- 
Julien Grall