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

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

[Ian Campbell]

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

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

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

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

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

I've also successfully booted the VM which originally caused XSA-102 to
be discovered (FSO successfully, since the VM image appears to be
broken, but it no longer crashes for 32-on-64 reasons...)

Ian.

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

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

[Ian Campbell]

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

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 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 019991f..c838b70 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1514,8 +1514,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

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

[Julien Grall]

Hi Ian,

On 09/09/14 09:23, Ian Campbell wrote:
> 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>

Regards,

-- 
Julien Grall

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

[Ian Campbell]

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

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

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 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 c838b70..1b1d29f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -451,14 +451,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

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

[Julien Grall]

Hi Ian,

On 09/09/14 09:23, Ian Campbell wrote:
> 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>

Regards,

-- 
Julien Grall

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

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 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 1b1d29f..353e38e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1370,7 +1370,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 );
 
@@ -1379,7 +1379,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;
@@ -1395,10 +1395,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

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

[Julien Grall]

Hi Ian,

On 09/09/14 09:23, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

[PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Ian Campbell]

We have allowed EL1 to access these registers directly for some time
(at least since 4.3.0). They were only ever trapped to support very
early models which had a buggy hypervisor timer, requiring us to use
the phys timer for Xen itself.

In the interests of minimising the patch for the security update just
remove the call to vtimer_emulate and inject an #undef exception. In
practice we will never see any of these traps.

Handle CNTPCT_EL0 explicitly for consistency with CNTPCT on 32-bit.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 353e38e..46ed21d 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1478,13 +1478,8 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         break;
     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();
-        }
-        break;
+        goto undef_cp15_32;
+
     case HSR_CPREG32(ACTLR):
         if ( cp32.read )
            *r = v->arch.actlr;
@@ -1526,6 +1521,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;
     }
@@ -1544,13 +1540,8 @@ static void do_cp15_64(struct cpu_user_regs *regs,
     switch ( hsr.bits & HSR_CP64_REGS_MASK )
     {
     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();
-        }
-        break;
+        goto undef_cp15_64;
+
     default:
         {
 #ifndef NDEBUG
@@ -1563,6 +1554,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;
         }
@@ -1729,18 +1721,13 @@ static void do_sysreg(struct cpu_user_regs *regs,
         break;
     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();
-        }
-        break;
+    case HSR_SYSREG_CNTPCT_EL0:
+        goto undef_sysreg;
     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",
@@ -1753,7 +1740,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;
         }
     }
@@ -1925,8 +1913,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));
         do_cp15_64(regs, hsr);
         break;
     case HSR_EC_CP14_32:
-- 
1.7.10.4

Re: [PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Julien Grall]

Hi Ian,

On 09/09/14 09:23, Ian Campbell wrote:
> We have allowed EL1 to access these registers directly for some time
> (at least since 4.3.0). They were only ever trapped to support very
> early models which had a buggy hypervisor timer, requiring us to use
> the phys timer for Xen itself.
> In the interests of minimising the patch for the security update just
> remove the call to vtimer_emulate and inject an #undef exception. In
> practice we will never see any of these traps.

I disagree with the commit message, a guest may use the physical timer 
rather than the virtual timer. It's the case when a guest doesn't have 
the necessary code to use the virtual timer.

Hence, the guest could decide to let the userspace access to CNTPCT_EL0 
(see CNTKCTL.PL0CTEN). In a such case, the application will be broken on 
Xen guest.

> Handle CNTPCT_EL0 explicitly for consistency with CNTPCT on 32-bit.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c |   37 ++++++++++++-------------------------
>   1 file changed, 12 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 353e38e..46ed21d 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1478,13 +1478,8 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>           break;
>       case HSR_CPREG32(CNTP_CTL):
>       case HSR_CPREG32(CNTP_TVAL):
> -        if ( !vtimer_emulate(regs, hsr) )

You dropped every call to vtimer_emulate. It may be interesting to 
remove the related code in vtimer.c

Regards,

-- 
Julien Grall

Re: [PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Ian Campbell]

On Tue, 2014-09-09 at 16:31 -0700, Julien Grall wrote:
> Hi Ian,
> 
> On 09/09/14 09:23, Ian Campbell wrote:
> > We have allowed EL1 to access these registers directly for some time
> > (at least since 4.3.0). They were only ever trapped to support very
> > early models which had a buggy hypervisor timer, requiring us to use
> > the phys timer for Xen itself.
> > In the interests of minimising the patch for the security update just
> > remove the call to vtimer_emulate and inject an #undef exception. In
> > practice we will never see any of these traps.
> 
> I disagree with the commit message, a guest may use the physical timer 
> rather than the virtual timer. It's the case when a guest doesn't have 
> the necessary code to use the virtual timer.

I think you've misunderstood. The guest is allowed direct access to the
physical timer ever since we removed the workaround for the buggy
hypervisor timer on the models. Hence we are never trapping these
registers anyway. Probably I should go further here and actually remove
all the phys timer emulation support from vtimer.c.

> Hence, the guest could decide to let the userspace access to CNTPCT_EL0 
> (see CNTKCTL.PL0CTEN). In a such case, the application will be broken on 
> Xen guest.
> 
> > Handle CNTPCT_EL0 explicitly for consistency with CNTPCT on 32-bit.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c |   37 ++++++++++++-------------------------
> >   1 file changed, 12 insertions(+), 25 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 353e38e..46ed21d 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1478,13 +1478,8 @@ static void do_cp15_32(struct cpu_user_regs *regs,
> >           break;
> >       case HSR_CPREG32(CNTP_CTL):
> >       case HSR_CPREG32(CNTP_TVAL):
> > -        if ( !vtimer_emulate(regs, hsr) )
> 
> You dropped every call to vtimer_emulate. It may be interesting to 
> remove the related code in vtimer.c

Yes, I didn't do that when this was going to be a security update to
keep the size of the patch down, but I should do so now though.

Ian.

Re: [PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Julien Grall]

Hi Ian,

On 10/09/14 02:46, Ian Campbell wrote:
> On Tue, 2014-09-09 at 16:31 -0700, Julien Grall wrote:
>> Hi Ian,
>>
>> On 09/09/14 09:23, Ian Campbell wrote:
>>> We have allowed EL1 to access these registers directly for some time
>>> (at least since 4.3.0). They were only ever trapped to support very
>>> early models which had a buggy hypervisor timer, requiring us to use
>>> the phys timer for Xen itself.
>>> In the interests of minimising the patch for the security update just
>>> remove the call to vtimer_emulate and inject an #undef exception. In
>>> practice we will never see any of these traps.
>>
>> I disagree with the commit message, a guest may use the physical timer
>> rather than the virtual timer. It's the case when a guest doesn't have
>> the necessary code to use the virtual timer.
>
> I think you've misunderstood. The guest is allowed direct access to the
> physical timer ever since we removed the workaround for the buggy
> hypervisor timer on the models. Hence we are never trapping these
> registers anyway. Probably I should go further here and actually remove
> all the phys timer emulation support from vtimer.c.

Are you sure? In init_interrupt_timer (xen/arch/arm/timer.c) we disable 
the access to the physical timer to the guest. See 
WRITE_SYSREG32(CNTHCTL_PA, CNTHCTL_EL2).

Hence, I don't see any save/restore for the physical timer in 
xen/arch/arm/vtimer.c. I only see them for the virtual timer.

Regards,

-- 
Julien Grall

Re: [PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Ian Campbell]

On Wed, 2014-09-10 at 11:54 -0700, Julien Grall wrote:
> Hi Ian,
> 
> On 10/09/14 02:46, Ian Campbell wrote:
> > On Tue, 2014-09-09 at 16:31 -0700, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 09/09/14 09:23, Ian Campbell wrote:
> >>> We have allowed EL1 to access these registers directly for some time
> >>> (at least since 4.3.0). They were only ever trapped to support very
> >>> early models which had a buggy hypervisor timer, requiring us to use
> >>> the phys timer for Xen itself.
> >>> In the interests of minimising the patch for the security update just
> >>> remove the call to vtimer_emulate and inject an #undef exception. In
> >>> practice we will never see any of these traps.
> >>
> >> I disagree with the commit message, a guest may use the physical timer
> >> rather than the virtual timer. It's the case when a guest doesn't have
> >> the necessary code to use the virtual timer.
> >
> > I think you've misunderstood. The guest is allowed direct access to the
> > physical timer ever since we removed the workaround for the buggy
> > hypervisor timer on the models. Hence we are never trapping these
> > registers anyway. Probably I should go further here and actually remove
> > all the phys timer emulation support from vtimer.c.
> 
> Are you sure? In init_interrupt_timer (xen/arch/arm/timer.c) we disable 
> the access to the physical timer to the guest. See 
> WRITE_SYSREG32(CNTHCTL_PA, CNTHCTL_EL2).

Hrm, I mistakenly thought that was enabling them, but we do indeed need
to set a second bit there, this only allows access to the counter, not
the control registers. I'll take another look.

> Hence, I don't see any save/restore for the physical timer in 
> xen/arch/arm/vtimer.c. I only see them for the virtual timer.
> 
> Regards,
> 

Re: [PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Ian Campbell]

On Thu, 2014-09-11 at 09:43 +0100, Ian Campbell wrote:
> On Wed, 2014-09-10 at 11:54 -0700, Julien Grall wrote:
> > Hi Ian,
> > 
> > On 10/09/14 02:46, Ian Campbell wrote:
> > > On Tue, 2014-09-09 at 16:31 -0700, Julien Grall wrote:
> > >> Hi Ian,
> > >>
> > >> On 09/09/14 09:23, Ian Campbell wrote:
> > >>> We have allowed EL1 to access these registers directly for some time
> > >>> (at least since 4.3.0). They were only ever trapped to support very
> > >>> early models which had a buggy hypervisor timer, requiring us to use
> > >>> the phys timer for Xen itself.
> > >>> In the interests of minimising the patch for the security update just
> > >>> remove the call to vtimer_emulate and inject an #undef exception. In
> > >>> practice we will never see any of these traps.
> > >>
> > >> I disagree with the commit message, a guest may use the physical timer
> > >> rather than the virtual timer. It's the case when a guest doesn't have
> > >> the necessary code to use the virtual timer.

I was just about to dive back into this and was thinking: Is a guest
which uses the phys timer something we actually wish to support? We
already require the guest to paravirtualise other aspects of its life,
and requiring vtimer doesn't seem to step outside that boundary.

Supporting the ptimer is going to take some effort as well as the
existing code+overhead in Xen.

Do we know of any existing supported OSes which use the phys timer?

I suppose we should consider access to the counter separately from
access to the registers which allow event generation -- I could
plausibly be convinced that a guest should be able to read both phys and
virt timers (for e.g. stolen time type reasons), but only configure virt
events.

Ian.

> > >
> > > I think you've misunderstood. The guest is allowed direct access to the
> > > physical timer ever since we removed the workaround for the buggy
> > > hypervisor timer on the models. Hence we are never trapping these
> > > registers anyway. Probably I should go further here and actually remove
> > > all the phys timer emulation support from vtimer.c.
> > 
> > Are you sure? In init_interrupt_timer (xen/arch/arm/timer.c) we disable 
> > the access to the physical timer to the guest. See 
> > WRITE_SYSREG32(CNTHCTL_PA, CNTHCTL_EL2).
> 
> Hrm, I mistakenly thought that was enabling them, but we do indeed need
> to set a second bit there, this only allows access to the counter, not
> the control registers. I'll take another look.
> 
> > Hence, I don't see any save/restore for the physical timer in 
> > xen/arch/arm/vtimer.c. I only see them for the virtual timer.
> > 
> > Regards,
> > 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Julien Grall]

Hi Ian,

On 14/01/15 16:33, Ian Campbell wrote:
> On Thu, 2014-09-11 at 09:43 +0100, Ian Campbell wrote:
>> On Wed, 2014-09-10 at 11:54 -0700, Julien Grall wrote:
>>> Hi Ian,
>>>
>>> On 10/09/14 02:46, Ian Campbell wrote:
>>>> On Tue, 2014-09-09 at 16:31 -0700, Julien Grall wrote:
>>>>> Hi Ian,
>>>>>
>>>>> On 09/09/14 09:23, Ian Campbell wrote:
>>>>>> We have allowed EL1 to access these registers directly for some time
>>>>>> (at least since 4.3.0). They were only ever trapped to support very
>>>>>> early models which had a buggy hypervisor timer, requiring us to use
>>>>>> the phys timer for Xen itself.
>>>>>> In the interests of minimising the patch for the security update just
>>>>>> remove the call to vtimer_emulate and inject an #undef exception. In
>>>>>> practice we will never see any of these traps.
>>>>>
>>>>> I disagree with the commit message, a guest may use the physical timer
>>>>> rather than the virtual timer. It's the case when a guest doesn't have
>>>>> the necessary code to use the virtual timer.
> 
> I was just about to dive back into this and was thinking: Is a guest
> which uses the phys timer something we actually wish to support? We
> already require the guest to paravirtualise other aspects of its life,
> and requiring vtimer doesn't seem to step outside that boundary.

Currently if a developer wants to support his OS on Xen, he only has to
add PV drivers (assuming DT support is there). It doesn't have to modify
the core of the OS.

This will be the case with requiring the vtimer. Futhermore, it can be
tricky to implement it on some OS. For instance, the OS may decide to
expose the timer to the user space. Any change would mean recompiler all
the user space for running on Xen.

> Supporting the ptimer is going to take some effort as well as the
> existing code+overhead in Xen.

We already support the physical timer. May I ask, what kind of effort
are necessary?

> Do we know of any existing supported OSes which use the phys timer?

AFAIK no. Even though FreeBSD, while it's not supported, the physical
timer is used by default.

Regards,

-- 
Julien Grall

Re: [PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Ian Campbell]

On Wed, 2015-01-14 at 16:57 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 14/01/15 16:33, Ian Campbell wrote:
> > On Thu, 2014-09-11 at 09:43 +0100, Ian Campbell wrote:
> >> On Wed, 2014-09-10 at 11:54 -0700, Julien Grall wrote:
> >>> Hi Ian,
> >>>
> >>> On 10/09/14 02:46, Ian Campbell wrote:
> >>>> On Tue, 2014-09-09 at 16:31 -0700, Julien Grall wrote:
> >>>>> Hi Ian,
> >>>>>
> >>>>> On 09/09/14 09:23, Ian Campbell wrote:
> >>>>>> We have allowed EL1 to access these registers directly for some time
> >>>>>> (at least since 4.3.0). They were only ever trapped to support very
> >>>>>> early models which had a buggy hypervisor timer, requiring us to use
> >>>>>> the phys timer for Xen itself.
> >>>>>> In the interests of minimising the patch for the security update just
> >>>>>> remove the call to vtimer_emulate and inject an #undef exception. In
> >>>>>> practice we will never see any of these traps.
> >>>>>
> >>>>> I disagree with the commit message, a guest may use the physical timer
> >>>>> rather than the virtual timer. It's the case when a guest doesn't have
> >>>>> the necessary code to use the virtual timer.
> > 
> > I was just about to dive back into this and was thinking: Is a guest
> > which uses the phys timer something we actually wish to support? We
> > already require the guest to paravirtualise other aspects of its life,
> > and requiring vtimer doesn't seem to step outside that boundary.
> 
> Currently if a developer wants to support his OS on Xen, he only has to
> add PV drivers (assuming DT support is there). It doesn't have to modify
> the core of the OS.
> 
> This will be the case with requiring the vtimer. Futhermore, it can be
> tricky to implement it on some OS. For instance, the OS may decide to
> expose the timer to the user space. Any change would mean recompiler all
> the user space for running on Xen.
> 
> > Supporting the ptimer is going to take some effort as well as the
> > existing code+overhead in Xen.
> 
> We already support the physical timer. May I ask, what kind of effort
> are necessary?

It needs a bunch of refactoring to handle the sysreg vs cp-reg aspects
in order to support 32 bit userspace on top of 64 bit guest kernel,
which is a lot of faff.

But I think the better approach would be to fix the vtimer mask-on-irq
thing (by using GICD I[SC]ACTIVER to context switch the state) and then
apply the same principal to the ptimer and do proper context switching
-- i.e. allow guests to access the physical timers directly.

I'm looking into the vtimer masking and ctxt switching the irq state
now, once I've got that going exposing the ptimer directly to guests
ought to be pretty simple.

> > Do we know of any existing supported OSes which use the phys timer?
> 
> AFAIK no. Even though FreeBSD, while it's not supported, the physical
> timer is used by default.

Sounds to me like the phys timer is a defacto part of our ABI, then,
since something in the real world is using it, which justifies the
effort in supporting it.

Ian.

Re: [PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Julien Grall]

Hi Ian,

On 15/01/15 10:26, Ian Campbell wrote:
> I'm looking into the vtimer masking and ctxt switching the irq state
> now, once I've got that going exposing the ptimer directly to guests
> ought to be pretty simple.

I don't think we can expose directly the physical timer to the guest.

If the guest tries to read CNTPCT_EL0, it will read the number of ticks
since the host is up.

Regards,

-- 
Julien Grall

Re: [PATCH 4/9] xen: arm: turn vtimer traps for cp32/64 and sysreg into #undef

[Ian Campbell]

On Thu, 2015-01-15 at 12:27 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 15/01/15 10:26, Ian Campbell wrote:
> > I'm looking into the vtimer masking and ctxt switching the irq state
> > now, once I've got that going exposing the ptimer directly to guests
> > ought to be pretty simple.
> 
> I don't think we can expose directly the physical timer to the guest.
> 
> If the guest tries to read CNTPCT_EL0, it will read the number of ticks
> since the host is up.

Yes, and they should expect that I think, that's the point of the
distinction between v and p timer. Of course maybe by providing it with
an offset until now maybe we've painted ourselves into a corner, which
sucks :-/

Ian.

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

[Ian Campbell]

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

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

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

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

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 46ed21d..e7a2791 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1446,6 +1446,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,
@@ -1455,6 +1456,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,
@@ -1464,6 +1466,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,
@@ -1481,6 +1484,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         goto undef_cp15_32;
 
     case HSR_CPREG32(ACTLR):
+        BUG_ON(psr_mode_is_user(regs));
         if ( cp32.read )
            *r = v->arch.actlr;
         break;
@@ -1493,6 +1497,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):
@@ -1504,12 +1520,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:
@@ -1908,8 +1931,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));
         do_cp15_32(regs, hsr);
         break;
     case HSR_EC_CP15_64:
-- 
1.7.10.4

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

[Julien Grall]

Hi Ian,

On 09/09/14 09:23, 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.

In the unlikely case it happens, I don't think it will harm Xen, but 
only the guest. So is the BUG_ON really necessary on these registers?

I think we should use BUG_ON when we know that it will harm the Xen and 
it's not possible to come back from the state.

Regards,


-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2014-09-09 at 16:42 -0700, Julien Grall wrote:
> Hi Ian,
> 
> On 09/09/14 09:23, 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.
> 
> In the unlikely case it happens, I don't think it will harm Xen, but 
> only the guest. So is the BUG_ON really necessary on these registers?
> 
> I think we should use BUG_ON when we know that it will harm the Xen and 
> it's not possible to come back from the state.

AIUI it would be a hardware bug to see these traps in Xen. I think a
BUG_ON is acceptable for such an occurrence.

Ian.

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

[Julien Grall]

On 10/09/14 02:48, Ian Campbell wrote:
> On Tue, 2014-09-09 at 16:42 -0700, Julien Grall wrote:
>> Hi Ian,
>>
>> On 09/09/14 09:23, 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.
>>
>> In the unlikely case it happens, I don't think it will harm Xen, but
>> only the guest. So is the BUG_ON really necessary on these registers?
>>
>> I think we should use BUG_ON when we know that it will harm the Xen and
>> it's not possible to come back from the state.
>
> AIUI it would be a hardware bug to see these traps in Xen. I think a
> BUG_ON is acceptable for such an occurrence.

I would turn into an ASSERT to avoid checking it in non-debug build.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Wed, 2014-09-10 at 11:56 -0700, Julien Grall wrote:
> 
> On 10/09/14 02:48, Ian Campbell wrote:
> > On Tue, 2014-09-09 at 16:42 -0700, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 09/09/14 09:23, 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.
> >>
> >> In the unlikely case it happens, I don't think it will harm Xen, but
> >> only the guest. So is the BUG_ON really necessary on these registers?
> >>
> >> I think we should use BUG_ON when we know that it will harm the Xen and
> >> it's not possible to come back from the state.
> >
> > AIUI it would be a hardware bug to see these traps in Xen. I think a
> > BUG_ON is acceptable for such an occurrence.
> 
> I would turn into an ASSERT to avoid checking it in non-debug build.

BUG_ON is what we normally use in these circumstances. If we did hit
this case we would want to stop, not carry on with hardware in a
potentially unknown/unpredictable state, even for a non-debug build.

Ian.

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

[Ian Campbell]

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

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

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

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

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index e7a2791..01cc3c0 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1600,10 +1600,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
@@ -1616,15 +1618,24 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
         break;
 
     case HSR_CPREG32(DBGDSCRINT):
+        if ( !cp32.read )
+            goto undef_cp14_32;
+
+        *r = 0;
+        break;
+
     case HSR_CPREG32(DBGDSCREXT):
+        if ( usr_mode(regs) )
+            goto undef_cp14_32;
+
         /* Implement debug status and control register as RAZ/WI.
          * The OS won't use Hardware debug if MDBGen not set
          */
         if ( cp32.read )
            *r = 0;
         break;
+
     case HSR_CPREG32(DBGVCR):
-    case HSR_CPREG32(DBGOSLAR):
     case HSR_CPREG32(DBGBVR0):
     case HSR_CPREG32(DBGBCR0):
     case HSR_CPREG32(DBGWVR0):
@@ -1632,13 +1643,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",
@@ -1647,6 +1667,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;
     }
@@ -1939,8 +1960,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));
         do_cp14_32(regs, hsr);
         break;
     case HSR_EC_CP14_DBG:
-- 
1.7.10.4

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

[Julien Grall]

Hi Ian,

On 09/09/14 09:23, Ian Campbell wrote:
> Accesses to these from 32-bit userspace would cause a hypervisor exception
> (host crash) when running a 64-bit kernel, which is worked around by the fix to
> XSA-102. On 32-bit kernels they would be implemented as RAZ/WI which is
> incorrect but harmless.
>
> Update as follows:
>   - DBGDSCRINT should be R/O.
>   - DBGDSCREXT should be EL1 only.
>   - DBGOSLAR is RO and EL1 only.
>   - DBGVCR, DBGB[VC]R*, DBGW[VC]R*, and DBGOSDLR are EL1 only.
>
> DBGDIDR and DBGDSCRINT are accessible from EL0 if DBGDSCRext.UDCCdis. Since we
> emulate that as RAZ/WI we allow access.

Shall we just set DBGDSCRext.UDCCdis to avoid taking care of EL0 access?

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2014-09-09 at 16:45 -0700, Julien Grall wrote:
> Hi Ian,
> 
> On 09/09/14 09:23, Ian Campbell wrote:
> > Accesses to these from 32-bit userspace would cause a hypervisor exception
> > (host crash) when running a 64-bit kernel, which is worked around by the fix to
> > XSA-102. On 32-bit kernels they would be implemented as RAZ/WI which is
> > incorrect but harmless.
> >
> > Update as follows:
> >   - DBGDSCRINT should be R/O.
> >   - DBGDSCREXT should be EL1 only.
> >   - DBGOSLAR is RO and EL1 only.
> >   - DBGVCR, DBGB[VC]R*, DBGW[VC]R*, and DBGOSDLR are EL1 only.
> >
> > DBGDIDR and DBGDSCRINT are accessible from EL0 if DBGDSCRext.UDCCdis. Since we
> > emulate that as RAZ/WI we allow access.
> 
> Shall we just set DBGDSCRext.UDCCdis to avoid taking care of EL0 access?

I'd need to lookup what the acceptable reset states for that bit are,
but perhaps.

Ian.

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

[Ian Campbell]

On Wed, 2014-09-10 at 10:48 +0100, Ian Campbell wrote:
> On Tue, 2014-09-09 at 16:45 -0700, Julien Grall wrote:
> > Hi Ian,
> > 
> > On 09/09/14 09:23, Ian Campbell wrote:
> > > Accesses to these from 32-bit userspace would cause a hypervisor exception
> > > (host crash) when running a 64-bit kernel, which is worked around by the fix to
> > > XSA-102. On 32-bit kernels they would be implemented as RAZ/WI which is
> > > incorrect but harmless.
> > >
> > > Update as follows:
> > >   - DBGDSCRINT should be R/O.
> > >   - DBGDSCREXT should be EL1 only.
> > >   - DBGOSLAR is RO and EL1 only.
> > >   - DBGVCR, DBGB[VC]R*, DBGW[VC]R*, and DBGOSDLR are EL1 only.
> > >
> > > DBGDIDR and DBGDSCRINT are accessible from EL0 if DBGDSCRext.UDCCdis. Since we
> > > emulate that as RAZ/WI we allow access.
> > 
> > Shall we just set DBGDSCRext.UDCCdis to avoid taking care of EL0 access?
> 
> I'd need to lookup what the acceptable reset states for that bit are,
> but perhaps.

The AArch32 version of this bit resets to 0, so I think the code is OK
as it is, at least for now.

I'd like to implement proper handling of dbg registers sooner rather
than later, but I think this series should go in first and the the dbg
stuff can be built on it later.

Ian.
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Julien Grall]

On 10/02/2015 11:40, Ian Campbell wrote:
> On Wed, 2014-09-10 at 10:48 +0100, Ian Campbell wrote:
>> On Tue, 2014-09-09 at 16:45 -0700, Julien Grall wrote:
>>> Hi Ian,
>>>
>>> On 09/09/14 09:23, Ian Campbell wrote:
>>>> Accesses to these from 32-bit userspace would cause a hypervisor exception
>>>> (host crash) when running a 64-bit kernel, which is worked around by the fix to
>>>> XSA-102. On 32-bit kernels they would be implemented as RAZ/WI which is
>>>> incorrect but harmless.
>>>>
>>>> Update as follows:
>>>>    - DBGDSCRINT should be R/O.
>>>>    - DBGDSCREXT should be EL1 only.
>>>>    - DBGOSLAR is RO and EL1 only.
>>>>    - DBGVCR, DBGB[VC]R*, DBGW[VC]R*, and DBGOSDLR are EL1 only.
>>>>
>>>> DBGDIDR and DBGDSCRINT are accessible from EL0 if DBGDSCRext.UDCCdis. Since we
>>>> emulate that as RAZ/WI we allow access.
>>>
>>> Shall we just set DBGDSCRext.UDCCdis to avoid taking care of EL0 access?
>>
>> I'd need to lookup what the acceptable reset states for that bit are,
>> but perhaps.
>
> The AArch32 version of this bit resets to 0, so I think the code is OK
> as it is, at least for now.
>
> I'd like to implement proper handling of dbg registers sooner rather
> than later, but I think this series should go in first and the the dbg
> stuff can be built on it later.

I'm fine with that. Let's have a 32-bit userspace support on 64-bit 
kernel before :).

Regards,


-- 
Julien Grall

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

[Ian Campbell]

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

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

PMUSERENR_EL0 and MDCCSR_EL0 are R/O to EL0.

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

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 01cc3c0..be02c68 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1726,11 +1726,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:
@@ -1742,16 +1771,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 */
@@ -1759,8 +1788,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:
@@ -1768,7 +1798,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
     case HSR_SYSREG_CNTPCT_EL0:
         goto undef_sysreg;
     default:
- bad_sysreg:
         {
 #ifndef NDEBUG
             struct hsr_sysreg sysreg = hsr.sysreg;
@@ -1999,8 +2028,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));
         do_sysreg(regs, hsr);
         break;
 #endif
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index b00871c..0e8c497 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -43,6 +43,7 @@
 #define HSR_SYSREG_MDSCR_EL1      HSR_SYSREG(2,0,c0,c2,2)
 #define HSR_SYSREG_OSLAR_EL1      HSR_SYSREG(2,0,c1,c0,4)
 #define HSR_SYSREG_OSDLR_EL1      HSR_SYSREG(2,0,c1,c3,4)
+#define HSR_SYSREG_MDCCSR_EL0     HSR_SYSREG(2,3,c0,c1,0)
 
 #define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
 #define HSR_SYSREG_DBGBCRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,5)
-- 
1.7.10.4

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

[Ian Campbell]

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

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

After this bad_trap is no longer used.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index be02c68..78017db 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1993,19 +1993,19 @@ 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));
         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));
         do_cp(regs, hsr);
         break;
     case HSR_EC_SMC32:
-        inject_undef32_exception(regs);
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        inject_undef_exception(regs, hsr.len);
         break;
     case HSR_EC_HVC32:
+        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
             return do_debug_trap(regs, hsr.iss & 0x00ff);
@@ -2016,6 +2016,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));
 #ifndef NDEBUG
         if ( (hsr.iss & 0xff00) == 0xff00 )
             return do_debug_trap(regs, hsr.iss & 0x00ff);
@@ -2025,7 +2026,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_trap_hypercall(regs, &regs->x16, hsr.iss);
         break;
     case HSR_EC_SMC64:
-        inject_undef64_exception(regs, hsr.len);
+        BUG_ON(psr_mode_is_32bit(regs->cpsr));
         break;
     case HSR_EC_SYSREG:
         BUG_ON(psr_mode_is_32bit(regs->cpsr));
@@ -2040,7 +2041,6 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_trap_data_abort_guest(regs, hsr);
         break;
     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 9/9] xen: arm: Allow traps from 32 bit userspace on 64 bit hypervisors again

[Ian Campbell]

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

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 78017db..7118a8c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1953,17 +1953,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 ( is_64bit_domain(current->domain) && psr_mode_is_32bit(regs->cpsr) )
-    {
-        inject_undef_exception(regs, hsr.len);
-        return;
-    }
-
     switch (hsr.ec) {
     case HSR_EC_WFI_WFE:
         if ( !check_conditional_instr(regs, hsr) )
-- 
1.7.10.4

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

[Ian Campbell]

On Tue, 2014-09-09 at 17:22 +0100, Ian Campbell wrote:

Not sure why I put RFC in the subject here, it's not, it's an actual
submission of patches.

> 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).
> 
> I've also successfully booted the VM which originally caused XSA-102 to
> be discovered (FSO successfully, since the VM image appears to be
> broken, but it no longer crashes for 32-on-64 reasons...)
> 
> Ian.
> 
> [0] http://xenbits.xen.org/xsa/advisory-102.html