[PATCH] vm_event: Implement ARM SMC events

[PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

From: Tamas K Lengyel <tklengyel@sec.in.tum.de>

The ARM SMC instructions are already configured to trap to Xen by default. In
this patch we allow a user-space process in a privileged domain to receive
notification of when such event happens through the vm_event subsystem.

This patch will likely needs to be broken up into several smaller patches.
Right now what this patch adds (and could be broken into smaller patches
accordingly):
    - Implement monitor_op domctl handler for SOFTWARE_BREAKPOINTs on ARM
    - Implement vm_event register fill/set routines for ARM. This required
        removing the function from common as the function prototype now
        differs on the two archs.
    - Sending notification as SOFTWARE_BREAKPOINT vm_event from the SMC trap
        handlers.
    - Extend the xen-access test tool to receive SMC notification and step
        the PC manually in the reply.

I'm sending it as an RFC to gather feedback on what has been overlooked in this
revision. This patch has been tested on a Cubietruck board and works fine,
but would probably not work on 64-bit boards.

Example output:

domu# insmod smc.ko
[   59.518258] Issuing SMC! Function @ bf000000

dom0# ./xen-access 1 breakpoint
xenaccess init
max_gpfn = 48000
starting breakpoint 1
Got event from Xen
Got event from Xen
SMC: pc=bf000020 gfn=bf000 (vcpu 0)

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/tests/xen-access/xen-access.c | 15 ++++--
 xen/arch/arm/Makefile               |  2 +
 xen/arch/arm/monitor.c              | 65 +++++++++++++++++++++++++
 xen/arch/arm/traps.c                | 20 +++++++-
 xen/arch/arm/vm_event.c             | 95 +++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/event.c            |  2 +
 xen/common/vm_event.c               |  1 -
 xen/include/asm-arm/domain.h        |  5 ++
 xen/include/asm-arm/monitor.h       | 15 +-----
 xen/include/asm-arm/vm_event.h      | 18 +++----
 xen/include/asm-x86/vm_event.h      |  4 +-
 xen/include/public/vm_event.h       | 26 ++++++++++
 12 files changed, 235 insertions(+), 33 deletions(-)
 create mode 100644 xen/arch/arm/monitor.c
 create mode 100644 xen/arch/arm/vm_event.c

diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index ef89246..a1cde82 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -333,7 +333,7 @@ void usage(char* progname)
 {
     fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname);
 #if defined(__i386__) || defined(__x86_64__)
-            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec");
+            fprintf(stderr, "|altp2m_write|altp2m_exec");
 #endif
             fprintf(stderr,
             "\n"
@@ -397,11 +397,11 @@ int main(int argc, char *argv[])
         default_access = XENMEM_access_rw;
         after_first_access = XENMEM_access_rwx;
     }
-#if defined(__i386__) || defined(__x86_64__)
     else if ( !strcmp(argv[0], "breakpoint") )
     {
         breakpoint = 1;
     }
+#if defined(__i386__) || defined(__x86_64__)
     else if ( !strcmp(argv[0], "altp2m_write") )
     {
         default_access = XENMEM_access_rx;
@@ -634,7 +634,8 @@ int main(int argc, char *argv[])
                 rsp.u.mem_access = req.u.mem_access;
                 break;
             case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
-                printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n",
+#if defined(__i386__) || defined(__x86_64__)
+                printf("Breakpoint: rip=%"PRIx64" gfn=%"PRIx64" (vcpu %d)\n",
                        req.data.regs.x86.rip,
                        req.u.software_breakpoint.gfn,
                        req.vcpu_id);
@@ -649,7 +650,15 @@ int main(int argc, char *argv[])
                     interrupted = -1;
                     continue;
                 }
+#elif defined(__arm__) || defined(__aarch64__)
+                printf("SMC: pc=%"PRIx64" (vcpu %d)\n",
+                       req.data.regs.arm.pc,
+                       req.vcpu_id);
 
+                rsp.data.regs.arm = req.data.regs.arm;
+                rsp.data.regs.arm.pc += 4;
+                rsp.flags |= VM_EVENT_FLAG_SET_REGISTERS;
+#endif
                 break;
             case VM_EVENT_REASON_SINGLESTEP:
                 printf("Singlestep: rip=%016"PRIx64", vcpu %d\n",
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 0328b50..118be99 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -40,6 +40,8 @@ obj-y += device.o
 obj-y += decode.o
 obj-y += processor.o
 obj-y += smc.o
+obj-y += monitor.o
+obj-y += vm_event.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/monitor.c b/xen/arch/arm/monitor.c
new file mode 100644
index 0000000..49bfddb
--- /dev/null
+++ b/xen/arch/arm/monitor.c
@@ -0,0 +1,65 @@
+/*
+ * arch/x86/monitor.c
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <asm/monitor.h>
+#include <public/vm_event.h>
+
+int arch_monitor_domctl_event(struct domain *d,
+                              struct xen_domctl_monitor_op *mop)
+{
+    struct arch_domain *ad = &d->arch;
+    bool_t requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
+
+    switch ( mop->event )
+    {
+    case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
+    {
+        bool_t old_status = ad->monitor.software_breakpoint_enabled;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        ad->monitor.software_breakpoint_enabled = requested_status;
+        domain_unpause(d);
+        break;
+    }
+
+    default:
+        /*
+         * Should not be reached unless vm_event_monitor_get_capabilities() is
+         * not properly implemented.
+         */
+        ASSERT_UNREACHABLE();
+        return -EOPNOTSUPP;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 83744e8..9c018c9 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -41,6 +41,7 @@
 #include <asm/mmio.h>
 #include <asm/cpufeature.h>
 #include <asm/flushtlb.h>
+#include <asm/vm_event.h>
 
 #include "decode.h"
 #include "vtimer.h"
@@ -2449,6 +2450,21 @@ bad_data_abort:
     inject_dabt_exception(regs, info.gva, hsr.len);
 }
 
+static void do_trap_smc(struct cpu_user_regs *regs)
+{
+    int rc = 0;
+    if ( current->domain->arch.monitor.software_breakpoint_enabled )
+    {
+        rc = vm_event_smc(regs);
+    }
+
+    if ( rc != 1 )
+    {
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        inject_undef32_exception(regs);
+    }
+}
+
 static void enter_hypervisor_head(struct cpu_user_regs *regs)
 {
     if ( guest_mode(regs) )
@@ -2524,7 +2540,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
          */
         GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc32);
-        inject_undef32_exception(regs);
+        do_trap_smc(regs);
         break;
     case HSR_EC_HVC32:
         GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
@@ -2557,7 +2573,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
          */
         GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc64);
-        inject_undef64_exception(regs, hsr.len);
+        do_trap_smc(regs);
         break;
     case HSR_EC_SYSREG:
         GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
new file mode 100644
index 0000000..d997313
--- /dev/null
+++ b/xen/arch/arm/vm_event.c
@@ -0,0 +1,95 @@
+/*
+ * arch/arm/vm_event.c
+ *
+ * Architecture-specific vm_event handling routines
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/sched.h>
+#include <asm/vm_event.h>
+
+static inline
+void vm_event_fill_regs(vm_event_request_t *req,
+                        const struct cpu_user_regs *regs)
+{
+    req->data.regs.arm.r0 = regs->r0;
+    req->data.regs.arm.r1 = regs->r1;
+    req->data.regs.arm.r2 = regs->r2;
+    req->data.regs.arm.r3 = regs->r3;
+    req->data.regs.arm.r4 = regs->r4;
+    req->data.regs.arm.r5 = regs->r5;
+    req->data.regs.arm.r6 = regs->r6;
+    req->data.regs.arm.r7 = regs->r7;
+    req->data.regs.arm.r8 = regs->r8;
+    req->data.regs.arm.r9 = regs->r9;
+    req->data.regs.arm.r10 = regs->r10;
+    req->data.regs.arm.r11 = regs->r11;
+    req->data.regs.arm.r12 = regs->r12;
+    req->data.regs.arm.sp = regs->sp;
+    req->data.regs.arm.lr = regs->lr;
+    req->data.regs.arm.pc = regs->pc;
+    req->data.regs.arm.cpsr = regs->cpsr;
+    req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
+    req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
+}
+
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
+{
+    v->arch.cpu_info->guest_cpu_user_regs.r0 = rsp->data.regs.arm.r0;
+    v->arch.cpu_info->guest_cpu_user_regs.r1 = rsp->data.regs.arm.r1;
+    v->arch.cpu_info->guest_cpu_user_regs.r2 = rsp->data.regs.arm.r2;
+    v->arch.cpu_info->guest_cpu_user_regs.r3 = rsp->data.regs.arm.r3;
+    v->arch.cpu_info->guest_cpu_user_regs.r4 = rsp->data.regs.arm.r4;
+    v->arch.cpu_info->guest_cpu_user_regs.r5 = rsp->data.regs.arm.r5;
+    v->arch.cpu_info->guest_cpu_user_regs.r6 = rsp->data.regs.arm.r6;
+    v->arch.cpu_info->guest_cpu_user_regs.r7 = rsp->data.regs.arm.r7;
+    v->arch.cpu_info->guest_cpu_user_regs.r8 = rsp->data.regs.arm.r8;
+    v->arch.cpu_info->guest_cpu_user_regs.r9 = rsp->data.regs.arm.r9;
+    v->arch.cpu_info->guest_cpu_user_regs.r10 = rsp->data.regs.arm.r10;
+    v->arch.cpu_info->guest_cpu_user_regs.r11 = rsp->data.regs.arm.r11;
+    v->arch.cpu_info->guest_cpu_user_regs.r12 = rsp->data.regs.arm.r12;
+    v->arch.cpu_info->guest_cpu_user_regs.sp = rsp->data.regs.arm.sp;
+    v->arch.cpu_info->guest_cpu_user_regs.lr = rsp->data.regs.arm.lr;
+    v->arch.cpu_info->guest_cpu_user_regs.pc = rsp->data.regs.arm.pc;
+    v->arch.cpu_info->guest_cpu_user_regs.cpsr = rsp->data.regs.arm.cpsr;
+    v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
+    v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
+}
+
+int vm_event_smc(const struct cpu_user_regs *regs) {
+    struct vcpu *curr = current;
+    struct arch_domain *ad = &curr->domain->arch;
+    vm_event_request_t req = { 0 };
+
+    if ( !ad->monitor.software_breakpoint_enabled )
+        return 0;
+
+    req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
+    req.vcpu_id = curr->vcpu_id;
+
+    vm_event_fill_regs(&req, regs);
+
+    return vm_event_monitor_traps(curr, 1, &req);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 56c5514..f7d1418 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -47,6 +47,7 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
             .u.write_ctrlreg.old_value = old
         };
 
+        vm_event_fill_regs(&req);
         vm_event_monitor_traps(curr, sync, &req);
         return 1;
     }
@@ -115,6 +116,7 @@ int hvm_event_breakpoint(unsigned long rip,
     }
 
     req.vcpu_id = curr->vcpu_id;
+    vm_event_fill_regs(&req);
 
     return vm_event_monitor_traps(curr, 1, &req);
 }
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 2906407..a29bda8 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -818,7 +818,6 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
         req->altp2m_idx = altp2m_vcpu_idx(v);
     }
 
-    vm_event_fill_regs(req);
     vm_event_put_request(d, &d->vm_event->monitor, req);
 
     return 1;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c35ed40..b2eb101 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -129,6 +129,11 @@ struct arch_domain
     paddr_t efi_acpi_gpa;
     paddr_t efi_acpi_len;
 #endif
+
+    /* Monitor options */
+    struct {
+        uint8_t software_breakpoint_enabled : 1;
+    } monitor;
 }  __cacheline_aligned;
 
 struct arch_vcpu
diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h
index 6e36e99..e1c2ff2 100644
--- a/xen/include/asm-arm/monitor.h
+++ b/xen/include/asm-arm/monitor.h
@@ -3,7 +3,7 @@
  *
  * Arch-specific monitor_op domctl handler.
  *
- * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@tklengyel.com)
  * Copyright (c) 2016, Bitdefender S.R.L.
  *
  * This program is free software; you can redistribute it and/or
@@ -32,18 +32,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
     return -EOPNOTSUPP;
 }
 
-static inline
 int arch_monitor_domctl_event(struct domain *d,
-                              struct xen_domctl_monitor_op *mop)
-{
-    /*
-     * No arch-specific monitor vm-events on ARM.
-     *
-     * Should not be reached unless vm_event_monitor_get_capabilities() is not
-     * properly implemented.
-     */
-    ASSERT_UNREACHABLE();
-    return -EOPNOTSUPP;
-}
+                              struct xen_domctl_monitor_op *mop);
 
 #endif /* __ASM_ARM_MONITOR_H__ */
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 014d9ba..e4526fc 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -1,7 +1,7 @@
 /*
  * vm_event.h: architecture specific vm_event handling routines
  *
- * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2015-2016 Tamas K Lengyel (tamas@tklengyel.com)
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -48,24 +48,18 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
     /* Not supported on ARM. */
 }
 
-static inline
-void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
-{
-    /* Not supported on ARM. */
-}
-
-static inline void vm_event_fill_regs(vm_event_request_t *req)
-{
-    /* Not supported on ARM. */
-}
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
 
 static inline uint32_t vm_event_monitor_get_capabilities(struct domain *d)
 {
     uint32_t capabilities = 0;
 
-    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+    capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST |
+                    1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
 
     return capabilities;
 }
 
+int vm_event_smc(const struct cpu_user_regs *regs);
+
 #endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 0470240..de7816f 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -42,8 +42,6 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp);
 
 void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
 
-void vm_event_fill_regs(vm_event_request_t *req);
-
 static inline uint32_t vm_event_monitor_get_capabilities(struct domain *d)
 {
     uint32_t capabilities = 0;
@@ -67,4 +65,6 @@ static inline uint32_t vm_event_monitor_get_capabilities(struct domain *d)
     return capabilities;
 }
 
+void vm_event_fill_regs(vm_event_request_t *req);
+
 #endif /* __ASM_X86_VM_EVENT_H__ */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 9270d52..5858711 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -166,6 +166,31 @@ struct vm_event_regs_x86 {
     uint32_t _pad;
 };
 
+struct vm_event_regs_arm {
+    uint32_t r0;
+    uint32_t r1;
+    uint32_t r2;
+    uint32_t r3;
+    uint32_t r4;
+    uint32_t r5;
+    uint32_t r6;
+    uint32_t r7;
+    uint32_t r8;
+    uint32_t r9;
+    uint32_t r10;
+    uint32_t r11;
+    uint32_t r12;
+
+    uint32_t sp; /* r13 - SP: Valid for Hyp. frames only, o/w banked (see below) */
+    uint32_t lr; /* r14 - LR: Valid for Hyp. Same physical register as lr_usr. */
+
+    uint32_t cpsr; /* Return mode */
+    uint64_t pc;
+    uint64_t ttbr0;
+    uint64_t ttbr1;
+    uint32_t _pad;
+};
+
 /*
  * mem_access flag definitions
  *
@@ -254,6 +279,7 @@ typedef struct vm_event_st {
     union {
         union {
             struct vm_event_regs_x86 x86;
+            struct vm_event_regs_arm arm;
         } regs;
 
         struct vm_event_emul_read_data emul_read_data;
-- 
2.1.4


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

Re: [PATCH] vm_event: Implement ARM SMC events

[Jan Beulich]

>>> Tamas K Lengyel <tamas@tklengyel.com> 04/11/16 9:47 PM >>>
>--- a/xen/include/public/vm_event.h
>+++ b/xen/include/public/vm_event.h
>@@ -166,6 +166,31 @@ struct vm_event_regs_x86 {
     >uint32_t _pad;
 >};
 >
>+struct vm_event_regs_arm {
>+    uint32_t r0;
>+    uint32_t r1;
>+    uint32_t r2;
>+    uint32_t r3;
>+    uint32_t r4;
>+    uint32_t r5;
>+    uint32_t r6;
>+    uint32_t r7;
>+    uint32_t r8;
>+    uint32_t r9;
>+    uint32_t r10;
>+    uint32_t r11;
>+    uint32_t r12;
>+
>+    uint32_t sp; /* r13 - SP: Valid for Hyp. frames only, o/w banked (see below) */
>+    uint32_t lr; /* r14 - LR: Valid for Hyp. Same physical register as lr_usr. */
>+
>+    uint32_t cpsr; /* Return mode */
>+    uint64_t pc;

Why uint64_t instead of uint32_t?

>+    uint64_t ttbr0;
>+    uint64_t ttbr1;
>+    uint32_t _pad;
>+};

This padding field is pretty strange: Without the adjustment to pc there are 16
32-bit fields (not counting the padding one), so I don't see why padding would be
needed at all. And with pc adjusted the padding field would surely better go
ahead of the two remaining 64-bit ones.

>@@ -254,6 +279,7 @@ typedef struct vm_event_st {
     >union {
         >union {
             >struct vm_event_regs_x86 x86;
>+            struct vm_event_regs_arm arm;
         >} regs;
 
         Does this alter the x86 ABI? Perhaps the ARM structure is small enough for
this to not happen now, but what's the general idea about not breaking other
arch'es ABIs when adding support for a new arch here?

Jan


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

Re: [PATCH] vm_event: Implement ARM SMC events

[Razvan Cojocaru]

On 04/12/16 07:31, Jan Beulich wrote:
>>>> Tamas K Lengyel <tamas@tklengyel.com> 04/11/16 9:47 PM >>>
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -166,6 +166,31 @@ struct vm_event_regs_x86 {
>      >uint32_t _pad;
>  >};
>  >
>> +struct vm_event_regs_arm {
>> +    uint32_t r0;
>> +    uint32_t r1;
>> +    uint32_t r2;
>> +    uint32_t r3;
>> +    uint32_t r4;
>> +    uint32_t r5;
>> +    uint32_t r6;
>> +    uint32_t r7;
>> +    uint32_t r8;
>> +    uint32_t r9;
>> +    uint32_t r10;
>> +    uint32_t r11;
>> +    uint32_t r12;
>> +
>> +    uint32_t sp; /* r13 - SP: Valid for Hyp. frames only, o/w banked (see below) */
>> +    uint32_t lr; /* r14 - LR: Valid for Hyp. Same physical register as lr_usr. */
>> +
>> +    uint32_t cpsr; /* Return mode */
>> +    uint64_t pc;
> 
> Why uint64_t instead of uint32_t?
> 
>> +    uint64_t ttbr0;
>> +    uint64_t ttbr1;
>> +    uint32_t _pad;
>> +};
> 
> This padding field is pretty strange: Without the adjustment to pc there are 16
> 32-bit fields (not counting the padding one), so I don't see why padding would be
> needed at all. And with pc adjusted the padding field would surely better go
> ahead of the two remaining 64-bit ones.
> 
>> @@ -254,6 +279,7 @@ typedef struct vm_event_st {
>      >union {
>          >union {
>              >struct vm_event_regs_x86 x86;
>> +            struct vm_event_regs_arm arm;
>          >} regs;
>  
>          Does this alter the x86 ABI? Perhaps the ARM structure is small enough for
> this to not happen now, but what's the general idea about not breaking other
> arch'es ABIs when adding support for a new arch here?

I'd suggest modifying VM_EVENT_INTERFACE_VERSION whenever that becomes
the case.


Thanks,
Razvan

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Corneliu ZUZU]

On 4/11/2016 10:47 PM, Tamas K Lengyel wrote:
> From: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>
> The ARM SMC instructions are already configured to trap to Xen by default. In
> this patch we allow a user-space process in a privileged domain to receive
> notification of when such event happens through the vm_event subsystem.
>
> This patch will likely needs to be broken up into several smaller patches.
> Right now what this patch adds (and could be broken into smaller patches
> accordingly):
>      - Implement monitor_op domctl handler for SOFTWARE_BREAKPOINTs on ARM
>      - Implement vm_event register fill/set routines for ARM. This required
>          removing the function from common as the function prototype now
>          differs on the two archs.
>      - Sending notification as SOFTWARE_BREAKPOINT vm_event from the SMC trap
>          handlers.
>      - Extend the xen-access test tool to receive SMC notification and step
>          the PC manually in the reply.
>
> I'm sending it as an RFC to gather feedback on what has been overlooked in this
> revision. This patch has been tested on a Cubietruck board and works fine,
> but would probably not work on 64-bit boards.

Hi Tamas,

If I may, I'm still unable to work at the moment, being ill, but I'm 
checking the xen-devel lists from time to time.
Your patch caught my attention, reminding me of the conversation we had 
some time ago on this matter.
The only real reason I don't see SMC (secure-monitor-call) as being an 
ideal candidate for this is that, according to the ARM manuals, SMC 
should directly cause undefined exception if executed from user-mode 
(EL0), instead of a hypervisor trap - isn't that the case on the machine 
you tested this on or is this really only for the EL1 of domains?

Also:
- SMC, by definition, is a call to the secure side, it doesn't relate to 
debugging directly (it's a syscall to the 'secure' side). There is a 
viable INT3 equivalent on ARM, that being the BKPT/BRK instruction, 
using that instead would require a bit more effort (but would, 
conceptually, be more correct) and might be less performant, I suppose 
that's why you didn't go for that?
- SMC can be disabled by the secure side (over which Xen doesn't have 
control) - not really a problem on though, since the hypervisor trap 
happens before that check
But these 2 are conceptual problems, they don't impede usage of SMC as 
you intend in practice.

Cheers,
Corneliu.

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Konrad Rzeszutek Wilk]

On Mon, Apr 11, 2016 at 01:47:22PM -0600, Tamas K Lengyel wrote:
> From: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> 
> The ARM SMC instructions are already configured to trap to Xen by default. In
> this patch we allow a user-space process in a privileged domain to receive
> notification of when such event happens through the vm_event subsystem.
> 
> This patch will likely needs to be broken up into several smaller patches.
> Right now what this patch adds (and could be broken into smaller patches
> accordingly):
>     - Implement monitor_op domctl handler for SOFTWARE_BREAKPOINTs on ARM
>     - Implement vm_event register fill/set routines for ARM. This required
>         removing the function from common as the function prototype now
>         differs on the two archs.
>     - Sending notification as SOFTWARE_BREAKPOINT vm_event from the SMC trap
>         handlers.
>     - Extend the xen-access test tool to receive SMC notification and step
>         the PC manually in the reply.
> 
> I'm sending it as an RFC to gather feedback on what has been overlooked in this
> revision. This patch has been tested on a Cubietruck board and works fine,
> but would probably not work on 64-bit boards.

I only have some small nitpicking.
> +++ b/xen/arch/arm/traps.c
> @@ -41,6 +41,7 @@
>  #include <asm/mmio.h>
>  #include <asm/cpufeature.h>
>  #include <asm/flushtlb.h>
> +#include <asm/vm_event.h>
>  
>  #include "decode.h"
>  #include "vtimer.h"
> @@ -2449,6 +2450,21 @@ bad_data_abort:
>      inject_dabt_exception(regs, info.gva, hsr.len);
>  }
>  
> +static void do_trap_smc(struct cpu_user_regs *regs)
> +{
> +    int rc = 0;

Newline here
> +    if ( current->domain->arch.monitor.software_breakpoint_enabled )
> +    {
> +        rc = vm_event_smc(regs);
> +    }
> +
> +    if ( rc != 1 )
> +    {
> +        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));

This differs a bit from below. Should there be an comment explaining why
we expect it be only in 32-bit mode?

> +        inject_undef32_exception(regs);

Below you do inject_undef64_exception?

Perhaps there should be an check if it is 32 or 64-bit?

> +    }
> +}
> +
>  static void enter_hypervisor_head(struct cpu_user_regs *regs)
>  {
>      if ( guest_mode(regs) )
> @@ -2524,7 +2540,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>           */
>          GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
>          perfc_incr(trap_smc32);
> -        inject_undef32_exception(regs);
> +        do_trap_smc(regs);
>          break;
>      case HSR_EC_HVC32:
>          GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> @@ -2557,7 +2573,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>           */
>          GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
>          perfc_incr(trap_smc64);
> -        inject_undef64_exception(regs, hsr.len);
> +        do_trap_smc(regs);

As in here.. Now it will call inject_undef32_exception. That can't
be healthy?

>          break;
>      case HSR_EC_SYSREG:
>          GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
> diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
> new file mode 100644
> index 0000000..d997313
> --- /dev/null
> +++ b/xen/arch/arm/vm_event.c
> @@ -0,0 +1,95 @@
> +/*
> + * arch/arm/vm_event.c
> + *
> + * Architecture-specific vm_event handling routines
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)

2016?
Also .. shouldn't the company be attributed as well? I see BitDefender
on some of them so not sure what hte relationship you have with them.

> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/sched.h>
> +#include <asm/vm_event.h>
> +
> +static inline
> +void vm_event_fill_regs(vm_event_request_t *req,
> +                        const struct cpu_user_regs *regs)
> +{
> +    req->data.regs.arm.r0 = regs->r0;
> +    req->data.regs.arm.r1 = regs->r1;
> +    req->data.regs.arm.r2 = regs->r2;
> +    req->data.regs.arm.r3 = regs->r3;
> +    req->data.regs.arm.r4 = regs->r4;
> +    req->data.regs.arm.r5 = regs->r5;
> +    req->data.regs.arm.r6 = regs->r6;
> +    req->data.regs.arm.r7 = regs->r7;
> +    req->data.regs.arm.r8 = regs->r8;
> +    req->data.regs.arm.r9 = regs->r9;
> +    req->data.regs.arm.r10 = regs->r10;
> +    req->data.regs.arm.r11 = regs->r11;
> +    req->data.regs.arm.r12 = regs->r12;
> +    req->data.regs.arm.sp = regs->sp;
> +    req->data.regs.arm.lr = regs->lr;
> +    req->data.regs.arm.pc = regs->pc;
> +    req->data.regs.arm.cpsr = regs->cpsr;
> +    req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
> +    req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
> +}
> +
> +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
> +{
> +    v->arch.cpu_info->guest_cpu_user_regs.r0 = rsp->data.regs.arm.r0;
> +    v->arch.cpu_info->guest_cpu_user_regs.r1 = rsp->data.regs.arm.r1;
> +    v->arch.cpu_info->guest_cpu_user_regs.r2 = rsp->data.regs.arm.r2;
> +    v->arch.cpu_info->guest_cpu_user_regs.r3 = rsp->data.regs.arm.r3;
> +    v->arch.cpu_info->guest_cpu_user_regs.r4 = rsp->data.regs.arm.r4;
> +    v->arch.cpu_info->guest_cpu_user_regs.r5 = rsp->data.regs.arm.r5;
> +    v->arch.cpu_info->guest_cpu_user_regs.r6 = rsp->data.regs.arm.r6;
> +    v->arch.cpu_info->guest_cpu_user_regs.r7 = rsp->data.regs.arm.r7;
> +    v->arch.cpu_info->guest_cpu_user_regs.r8 = rsp->data.regs.arm.r8;
> +    v->arch.cpu_info->guest_cpu_user_regs.r9 = rsp->data.regs.arm.r9;
> +    v->arch.cpu_info->guest_cpu_user_regs.r10 = rsp->data.regs.arm.r10;
> +    v->arch.cpu_info->guest_cpu_user_regs.r11 = rsp->data.regs.arm.r11;
> +    v->arch.cpu_info->guest_cpu_user_regs.r12 = rsp->data.regs.arm.r12;
> +    v->arch.cpu_info->guest_cpu_user_regs.sp = rsp->data.regs.arm.sp;
> +    v->arch.cpu_info->guest_cpu_user_regs.lr = rsp->data.regs.arm.lr;
> +    v->arch.cpu_info->guest_cpu_user_regs.pc = rsp->data.regs.arm.pc;
> +    v->arch.cpu_info->guest_cpu_user_regs.cpsr = rsp->data.regs.arm.cpsr;
> +    v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
> +    v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
> +}
> +
> +int vm_event_smc(const struct cpu_user_regs *regs) {
> +    struct vcpu *curr = current;
> +    struct arch_domain *ad = &curr->domain->arch;
> +    vm_event_request_t req = { 0 };
> +
> +    if ( !ad->monitor.software_breakpoint_enabled )
> +        return 0;
> +
> +    req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
> +    req.vcpu_id = curr->vcpu_id;
> +
> +    vm_event_fill_regs(&req, regs);
> +
> +    return vm_event_monitor_traps(curr, 1, &req);

Perhaps a comment right past 1 explaining what this mystical
1 value means?

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 3385 bytes --]

On Apr 12, 2016 01:51, "Corneliu ZUZU" <czuzu@bitdefender.com> wrote:
>
> On 4/11/2016 10:47 PM, Tamas K Lengyel wrote:
>>
>> From: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>>
>> The ARM SMC instructions are already configured to trap to Xen by
default. In
>> this patch we allow a user-space process in a privileged domain to
receive
>> notification of when such event happens through the vm_event subsystem.
>>
>> This patch will likely needs to be broken up into several smaller
patches.
>> Right now what this patch adds (and could be broken into smaller patches
>> accordingly):
>>      - Implement monitor_op domctl handler for SOFTWARE_BREAKPOINTs on
ARM
>>      - Implement vm_event register fill/set routines for ARM. This
required
>>          removing the function from common as the function prototype now
>>          differs on the two archs.
>>      - Sending notification as SOFTWARE_BREAKPOINT vm_event from the SMC
trap
>>          handlers.
>>      - Extend the xen-access test tool to receive SMC notification and
step
>>          the PC manually in the reply.
>>
>> I'm sending it as an RFC to gather feedback on what has been overlooked
in this
>> revision. This patch has been tested on a Cubietruck board and works
fine,
>> but would probably not work on 64-bit boards.
>
>
> Hi Tamas,
>
> If I may, I'm still unable to work at the moment, being ill, but I'm
checking the xen-devel lists from time to time.
> Your patch caught my attention, reminding me of the conversation we had
some time ago on this matter.
> The only real reason I don't see SMC (secure-monitor-call) as being an
ideal candidate for this is that, according to the ARM manuals, SMC should
directly cause undefined exception if executed from user-mode (EL0),
instead of a hypervisor trap - isn't that the case on the machine you
tested this on or is this really only for the EL1 of domains?

That's correct, it can only be issued by the kernel. So as long as you want
to monitor the kernel it can be used just fine. I can also envision
trampoline-like traps (syscalls injected into EL0 to trigger SMC) but
that's beyond the scope I intend this for now.

>
> Also:
> - SMC, by definition, is a call to the secure side, it doesn't relate to
debugging directly (it's a syscall to the 'secure' side). There is a viable
INT3 equivalent on ARM, that being the BKPT/BRK instruction, using that
instead would require a bit more effort (but would, conceptually, be more
correct) and might be less performant, I suppose that's why you didn't go
for that?

I would have to double check but AFAIK those instructions can't be
configured to trap to the hypervisor directly. So while SMC was not
intended to be a breakpoint, conceptually it's the closest thing we have an
on ARM to the INT3 instruction when configured to trap to the VMM.

> - SMC can be disabled by the secure side (over which Xen doesn't have
control) - not really a problem on though, since the hypervisor trap
happens before that check
> But these 2 are conceptual problems, they don't impede usage of SMC as
you intend in practice.

Sure, the TrustZone is more privileged then the hypervisor so you need to
take that into account as well when you consider your threat model. If the
TZ is malicious though IMHO there isn't much you can do on the hypervisor
side anyway. So in the usecase I have for this I control the TZ as well.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4075 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1438 bytes --]

On Apr 11, 2016 22:31, "Jan Beulich" <jbeulich@suse.com> wrote:
>
> >>> Tamas K Lengyel <tamas@tklengyel.com> 04/11/16 9:47 PM >>>
> >--- a/xen/include/public/vm_event.h
> >+++ b/xen/include/public/vm_event.h
> >@@ -166,6 +166,31 @@ struct vm_event_regs_x86 {
>      >uint32_t _pad;
>  >};
>  >
> >+struct vm_event_regs_arm {
> >+    uint32_t r0;
> >+    uint32_t r1;
> >+    uint32_t r2;
> >+    uint32_t r3;
> >+    uint32_t r4;
> >+    uint32_t r5;
> >+    uint32_t r6;
> >+    uint32_t r7;
> >+    uint32_t r8;
> >+    uint32_t r9;
> >+    uint32_t r10;
> >+    uint32_t r11;
> >+    uint32_t r12;
> >+
> >+    uint32_t sp; /* r13 - SP: Valid for Hyp. frames only, o/w banked
(see below) */
> >+    uint32_t lr; /* r14 - LR: Valid for Hyp. Same physical register as
lr_usr. */
> >+
> >+    uint32_t cpsr; /* Return mode */
> >+    uint64_t pc;
>
> Why uint64_t instead of uint32_t?

No apparent reason, will fix.

>
> >+    uint64_t ttbr0;
> >+    uint64_t ttbr1;
> >+    uint32_t _pad;
> >+};
>
> This padding field is pretty strange: Without the adjustment to pc there
are 16
> 32-bit fields (not counting the padding one), so I don't see why padding
would be
> needed at all. And with pc adjusted the padding field would surely better
go
> ahead of the two remaining 64-bit ones.

Yes, I have been shuffling this struct around and didn't check the layout.
Will fix. I'll also try to make this struct usable for aarch64 too.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2118 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 667 bytes --]

> >> @@ -254,6 +279,7 @@ typedef struct vm_event_st {
> >      >union {
> >          >union {
> >              >struct vm_event_regs_x86 x86;
> >> +            struct vm_event_regs_arm arm;
> >          >} regs;
> >
> >          Does this alter the x86 ABI? Perhaps the ARM structure is
small enough for
> > this to not happen now, but what's the general idea about not breaking
other
> > arch'es ABIs when adding support for a new arch here?
>
> I'd suggest modifying VM_EVENT_INTERFACE_VERSION whenever that becomes
> the case.
>

Yeap, that's what I had in mind too, if we end up changing the layout or
the size of the struct we will increment the version.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 933 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 8192 bytes --]

On Apr 12, 2016 08:58, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>
wrote:
>
> On Mon, Apr 11, 2016 at 01:47:22PM -0600, Tamas K Lengyel wrote:
> > From: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> >
> > The ARM SMC instructions are already configured to trap to Xen by
default. In
> > this patch we allow a user-space process in a privileged domain to
receive
> > notification of when such event happens through the vm_event subsystem.
> >
> > This patch will likely needs to be broken up into several smaller
patches.
> > Right now what this patch adds (and could be broken into smaller patches
> > accordingly):
> >     - Implement monitor_op domctl handler for SOFTWARE_BREAKPOINTs on
ARM
> >     - Implement vm_event register fill/set routines for ARM. This
required
> >         removing the function from common as the function prototype now
> >         differs on the two archs.
> >     - Sending notification as SOFTWARE_BREAKPOINT vm_event from the SMC
trap
> >         handlers.
> >     - Extend the xen-access test tool to receive SMC notification and
step
> >         the PC manually in the reply.
> >
> > I'm sending it as an RFC to gather feedback on what has been overlooked
in this
> > revision. This patch has been tested on a Cubietruck board and works
fine,
> > but would probably not work on 64-bit boards.
>
> I only have some small nitpicking.
> > +++ b/xen/arch/arm/traps.c
> > @@ -41,6 +41,7 @@
> >  #include <asm/mmio.h>
> >  #include <asm/cpufeature.h>
> >  #include <asm/flushtlb.h>
> > +#include <asm/vm_event.h>
> >
> >  #include "decode.h"
> >  #include "vtimer.h"
> > @@ -2449,6 +2450,21 @@ bad_data_abort:
> >      inject_dabt_exception(regs, info.gva, hsr.len);
> >  }
> >
> > +static void do_trap_smc(struct cpu_user_regs *regs)
> > +{
> > +    int rc = 0;
>
> Newline here

Ack.

> > +    if ( current->domain->arch.monitor.software_breakpoint_enabled )
> > +    {
> > +        rc = vm_event_smc(regs);
> > +    }
> > +
> > +    if ( rc != 1 )
> > +    {
> > +        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
>
> This differs a bit from below. Should there be an comment explaining why
> we expect it be only in 32-bit mode?
>
> > +        inject_undef32_exception(regs);
>
> Below you do inject_undef64_exception?
>
> Perhaps there should be an check if it is 32 or 64-bit?

Yes, indeed there should be.

>
> > +    }
> > +}
> > +
> >  static void enter_hypervisor_head(struct cpu_user_regs *regs)
> >  {
> >      if ( guest_mode(regs) )
> > @@ -2524,7 +2540,7 @@ asmlinkage void do_trap_hypervisor(struct
cpu_user_regs *regs)
> >           */
> >          GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> >          perfc_incr(trap_smc32);
> > -        inject_undef32_exception(regs);
> > +        do_trap_smc(regs);
> >          break;
> >      case HSR_EC_HVC32:
> >          GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> > @@ -2557,7 +2573,7 @@ asmlinkage void do_trap_hypervisor(struct
cpu_user_regs *regs)
> >           */
> >          GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
> >          perfc_incr(trap_smc64);
> > -        inject_undef64_exception(regs, hsr.len);
> > +        do_trap_smc(regs);
>
> As in here.. Now it will call inject_undef32_exception. That can't
> be healthy?

Ack.

>
> >          break;
> >      case HSR_EC_SYSREG:
> >          GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
> > diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
> > new file mode 100644
> > index 0000000..d997313
> > --- /dev/null
> > +++ b/xen/arch/arm/vm_event.c
> > @@ -0,0 +1,95 @@
> > +/*
> > + * arch/arm/vm_event.c
> > + *
> > + * Architecture-specific vm_event handling routines
> > + *
> > + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
>
> 2016?

Yeap.

> Also .. shouldn't the company be attributed as well? I see BitDefender
> on some of them so not sure what hte relationship you have with them.

I'm not affiliated with BitDefender in any way and at the moment I'm just
doing this on my own as I'm no longer with Novetta either.

>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public
> > + * License v2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > + * License along with this program; If not, see <
http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <xen/sched.h>
> > +#include <asm/vm_event.h>
> > +
> > +static inline
> > +void vm_event_fill_regs(vm_event_request_t *req,
> > +                        const struct cpu_user_regs *regs)
> > +{
> > +    req->data.regs.arm.r0 = regs->r0;
> > +    req->data.regs.arm.r1 = regs->r1;
> > +    req->data.regs.arm.r2 = regs->r2;
> > +    req->data.regs.arm.r3 = regs->r3;
> > +    req->data.regs.arm.r4 = regs->r4;
> > +    req->data.regs.arm.r5 = regs->r5;
> > +    req->data.regs.arm.r6 = regs->r6;
> > +    req->data.regs.arm.r7 = regs->r7;
> > +    req->data.regs.arm.r8 = regs->r8;
> > +    req->data.regs.arm.r9 = regs->r9;
> > +    req->data.regs.arm.r10 = regs->r10;
> > +    req->data.regs.arm.r11 = regs->r11;
> > +    req->data.regs.arm.r12 = regs->r12;
> > +    req->data.regs.arm.sp = regs->sp;
> > +    req->data.regs.arm.lr = regs->lr;
> > +    req->data.regs.arm.pc = regs->pc;
> > +    req->data.regs.arm.cpsr = regs->cpsr;
> > +    req->data.regs.arm.ttbr0 = READ_SYSREG64(TTBR0_EL1);
> > +    req->data.regs.arm.ttbr1 = READ_SYSREG64(TTBR1_EL1);
> > +}
> > +
> > +void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
> > +{
> > +    v->arch.cpu_info->guest_cpu_user_regs.r0 = rsp->data.regs.arm.r0;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r1 = rsp->data.regs.arm.r1;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r2 = rsp->data.regs.arm.r2;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r3 = rsp->data.regs.arm.r3;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r4 = rsp->data.regs.arm.r4;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r5 = rsp->data.regs.arm.r5;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r6 = rsp->data.regs.arm.r6;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r7 = rsp->data.regs.arm.r7;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r8 = rsp->data.regs.arm.r8;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r9 = rsp->data.regs.arm.r9;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r10 = rsp->data.regs.arm.r10;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r11 = rsp->data.regs.arm.r11;
> > +    v->arch.cpu_info->guest_cpu_user_regs.r12 = rsp->data.regs.arm.r12;
> > +    v->arch.cpu_info->guest_cpu_user_regs.sp = rsp->data.regs.arm.sp;
> > +    v->arch.cpu_info->guest_cpu_user_regs.lr = rsp->data.regs.arm.lr;
> > +    v->arch.cpu_info->guest_cpu_user_regs.pc = rsp->data.regs.arm.pc;
> > +    v->arch.cpu_info->guest_cpu_user_regs.cpsr =
rsp->data.regs.arm.cpsr;
> > +    v->arch.ttbr0 = rsp->data.regs.arm.ttbr0;
> > +    v->arch.ttbr1 = rsp->data.regs.arm.ttbr1;
> > +}
> > +
> > +int vm_event_smc(const struct cpu_user_regs *regs) {
> > +    struct vcpu *curr = current;
> > +    struct arch_domain *ad = &curr->domain->arch;
> > +    vm_event_request_t req = { 0 };
> > +
> > +    if ( !ad->monitor.software_breakpoint_enabled )
> > +        return 0;
> > +
> > +    req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
> > +    req.vcpu_id = curr->vcpu_id;
> > +
> > +    vm_event_fill_regs(&req, regs);
> > +
> > +    return vm_event_monitor_traps(curr, 1, &req);
>
> Perhaps a comment right past 1 explaining what this mystical
> 1 value means?

The function prototype is pretty self explanatory IMHO, while the call
itself may not be. It's a boolean to determine if the trap is synchronous
or not so should the vCPU be paused after. I can add a comment for it but I
don't think it's necessary. That just reminds me though that MAINTAINERS
needs to be updated to add this file to the vm_event stack as well.

Thanks!
Tamas

[-- Attachment #1.2: Type: text/html, Size: 11378 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Julien Grall]

Hello Tamas,

On 12/04/2016 16:05, Tamas K Lengyel wrote:
> On Apr 11, 2016 22:31, "Jan Beulich" <jbeulich@suse.com
> <mailto:jbeulich@suse.com>> wrote:
>  >
>  > >>> Tamas K Lengyel <tamas@tklengyel.com
> <mailto:tamas@tklengyel.com>> 04/11/16 9:47 PM >>>

[...]

>  >
>  > >+    uint64_t ttbr0;
>  > >+    uint64_t ttbr1;
>  > >+    uint32_t _pad;
>  > >+};
>  >
>  > This padding field is pretty strange: Without the adjustment to pc
> there are 16
>  > 32-bit fields (not counting the padding one), so I don't see why
> padding would be
>  > needed at all. And with pc adjusted the padding field would surely
> better go
>  > ahead of the two remaining 64-bit ones.
>
> Yes, I have been shuffling this struct around and didn't check the
> layout. Will fix. I'll also try to make this struct usable for aarch64 too.

You may want to give a look to vcpu_guest_core_regs in public/arch-arm.h

Regards,

-- 
Julien Grall

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Julien Grall]

Hello,

On 12/04/2016 16:01, Tamas K Lengyel wrote:
>
> On Apr 12, 2016 01:51, "Corneliu ZUZU" <czuzu@bitdefender.com
> <mailto:czuzu@bitdefender.com>> wrote:
>  >
>  > On 4/11/2016 10:47 PM, Tamas K Lengyel wrote:
>  >>
>  >> From: Tamas K Lengyel <tklengyel@sec.in.tum.de
> <mailto:tklengyel@sec.in.tum.de>>
>  >>
>  >> The ARM SMC instructions are already configured to trap to Xen by
> default. In
>  >> this patch we allow a user-space process in a privileged domain to
> receive
>  >> notification of when such event happens through the vm_event subsystem.
>  >>
>  >> This patch will likely needs to be broken up into several smaller
> patches.
>  >> Right now what this patch adds (and could be broken into smaller patches
>  >> accordingly):
>  >>      - Implement monitor_op domctl handler for SOFTWARE_BREAKPOINTs
> on ARM
>  >>      - Implement vm_event register fill/set routines for ARM. This
> required
>  >>          removing the function from common as the function prototype now
>  >>          differs on the two archs.
>  >>      - Sending notification as SOFTWARE_BREAKPOINT vm_event from the
> SMC trap
>  >>          handlers.
>  >>      - Extend the xen-access test tool to receive SMC notification
> and step
>  >>          the PC manually in the reply.
>  >>
>  >> I'm sending it as an RFC to gather feedback on what has been
> overlooked in this
>  >> revision. This patch has been tested on a Cubietruck board and works
> fine,
>  >> but would probably not work on 64-bit boards.
>  >
>  >
>  > Hi Tamas,
>  >
>  > If I may, I'm still unable to work at the moment, being ill, but I'm
> checking the xen-devel lists from time to time.
>  > Your patch caught my attention, reminding me of the conversation we
> had some time ago on this matter.
>  > The only real reason I don't see SMC (secure-monitor-call) as being
> an ideal candidate for this is that, according to the ARM manuals, SMC
> should directly cause undefined exception if executed from user-mode
> (EL0), instead of a hypervisor trap - isn't that the case on the machine
> you tested this on or is this really only for the EL1 of domains?

This paragraph is part of Corneliu's answer but it gives the impression 
you wrote it. Can you configure your e-mail client to quote properly?

>
> That's correct, it can only be issued by the kernel. So as long as you
> want to monitor the kernel it can be used just fine. I can also envision
> trampoline-like traps (syscalls injected into EL0 to trigger SMC) but
> that's beyond the scope I intend this for now.
>
>  >
>  > Also:
>  > - SMC, by definition, is a call to the secure side, it doesn't relate
> to debugging directly (it's a syscall to the 'secure' side). There is a
> viable INT3 equivalent on ARM, that being the BKPT/BRK instruction,
> using that instead would require a bit more effort (but would,
> conceptually, be more correct) and might be less performant, I suppose
> that's why you didn't go for that?

BKPT/BRK could be used by the guest for debugging. You would need to 
differentiate a breakpoint inserted by Xen or by a debugger in the guest.

>
> I would have to double check but AFAIK those instructions can't be
> configured to trap to the hypervisor directly. So while SMC was not
> intended to be a breakpoint, conceptually it's the closest thing we have
> an on ARM to the INT3 instruction when configured to trap to the VMM.

Whilst any access to SMC currently results to inject an undefined 
exception, it may not be the case in the future. There have been 
discussion to allow guest issuing SMC call (see [1]).

I think the safest instruction would be HVC #imm. Xen is only using a 
small number of immediate. You could allocate a specific value for 
software debugging.

>
>  > - SMC can be disabled by the secure side (over which Xen doesn't have
> control) - not really a problem on though, since the hypervisor trap
> happens before that check
>  > But these 2 are conceptual problems, they don't impede usage of SMC
> as you intend in practice.
>
> Sure, the TrustZone is more privileged then the hypervisor so you need
> to take that into account as well when you consider your threat model.
> If the TZ is malicious though IMHO there isn't much you can do on the
> hypervisor side anyway. So in the usecase I have for this I control the
> TZ as well.

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2015-07/txtwZfvJnXlYG.txt

-- 
Julien Grall

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Corneliu ZUZU]

On 4/12/2016 7:24 PM, Julien Grall wrote:
> Hello,
>
> On 12/04/2016 16:01, Tamas K Lengyel wrote:
>>
>> On Apr 12, 2016 01:51, "Corneliu ZUZU" <czuzu@bitdefender.com
>> <mailto:czuzu@bitdefender.com>> wrote:
>>  >
>>  > On 4/11/2016 10:47 PM, Tamas K Lengyel wrote:
>>  >>
>>  >> From: Tamas K Lengyel <tklengyel@sec.in.tum.de
>> <mailto:tklengyel@sec.in.tum.de>>
>>  >>
>>  >> The ARM SMC instructions are already configured to trap to Xen by
>> default. In
>>  >> this patch we allow a user-space process in a privileged domain to
>> receive
>>  >> notification of when such event happens through the vm_event 
>> subsystem.
>>  >>
>>  >> This patch will likely needs to be broken up into several smaller
>> patches.
>>  >> Right now what this patch adds (and could be broken into smaller 
>> patches
>>  >> accordingly):
>>  >>      - Implement monitor_op domctl handler for SOFTWARE_BREAKPOINTs
>> on ARM
>>  >>      - Implement vm_event register fill/set routines for ARM. This
>> required
>>  >>          removing the function from common as the function 
>> prototype now
>>  >>          differs on the two archs.
>>  >>      - Sending notification as SOFTWARE_BREAKPOINT vm_event from the
>> SMC trap
>>  >>          handlers.
>>  >>      - Extend the xen-access test tool to receive SMC notification
>> and step
>>  >>          the PC manually in the reply.
>>  >>
>>  >> I'm sending it as an RFC to gather feedback on what has been
>> overlooked in this
>>  >> revision. This patch has been tested on a Cubietruck board and works
>> fine,
>>  >> but would probably not work on 64-bit boards.
>>  >
>>  >
>>  > Hi Tamas,
>>  >
>>  > If I may, I'm still unable to work at the moment, being ill, but I'm
>> checking the xen-devel lists from time to time.
>>  > Your patch caught my attention, reminding me of the conversation we
>> had some time ago on this matter.
>>  > The only real reason I don't see SMC (secure-monitor-call) as being
>> an ideal candidate for this is that, according to the ARM manuals, SMC
>> should directly cause undefined exception if executed from user-mode
>> (EL0), instead of a hypervisor trap - isn't that the case on the machine
>> you tested this on or is this really only for the EL1 of domains?
>
> This paragraph is part of Corneliu's answer but it gives the 
> impression you wrote it. Can you configure your e-mail client to quote 
> properly?
>
>>
>> That's correct, it can only be issued by the kernel. So as long as you
>> want to monitor the kernel it can be used just fine. I can also envision
>> trampoline-like traps (syscalls injected into EL0 to trigger SMC) but
>> that's beyond the scope I intend this for now.

Then indeed SMC is the -easiest- way to go, @ least until user-mode 
breakpoints are implemented.

>>
>>  >
>>  > Also:
>>  > - SMC, by definition, is a call to the secure side, it doesn't relate
>> to debugging directly (it's a syscall to the 'secure' side). There is a
>> viable INT3 equivalent on ARM, that being the BKPT/BRK instruction,
>> using that instead would require a bit more effort (but would,
>> conceptually, be more correct) and might be less performant, I suppose
>> that's why you didn't go for that?
>
> BKPT/BRK could be used by the guest for debugging. You would need to 
> differentiate a breakpoint inserted by Xen or by a debugger in the guest.

Isn't that also the case for X86's INT3? I guess differentiation is done 
based on the bkpt address/privilege level? On ARM that could also 
(partially) be done by looking @ the immediate value of the BKPT/BRK 
instruction. Another thing I realized might be troublesome with NOT 
using BKPT/BRK would be that on ARMv7 BKPT is always unconditional, even 
in IT blocks. IDK if that applies to SMC, but if it doesn't you'd have 
to check for that as well to make sure the breakpoint is actually executed.

>
>>
>> I would have to double check but AFAIK those instructions can't be
>> configured to trap to the hypervisor directly. So while SMC was not
>> intended to be a breakpoint, conceptually it's the closest thing we have
>> an on ARM to the INT3 instruction when configured to trap to the VMM.
>

Please see AArch32 HDCR.TDE and AArch64 MDCR_EL2.TDE bits. Since 
activating this bit would imply additional (in this context -unwanted-) 
traps, the performance hit of having this bit set might be significant.


> Whilst any access to SMC currently results to inject an undefined 
> exception, it may not be the case in the future. There have been 
> discussion to allow guest issuing SMC call (see [1]).
>
> I think the safest instruction would be HVC #imm. Xen is only using a 
> small number of immediate. You could allocate a specific value for 
> software debugging.
>

IMHO that would also be better conceptually, although it would still 
suffer from the limitation of not being available from user-space (and 
potentially from the above IT block issue).

>>
>>  > - SMC can be disabled by the secure side (over which Xen doesn't have
>> control) - not really a problem on though, since the hypervisor trap
>> happens before that check
>>  > But these 2 are conceptual problems, they don't impede usage of SMC
>> as you intend in practice.
>>
>> Sure, the TrustZone is more privileged then the hypervisor so you need
>> to take that into account as well when you consider your threat model.
>> If the TZ is malicious though IMHO there isn't much you can do on the
>> hypervisor side anyway. So in the usecase I have for this I control the
>> TZ as well.
>
> Regards,
>
> [1] 
> http://lists.xen.org/archives/html/xen-devel/2015-07/txtwZfvJnXlYG.txt
>

Regards,
Corneliu.

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 6600 bytes --]

On Tue, Apr 12, 2016 at 11:05 AM, Corneliu ZUZU <czuzu@bitdefender.com>
wrote:

> On 4/12/2016 7:24 PM, Julien Grall wrote:
>
>> Hello,
>>
>> On 12/04/2016 16:01, Tamas K Lengyel wrote:
>>
>>>
>>> On Apr 12, 2016 01:51, "Corneliu ZUZU" <czuzu@bitdefender.com
>>> <mailto:czuzu@bitdefender.com>> wrote:
>>>  >
>>>  > On 4/11/2016 10:47 PM, Tamas K Lengyel wrote:
>>>  >>
>>>  >> From: Tamas K Lengyel <tklengyel@sec.in.tum.de
>>> <mailto:tklengyel@sec.in.tum.de>>
>>>  >>
>>>  >> The ARM SMC instructions are already configured to trap to Xen by
>>> default. In
>>>  >> this patch we allow a user-space process in a privileged domain to
>>> receive
>>>  >> notification of when such event happens through the vm_event
>>> subsystem.
>>>  >>
>>>  >> This patch will likely needs to be broken up into several smaller
>>> patches.
>>>  >> Right now what this patch adds (and could be broken into smaller
>>> patches
>>>  >> accordingly):
>>>  >>      - Implement monitor_op domctl handler for SOFTWARE_BREAKPOINTs
>>> on ARM
>>>  >>      - Implement vm_event register fill/set routines for ARM. This
>>> required
>>>  >>          removing the function from common as the function prototype
>>> now
>>>  >>          differs on the two archs.
>>>  >>      - Sending notification as SOFTWARE_BREAKPOINT vm_event from the
>>> SMC trap
>>>  >>          handlers.
>>>  >>      - Extend the xen-access test tool to receive SMC notification
>>> and step
>>>  >>          the PC manually in the reply.
>>>  >>
>>>  >> I'm sending it as an RFC to gather feedback on what has been
>>> overlooked in this
>>>  >> revision. This patch has been tested on a Cubietruck board and works
>>> fine,
>>>  >> but would probably not work on 64-bit boards.
>>>  >
>>>  >
>>>  > Hi Tamas,
>>>  >
>>>  > If I may, I'm still unable to work at the moment, being ill, but I'm
>>> checking the xen-devel lists from time to time.
>>>  > Your patch caught my attention, reminding me of the conversation we
>>> had some time ago on this matter.
>>>  > The only real reason I don't see SMC (secure-monitor-call) as being
>>> an ideal candidate for this is that, according to the ARM manuals, SMC
>>> should directly cause undefined exception if executed from user-mode
>>> (EL0), instead of a hypervisor trap - isn't that the case on the machine
>>> you tested this on or is this really only for the EL1 of domains?
>>>
>>
>> This paragraph is part of Corneliu's answer but it gives the impression
>> you wrote it. Can you configure your e-mail client to quote properly?
>>
>>
>>> That's correct, it can only be issued by the kernel. So as long as you
>>> want to monitor the kernel it can be used just fine. I can also envision
>>> trampoline-like traps (syscalls injected into EL0 to trigger SMC) but
>>> that's beyond the scope I intend this for now.
>>>
>>
> Then indeed SMC is the -easiest- way to go, @ least until user-mode
> breakpoints are implemented.
>
>
>>>  >
>>>  > Also:
>>>  > - SMC, by definition, is a call to the secure side, it doesn't relate
>>> to debugging directly (it's a syscall to the 'secure' side). There is a
>>> viable INT3 equivalent on ARM, that being the BKPT/BRK instruction,
>>> using that instead would require a bit more effort (but would,
>>> conceptually, be more correct) and might be less performant, I suppose
>>> that's why you didn't go for that?
>>>
>>
>> BKPT/BRK could be used by the guest for debugging. You would need to
>> differentiate a breakpoint inserted by Xen or by a debugger in the guest.
>>
>
> Isn't that also the case for X86's INT3? I guess differentiation is done
> based on the bkpt address/privilege level? On ARM that could also
> (partially) be done by looking @ the immediate value of the BKPT/BRK
> instruction. Another thing I realized might be troublesome with NOT using
> BKPT/BRK would be that on ARMv7 BKPT is always unconditional, even in IT
> blocks. IDK if that applies to SMC, but if it doesn't you'd have to check
> for that as well to make sure the breakpoint is actually executed.
>
>
>>
>>> I would have to double check but AFAIK those instructions can't be
>>> configured to trap to the hypervisor directly. So while SMC was not
>>> intended to be a breakpoint, conceptually it's the closest thing we have
>>> an on ARM to the INT3 instruction when configured to trap to the VMM.
>>>
>>
>>
> Please see AArch32 HDCR.TDE and AArch64 MDCR_EL2.TDE bits. Since
> activating this bit would imply additional (in this context -unwanted-)
> traps, the performance hit of having this bit set might be significant.
>

Right, actually I believe KVM already implemented this, I was just under
the impression it was only for aarch64. As for performance overhead I'm not
that worried about it, rather I need to make sure the presence of the
monitoring can remain stealthy. I know on KVM for example this type of
trapping renders the guest to be unable to use singlestepping, which would
easily reveal the presence of the external monitor (see
https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014589.html). So
this will need to be looked at carefully.


>
> Whilst any access to SMC currently results to inject an undefined
>> exception, it may not be the case in the future. There have been discussion
>> to allow guest issuing SMC call (see [1]).
>>
>
I don't see a problem with that, as long as it's configurable whether SMC
calls are trapped or pass-through.


>
>> I think the safest instruction would be HVC #imm. Xen is only using a
>> small number of immediate. You could allocate a specific value for software
>> debugging.
>>
>>
> IMHO that would also be better conceptually, although it would still
> suffer from the limitation of not being available from user-space (and
> potentially from the above IT block issue).
>

Sure, HVC would also be a possibility but I do see use-cases for trapping
SMC calls and forwarding them to a guest instead of the TZ. For example, if
malware tries to exploit TZ vulnerabilities, it would be a lot easier to
contain and monitor such exploits if the TZ is virtualized rather then just
crashing the guest or forwarding the calls to a real TZ. So trapping SMC
would allow us to use the real TZ for other purposes and maintain a barrier
between malicious guests and the TZ.

So what I will do instead of issuing a software_breakpoint vm_event for
SMCs, I'll introduce a new type, say VM_EVENT_REASON_PRIVILEGED_CALL, that
can be used to forward both hypercalls and SMCs to a monitoring guest. This
would also allow us to use the software_breakpoint type for the actual
software breakpoint events in the future.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 9489 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 405 bytes --]

Yes, I have been shuffling this struct around and didn't check the
>> layout. Will fix. I'll also try to make this struct usable for aarch64
>> too.
>>
>
> You may want to give a look to vcpu_guest_core_regs in public/arch-arm.h
>

Thanks, so the easiest way to be compatible with both it seems is to just
declare all registers 64-bit wide. The userspace can then mask the unused
bits if necessary.

Tama

[-- Attachment #1.2: Type: text/html, Size: 873 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Corneliu ZUZU]

[-- Attachment #1.1: Type: text/plain, Size: 10044 bytes --]

On 4/12/2016 8:24 PM, Tamas K Lengyel wrote:
>
>
> On Tue, Apr 12, 2016 at 11:05 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>     On 4/12/2016 7:24 PM, Julien Grall wrote:
>
>         Hello,
>
>         On 12/04/2016 16:01, Tamas K Lengyel wrote:
>
>
>             On Apr 12, 2016 01:51, "Corneliu ZUZU"
>             <czuzu@bitdefender.com <mailto:czuzu@bitdefender.com>
>             <mailto:czuzu@bitdefender.com
>             <mailto:czuzu@bitdefender.com>>> wrote:
>              >
>              > On 4/11/2016 10:47 PM, Tamas K Lengyel wrote:
>              >>
>              >> From: Tamas K Lengyel <tklengyel@sec.in.tum.de
>             <mailto:tklengyel@sec.in.tum.de>
>             <mailto:tklengyel@sec.in.tum.de
>             <mailto:tklengyel@sec.in.tum.de>>>
>              >>
>              >> The ARM SMC instructions are already configured to
>             trap to Xen by
>             default. In
>              >> this patch we allow a user-space process in a
>             privileged domain to
>             receive
>              >> notification of when such event happens through the
>             vm_event subsystem.
>              >>
>              >> This patch will likely needs to be broken up into
>             several smaller
>             patches.
>              >> Right now what this patch adds (and could be broken
>             into smaller patches
>              >> accordingly):
>              >>      - Implement monitor_op domctl handler for
>             SOFTWARE_BREAKPOINTs
>             on ARM
>              >>      - Implement vm_event register fill/set routines
>             for ARM. This
>             required
>              >>          removing the function from common as the
>             function prototype now
>              >>          differs on the two archs.
>              >>      - Sending notification as SOFTWARE_BREAKPOINT
>             vm_event from the
>             SMC trap
>              >>          handlers.
>              >>      - Extend the xen-access test tool to receive SMC
>             notification
>             and step
>              >>          the PC manually in the reply.
>              >>
>              >> I'm sending it as an RFC to gather feedback on what
>             has been
>             overlooked in this
>              >> revision. This patch has been tested on a Cubietruck
>             board and works
>             fine,
>              >> but would probably not work on 64-bit boards.
>              >
>              >
>              > Hi Tamas,
>              >
>              > If I may, I'm still unable to work at the moment, being
>             ill, but I'm
>             checking the xen-devel lists from time to time.
>              > Your patch caught my attention, reminding me of the
>             conversation we
>             had some time ago on this matter.
>              > The only real reason I don't see SMC
>             (secure-monitor-call) as being
>             an ideal candidate for this is that, according to the ARM
>             manuals, SMC
>             should directly cause undefined exception if executed from
>             user-mode
>             (EL0), instead of a hypervisor trap - isn't that the case
>             on the machine
>             you tested this on or is this really only for the EL1 of
>             domains?
>
>
>         This paragraph is part of Corneliu's answer but it gives the
>         impression you wrote it. Can you configure your e-mail client
>         to quote properly?
>
>
>             That's correct, it can only be issued by the kernel. So as
>             long as you
>             want to monitor the kernel it can be used just fine. I can
>             also envision
>             trampoline-like traps (syscalls injected into EL0 to
>             trigger SMC) but
>             that's beyond the scope I intend this for now.
>
>
>     Then indeed SMC is the -easiest- way to go, @ least until
>     user-mode breakpoints are implemented.
>
>
>              >
>              > Also:
>              > - SMC, by definition, is a call to the secure side, it
>             doesn't relate
>             to debugging directly (it's a syscall to the 'secure'
>             side). There is a
>             viable INT3 equivalent on ARM, that being the BKPT/BRK
>             instruction,
>             using that instead would require a bit more effort (but would,
>             conceptually, be more correct) and might be less
>             performant, I suppose
>             that's why you didn't go for that?
>
>
>         BKPT/BRK could be used by the guest for debugging. You would
>         need to differentiate a breakpoint inserted by Xen or by a
>         debugger in the guest.
>
>
>     Isn't that also the case for X86's INT3? I guess differentiation
>     is done based on the bkpt address/privilege level? On ARM that
>     could also (partially) be done by looking @ the immediate value of
>     the BKPT/BRK instruction. Another thing I realized might be
>     troublesome with NOT using BKPT/BRK would be that on ARMv7 BKPT is
>     always unconditional, even in IT blocks. IDK if that applies to
>     SMC, but if it doesn't you'd have to check for that as well to
>     make sure the breakpoint is actually executed.
>
>
>
>             I would have to double check but AFAIK those instructions
>             can't be
>             configured to trap to the hypervisor directly. So while
>             SMC was not
>             intended to be a breakpoint, conceptually it's the closest
>             thing we have
>             an on ARM to the INT3 instruction when configured to trap
>             to the VMM.
>
>
>
>     Please see AArch32 HDCR.TDE and AArch64 MDCR_EL2.TDE bits. Since
>     activating this bit would imply additional (in this context
>     -unwanted-) traps, the performance hit of having this bit set
>     might be significant.
>
>
> Right, actually I believe KVM already implemented this, I was just 
> under the impression it was only for aarch64. As for performance 
> overhead I'm not that worried about it, rather I need to make sure the 
> presence of the monitoring can remain stealthy. I know on KVM for 
> example this type of trapping renders the guest to be unable to use 
> singlestepping, which would easily reveal the presence of the external 
> monitor (see 
> https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014589.html). 
> So this will need to be looked at carefully.

That seems to apply to single-stepping only, which would be a different 
matter. As for stealthiness or not limiting the guest, IMO that 
shouldn't be a problem with BKPT/BRK, since I believe you can inject the 
breakpoint exception into the guest as if no hypervisor trap occured in 
between (of course, once you decide whether that breakpoint is Xen's or 
guest-internal). But what about X86? How is stealthiness achieved there? 
Is INT3 entirely not available for the guest anymore when 
guest-debugging is enabled or are ALL INT3's reported by Xen as software 
breakpoint vm-events?

>
>         Whilst any access to SMC currently results to inject an
>         undefined exception, it may not be the case in the future.
>         There have been discussion to allow guest issuing SMC call
>         (see [1]).
>
>
> I don't see a problem with that, as long as it's configurable whether 
> SMC calls are trapped or pass-through.

Pass-through meaning "not trapped at all"? If yes, the problem would be 
that you won't be able to set breakpoints when SMC is configured to 
"completely" pass-through. But there's also the option of emulating the 
SMC, instead of not trapping it at all, when pass-through is needed, 
although IDK how complex that emulation would be.

>
>         I think the safest instruction would be HVC #imm. Xen is only
>         using a small number of immediate. You could allocate a
>         specific value for software debugging.
>

Another issue came to my mind: "HVC #imm", if handled through the 
hvm-ops code, currently requires setting other registers to predefined 
values before the HVC is actually issued. That would imply additional 
effort to save/restore those registers if an external privileged domain 
would want to set guest breakpoints. Given that, if we were to use HVC 
for sw-bkpts, IMO it would be nice if the hvm-ops code architecture 
would be slightly changed such that -lone- "HVM #imm" calls would be 
achievable for some use cases, such as this.

>
>
>     IMHO that would also be better conceptually, although it would
>     still suffer from the limitation of not being available from
>     user-space (and potentially from the above IT block issue).
>
>
> Sure, HVC would also be a possibility but I do see use-cases for 
> trapping SMC calls and forwarding them to a guest instead of the TZ. 
> For example, if malware tries to exploit TZ vulnerabilities, it would 
> be a lot easier to contain and monitor such exploits if the TZ is 
> virtualized rather then just crashing the guest or forwarding the 
> calls to a real TZ. So trapping SMC would allow us to use the real TZ 
> for other purposes and maintain a barrier between malicious guests and 
> the TZ.

Then you'd have to differentiate between a genuine guest SMC and a 
software-breakpoint SMC. IDK how much of a problem that would be.

>
> So what I will do instead of issuing a software_breakpoint vm_event 
> for SMCs, I'll introduce a new type, say 
> VM_EVENT_REASON_PRIVILEGED_CALL, that can be used to forward both 
> hypercalls and SMCs to a monitoring guest. This would also allow us to 
> use the software_breakpoint type for the actual software breakpoint 
> events in the future.
>
> Tamas

Isn't the HVC-part already achieved by guest-request vm-events? Maybe 
tying this vm-event specifically to SMC (in which case the name could be 
something like VM_EVENT_REASON_SECURE_CALL) and thus making it 
ARM-specific would avoid that redundancy?

Cheers,
Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 18568 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2330 bytes --]

On 13/04/16 09:55, Corneliu ZUZU wrote:
>
>
>>
>>
>>             I would have to double check but AFAIK those instructions
>>             can't be
>>             configured to trap to the hypervisor directly. So while
>>             SMC was not
>>             intended to be a breakpoint, conceptually it's the
>>             closest thing we have
>>             an on ARM to the INT3 instruction when configured to trap
>>             to the VMM.
>>
>>
>>
>>     Please see AArch32 HDCR.TDE and AArch64 MDCR_EL2.TDE bits. Since
>>     activating this bit would imply additional (in this context
>>     -unwanted-) traps, the performance hit of having this bit set
>>     might be significant.
>>
>>
>> Right, actually I believe KVM already implemented this, I was just
>> under the impression it was only for aarch64. As for performance
>> overhead I'm not that worried about it, rather I need to make sure
>> the presence of the monitoring can remain stealthy. I know on KVM for
>> example this type of trapping renders the guest to be unable to use
>> singlestepping, which would easily reveal the presence of the
>> external monitor (see
>> https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014589.html).
>> So this will need to be looked at carefully.
>
> That seems to apply to single-stepping only, which would be a
> different matter. As for stealthiness or not limiting the guest, IMO
> that shouldn't be a problem with BKPT/BRK, since I believe you can
> inject the breakpoint exception into the guest as if no hypervisor
> trap occured in between (of course, once you decide whether that
> breakpoint is Xen's or guest-internal). But what about X86? How is
> stealthiness achieved there? Is INT3 entirely not available for the
> guest anymore when guest-debugging is enabled or are ALL INT3's
> reported by Xen as software breakpoint vm-events?

In x86, attaching a debugger to the domain causes #DB and #BP exceptions
to be intercepted by Xen, rather than handled directly by the domain
(actually, since XSA-156, #DB is intercepted under all circumstances, to
avoid security issues).  The debugger receives all debug events, and
should filer the ones it has introduced vs the ones present from
in-guest activities.

~Andrew

(Whether this works or not is a separate matter, and largely depends on
the debugger.)

[-- Attachment #1.2: Type: text/html, Size: 4281 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Julien Grall]

Hello Corneliu,

On 13/04/16 09:55, Corneliu ZUZU wrote:
> On 4/12/2016 8:24 PM, Tamas K Lengyel wrote:
> Another issue came to my mind: "HVC #imm", if handled through the
> hvm-ops code, currently requires setting other registers to predefined
> values before the HVC is actually issued. That would imply additional
> effort to save/restore those registers if an external privileged domain
> would want to set guest breakpoints. Given that, if we were to use HVC
> for sw-bkpts, IMO it would be nice if the hvm-ops code architecture
> would be slightly changed such that -lone- "HVM #imm" calls would be
> achievable for some use cases, such as this.

That is not true. All the hypercalls are using the same #imm 
(XEN_HYPERCALL_TAG = 0xEA1), which indeed requires specific value in 
various registers to differentiate the HVM-ops.

It's up to us to define the requirements for the other #imm. For 
instance there are some #imm allocated for debugging (see do_debug_trap) 
that will dump the content of the registers.

>
>>
>> So what I will do instead of issuing a software_breakpoint vm_event
>> for SMCs, I'll introduce a new type, say
>> VM_EVENT_REASON_PRIVILEGED_CALL, that can be used to forward both
>> hypercalls and SMCs to a monitoring guest. This would also allow us to
>> use the software_breakpoint type for the actual software breakpoint
>> events in the future.
>>
>> Tamas
>
> Isn't the HVC-part already achieved by guest-request vm-events? Maybe
> tying this vm-event specifically to SMC (in which case the name could be
> something like VM_EVENT_REASON_SECURE_CALL) and thus making it
> ARM-specific would avoid that redundancy?

I would create 2 distinct events for HVC and SMC. It would make easier 
to differentiate them in userspace.

Regards,

-- 
Julien Grall

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Corneliu ZUZU]

[-- Attachment #1.1: Type: text/plain, Size: 3446 bytes --]

On 4/13/2016 1:17 PM, Andrew Cooper wrote:
> On 13/04/16 09:55, Corneliu ZUZU wrote:
>>
>>
>>>
>>>
>>>             I would have to double check but AFAIK those
>>>             instructions can't be
>>>             configured to trap to the hypervisor directly. So while
>>>             SMC was not
>>>             intended to be a breakpoint, conceptually it's the
>>>             closest thing we have
>>>             an on ARM to the INT3 instruction when configured to
>>>             trap to the VMM.
>>>
>>>
>>>
>>>     Please see AArch32 HDCR.TDE and AArch64 MDCR_EL2.TDE bits. Since
>>>     activating this bit would imply additional (in this context
>>>     -unwanted-) traps, the performance hit of having this bit set
>>>     might be significant.
>>>
>>>
>>> Right, actually I believe KVM already implemented this, I was just 
>>> under the impression it was only for aarch64. As for performance 
>>> overhead I'm not that worried about it, rather I need to make sure 
>>> the presence of the monitoring can remain stealthy. I know on KVM 
>>> for example this type of trapping renders the guest to be unable to 
>>> use singlestepping, which would easily reveal the presence of the 
>>> external monitor (see 
>>> https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014589.html). So 
>>> this will need to be looked at carefully.
>>
>> That seems to apply to single-stepping only, which would be a 
>> different matter. As for stealthiness or not limiting the guest, IMO 
>> that shouldn't be a problem with BKPT/BRK, since I believe you can 
>> inject the breakpoint exception into the guest as if no hypervisor 
>> trap occured in between (of course, once you decide whether that 
>> breakpoint is Xen's or guest-internal). But what about X86? How is 
>> stealthiness achieved there? Is INT3 entirely not available for the 
>> guest anymore when guest-debugging is enabled or are ALL INT3's 
>> reported by Xen as software breakpoint vm-events?
>
> In x86, attaching a debugger to the domain causes #DB and #BP 
> exceptions to be intercepted by Xen, rather than handled directly by 
> the domain (actually, since XSA-156, #DB is intercepted under all 
> circumstances, to avoid security issues).  The debugger receives all 
> debug events, and should filer the ones it has introduced vs the ones 
> present from in-guest activities.
>
> ~Andrew
>
> (Whether this works or not is a separate matter, and largely depends 
> on the debugger.) 

And after this filtering, I guess if the debug event proves to be 
introduced by in-guest activities, the exception is reintroduced to 
preserve transparency, correct?
I'm curious if behind the scenes Xen also write-protects that page such 
that the guest does not overwrite the INT3.
This approach, I think, could be used w/ BKPT/BRK instructions on ARM as 
well. After all, BKPT/BRK functionality is precisely that of INT3's with 
the slight enhancement of having an #imm attached.
But, as I said, I anticipate that the actual implementation differences 
for this vm-event on ARM, if using BKPT/BRK (compared to X86) will 
emerge due to the fact that on X86 INT3 can be trapped all by itself, 
whereas on ARM such granularity is not available for BKPT/BRK. Also, 
working with the debug architecture might prove to be a little bit 
elaborate. So I guess this is a question of balancing conceptual 
correctness vs being practical for the short run with far less effort. :-)

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 5849 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Corneliu ZUZU]

On 4/13/2016 1:52 PM, Julien Grall wrote:
> Hello Corneliu,
>
> On 13/04/16 09:55, Corneliu ZUZU wrote:
>> On 4/12/2016 8:24 PM, Tamas K Lengyel wrote:
>> Another issue came to my mind: "HVC #imm", if handled through the
>> hvm-ops code, currently requires setting other registers to predefined
>> values before the HVC is actually issued. That would imply additional
>> effort to save/restore those registers if an external privileged domain
>> would want to set guest breakpoints. Given that, if we were to use HVC
>> for sw-bkpts, IMO it would be nice if the hvm-ops code architecture
>> would be slightly changed such that -lone- "HVM #imm" calls would be
>> achievable for some use cases, such as this.
>
> That is not true. All the hypercalls are using the same #imm 
> (XEN_HYPERCALL_TAG = 0xEA1), which indeed requires specific value in 
> various registers to differentiate the HVM-ops.
>
> It's up to us to define the requirements for the other #imm. For 
> instance there are some #imm allocated for debugging (see 
> do_debug_trap) that will dump the content of the registers.
>

That's nice, I wonder why we didn't do that for guest-request vm-events...

>>
>>>
>>> So what I will do instead of issuing a software_breakpoint vm_event
>>> for SMCs, I'll introduce a new type, say
>>> VM_EVENT_REASON_PRIVILEGED_CALL, that can be used to forward both
>>> hypercalls and SMCs to a monitoring guest. This would also allow us to
>>> use the software_breakpoint type for the actual software breakpoint
>>> events in the future.
>>>
>>> Tamas
>>
>> Isn't the HVC-part already achieved by guest-request vm-events? Maybe
>> tying this vm-event specifically to SMC (in which case the name could be
>> something like VM_EVENT_REASON_SECURE_CALL) and thus making it
>> ARM-specific would avoid that redundancy?
>
> I would create 2 distinct events for HVC and SMC. It would make easier 
> to differentiate them in userspace.
>
> Regards,
>

As stated, HVC events are already implemented as guest-request vm-events 
(hence the redundancy that a VM_EVENT_REASON_PRIVILEGED_CALL would add).
So indeed, that would leave us with the option of simply creating 
another event for SMC.

Corneliu.

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3159 bytes --]

On 13/04/16 11:53, Corneliu ZUZU wrote:
> On 4/13/2016 1:17 PM, Andrew Cooper wrote:
>> On 13/04/16 09:55, Corneliu ZUZU wrote:
>>>
>>>
>>>>
>>>>
>>>>             I would have to double check but AFAIK those
>>>>             instructions can't be
>>>>             configured to trap to the hypervisor directly. So while
>>>>             SMC was not
>>>>             intended to be a breakpoint, conceptually it's the
>>>>             closest thing we have
>>>>             an on ARM to the INT3 instruction when configured to
>>>>             trap to the VMM.
>>>>
>>>>
>>>>
>>>>     Please see AArch32 HDCR.TDE and AArch64 MDCR_EL2.TDE bits.
>>>>     Since activating this bit would imply additional (in this
>>>>     context -unwanted-) traps, the performance hit of having this
>>>>     bit set might be significant.
>>>>
>>>>
>>>> Right, actually I believe KVM already implemented this, I was just
>>>> under the impression it was only for aarch64. As for performance
>>>> overhead I'm not that worried about it, rather I need to make sure
>>>> the presence of the monitoring can remain stealthy. I know on KVM
>>>> for example this type of trapping renders the guest to be unable to
>>>> use singlestepping, which would easily reveal the presence of the
>>>> external monitor (see
>>>> https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014589.html).
>>>> So this will need to be looked at carefully.
>>>
>>> That seems to apply to single-stepping only, which would be a
>>> different matter. As for stealthiness or not limiting the guest, IMO
>>> that shouldn't be a problem with BKPT/BRK, since I believe you can
>>> inject the breakpoint exception into the guest as if no hypervisor
>>> trap occured in between (of course, once you decide whether that
>>> breakpoint is Xen's or guest-internal). But what about X86? How is
>>> stealthiness achieved there? Is INT3 entirely not available for the
>>> guest anymore when guest-debugging is enabled or are ALL INT3's
>>> reported by Xen as software breakpoint vm-events?
>>
>> In x86, attaching a debugger to the domain causes #DB and #BP
>> exceptions to be intercepted by Xen, rather than handled directly by
>> the domain (actually, since XSA-156, #DB is intercepted under all
>> circumstances, to avoid security issues).  The debugger receives all
>> debug events, and should filer the ones it has introduced vs the ones
>> present from in-guest activities.
>>
>> ~Andrew
>>
>> (Whether this works or not is a separate matter, and largely depends
>> on the debugger.) 
>
> And after this filtering, I guess if the debug event proves to be
> introduced by in-guest activities, the exception is reintroduced to
> preserve transparency, correct?

That is certainly the idea.

> I'm curious if behind the scenes Xen also write-protects that page
> such that the guest does not overwrite the INT3.

Haha.  I refer to my "Whether this works or not is a separate matter"
statement.

No-one has made a product feature out of external debugging of a guest,
which means there are almost certainly logic holes and bugs.  This
write-protection looks like a prime issue which hasn't been considered.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 5984 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 3746 bytes --]

On Apr 13, 2016 6:06 AM, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>
> On 13/04/16 11:53, Corneliu ZUZU wrote:
>>
>> On 4/13/2016 1:17 PM, Andrew Cooper wrote:
>>>
>>> On 13/04/16 09:55, Corneliu ZUZU wrote:
>>>>
>>>>
>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> I would have to double check but AFAIK those instructions can't be
>>>>>>>> configured to trap to the hypervisor directly. So while SMC was not
>>>>>>>> intended to be a breakpoint, conceptually it's the closest thing
we have
>>>>>>>> an on ARM to the INT3 instruction when configured to trap to the
VMM.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Please see AArch32 HDCR.TDE and AArch64 MDCR_EL2.TDE bits. Since
activating this bit would imply additional (in this context -unwanted-)
traps, the performance hit of having this bit set might be significant.
>>>>>
>>>>>
>>>>> Right, actually I believe KVM already implemented this, I was just
under the impression it was only for aarch64. As for performance overhead
I'm not that worried about it, rather I need to make sure the presence of
the monitoring can remain stealthy. I know on KVM for example this type of
trapping renders the guest to be unable to use singlestepping, which would
easily reveal the presence of the external monitor (see
https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014589.html). So
this will need to be looked at carefully.
>>>>
>>>>
>>>> That seems to apply to single-stepping only, which would be a
different matter. As for stealthiness or not limiting the guest, IMO that
shouldn't be a problem with BKPT/BRK, since I believe you can inject the
breakpoint exception into the guest as if no hypervisor trap occured in
between (of course, once you decide whether that breakpoint is Xen's or
guest-internal). But what about X86? How is stealthiness achieved there? Is
INT3 entirely not available for the guest anymore when guest-debugging is
enabled or are ALL INT3's reported by Xen as software breakpoint vm-events?
>>>
>>>
>>> In x86, attaching a debugger to the domain causes #DB and #BP
exceptions to be intercepted by Xen, rather than handled directly by the
domain (actually, since XSA-156, #DB is intercepted under all
circumstances, to avoid security issues).  The debugger receives all debug
events, and should filer the ones it has introduced vs the ones present
from in-guest activities.
>>>
>>> ~Andrew
>>>
>>> (Whether this works or not is a separate matter, and largely depends on
the debugger.)
>>
>>
>> And after this filtering, I guess if the debug event proves to be
introduced by in-guest activities, the exception is reintroduced to
preserve transparency, correct?
>
>
> That is certainly the idea.
>
>
>> I'm curious if behind the scenes Xen also write-protects that page such
that the guest does not overwrite the INT3.
>
>
> Haha.  I refer to my "Whether this works or not is a separate matter"
statement.
>
> No-one has made a product feature out of external debugging of a guest,
which means there are almost certainly logic holes and bugs.  This
write-protection looks like a prime issue which hasn't been considered.
>

In the DRAKVUF system that's exactly what I do, I mark the page execute
only so that the guest is unable to locate/overwrite injected breakpoints
without notice. If it were to overwrite injected breakpoints with its own,
then we would be able to tell that the trap is both for external and
internal use. So there isn't much of an issue there. The main issue is with
the racecondition in multi-vCPU guests when the purely external-use
breakpoint has to be removed to allow the guest to continue. It can be
solved nicely though with altp2m. I did a write-up for the Xen blog about
it a couple months ago and sent it to publicity but has not appeared yet.
Lars?

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4735 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Lars Kurth]

[-- Attachment #1.1: Type: text/plain, Size: 1317 bytes --]


> On 13 Apr 2016, at 14:25, Tamas K Lengyel <tamas.k.lengyel@gmail.com> wrote:
> In the DRAKVUF system that's exactly what I do, I mark the page execute only so that the guest is unable to locate/overwrite injected breakpoints without notice. If it were to overwrite injected breakpoints with its own, then we would be able to tell that the trap is both for external and internal use. So there isn't much of an issue there. The main issue is with the racecondition in multi-vCPU guests when the purely external-use breakpoint has to be removed to allow the guest to continue. It can be solved nicely though with altp2m. I did a write-up for the Xen blog about it a couple months ago and sent it to publicity but has not appeared yet. Lars?
> 
Tamas,

it hasn't because I was under the impression that Razvan and you disagreed on some aspects of the article. And then I forgot to chase either of you. I am happy with the article: feel free to upload it to the blog (or let me know, if I should) and press the button. Apologies

I think there are a couple of minor knit-picks, such as replace "In the latest release of Xen last summer several new features have been introduced" In "Xen 4.6 several new features have been introduced" ... assuming 4.6 is correct

I will reply to publicity

Regards
Lars


[-- Attachment #1.2: Type: text/html, Size: 1856 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1845 bytes --]

On Wed, Apr 13, 2016 at 9:06 AM, Lars Kurth <lars.kurth.xen@gmail.com>
wrote:

>
> On 13 Apr 2016, at 14:25, Tamas K Lengyel <tamas.k.lengyel@gmail.com>
> wrote:
>
> In the DRAKVUF system that's exactly what I do, I mark the page execute
> only so that the guest is unable to locate/overwrite injected breakpoints
> without notice. If it were to overwrite injected breakpoints with its own,
> then we would be able to tell that the trap is both for external and
> internal use. So there isn't much of an issue there. The main issue is with
> the racecondition in multi-vCPU guests when the purely external-use
> breakpoint has to be removed to allow the guest to continue. It can be
> solved nicely though with altp2m. I did a write-up for the Xen blog about
> it a couple months ago and sent it to publicity but has not appeared yet.
> Lars?
>
> Tamas,
>
> it hasn't because I was under the impression that Razvan and you disagreed
> on some aspects of the article. And then I forgot to chase either of you. I
> am happy with the article: feel free to upload it to the blog (or let me
> know, if I should) and press the button. Apologies
>
> I think there are a couple of minor knit-picks, such as replace "In the
> latest release of Xen last summer several new features have been
> introduced" In "Xen 4.6 several new features have been introduced" ...
> assuming 4.6 is correct
>
> I will reply to publicity
>
> Regards
> Lars
>

Hey Lars,
I think the discussion with Razvan was mostly just around the difference of
our perception of how emulation based solutions fare compared to altp2m. I
think it's a discussion that actually could continue on the blogpost
comment wall, or if Razvan feel like it in a follow-up blogpost ;) So feel
free to make any changes to the article you see fit and hit release
whenever you feel like.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 2582 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 2338 bytes --]

>
>
>
>>>
>>>> I would have to double check but AFAIK those instructions can't be
>>>> configured to trap to the hypervisor directly. So while SMC was not
>>>> intended to be a breakpoint, conceptually it's the closest thing we have
>>>> an on ARM to the INT3 instruction when configured to trap to the VMM.
>>>>
>>>
>>>
>> Please see AArch32 HDCR.TDE and AArch64 MDCR_EL2.TDE bits. Since
>> activating this bit would imply additional (in this context -unwanted-)
>> traps, the performance hit of having this bit set might be significant.
>>
>
> Right, actually I believe KVM already implemented this, I was just under
> the impression it was only for aarch64. As for performance overhead I'm not
> that worried about it, rather I need to make sure the presence of the
> monitoring can remain stealthy. I know on KVM for example this type of
> trapping renders the guest to be unable to use singlestepping, which would
> easily reveal the presence of the external monitor (see
> https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014589.html). So
> this will need to be looked at carefully.
>
>
> That seems to apply to single-stepping only, which would be a different
> matter.
>

If you read the commit message on the previous patch in that thread that
actually enables TDE trapping (
https://lists.cs.columbia.edu/pipermail/kvmarm/2015-May/014621.html) it
says: "Any other guest software debug exception (e.g. single step or HW
assisted breakpoints) will cause an error and the VM to be killed." So it
sounds to me singlestep on ARM is also routed as a software debug exception
and thus would be trapped (again, I would need to double-check the manual).
The follow up patch I linked earlier implements handling it but requires
the supression of the guest being able to singlestep itself. We might be
able to work around that if we can reinject the singlestep exception to the
guest. So all I'm saying is that this needs to be looked at carefully as
there may be issues there, especially for the use-case I have in mind.

And while having singlestepping trap to the hypervisor is very handy I
actually have a better method to hide the presence of say injected SMCs,
albeit it requires altp2m. Fortunately we have some students who proposed
implementing it this summer through the Honeynet Project's Google Summer of
Code ;)

Tamas

[-- Attachment #1.2: Type: text/html, Size: 4106 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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