From: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
To: Jan Beulich <jbeulich@suse.com>,
Razvan COJOCARU <rcojocaru@bitdefender.com>,
"tamas@tklengyel.com" <tamas@tklengyel.com>
Cc: Petre Ovidiu PIRCALABU <ppircalabu@bitdefender.com>,
"wl@xen.org" <wl@xen.org>,
"george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>,
"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
"paul.durrant@citrix.com" <paul.durrant@citrix.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
"roger.pau@citrix.com" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v9] x86/emulate: Send vm_event from emulate
Date: Wed, 11 Sep 2019 10:39:02 +0000 [thread overview]
Message-ID: <e7330f19-9350-54f2-a066-730f1ab3c145@bitdefender.com> (raw)
In-Reply-To: <4ae96ee9-192d-f790-b2cb-9d60a5aab292@suse.com>
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-09-11 10:39 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-09 15:35 [Xen-devel] [PATCH v9] x86/emulate: Send vm_event from emulate Alexandru Stefan ISAILA
2019-09-11 9:57 ` Jan Beulich
2019-09-11 10:39 ` Alexandru Stefan ISAILA [this message]
2019-09-11 11:21 ` Razvan Cojocaru
2019-09-11 11:41 ` Jan Beulich
2019-09-11 11:44 ` Razvan Cojocaru
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e7330f19-9350-54f2-a066-730f1ab3c145@bitdefender.com \
--to=aisaila@bitdefender.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=paul.durrant@citrix.com \
--cc=ppircalabu@bitdefender.com \
--cc=rcojocaru@bitdefender.com \
--cc=roger.pau@citrix.com \
--cc=tamas@tklengyel.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).