Re: [Xen-devel] [PATCH v3] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 39a05fd..cf58fd5 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -41,6 +41,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "decode.h"
>>  #include "vtimer.h"
>> @@ -2527,6 +2528,16 @@ bad_data_abort:
>>  inject_dabt_exception(regs, info.gva, hsr.len);
>>  }
>>
>> +static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
>> +{
>> +int rc = 0;
>
>
> Missing blank line.
>
>> +if ( current->domain->arch.monitor.privileged_call_enabled )
>> +rc = monitor_smc();
>> +
>> +if ( rc != 1 )
>> +inject_undef_exception(regs, hsr);
>
>
> It would be worth mentioning somewhere that you expect the monitor app
> skipping the instruction.
>

Not necessarily. If it's an SMC the guest issues by itself then yes,
it can be safely turned effectively into a NOP by incrementing PC. But
there are other things the monitor application can do as well. For
example, during malware analysis if we want to remain stealthy we
would still want to inject an undefined instruction exception, or even
destroy the guest altogether, otherwise the NOP-d SMC would reveal the
presence of the monitor application. For SMC's that the monitor
application itself injects, just incrementing the PC would not be
enough either as we overwrote an instruction that should be executed
to continue normal execution. That problem can also be solved, and
also not by incrementing the PC.

Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

On Fri, Sep 16, 2016 at 7:54 AM, Julien Grall  wrote:
> Hi Tamas,
>
> On 15/09/2016 20:24, Tamas K Lengyel wrote:
>>
>> 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 by
>> introducing the PRIVILEGED_CALL type.
>>
>> The intended use-case for this feature is for a monitor application to be
>> able
>> insert tap-points into the domU kernel-code. For this task only
>> unconditional
>> SMC instruction should be used.
>>
>> Signed-off-by: Tamas K Lengyel 
>> Acked-by: Wei Liu 
>> ---
>> Cc: Ian Jackson 
>> Cc: Razvan Cojocaru 
>> Cc: Stefano Stabellini 
>> Cc: Julien Grall 
>>
>> v3: Rebase on latest master
>>
>> Note: previous discussion around this patch proposed filtering SMCs with
>> failed
>>   condition checks. As that patch is yet-to-be implemented and the 4.8
>>   code-freeze rapidly approaching I would like this patch to get
>> included.
>
>
> It is still in my todo list. But as I said last time, it is towards the
> bottom of it. I would have expected a bit of help from your side here rather
> than putting the fault on me.

Not putting a fault on anyone. I did take a look at it but there had
been enough unknowns for me that I would rather have you do it when
you get to it. I think this is just a minor issue that should not be a
roadblock for this patch. If it gets filtered at some point in the
future it will have no impact on applications that this interface is
intended for. For all else, a warning is in place not to rely on this
behavior being a "feature".

>
>>   In this patch a proper warning is placed in the public header for
>>   potential users not to rely on SMCs with failed condition checks
>> being
>>   trapped. As the intended use-case for this feature doesn't use
>>   conditional SMCs this warning should be sufficient. Hardware that
>> does
>>   issue events for SMCs with failed condition checks doesn't pose a
>> problem
>>   for a monitor application in any way. The xen-access test tool
>> illustrates
>>   how SMCs issued by the guest can be safely handled for all cases.
>
>
> This is nice, but how about passing the immediate to the event monitor? The
> SMC call is not meant to be a breakpoint instruction but a way to call the
> supervisor monitor (and potentially be emulated by the hypervisor).

I understand that was the original intention behind the instruction.
So when the emulation use-case becomes actual the immediate can be
included and passed along too, we would just bump the
VM_EVENT_INTERFACE_VERSION. As that was not my usecase it is beyond
scope for this patch. It can certainly be implemented at some later
time.

>
>
>> ---
>>  tools/libxc/include/xenctrl.h   |  2 +
>>  tools/libxc/xc_monitor.c| 14 +++
>>  tools/tests/xen-access/xen-access.c | 32 ++-
>>  xen/arch/arm/Makefile   |  1 +
>>  xen/arch/arm/monitor.c  | 79
>> +
>>  xen/arch/arm/traps.c| 15 ++-
>>  xen/include/asm-arm/domain.h|  5 +++
>>  xen/include/asm-arm/monitor.h   | 18 +++--
>>  xen/include/public/domctl.h |  1 +
>>  xen/include/public/vm_event.h   |  7 
>>  10 files changed, 158 insertions(+), 16 deletions(-)
>>  create mode 100644 xen/arch/arm/monitor.c
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 560ce7b..eb53172 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -2168,6 +2168,8 @@ int xc_monitor_guest_request(xc_interface *xch,
>> domid_t domain_id,
>>  int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
>>  bool enable, bool sync);
>>  int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
>> +int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
>> +   bool enable);
>>  /**
>>   * This function enables / disables emulation for each REP for a
>>   * REP-compatible instruction.
>> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
>> index 4298813..15a7c32 100644
>> --- a/tools/libxc/xc_monitor.c
>> +++ b/tools/libxc/xc_monitor.c
>> @@ -185,6 +185,20 @@ int xc_monitor_cpuid(xc_interface *xch, domid_t
>> domain_id, bool enable)
>>  return do_domctl(xch, );
>>  }
>>
>> +int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
>> +   bool enable)
>> +{
>> +DECLARE_DOMCTL;
>> +
>> +domctl.cmd = XEN_DOMCTL_monitor_op;
>> +domctl.domain = domain_id;
>> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
>> +

Re: [Xen-devel] [PATCH v3] vm_event: Implement ARM SMC events

[Julien Grall]

Hi Tamas,

On 15/09/2016 20:24, Tamas K Lengyel wrote:

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 by
introducing the PRIVILEGED_CALL type.

The intended use-case for this feature is for a monitor application to be able
insert tap-points into the domU kernel-code. For this task only unconditional
SMC instruction should be used.

Signed-off-by: Tamas K Lengyel 
Acked-by: Wei Liu 
---
Cc: Ian Jackson 
Cc: Razvan Cojocaru 
Cc: Stefano Stabellini 
Cc: Julien Grall 

v3: Rebase on latest master

Note: previous discussion around this patch proposed filtering SMCs with failed
  condition checks. As that patch is yet-to-be implemented and the 4.8
  code-freeze rapidly approaching I would like this patch to get included.


It is still in my todo list. But as I said last time, it is towards the 
bottom of it. I would have expected a bit of help from your side here 
rather than putting the fault on me.



  In this patch a proper warning is placed in the public header for
  potential users not to rely on SMCs with failed condition checks being
  trapped. As the intended use-case for this feature doesn't use
  conditional SMCs this warning should be sufficient. Hardware that does
  issue events for SMCs with failed condition checks doesn't pose a problem
  for a monitor application in any way. The xen-access test tool illustrates
  how SMCs issued by the guest can be safely handled for all cases.


This is nice, but how about passing the immediate to the event monitor? 
The SMC call is not meant to be a breakpoint instruction but a way to 
call the supervisor monitor (and potentially be emulated by the hypervisor).



---
 tools/libxc/include/xenctrl.h   |  2 +
 tools/libxc/xc_monitor.c| 14 +++
 tools/tests/xen-access/xen-access.c | 32 ++-
 xen/arch/arm/Makefile   |  1 +
 xen/arch/arm/monitor.c  | 79 +
 xen/arch/arm/traps.c| 15 ++-
 xen/include/asm-arm/domain.h|  5 +++
 xen/include/asm-arm/monitor.h   | 18 +++--
 xen/include/public/domctl.h |  1 +
 xen/include/public/vm_event.h   |  7 
 10 files changed, 158 insertions(+), 16 deletions(-)
 create mode 100644 xen/arch/arm/monitor.c

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 560ce7b..eb53172 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2168,6 +2168,8 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t 
domain_id,
 int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
 bool enable, bool sync);
 int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
+int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
+   bool enable);
 /**
  * This function enables / disables emulation for each REP for a
  * REP-compatible instruction.
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 4298813..15a7c32 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -185,6 +185,20 @@ int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, 
bool enable)
 return do_domctl(xch, );
 }

+int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
+   bool enable)
+{
+DECLARE_DOMCTL;
+
+domctl.cmd = XEN_DOMCTL_monitor_op;
+domctl.domain = domain_id;
+domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+: XEN_DOMCTL_MONITOR_OP_DISABLE;
+domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL;
+
+return do_domctl(xch, );
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/tests/xen-access/xen-access.c 
b/tools/tests/xen-access/xen-access.c
index ed18c71..6eefe0c 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -338,6 +338,8 @@ void usage(char* progname)
 fprintf(stderr, "Usage: %s [-m]  write|exec", progname);
 #if defined(__i386__) || defined(__x86_64__)
 fprintf(stderr, 
"|breakpoint|altp2m_write|altp2m_exec|debug|cpuid");
+#elif defined(__arm__) || defined(__aarch64__)
+fprintf(stderr, "|privcall");
 #endif
 fprintf(stderr,
 "\n"
@@ -362,6 +364,7 @@ int main(int argc, char *argv[])
 int required = 0;
 int breakpoint = 0;
 int shutting_down = 0;
+int privcall = 0;
 int altp2m = 0;
 int debug = 0;
 int cpuid = 0;
@@ -431,6 +434,11 @@ int main(int argc, char *argv[])
 {
 cpuid = 1;
 }
+#elif defined(__arm__) 

Re: [Xen-devel] [PATCH v3] vm_event: Implement ARM SMC events

[Razvan Cojocaru]

On 09/15/16 21:24, Tamas K Lengyel wrote:
> 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 by
> introducing the PRIVILEGED_CALL type.
> 
> The intended use-case for this feature is for a monitor application to be able
> insert tap-points into the domU kernel-code. For this task only unconditional
> SMC instruction should be used.
> 
> Signed-off-by: Tamas K Lengyel 
> Acked-by: Wei Liu 
> ---
> Cc: Ian Jackson 
> Cc: Razvan Cojocaru 
> Cc: Stefano Stabellini 
> Cc: Julien Grall 

Acked-by: Razvan Cojocaru 


Thanks,
Razvan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v3] vm_event: Implement ARM SMC events

[Tamas K Lengyel]

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 by
introducing the PRIVILEGED_CALL type.

The intended use-case for this feature is for a monitor application to be able
insert tap-points into the domU kernel-code. For this task only unconditional
SMC instruction should be used.

Signed-off-by: Tamas K Lengyel 
Acked-by: Wei Liu 
---
Cc: Ian Jackson 
Cc: Razvan Cojocaru 
Cc: Stefano Stabellini 
Cc: Julien Grall 

v3: Rebase on latest master

Note: previous discussion around this patch proposed filtering SMCs with failed
  condition checks. As that patch is yet-to-be implemented and the 4.8
  code-freeze rapidly approaching I would like this patch to get included.
  In this patch a proper warning is placed in the public header for
  potential users not to rely on SMCs with failed condition checks being
  trapped. As the intended use-case for this feature doesn't use
  conditional SMCs this warning should be sufficient. Hardware that does
  issue events for SMCs with failed condition checks doesn't pose a problem
  for a monitor application in any way. The xen-access test tool illustrates
  how SMCs issued by the guest can be safely handled for all cases.
---
 tools/libxc/include/xenctrl.h   |  2 +
 tools/libxc/xc_monitor.c| 14 +++
 tools/tests/xen-access/xen-access.c | 32 ++-
 xen/arch/arm/Makefile   |  1 +
 xen/arch/arm/monitor.c  | 79 +
 xen/arch/arm/traps.c| 15 ++-
 xen/include/asm-arm/domain.h|  5 +++
 xen/include/asm-arm/monitor.h   | 18 +++--
 xen/include/public/domctl.h |  1 +
 xen/include/public/vm_event.h   |  7 
 10 files changed, 158 insertions(+), 16 deletions(-)
 create mode 100644 xen/arch/arm/monitor.c

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 560ce7b..eb53172 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2168,6 +2168,8 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t 
domain_id,
 int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
 bool enable, bool sync);
 int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
+int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
+   bool enable);
 /**
  * This function enables / disables emulation for each REP for a
  * REP-compatible instruction.
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 4298813..15a7c32 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -185,6 +185,20 @@ int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, 
bool enable)
 return do_domctl(xch, );
 }
 
+int xc_monitor_privileged_call(xc_interface *xch, domid_t domain_id,
+   bool enable)
+{
+DECLARE_DOMCTL;
+
+domctl.cmd = XEN_DOMCTL_monitor_op;
+domctl.domain = domain_id;
+domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+: XEN_DOMCTL_MONITOR_OP_DISABLE;
+domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL;
+
+return do_domctl(xch, );
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/tests/xen-access/xen-access.c 
b/tools/tests/xen-access/xen-access.c
index ed18c71..6eefe0c 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -338,6 +338,8 @@ void usage(char* progname)
 fprintf(stderr, "Usage: %s [-m]  write|exec", progname);
 #if defined(__i386__) || defined(__x86_64__)
 fprintf(stderr, 
"|breakpoint|altp2m_write|altp2m_exec|debug|cpuid");
+#elif defined(__arm__) || defined(__aarch64__)
+fprintf(stderr, "|privcall");
 #endif
 fprintf(stderr,
 "\n"
@@ -362,6 +364,7 @@ int main(int argc, char *argv[])
 int required = 0;
 int breakpoint = 0;
 int shutting_down = 0;
+int privcall = 0;
 int altp2m = 0;
 int debug = 0;
 int cpuid = 0;
@@ -431,6 +434,11 @@ int main(int argc, char *argv[])
 {
 cpuid = 1;
 }
+#elif defined(__arm__) || defined(__aarch64__)
+else if ( !strcmp(argv[0], "privcall") )
+{
+privcall = 1;
+}
 #endif
 else
 {
@@ -563,6 +571,16 @@ int main(int argc, char *argv[])
 }
 }
 
+if ( privcall )
+{
+rc = xc_monitor_privileged_call(xch, domain_id, 1);
+if ( rc < 0 )
+{
+ERROR("Error %d setting privileged call trapping with vm_event\n", 
rc);
+goto exit;
+}
+}
+
 /*