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

[Ian Campbell <ian.campbell@citrix.com>]

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

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

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

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

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

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

Ian.

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