thr3ads.net - Xen devel - [PATCH] xen: arm: inject unhandled instruction and data aborts to the guest. [Dec 2013]

If this information is useful, please help other people find it:
Share via:

Ian Campbell

2013-Dec-09 14:58 UTC

[PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.

Currently an unhandled data abort in guest context leads to us killing the
guest and an unhandled instruction abort in guest context leads to us killing
the host!

Andre pointed out that an unhandled data abort can be caused by e.g. dmidecode
looking for things which are not there in the guests physical address space.
Propagating the fault to the guest allows it to properly SIGSEGV the
processes.

A guest kernel can trivially jump to an unmapped physical address which would
cause an instruction abort. Killing the host for that is obviously bad.
Instead inject the exception so the guest kernel can SIGSEGV or panic() etc as
it deems appropriate.

Tested on arm64 (Mustang) and arm32 (Midway) with a dom0 kernel late_initcall
which either dereferences or jumps to address 0, provoking both behaviours and
resulting correctly in a guest kernel panic. Also tested on fast models with a
32-bit dom0 on a 64-bit hypervisor, which behaved correctly.

In addition tested on both platforms with a userspace program which either
calls to or dereferences address 0. The process is correctly killed with SEGV.

Lastly tested on Mustang with a 32-bit version of the userspace test on a
64-bit dom0 kernel.

I think that covers all the cases.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Andre Przywara <andre.przywara@calxeda.com>
---
Release wise this is a(n important) bug fix.
---
 xen/arch/arm/traps.c            |  199 +++++++++++++++++++++++++++++++++------
 xen/include/asm-arm/cpregs.h    |    1 +
 xen/include/asm-arm/processor.h |   15 ++-
 xen/include/asm-arm/regs.h      |    2 +
 4 files changed, 184 insertions(+), 33 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2a85d37..e4e7f83 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -264,6 +264,8 @@ static void cpsr_switch_mode(struct cpu_user_regs *regs, int
mode)
 
     regs->cpsr |= mode;
     regs->cpsr |= PSR_IRQ_MASK;
+    if (mode == PSR_MODE_ABT)
+        regs->cpsr |= PSR_ABT_MASK;
     if (sctlr & SCTLR_TE)
         regs->cpsr |= PSR_THUMB;
     if (sctlr & SCTLR_EE)
@@ -283,7 +285,7 @@ static vaddr_t exception_handler(vaddr_t offset)
 /* Injects an Undefined Instruction exception into the current vcpu,
  * PC is the exact address of the faulting instruction (without
  * pipeline adjustments). See TakeUndefInstrException pseudocode in
- * ARM.
+ * ARM ARM.
  */
 static void inject_undef32_exception(struct cpu_user_regs *regs)
 {
@@ -305,6 +307,81 @@ static void inject_undef32_exception(struct cpu_user_regs
*regs)
     regs->pc32 = exception_handler(VECTOR32_UND);
 }
 
+/* Injects an Abort exception into the current vcpu, PC is the exact
+ * address of the faulting instruction (without pipeline
+ * adjustments). See TakePrefetchAbortException and
+ * TakeDataAbortException pseudocode in ARM ARM.
+ */
+static void inject_abt32_exception(struct cpu_user_regs *regs,
+                                   int prefetch,
+                                   register_t addr)
+{
+    uint32_t spsr = regs->cpsr;
+    int is_thumb = (regs->cpsr & PSR_THUMB);
+    /* Saved PC points to the instruction past the faulting instruction. */
+    uint32_t return_offset = is_thumb ? 4 : 0;
+    register_t fsr;
+
+    BUG_ON( !is_pv32_domain(current->domain) );
+
+    cpsr_switch_mode(regs, PSR_MODE_ABT);
+
+    /* Update banked registers */
+    regs->spsr_abt = spsr;
+    regs->lr_abt = regs->pc32 + return_offset;
+
+    regs->pc32 = exception_handler(prefetch ? VECTOR32_PABT :
VECTOR32_DABT);
+
+    /* Inject a debug fault, best we can do right now */
+    if ( READ_SYSREG(TCR_EL1) & TTBCR_EAE )
+        fsr = FSR_LPAE | FSRL_STATUS_DEBUG;
+    else
+        fsr = FSRS_FS_DEBUG;
+
+    if ( prefetch )
+    {
+        /* Set IFAR and IFSR */
+#ifdef CONFIG_ARM_32
+        WRITE_SYSREG(addr, IFAR);
+        WRITE_SYSREG(fsr, IFSR);
+#else
+        /* FAR_EL1[63:32] is AArch32 register IFAR */
+        register_t far = READ_SYSREG(FAR_EL1) & 0xffffffffUL;
+        far |= addr << 32;
+        WRITE_SYSREG(far, FAR_EL1);
+        WRITE_SYSREG(fsr, IFSR32_EL2);
+
+#endif
+    }
+    else
+    {
+#ifdef CONFIG_ARM_32
+        /* Set DFAR and DFSR */
+        WRITE_SYSREG(addr, DFAR);
+        WRITE_SYSREG(fsr, DFSR);
+#else
+        /* FAR_EL1[31:0] is AArch32 register DFAR */
+        register_t far = READ_SYSREG(FAR_EL1) & ~0xffffffffUL;
+        far |= addr;
+        WRITE_SYSREG(far, FAR_EL1);
+        /* ESR_EL1 is AArch32 register DFSR */
+        WRITE_SYSREG(fsr, ESR_EL1);
+#endif
+    }
+}
+
+static void inject_dabt32_exception(struct cpu_user_regs *regs,
+                                    register_t addr)
+{
+    inject_abt32_exception(regs, 0, addr);
+}
+
+static void inject_pabt32_exception(struct cpu_user_regs *regs,
+                                    register_t addr)
+{
+    inject_abt32_exception(regs, 1, addr);
+}
+
 #ifdef CONFIG_ARM_64
 /* Inject an undefined exception into a 64 bit guest */
 static void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len)
@@ -326,8 +403,85 @@ static void inject_undef64_exception(struct cpu_user_regs
*regs, int instr_len)
 
     WRITE_SYSREG32(esr.bits, ESR_EL1);
 }
+
+/* Inject an abort exception into a 64 bit guest */
+static void inject_abt64_exception(struct cpu_user_regs *regs,
+                                   int prefetch,
+                                   register_t addr,
+                                   int instr_len)
+{
+    union hsr esr = {
+        .iss = 0,
+        .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) )
+        esr.ec = prefetch
+            ? HSR_EC_INSTR_ABORT_LOWER_EL : HSR_EC_DATA_ABORT_LOWER_EL;
+    else
+        esr.ec = prefetch
+            ? HSR_EC_INSTR_ABORT_CURR_EL : HSR_EC_DATA_ABORT_CURR_EL;
+
+    BUG_ON( is_pv32_domain(current->domain) );
+
+    regs->spsr_el1 = regs->cpsr;
+    regs->elr_el1 = regs->pc;
+
+    regs->cpsr = PSR_MODE_EL1h | PSR_ABT_MASK | PSR_FIQ_MASK | \
+        PSR_IRQ_MASK | PSR_DBG_MASK;
+    regs->pc = READ_SYSREG(VBAR_EL1) + VECTOR64_CURRENT_SPx_SYNC;
+
+    WRITE_SYSREG(addr, FAR_EL1);
+    WRITE_SYSREG32(esr.bits, ESR_EL1);
+}
+
+static void inject_dabt64_exception(struct cpu_user_regs *regs,
+                                   register_t addr,
+                                   int instr_len)
+{
+    inject_abt64_exception(regs, 0, addr, instr_len);
+}
+
+static void inject_iabt64_exception(struct cpu_user_regs *regs,
+                                   register_t addr,
+                                   int instr_len)
+{
+    inject_abt64_exception(regs, 1, addr, instr_len);
+}
+
 #endif
 
+static void inject_iabt_exception(struct cpu_user_regs *regs,
+                                  register_t addr,
+                                  int instr_len)
+{
+        if ( is_pv32_domain(current->domain) )
+            inject_pabt32_exception(regs, addr);
+#ifdef CONFIG_ARM_64
+        else
+            inject_iabt64_exception(regs, addr, instr_len);
+#endif
+}
+
+static void inject_dabt_exception(struct cpu_user_regs *regs,
+                                  register_t addr,
+                                  int instr_len)
+{
+        if ( is_pv32_domain(current->domain) )
+            inject_dabt32_exception(regs, addr);
+#ifdef CONFIG_ARM_64
+        else
+            inject_dabt64_exception(regs, addr, instr_len);
+#endif
+}
+
 struct reg_ctxt {
     /* Guest-side state */
     uint32_t sctlr_el1;
@@ -1315,12 +1469,19 @@ done:
     if (first) unmap_domain_page(first);
 }
 
+
+static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
+                                      union hsr hsr)
+{
+    register_t addr = READ_SYSREG(FAR_EL2);
+    inject_iabt_exception(regs, addr, hsr.len);
+}
+
 static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
                                      union hsr hsr)
 {
     struct hsr_dabt dabt = hsr.dabt;
-    const char *msg;
-    int rc, level = -1;
+    int rc;
     mmio_info_t info;
 
     if ( !check_conditional_instr(regs, hsr) )
@@ -1356,7 +1517,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs
*regs,
         rc = decode_instruction(regs, &info.dabt);
         if ( rc )
         {
-            gdprintk(XENLOG_ERR, "Unable to decode instruction\n");
+            gdprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
             goto bad_data_abort;
         }
     }
@@ -1368,30 +1529,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs
*regs,
     }
 
 bad_data_abort:
-
-    msg = decode_fsc( dabt.dfsc, &level);
-
-    /* XXX inject a suitable fault into the guest */
-    printk("Guest data abort: %s%s%s\n"
-           "    gva=%"PRIvaddr"\n",
-           msg, dabt.s1ptw ? " S2 during S1" : "",
-           fsc_level_str(level),
-           info.gva);
-    if ( !dabt.s1ptw )
-        printk("    gpa=%"PRIpaddr"\n", info.gpa);
-    if ( dabt.valid )
-        printk("    size=%d sign=%d write=%d reg=%d\n",
-               dabt.size, dabt.sign, dabt.write, dabt.reg);
-    else
-        printk("    instruction syndrome invalid\n");
-    printk("    eat=%d cm=%d s1ptw=%d dfsc=%d\n",
-           dabt.eat, dabt.cache, dabt.s1ptw, dabt.dfsc);
-    if ( !dabt.s1ptw )
-        dump_p2m_lookup(current->domain, info.gpa);
-    else
-        dump_guest_s1_walk(current->domain, info.gva);
-    show_execution_state(regs);
-    domain_crash_synchronous();
+    inject_dabt_exception(regs, info.gva, hsr.len);
 }
 
 asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
@@ -1459,7 +1597,10 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs
*regs)
         break;
 #endif
 
-    case HSR_EC_DATA_ABORT_GUEST:
+    case HSR_EC_INSTR_ABORT_LOWER_EL:
+        do_trap_instr_abort_guest(regs, hsr);
+        break;
+    case HSR_EC_DATA_ABORT_LOWER_EL:
         do_trap_data_abort_guest(regs, hsr);
         break;
     default:
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 29cd9d7..d5f138a 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -259,6 +259,7 @@
 #define CSSELR_EL1              CSSELR
 #define DACR32_EL2              DACR
 #define ESR_EL2                 HSR
+#define FAR_EL2                 HIFAR
 #define HCR_EL2                 HCR
 #define HPFAR_EL2               HPFAR
 #define ID_AFR0_EL1             ID_AFR0
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index b2746cc..dfe807d 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -103,10 +103,17 @@
 #define HSR_EC_SMC64                0x17
 #define HSR_EC_SYSREG               0x18
 #endif
-#define HSR_EC_INSTR_ABORT_GUEST    0x20
-#define HSR_EC_INSTR_ABORT_HYP      0x21
-#define HSR_EC_DATA_ABORT_GUEST     0x24
-#define HSR_EC_DATA_ABORT_HYP       0x25
+#define HSR_EC_INSTR_ABORT_LOWER_EL 0x20
+#define HSR_EC_INSTR_ABORT_CURR_EL  0x21
+#define HSR_EC_DATA_ABORT_LOWER_EL  0x24
+#define HSR_EC_DATA_ABORT_CURR_EL   0x25
+
+/* FSR format, common */
+#define FSR_LPAE                (_AC(1,UL)<<9)
+/* FSR short format */
+#define FSRS_FS_DEBUG           (_AC(0,UL)<<10|_AC(0x2,UL)<<0)
+/* FSR long format */
+#define FSRL_STATUS_DEBUG       (_AC(0x22,UL)<<0)
 
 #ifndef __ASSEMBLY__
 
diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
index 0130b94..0951857 100644
--- a/xen/include/asm-arm/regs.h
+++ b/xen/include/asm-arm/regs.h
@@ -11,6 +11,8 @@
 
 #define psr_mode(psr,m) (((psr) & PSR_MODE_MASK) == m)
 
+#define psr_mode_is_32bit(psr) !!((psr) & PSR_MODE_BIT)
+
 #define usr_mode(r)     psr_mode((r)->cpsr,PSR_MODE_USR)
 #define fiq_mode(r)     psr_mode((r)->cpsr,PSR_MODE_FIQ)
 #define irq_mode(r)     psr_mode((r)->cpsr,PSR_MODE_IRQ)
-- 
1.7.10.4

Julien Grall

2013-Dec-09 15:57 UTC

head link

Re: [PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.

On 12/09/2013 02:58 PM, Ian Campbell wrote:> Currently an unhandled data abort in guest context leads to us killing the
> guest and an unhandled instruction abort in guest context leads to us
killing
> the host!
> 
> Andre pointed out that an unhandled data abort can be caused by e.g.
dmidecode
> looking for things which are not there in the guests physical address
space.
> Propagating the fault to the guest allows it to properly SIGSEGV the
> processes.
> 
> A guest kernel can trivially jump to an unmapped physical address which
would
> cause an instruction abort. Killing the host for that is obviously bad.
> Instead inject the exception so the guest kernel can SIGSEGV or panic() etc
as
> it deems appropriate.
> 
> Tested on arm64 (Mustang) and arm32 (Midway) with a dom0 kernel
late_initcall
> which either dereferences or jumps to address 0, provoking both behaviours
and
> resulting correctly in a guest kernel panic. Also tested on fast models
with a
> 32-bit dom0 on a 64-bit hypervisor, which behaved correctly.
> 
> In addition tested on both platforms with a userspace program which either
> calls to or dereferences address 0. The process is correctly killed with
SEGV.
> 
> Lastly tested on Mustang with a 32-bit version of the userspace test on a
> 64-bit dom0 kernel.
> 
> I think that covers all the cases.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Andre Przywara <andre.przywara@calxeda.com>
> ---
> Release wise this is a(n important) bug fix.
> ---
>  xen/arch/arm/traps.c            |  199
+++++++++++++++++++++++++++++++++------
>  xen/include/asm-arm/cpregs.h    |    1 +
>  xen/include/asm-arm/processor.h |   15 ++-
>  xen/include/asm-arm/regs.h      |    2 +
>  4 files changed, 184 insertions(+), 33 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 2a85d37..e4e7f83 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -264,6 +264,8 @@ static void cpsr_switch_mode(struct cpu_user_regs
*regs, int mode)
>  
>      regs->cpsr |= mode;
>      regs->cpsr |= PSR_IRQ_MASK;
> +    if (mode == PSR_MODE_ABT)
if ( ... )

The patch looks good to me:
Acked-by: Julien Grall <julien.grall@linaro.org>


-- 
Julien Grall

Ian Campbell

2013-Dec-09 16:46 UTC

head link

Re: [PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.

On Mon, 2013-12-09 at 15:57 +0000, Julien Grall wrote:> On 12/09/2013 02:58 PM, Ian Campbell wrote:
> > Currently an unhandled data abort in guest context leads to us killing
the
> > guest and an unhandled instruction abort in guest context leads to us
killing
> > the host!
> > 
> > Andre pointed out that an unhandled data abort can be caused by e.g.
dmidecode
> > looking for things which are not there in the guests physical address
space.
> > Propagating the fault to the guest allows it to properly SIGSEGV the
> > processes.
> > 
> > A guest kernel can trivially jump to an unmapped physical address
which would
> > cause an instruction abort. Killing the host for that is obviously
bad.
> > Instead inject the exception so the guest kernel can SIGSEGV or
panic() etc as
> > it deems appropriate.
> > 
> > Tested on arm64 (Mustang) and arm32 (Midway) with a dom0 kernel
late_initcall
> > which either dereferences or jumps to address 0, provoking both
behaviours and
> > resulting correctly in a guest kernel panic. Also tested on fast
models with a
> > 32-bit dom0 on a 64-bit hypervisor, which behaved correctly.
> > 
> > In addition tested on both platforms with a userspace program which
either
> > calls to or dereferences address 0. The process is correctly killed
with SEGV.
> > 
> > Lastly tested on Mustang with a 32-bit version of the userspace test
on a
> > 64-bit dom0 kernel.
> > 
> > I think that covers all the cases.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Andre Przywara <andre.przywara@calxeda.com>
> > ---
> > Release wise this is a(n important) bug fix.
> > ---
> >  xen/arch/arm/traps.c            |  199
+++++++++++++++++++++++++++++++++------
> >  xen/include/asm-arm/cpregs.h    |    1 +
> >  xen/include/asm-arm/processor.h |   15 ++-
> >  xen/include/asm-arm/regs.h      |    2 +
> >  4 files changed, 184 insertions(+), 33 deletions(-)
> > 
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 2a85d37..e4e7f83 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -264,6 +264,8 @@ static void cpsr_switch_mode(struct cpu_user_regs
*regs, int mode)
> >  
> >      regs->cpsr |= mode;
> >      regs->cpsr |= PSR_IRQ_MASK;
> > +    if (mode == PSR_MODE_ABT)
> 
> if ( ... )
Wow, if that turns out to be the only thing wrong with a patch of this
scope then I''ll have done well!

This whole function is wrong in this regard, I''m in two minds about
leaving it as is, but it''s only a couple of lines so I suppose I may as
well fix it.
> The patch looks good to me:
> Acked-by: Julien Grall <julien.grall@linaro.org>
Thanks.

Ian.

Ian Campbell

2013-Dec-11 13:37 UTC

head link

Re: [PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.

On Mon, 2013-12-09 at 15:57 +0000, Julien Grall wrote:> On 12/09/2013 02:58 PM, Ian Campbell wrote:
> > +    if (mode == PSR_MODE_ABT)
> 
> if ( ... )
> 
> The patch looks good to me:
> Acked-by: Julien Grall <julien.grall@linaro.org>
Applied, with whitespace in cpsr_switch_mode fixed.

Xen devel - Dec 2013 - [PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.

[PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.

Re: [PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.

Re: [PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.

Re: [PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.