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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 11.09.2019 12:57, Jan Beulich wrote:
> 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.

I thought it is simpler to init err with NULL but you are right there is 
no need for this in this patch. I will revert the changes.

> 
>> @@ -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?

The name was suggested by Razvan Cojocaru. I have no preference in the 
name. Maybe Tamas can suggest a good one.

> 
>> --- 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.

I agree, I will update that comment.

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