Re: [Xen-devel] [PATCH v9] x86/emulate: Send vm_event from emulate

[Jan Beulich <jbeulich@suse.com>]

On 09.09.2019 17:35, Alexandru Stefan ISAILA wrote:
> A/D bit writes (on page walks) can be considered benign by an introspection
> agent, so receiving vm_events for them is a pessimization. We try here to
> optimize by filtering these events out.
> Currently, we are fully emulating the instruction at RIP when the hardware sees
> an EPT fault with npfec.kind != npfec_kind_with_gla. This is, however,
> incorrect, because the instruction at RIP might legitimately cause an
> EPT fault of its own while accessing a _different_ page from the original one,
> where A/D were set.
> The solution is to perform the whole emulation, while ignoring EPT restrictions
> for the walk part, and taking them into account for the "actual" emulating of
> the instruction at RIP. When we send out a vm_event, we don't want the emulation
> to complete, since in that case we won't be able to veto whatever it is doing.
> That would mean that we can't actually prevent any malicious activity, instead
> we'd only be able to report on it.
> When we see a "send-vm_event" case while emulating, we need to first send the
> event out and then suspend the emulation (return X86EMUL_RETRY).
> After the emulation stops we'll call hvm_vm_event_do_resume() again after the
> introspection agent treats the event and resumes the guest. There, the
> instruction at RIP will be fully emulated (with the EPT ignored) if the
> introspection application allows it, and the guest will continue to run past
> the instruction.
> 
> A common example is if the hardware exits because of an EPT fault caused by a
> page walk, p2m_mem_access_check() decides if it is going to send a vm_event.
> If the vm_event was sent and it would be treated so it runs the instruction
> at RIP, that instruction might also hit a protected page and provoke a vm_event.
> 
> Now if npfec.kind == npfec_kind_in_gpt and d->arch.monitor.inguest_pagefault_disabled
> is true then we are in the page walk case and we can do this emulation optimization
> and emulate the page walk while ignoring the EPT, but don't ignore the EPT for the
> emulation of the actual instruction.
> 
> In the first case we would have 2 EPT events, in the second case we would have
> 1 EPT event if the instruction at the RIP triggers an EPT event.
> 
> We use hvmemul_map_linear_addr() to intercept r/w access and
> __hvm_copy() to intercept exec access.

Just like said for v8 - this doesn't look to match the implementation.

> hvm_emulate_send_vm_event() can return false if there was no violation,
> if there was an error from monitor_traps() or p2m_get_mem_access().
> Returning false if p2m_get_mem_access() fails is needed because the EPT
> entry will have rwx memory access rights.

I have to admit I still don't understand this reasoning, but I
guess I should leave it to the VM event maintainers to judge.
In particular it's unclear to me why p2m_get_mem_access()
failure would imply rwx access.

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -544,10 +544,11 @@ static void *hvmemul_map_linear_addr(
>      struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
>      struct vcpu *curr = current;
> -    void *err, *mapping;
> +    void *err = NULL, *mapping;

As also said during v8 review, I don't think this (and the related)
changes is needed anymore now that you've moved your new goto into
the loop.

> @@ -215,6 +217,79 @@ void hvm_monitor_interrupt(unsigned int vector, unsigned int type,
>      monitor_traps(current, 1, &req);
>  }
>  
> +/*
> + * Send memory access vm_events based on pfec. Returns true if the event was
> + * sent and false for p2m_get_mem_access() error, no violation and event send
> + * error. Assumes the caller will check arch.vm_event->send_event.
> + *
> + * NOTE: p2m_get_mem_access() can fail if the entry was not found in the EPT
> + * (in which case access to it is unrestricted, so no violations can occur).
> + * In this cases it is fine to continue the emulation.
> + */
> +bool hvm_monitor_check_ept(unsigned long gla, gfn_t gfn, uint32_t pfec,
> +                           uint16_t kind)

Why did you choose to have "ept" in the name and also mention it
in the commit? Is there anything in here which isn't generic p2m?

> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -212,8 +212,9 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>      }
>      if ( vm_event_check_ring(d->vm_event_monitor) &&
>           d->arch.monitor.inguest_pagefault_disabled &&
> -         npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
> +         npfec.kind == npfec_kind_in_gpt ) /* don't send a mem_event */
>      {
> +        v->arch.vm_event->send_event = true;

Since I'm being puzzled every time I see this: The comment and
the line you add look to be in curious disagreement. Do you
perhaps want to extend it to include something like "right
away", or make it e.g. "try to avoid sending a mem event"?
Personally I think it wouldn't hurt to even mention the "why"
here.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel