Re: [RFC PATCH v6 02/92] kvm: introspection: add basic ioctls (hook/unhook)

2019-08-13 Thread Adalbert Lazăr
We'll do.

On Tue, 13 Aug 2019 10:44:28 +0200, Paolo Bonzini  wrote:
> On 09/08/19 17:59, Adalbert Lazăr wrote:
> > +static int kvmi_recv(void *arg)
> > +{
> > +   struct kvmi *ikvm = arg;
> > +
> > +   kvmi_info(ikvm, "Hooking VM\n");
> > +
> > +   while (kvmi_msg_process(ikvm))
> > +   ;
> > +
> > +   kvmi_info(ikvm, "Unhooking VM\n");
> > +
> > +   kvmi_end_introspection(ikvm);
> > +
> > +   return 0;
> > +}
> > +
> 
> Rename this to kvmi_recv_thread instead, please.
> 
> > +
> > +   /*
> > +* Make sure all the KVM/KVMI structures are linked and no pointer
> > +* is read as NULL after the reference count has been set.
> > +*/
> > +   smp_mb__before_atomic();
> 
> This is an smp_wmb(), not an smp_mb__before_atomic().  Add a comment
> that it pairs with the refcount_inc_not_zero in kvmi_get.
> 
> > +   refcount_set(>kvmi_ref, 1);
> > +
> 
> 
> > @@ -57,8 +183,27 @@ void kvmi_destroy_vm(struct kvm *kvm)
> > if (!ikvm)
> > return;
> >  
> > +   /* trigger socket shutdown - kvmi_recv() will start shutdown process */
> > +   kvmi_sock_shutdown(ikvm);
> > +
> > kvmi_put(kvm);
> >  
> > /* wait for introspection resources to be released */
> > wait_for_completion_killable(>kvmi_completed);
> >  }
> > +
> 
> This addition means that kvmi_destroy_vm should have called
> kvmi_end_introspection instead.  In patch 1, kvmi_end_introspection
> should have been just kvmi_put, now this patch can add kvmi_sock_shutdown.
> 
> Paolo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH v6 02/92] kvm: introspection: add basic ioctls (hook/unhook)

2019-08-13 Thread Paolo Bonzini
On 09/08/19 17:59, Adalbert Lazăr wrote:
> +static int kvmi_recv(void *arg)
> +{
> + struct kvmi *ikvm = arg;
> +
> + kvmi_info(ikvm, "Hooking VM\n");
> +
> + while (kvmi_msg_process(ikvm))
> + ;
> +
> + kvmi_info(ikvm, "Unhooking VM\n");
> +
> + kvmi_end_introspection(ikvm);
> +
> + return 0;
> +}
> +

Rename this to kvmi_recv_thread instead, please.

> +
> + /*
> +  * Make sure all the KVM/KVMI structures are linked and no pointer
> +  * is read as NULL after the reference count has been set.
> +  */
> + smp_mb__before_atomic();

This is an smp_wmb(), not an smp_mb__before_atomic().  Add a comment
that it pairs with the refcount_inc_not_zero in kvmi_get.

> + refcount_set(>kvmi_ref, 1);
> +


> @@ -57,8 +183,27 @@ void kvmi_destroy_vm(struct kvm *kvm)
>   if (!ikvm)
>   return;
>  
> + /* trigger socket shutdown - kvmi_recv() will start shutdown process */
> + kvmi_sock_shutdown(ikvm);
> +
>   kvmi_put(kvm);
>  
>   /* wait for introspection resources to be released */
>   wait_for_completion_killable(>kvmi_completed);
>  }
> +

This addition means that kvmi_destroy_vm should have called
kvmi_end_introspection instead.  In patch 1, kvmi_end_introspection
should have been just kvmi_put, now this patch can add kvmi_sock_shutdown.

Paolo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[RFC PATCH v6 02/92] kvm: introspection: add basic ioctls (hook/unhook)

2019-08-09 Thread Adalbert Lazăr
The connection of the introspection socket with the introspection tool
is initialized by userspace/QEMU. Once the handshake is done, the file
descriptor is passed to KVMi using the KVM_INTROSPECTION_HOOK ioctl. A
new thread will be created to handle/dispatch all introspection commands
or replies to introspection events. This thread will finish when the
socket is closed by userspace (eg. when the guest is restarted) or by
the introspection tool. The uuid member of struct kvm_introspection is
used to show the guest id with the error messages.

On certain actions from userspace (pause, suspend, migrate, etc.) the
KVM_INTROSPECTION_UNHOOK ioctl is used to notify the introspection tool
to remove its hooks (eg. breakpoints).

Suggested-by: Stefan Hajnoczi 
Suggested-by: Paolo Bonzini 
Co-developed-by: Mihai Donțu 
Signed-off-by: Mihai Donțu 
Co-developed-by: Mircea Cîrjaliu 
Signed-off-by: Mircea Cîrjaliu 
Co-developed-by: Adalbert Lazăr 
Signed-off-by: Adalbert Lazăr 
---
 Documentation/virtual/kvm/api.txt  |  50 ++
 Documentation/virtual/kvm/kvmi.rst |  65 +
 arch/x86/kvm/Makefile  |   2 +-
 arch/x86/kvm/x86.c |   7 ++
 include/linux/kvmi.h   |   4 +
 include/uapi/linux/kvm.h   |  11 +++
 virt/kvm/kvm_main.c|   8 ++
 virt/kvm/kvmi.c| 145 +
 virt/kvm/kvmi_int.h|  31 ++
 virt/kvm/kvmi_msg.c|  42 +
 10 files changed, 364 insertions(+), 1 deletion(-)
 create mode 100644 virt/kvm/kvmi_msg.c

diff --git a/Documentation/virtual/kvm/api.txt 
b/Documentation/virtual/kvm/api.txt
index 356156f5c52d..28d4429f9ae9 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3857,6 +3857,56 @@ number of valid entries in the 'entries' array, which is 
then filled.
 'index' and 'flags' fields in 'struct kvm_cpuid_entry2' are currently reserved,
 userspace should not expect to get any particular value there.
 
+4.996 KVM_INTROSPECTION_HOOK
+
+Capability: KVM_CAP_INTROSPECTION
+Architectures: x86
+Type: vm ioctl
+Parameters: struct kvm_introspection (in)
+Returns: 0 on success, a negative value on error
+
+This ioctl is used to enable the introspection of the current VM.
+
+struct kvm_introspection {
+   __s32 fd;
+   __u32 padding;
+   __u8 uuid[16];
+};
+
+fd is the file handle of a socket connected to the introspection tool,
+
+padding must be zero (it might be used in the future),
+
+uuid is used for debug and error messages.
+
+It can fail with -EFAULT if:
+ - memory allocation failed
+ - this VM is already introspected
+ - the file handle doesn't correspond to an active socket
+
+It will fail with -EINVAL if padding is not zero.
+
+The KVMI version can be retrieved using the KVM_CAP_INTROSPECTION of
+the KVM_CHECK_EXTENSION ioctl() at run-time.
+
+4.997 KVM_INTROSPECTION_UNHOOK
+
+Capability: KVM_CAP_INTROSPECTION
+Architectures: x86
+Type: vm ioctl
+Parameters: none
+Returns: 0 on success, a negative value on error
+
+This ioctl is used to disable the introspection of the current VM.
+It is useful when the VM is paused/suspended/migrated.
+
+It can fail with -EFAULT if:
+  - the introspection is not enabled
+  - the socket (passed with KVM_INTROSPECTION_HOOK) had an error
+
+If the ioctl is successful, the userspace should give the introspection
+tool a chance to unhook the VM.
+
 5. The kvm_run structure
 
 
diff --git a/Documentation/virtual/kvm/kvmi.rst 
b/Documentation/virtual/kvm/kvmi.rst
index d54caf8d974f..47b7c36d334a 100644
--- a/Documentation/virtual/kvm/kvmi.rst
+++ b/Documentation/virtual/kvm/kvmi.rst
@@ -64,6 +64,71 @@ used on that guest. Obviously, whether the guest can really 
continue
 normal execution depends on whether the introspection tool has made any
 modifications that require an active KVMI channel.
 
+Handshake
+-
+
+Although this falls out of the scope of the introspection subsystem, below
+is a proposal of a handshake that can be used by implementors.
+
+Based on the system administration policies, the management tool
+(eg. libvirt) starts device managers (eg. QEMU) with some extra arguments:
+what introspector could monitor/control that specific guest (and how to
+connect to) and what introspection commands/events are allowed.
+
+The device manager will connect to the introspection tool and wait for a
+cryptographic hash of a cookie that should be known by both peers. If the
+hash is correct (the destination has been "authenticated"), the device
+manager will send another cryptographic hash and random salt. The peer
+recomputes the hash of the cookie bytes including the salt and if they match,
+the device manager has been "authenticated" too. This is a rather crude
+system that makes it difficult for device manager exploits to trick the
+introspection tool into believing its working OK.
+
+The cookie would normally be generated by a management