Add basic KVMI event support for CR/MSR/interrupt/mem_access (v5) #9

mtarral · 2019-08-28T13:47:46Z

Hi,

this PR adds basic event support for

CR
MSR
memory
interrupt (int 3)

Implementation

Details

no event response logic is implemented ATM, simply reply to continue execution
kvm_set_reg_access
- handles CR0/CR3/CR4
- handles MSR_ANY, where the MSR index is specified in reg_event.msr field
- handles MSR_ALL by looping over all MSRs in Libvmi MSR index msr_all
- CR0/CR3/CR4 kvmi equivalent reg values are hardcoded
- VMI_REGACCESS_R and VMI_REGACCESS_RW not supported (?)
- doesn't support MSR_HYPERVISOR: 0x40000000
- on failure (a call to a VCPU failed, goto error_exit), it disables everything, but without checking the return value. (any ideas how to improve that ?)
msr-event-example
- modified to accept kvmi socket path (optional)
- modified to init libvmi without os config, so can be init when OS boots, and intercept MSR being setup
kvm_set_intr_access
- simply call kvmi_control_events for every VCPU, enabling breakpint, or disabling everything
kvm_set_mem_access
- declares a static variable, and checks it to enable KVMI_EVENT_PF_FLAG on the first call, if necessary, this avoids to enable them at kvm_init_vmi, even though the app might never use them. What do you think ?
- assumes VMI_MEMACCESS_N and VMI_MEMACCESS_RWX are the same (is it though ?)
- hardcoded 12 shift to get a gpa from a gfn
- again, per VCPU events and page config makes it difficult for error handling
kvm_events_listen
- wait next event
- pop from event queue
- checks if event reason makes sense
- call the handler if defined

For all events i'm assigning a pointer to a x86_regs struct from the stack,
and call fill_ev_common_kvmi_to_libvmi to fill the regs.

-> Any suggestions of how this should be done ?

process_register
not handled because it doesn't work for CR3 when i tested :-)
process_msr
- TODO: fill reg_event
  - out_access
process_interrupt
- hardcoded page shift 12
- TODO: fill interrupt_event
  - vector
  - type
  - Should we set the insn_length here ?
  - What are INT_NEXT interrupts ?
process_pagefault
- hardcoded page shift 12
- TODO: fill mem_event
  - valid
  - gptw
process_pause_event
- we should never reach this code, since kvm_resume_vm should pop this type of event
- print and error and return a failure
kvm_init_vmi
- set event handlers if VMI_INIT_EVENTS

Notes

msr-index.h has been moved from libvmi/driver/xen/ to libvmi
add enum to string msr_to_str array
activating the same event/flag for every VCPU can be a bit cumbersome sometimes, and makes error handling more tricky in my opinion.
sorry for this big PR, I wanted to makes multiples PR for every feature, but I ended up building static functions and helpers, so I had to stack them up.

Issues

catching CR3 doesn't work as I expected (at context switch) (fix in Kvmi v6 kvm#21)
catching MSR_HYPERVISOR 0x40000000 is not supported by the API

Future

Fixes

use a global event_flags for all VCPU, to store the old state and avoid erasing it

V6

Rebase on #8

cc @mdontu, @adlazar, @tklengyel

Thanks.

tklengyel · 2019-08-28T21:28:52Z

libvmi/events.c

@@ -94,6 +94,8 @@ void step_event_free(vmi_event_t *event, status_t rc)
 status_t events_init(vmi_instance_t vmi)
 {
    switch (vmi->mode) {
+        case VMI_KVM:
+            // intentional fall-through


Why fall-through instead of break?

good catch, it's a mistake.

mtarral · 2019-08-30T08:48:54Z

libvmi/driver/kvm/kvm.c

+    if (event->reg != MSR_ALL) {
+        // enable event monitoring for all vcpus
+        for (unsigned int i = 0; i < vmi->num_vcpus; i++) {
+            if (kvmi_control_events(kvm->kvmi_dom, i, event_flags)) {


@adlazar: does enabling the interception of KVMi events, without generating this type of events, has a slight performance impact, or close to none ?

The idea is that I would like to move enabling/disabling event interception at driver init, so I could enable all of them at once, and disable them at teardown.

Otherwise, with the current implementation, we have to keep track of what reg_access has been enabled for example, or the following use case won't work:

enable an event to intercept CR3

enable an event to intercept CR0

disable CR0 event -> CR3 is disabled too because KVMI_EVENT_CR(_FLAG) has been disabled

For MSR and CR events, you can enable them with kvmi_control_events() without worrying about the the performance impact, because the interception does something when you enable a specific MSR/CR (kvmi_control_msr/kvmi_control_cr).

But, if you enable KVMI_EVENT_BREAKPOINT and KVMI_EVENT_DESCRIPTOR, you might get a lot of events when the guest starts. For the second one we might filter out the read events in the future (intercepting only the write accesses).

mtarral

General questions I have regarding the implementation

mtarral · 2019-08-30T12:48:38Z

libvmi/msr-index.h

+
+#include "libvmi.h"
+
+extern const reg_t msr_all[];


@tklengyel I also wanted your review about moving msr-index out of the Xen driver, to be used by KVM as well.
I had to declare the arrays as extern to use them multiple times in the code, and give them an implementation in libvmi/msr-index.c

mtarral · 2019-08-30T12:52:23Z

libvmi/driver/kvm/kvm.c

+}
+
+static status_t
+process_msr(vmi_instance_t vmi, struct kvmi_dom_event *kvmi_event)


@tklengyel a naïve implementation of event handlers that I choose:

fill the libvmi event with fill_ev_common_kvmi_to_libvmi()

call the user callback with call_event_callback()

declare the kvmi reply struct

process the callback response with process_cb_response()

As you have written the Xen driver, you may have suggestions how the code should be structured, which helpers should be used, etc.

mtarral · 2019-08-30T12:53:54Z

libvmi/driver/kvm/kvm.c

@@ -52,6 +53,9 @@
 #include <sys/time.h>
 #include "driver/kvm/include/kvmi/libkvmi.h"

+/*
+ * Helpers
+ */
 static uint32_t
 translate_msr_index(int index, int *err) {


this function will be removed thanks to msr-index array:
https://github.com/KVM-VMI/libvmi/pull/9/files#diff-a342220ea96575ee149235454636ce4eR55

mtarral · 2019-08-30T12:55:55Z

Once this will be approved and merged in kvmi branch, I will proceed to rebase on V6.

Wenzel · 2019-09-16T21:04:58Z

pinging also @smaresca, if he is interested for a review 👋

smaresca · 2019-09-16T21:08:02Z

@Wenzel thanks! Will take a look

tklengyel · 2019-09-16T21:37:42Z

@Wenzel I would suggest moving the events code out of kvm.c into a separate kvm_events.c, just helps with not making these files so large

Wenzel · 2019-09-16T21:38:40Z

@tklengyel this has been done already, it's in a future PR, after a rebase on V6.

mtarral · 2019-09-18T12:49:45Z

@tklengyel , @smaresca actually I'm waiting for this one to be merged to publish my other PR based on v6.
It will bring events splitted into their own modules, singlestepping and other small improvments.

How do you feel so far about this one ?

smaresca · 2019-09-25T13:54:35Z

@mtarral I had some time over the weekend to set up a test environment. I'll have specific feedback tonight/tomorrow.

mtarral · 2019-09-25T14:28:18Z

@smaresca thanks for the feedback.
If you have any issues regarding the setup, or any questions, don't forget the KVM-VMI Slack channel.

mtarral · 2019-10-01T11:20:48Z

@smaresca how did it go ?
were you able to run the examples ?

mtarral · 2019-10-04T15:46:20Z

Rebased on latest kvmi branch.

only handles CR3 events

mtarral · 2019-10-04T16:00:12Z

@smaresca , @tklengyel since I published the v6 PR, which is cleaner because the events are splitted and it uses the new libkvmi API to control events (which is better), I'm inclined to merge this one and focus on v6.

tklengyel reviewed Aug 28, 2019

View reviewed changes

mtarral force-pushed the kvmi_events branch from 3417a7e to 321b279 Compare August 29, 2019 09:47

mtarral mentioned this pull request Aug 29, 2019

How to make event-example in example support KVM ? libvmi/libvmi#778

Open

mtarral commented Aug 30, 2019

View reviewed changes

mtarral mentioned this pull request Oct 1, 2019

Implementing vmi_events_listen(0) #12

Closed

mtarral force-pushed the kvmi_events branch from 321b279 to e585baa Compare October 4, 2019 15:45

mtarral added 13 commits October 4, 2019 17:49

kvm: add kvm_set_reg_access

d05c731

only handles CR3 events

kvm: add kvm_events_listen

34c3bd2

events: add KVM support at event_init

a9fe718

kvm: check access type in kvm_set_reg_access

9bf5ee3

examples: add cr3-load example

ff8261b

kvm: use errprint instead of dbprint in kvm_set_reg_access

435394d

kvm: add debug output in kvm_set_reg_access

0996c0e

kvm: add support for CR0/CR4 in kvm_set_reg_access

7a48737

kvm: add safety checks in kvm_set_reg_access

3043522

kvm: handle MSR_STAR in kvm_set_reg_access

05ad8c5

kvm: add safety checks in kvm_events_listen

9a94111

kvm: add dispatcher to process events in their own handlers

382eb34

kvm: add basic event handler for registers (CR/MSR)

cf17f5b

mtarral added 24 commits October 4, 2019 17:49

kvm: enable PF events at first call in kvm_set_mem_access

816385e

kvm: reset original page permissions on failure

8b8895f

kvm: impl process_pagefault handler

3e55e98

example: add logging mem-event-example's callback

7b135b2

kvm: add basic event response handler

ab5481e

kvm: remove commented code in kvm_events_listen

7f4d4a0

kvm: set default event response as continue

40656f7

moved msr-index.h under libvmi

9ca85bc

msr-index: add array to convert MSR index to string

348ab4a

examples: add support for kvmi socket in msr-event-example

72bb53a

examples: improve error handling in msr-event-example

44186ff

examples: use vmi_init in msr-event-example

db6ab30

kvm: add basic handler for MSR events

3fb7fea

examples: print values using PRI notation in msr-event-example

16a6b98

kvm: impl MSR event handler

4e4fa83

kvm: add comments

b6ee23a

kvm: add debug output

d564ad8

kvm: print errno message when kvmi calls failed

3c12d3e

kvm: handle MSR_ANY

3010988

kvm: add TODOs

7bcf71c

kvm: set mem_event gfn in process_pagefault

07b2069

kvm: use specific reply structs instead of the generic one

95ac5fc

kvm: write kvmi event MSR value in MSR event reply

f9678bd

examples: fix warnings for unused parameters

cf2ff35

mtarral force-pushed the kvmi_events branch from e585baa to cf2ff35 Compare October 4, 2019 15:49

mtarral mentioned this pull request Oct 4, 2019

Add handler for CR events #13

Merged

Wenzel merged commit deaaa74 into KVM-VMI:kvmi Oct 7, 2019

mtarral deleted the kvmi_events branch October 7, 2019 08:08

mtarral mentioned this pull request Oct 18, 2019

Integration of the new KVM driver based on KVMi APIs libvmi/libvmi#827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic KVMI event support for CR/MSR/interrupt/mem_access (v5) #9

Add basic KVMI event support for CR/MSR/interrupt/mem_access (v5) #9

mtarral commented Aug 28, 2019

tklengyel Aug 28, 2019

mtarral Aug 29, 2019

mtarral Aug 29, 2019

mtarral Aug 30, 2019 •

edited

Loading

adlazar Aug 30, 2019

mtarral left a comment

mtarral Aug 30, 2019

mtarral Aug 30, 2019 •

edited

Loading

mtarral Aug 30, 2019

mtarral commented Aug 30, 2019 •

edited

Loading

Wenzel commented Sep 16, 2019

smaresca commented Sep 16, 2019

tklengyel commented Sep 16, 2019

Wenzel commented Sep 16, 2019 •

edited

Loading

mtarral commented Sep 18, 2019

smaresca commented Sep 25, 2019

mtarral commented Sep 25, 2019

mtarral commented Oct 1, 2019

mtarral commented Oct 4, 2019

mtarral commented Oct 4, 2019

Add basic KVMI event support for CR/MSR/interrupt/mem_access (v5) #9

Add basic KVMI event support for CR/MSR/interrupt/mem_access (v5) #9

Conversation

mtarral commented Aug 28, 2019

Implementation

Details

Notes

Issues

Future

Fixes

V6

tklengyel Aug 28, 2019

Choose a reason for hiding this comment

mtarral Aug 29, 2019

Choose a reason for hiding this comment

mtarral Aug 29, 2019

Choose a reason for hiding this comment

mtarral Aug 30, 2019 • edited Loading

Choose a reason for hiding this comment

adlazar Aug 30, 2019

Choose a reason for hiding this comment

mtarral left a comment

Choose a reason for hiding this comment

mtarral Aug 30, 2019

Choose a reason for hiding this comment

mtarral Aug 30, 2019 • edited Loading

Choose a reason for hiding this comment

mtarral Aug 30, 2019

Choose a reason for hiding this comment

mtarral commented Aug 30, 2019 • edited Loading

Wenzel commented Sep 16, 2019

smaresca commented Sep 16, 2019

tklengyel commented Sep 16, 2019

Wenzel commented Sep 16, 2019 • edited Loading

mtarral commented Sep 18, 2019

smaresca commented Sep 25, 2019

mtarral commented Sep 25, 2019

mtarral commented Oct 1, 2019

mtarral commented Oct 4, 2019

mtarral commented Oct 4, 2019

mtarral Aug 30, 2019 •

edited

Loading

mtarral Aug 30, 2019 •

edited

Loading

mtarral commented Aug 30, 2019 •

edited

Loading

Wenzel commented Sep 16, 2019 •

edited

Loading