| Message ID | 3f577545b8ee6846ff98c4411cdc96dfe1412b3e.1678505295.git.isaikin-dmitry@yandex.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN] x86/monitor: Add new monitor event to catch I/O instructions | expand |
On Fri, Mar 10, 2023 at 10:57 PM Dmitry Isaykin <isaikin-dmitry@yandex.ru> wrote: > > Adds monitor support for I/O instructions. > > Signed-off-by: Dmitry Isaykin <isaikin-dmitry@yandex.ru> > Signed-off-by: Anton Belousov <abelousov@ptsecurity.com> > --- > tools/include/xenctrl.h | 1 + > tools/libs/ctrl/xc_monitor.c | 13 +++++++++++++ > xen/arch/x86/hvm/hvm.c | 5 +++++ > xen/arch/x86/hvm/monitor.c | 21 +++++++++++++++++++++ > xen/arch/x86/hvm/vmx/vmx.c | 2 ++ > xen/arch/x86/include/asm/domain.h | 1 + > xen/arch/x86/include/asm/hvm/monitor.h | 3 +++ > xen/arch/x86/include/asm/hvm/support.h | 3 +++ > xen/arch/x86/include/asm/monitor.h | 3 ++- > xen/arch/x86/monitor.c | 13 +++++++++++++ > xen/include/public/domctl.h | 1 + > xen/include/public/vm_event.h | 10 ++++++++++ > 12 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index 23037874d3..05967ecc92 100644 > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -2102,6 +2102,7 @@ int xc_monitor_emul_unimplemented(xc_interface *xch, uint32_t domain_id, > bool enable); > int xc_monitor_vmexit(xc_interface *xch, uint32_t domain_id, bool enable, > bool sync); > +int xc_monitor_io(xc_interface *xch, uint32_t domain_id, bool enable); > /** > * This function enables / disables emulation for each REP for a > * REP-compatible instruction. > diff --git a/tools/libs/ctrl/xc_monitor.c b/tools/libs/ctrl/xc_monitor.c > index c5fa62ff30..3cb96f444f 100644 > --- a/tools/libs/ctrl/xc_monitor.c > +++ b/tools/libs/ctrl/xc_monitor.c > @@ -261,6 +261,19 @@ int xc_monitor_vmexit(xc_interface *xch, uint32_t domain_id, bool enable, > return do_domctl(xch, &domctl); > } > > +int xc_monitor_io(xc_interface *xch, uint32_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_IO; > + > + return do_domctl(xch, &domctl); > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 0c81e2afc7..72c9f65626 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3484,6 +3484,11 @@ int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len) > return hvm_monitor_cpuid(inst_len, leaf, subleaf); > } > > +void hvm_io_instruction_intercept(uint16_t port, int dir, unsigned int bytes, unsigned int string_ins) > +{ > + hvm_monitor_io_instruction(port, dir, bytes, string_ins); > +} > + > void hvm_rdtsc_intercept(struct cpu_user_regs *regs) > { > msr_split(regs, hvm_get_guest_tsc(current)); > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c > index a11cd76f4d..f8b11d1de9 100644 > --- a/xen/arch/x86/hvm/monitor.c > +++ b/xen/arch/x86/hvm/monitor.c > @@ -233,6 +233,27 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf, > return monitor_traps(curr, 1, &req); > } > > +void hvm_monitor_io_instruction(uint16_t port, int dir, > + unsigned int bytes, unsigned int string_ins) > +{ > + struct vcpu *curr = current; > + struct arch_domain *ad = &curr->domain->arch; > + vm_event_request_t req = {}; > + > + if ( !ad->monitor.io_enabled ) > + return; > + > + req.reason = VM_EVENT_REASON_IO_INSTRUCTION; > + req.u.io_instruction.data_size = bytes; > + req.u.io_instruction.port = port; > + req.u.io_instruction.dir = dir; > + req.u.io_instruction.string_ins = string_ins; > + > + set_npt_base(curr, &req); > + > + monitor_traps(curr, true, &req); > +} > + > void hvm_monitor_interrupt(unsigned int vector, unsigned int type, > unsigned int err, uint64_t cr2) > { > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 278b829f73..a64c5078c5 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -4579,6 +4579,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > uint16_t port = (exit_qualification >> 16) & 0xFFFF; > int bytes = (exit_qualification & 0x07) + 1; > int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE; > + int str_ins = (exit_qualification & 0x10) ? 1 : 0; You are already in a branch here where str_ins is checked and known to be 1. > + hvm_io_instruction_intercept(port, dir, bytes, str_ins); IMHO you should have this intercept be called outside the if-else. The function already kind-of indicates str_ins is an input yet right now only called when it's 1. The rest of the plumbing in the patch LGTM. Thanks, Tamas
On 11/03/2023 3:57 am, Dmitry Isaykin wrote: > Adds monitor support for I/O instructions. > > Signed-off-by: Dmitry Isaykin <isaikin-dmitry@yandex.ru> > Signed-off-by: Anton Belousov <abelousov@ptsecurity.com> Thankyou for the patch. A couple of questions. Right now, you monitor all IO ports that end up in a vmexit. But this is already subject to hvm_io_bitmap[] which means by default that port 0x80 and 0xed are not intercepted. Therefore, you'll not pick them up with this monitor either. Is this intentional? MSRs (which are the other monitored resource with a fine grain bitmap) take a different approach, requiring each monitored MSR to be explicitly opted in to. Are you able to discuss your intended usecase here at all? > --- > tools/include/xenctrl.h | 1 + > tools/libs/ctrl/xc_monitor.c | 13 +++++++++++++ > xen/arch/x86/hvm/hvm.c | 5 +++++ > xen/arch/x86/hvm/monitor.c | 21 +++++++++++++++++++++ > xen/arch/x86/hvm/vmx/vmx.c | 2 ++ You've wired up all the control infrastructure as common, but only plugged hvm_io_instruction_intercept() in to the Intel side. All you need to make the AMD side work equivalently is to hook VMEXIT_IOIO in svm.c in a similar manner to how you've already modified EXIT_REASON_IO_INSTRUCTION in vmx.c Please do this. I know the monitor subsystem doesn't get much love on AMD, but we do want to do a best-effort attempt to maintain parity. > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 278b829f73..a64c5078c5 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -4579,6 +4579,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > uint16_t port = (exit_qualification >> 16) & 0xFFFF; > int bytes = (exit_qualification & 0x07) + 1; > int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE; > + int str_ins = (exit_qualification & 0x10) ? 1 : 0; > + hvm_io_instruction_intercept(port, dir, bytes, str_ins); I'm afraid this can't be correct. Separate to Tamas' observation, in the case that we do monitor the IO port, we must not continue with the rest of the logic. Otherwise what we'll end up doing is putting a monitor event on the monitor ring, *and* sending the same event to qemu (or terminating it with internal emulation). This is fine if all you're trying to do is log the access, but doesn't work if the monitor framework wants first refusal of the access. Monitor functions typically return int, identifying whether the vCPU has been paused, which then controls whether the subsequent intercept logic is performed. See hvm_monitor_{vmexit,debug}() as examples. > diff --git a/xen/arch/x86/include/asm/hvm/monitor.h b/xen/arch/x86/include/asm/hvm/monitor.h > index 639f6dfa37..22d2b366a6 100644 > --- a/xen/arch/x86/include/asm/hvm/monitor.h > +++ b/xen/arch/x86/include/asm/hvm/monitor.h > @@ -54,6 +54,9 @@ bool hvm_monitor_check_p2m(unsigned long gla, gfn_t gfn, uint32_t pfec, > int hvm_monitor_vmexit(unsigned long exit_reason, > unsigned long exit_qualification); > > +void hvm_monitor_io_instruction(uint16_t port, int dir, > + unsigned int bytes, unsigned int string_ins); Please could this be named hvm_monitor_io() for consistency with the rest of the monitor subsystem? > diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > index 0035c26e12..86e4cdba7c 100644 > --- a/xen/include/public/vm_event.h > +++ b/xen/include/public/vm_event.h > @@ -388,6 +390,13 @@ struct vm_event_vmexit { > } arch; > }; > > +struct vm_event_io_instruction { > + uint32_t data_size; > + uint32_t port; > + uint32_t dir; > + uint32_t string_ins; > +}; For a string instruction, don't you need %rsi/%rdi too? ~Andrew
On 11.03.2023 15:50, Tamas K Lengyel wrote: > On Fri, Mar 10, 2023 at 10:57 PM Dmitry Isaykin <isaikin-dmitry@yandex.ru> >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -4579,6 +4579,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) >> uint16_t port = (exit_qualification >> 16) & 0xFFFF; >> int bytes = (exit_qualification & 0x07) + 1; >> int dir = (exit_qualification & 0x08) ? IOREQ_READ : > IOREQ_WRITE; >> + int str_ins = (exit_qualification & 0x10) ? 1 : 0; > > You are already in a branch here where str_ins is checked and known to be 1. 0 that is, i.e. only non-string insns would presently be reported. Dmitry, you also want to make this variable and the corresponding function parameters "bool". Unless of course they need changing anyway to e.g. communicate the address of the data (as also indicated by Andrew). >> + hvm_io_instruction_intercept(port, dir, bytes, str_ins); > > IMHO you should have this intercept be called outside the if-else. The > function already kind-of indicates str_ins is an input yet right now only > called when it's 1. I agree. Question is, as above, what further data may need supplying to the monitor agent to make the intercept useful. Merely %rsi / %rdi as Andrew suggested may not suffice, as especially outside of 64-bit mode the involved segment base may be non-zero. Jan
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index 23037874d3..05967ecc92 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -2102,6 +2102,7 @@ int xc_monitor_emul_unimplemented(xc_interface *xch, uint32_t domain_id, bool enable); int xc_monitor_vmexit(xc_interface *xch, uint32_t domain_id, bool enable, bool sync); +int xc_monitor_io(xc_interface *xch, uint32_t domain_id, bool enable); /** * This function enables / disables emulation for each REP for a * REP-compatible instruction. diff --git a/tools/libs/ctrl/xc_monitor.c b/tools/libs/ctrl/xc_monitor.c index c5fa62ff30..3cb96f444f 100644 --- a/tools/libs/ctrl/xc_monitor.c +++ b/tools/libs/ctrl/xc_monitor.c @@ -261,6 +261,19 @@ int xc_monitor_vmexit(xc_interface *xch, uint32_t domain_id, bool enable, return do_domctl(xch, &domctl); } +int xc_monitor_io(xc_interface *xch, uint32_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_IO; + + return do_domctl(xch, &domctl); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 0c81e2afc7..72c9f65626 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3484,6 +3484,11 @@ int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len) return hvm_monitor_cpuid(inst_len, leaf, subleaf); } +void hvm_io_instruction_intercept(uint16_t port, int dir, unsigned int bytes, unsigned int string_ins) +{ + hvm_monitor_io_instruction(port, dir, bytes, string_ins); +} + void hvm_rdtsc_intercept(struct cpu_user_regs *regs) { msr_split(regs, hvm_get_guest_tsc(current)); diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c index a11cd76f4d..f8b11d1de9 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -233,6 +233,27 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf, return monitor_traps(curr, 1, &req); } +void hvm_monitor_io_instruction(uint16_t port, int dir, + unsigned int bytes, unsigned int string_ins) +{ + struct vcpu *curr = current; + struct arch_domain *ad = &curr->domain->arch; + vm_event_request_t req = {}; + + if ( !ad->monitor.io_enabled ) + return; + + req.reason = VM_EVENT_REASON_IO_INSTRUCTION; + req.u.io_instruction.data_size = bytes; + req.u.io_instruction.port = port; + req.u.io_instruction.dir = dir; + req.u.io_instruction.string_ins = string_ins; + + set_npt_base(curr, &req); + + monitor_traps(curr, true, &req); +} + void hvm_monitor_interrupt(unsigned int vector, unsigned int type, unsigned int err, uint64_t cr2) { diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 278b829f73..a64c5078c5 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -4579,6 +4579,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) uint16_t port = (exit_qualification >> 16) & 0xFFFF; int bytes = (exit_qualification & 0x07) + 1; int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE; + int str_ins = (exit_qualification & 0x10) ? 1 : 0; + hvm_io_instruction_intercept(port, dir, bytes, str_ins); if ( handle_pio(port, bytes, dir) ) update_guest_eip(); /* Safe: IN, OUT */ } diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h index b5354c3677..86bd3493e6 100644 --- a/xen/arch/x86/include/asm/domain.h +++ b/xen/arch/x86/include/asm/domain.h @@ -428,6 +428,7 @@ struct arch_domain unsigned int descriptor_access_enabled : 1; unsigned int guest_request_userspace_enabled : 1; unsigned int emul_unimplemented_enabled : 1; + unsigned int io_enabled : 1; /* * By default all events are sent. * This is used to filter out pagefaults. diff --git a/xen/arch/x86/include/asm/hvm/monitor.h b/xen/arch/x86/include/asm/hvm/monitor.h index 639f6dfa37..22d2b366a6 100644 --- a/xen/arch/x86/include/asm/hvm/monitor.h +++ b/xen/arch/x86/include/asm/hvm/monitor.h @@ -54,6 +54,9 @@ bool hvm_monitor_check_p2m(unsigned long gla, gfn_t gfn, uint32_t pfec, int hvm_monitor_vmexit(unsigned long exit_reason, unsigned long exit_qualification); +void hvm_monitor_io_instruction(uint16_t port, int dir, + unsigned int bytes, unsigned int string_ins); + #endif /* __ASM_X86_HVM_MONITOR_H__ */ /* diff --git a/xen/arch/x86/include/asm/hvm/support.h b/xen/arch/x86/include/asm/hvm/support.h index 6b583738ec..ecb2b9ecb8 100644 --- a/xen/arch/x86/include/asm/hvm/support.h +++ b/xen/arch/x86/include/asm/hvm/support.h @@ -157,6 +157,9 @@ int __must_check hvm_msr_read_intercept( int __must_check hvm_msr_write_intercept( unsigned int msr, uint64_t msr_content, bool may_defer); +void hvm_io_instruction_intercept(uint16_t port, int dir, + unsigned int bytes, unsigned int string_ins); + #endif /* __ASM_X86_HVM_SUPPORT_H__ */ /* diff --git a/xen/arch/x86/include/asm/monitor.h b/xen/arch/x86/include/asm/monitor.h index d8d54c5f23..96e6a9d0d8 100644 --- a/xen/arch/x86/include/asm/monitor.h +++ b/xen/arch/x86/include/asm/monitor.h @@ -90,7 +90,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d) (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) | (1U << XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED) | (1U << XEN_DOMCTL_MONITOR_EVENT_INGUEST_PAGEFAULT) | - (1U << XEN_DOMCTL_MONITOR_EVENT_VMEXIT)); + (1U << XEN_DOMCTL_MONITOR_EVENT_VMEXIT) | + (1U << XEN_DOMCTL_MONITOR_EVENT_IO)); if ( hvm_is_singlestep_supported() ) capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP); diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 30ca71432c..d4857faf8a 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -346,6 +346,19 @@ int arch_monitor_domctl_event(struct domain *d, break; } + case XEN_DOMCTL_MONITOR_EVENT_IO: + { + bool old_status = ad->monitor.io_enabled; + + if ( unlikely(old_status == requested_status) ) + return -EEXIST; + + domain_pause(d); + ad->monitor.io_enabled = requested_status; + domain_unpause(d); + break; + } + default: /* * Should not be reached unless arch_monitor_get_capabilities() is diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 51be28c3de..7280e9f968 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1063,6 +1063,7 @@ struct xen_domctl_psr_cmt_op { /* Enabled by default */ #define XEN_DOMCTL_MONITOR_EVENT_INGUEST_PAGEFAULT 11 #define XEN_DOMCTL_MONITOR_EVENT_VMEXIT 12 +#define XEN_DOMCTL_MONITOR_EVENT_IO 13 struct xen_domctl_monitor_op { uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */ diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index 0035c26e12..86e4cdba7c 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -160,6 +160,8 @@ #define VM_EVENT_REASON_EMUL_UNIMPLEMENTED 14 /* VMEXIT */ #define VM_EVENT_REASON_VMEXIT 15 +/* IN/OUT Instruction executed */ +#define VM_EVENT_REASON_IO_INSTRUCTION 16 /* Supported values for the vm_event_write_ctrlreg index. */ #define VM_EVENT_X86_CR0 0 @@ -388,6 +390,13 @@ struct vm_event_vmexit { } arch; }; +struct vm_event_io_instruction { + uint32_t data_size; + uint32_t port; + uint32_t dir; + uint32_t string_ins; +}; + typedef struct vm_event_st { uint32_t version; /* VM_EVENT_INTERFACE_VERSION */ uint32_t flags; /* VM_EVENT_FLAG_* */ @@ -409,6 +418,7 @@ typedef struct vm_event_st { struct vm_event_debug debug_exception; struct vm_event_cpuid cpuid; struct vm_event_vmexit vmexit; + struct vm_event_io_instruction io_instruction; union { struct vm_event_interrupt_x86 x86; } interrupt;