Re: [Xen-devel] [PATCH V6 5/5] xen: Handle resumed instruction based on previous mem_event reply

[Andres Lagar Cavilla <andres@xxxxxxxxxxxxxxxx>]

From: George Dunlap <dunlapg@xxxxxxxxx>
Date: Wed, Sep 10, 2014 at 9:38 AM
Subject: Re: [Xen-devel] [PATCH V6 5/5] xen: Handle resumed instruction based on previous mem_event reply
To: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
Cc: "Tian, Kevin" <kevin.tian@xxxxxxxxx>, Keir Fraser <keir@xxxxxxx>, Ian Campbell <ian.campbell@xxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, "Dong, Eddie" <eddie.dong@xxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxx>


On Wed, Sep 10, 2014 at 5:12 PM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 09/10/2014 07:03 PM, George Dunlap wrote:
>> On Tue, Sep 9, 2014 at 11:28 AM, Razvan Cojocaru
>> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> In a scenario where a page fault that triggered a mem_event occured,
>>> p2m_mem_access_check() will now be able to either 1) emulate the
>>> current instruction, or 2) emulate it, but don't allow it to perform
>>> any writes.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>> Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>> [snip]
>>
>>> +Â Â else if ( v->arch.mem_event.emulate_flags == 0 &&
>>> +Â Â Â Â Â Â Â npfec.kind != npfec_kind_with_gla ) /* don't send a mem_event */
>>> +Â Â {
>>> +Â Â Â Â v->arch.mem_event.emulate_flags = MEM_EVENT_FLAG_EMULATE;
>>> +Â Â Â Â v->arch.mem_event.gpa = gpa;
>>> +Â Â Â Â v->arch.mem_event.eip = eip;
>>> +Â Â }
>>
>> It looks like the previous if() is true, that it will never get to
>> this point (because it will either return 0 or 1 depending on whether
>> p2m->access_required is set). So you don't need to make this an
>> "else" here -- you should just add a blank line and make this a normal
>> if().
>>
>> Also, maybe it's just because I'm not familiar with the mem_event
>> interface, but I don't really see what this code is doing. It seems
>> to be changing the behavior even for clients that aren't using
>> MEM_EVENT_FLAG_EMULATE*. Is that intended? In any case it seems like
>> there could be a better comment here.
>
> Thanks, those are very good points. I'll make that a regular if(), and
> test also if introspection monitoring is enabled (please see patch 3/5:
> d->arch.hvm_domain.introspection_enabled) before setting the emulate
> flag, that way we won't alter the behaviour for other clients.

...and you should also put a comment there explaining why someone with
introspection enabled wouldn't want an event here (something I'm still
not clear on).

Are you *sure* that everyone who enables introspection will want that
event suppressed (not just you), and that no one else will?
Otherwise, it might make more sense to add some kind of flag to enable
or disable it, rather than gating it on introspection. Or it's
possible everyone actually does want that event suppressed -- in which
case making it universal is the best option.

Andres, any opinions here?

My view of the mem event interface is that it should err on the side of informing the consumer. Now, if the consumer doesn't sign up for something, why bother (i.e. we don't inform of writes, if the access mode set for the gfn does not mask writes, etc).

In an ideal world, the emulation of the instruction should raise all relevant new mem events. We don't know a priori what the consumer might learn throughout the execution of this specific instruction. Does it read from or write to new gfns which have mem access masks set? TTBOMK, because the emulation does not go through the EPT fault handler, no mem access events will be generated, even if they should.

This is a long-standing shortcoming of mem event in security frameworks, in that mem access is only defined as raising events through EPT faults. One could conceivably craft an attack by having an instruction that through its emulation reads/writes a massive buffer going into other gfns. Conversely, "virtual DMA", i.e. qemu accesses via map_foreign_pages and grant accesses form backends don't raise mem access events. So there are many (conceptual) holes.

A decent thing to do for now would be to add a flag ..._EMULATE_SILENT, which resolves to the current behavior, and lack of ..._EMULATE_SILENT in a brave future would raise all the mem access events resulting from the full emulation of this instruction. Fix the API at least, before it's set in stone.

Thanks

Andres

Â

> As for the previous if, I think that if it holds then it won't be
> possible to send a mem_event anyway, hence the else.

Sure, it won't be possible to send a mem event, because that code will
not be executed at all. :-)

Putting an "else" there sort of implies to someone reading the code
that you think the if() block might be executed and then continue
executing, which is misleading. In your patch it's even more
misleading, because the else only covers the first if() and not the
subsequent conditionals you've added right after; which implies that
the if() block might be executed and then execute the conditionals
below this one, but not this one.

The less things a programmer has to remember / figure out /Â keep in
her head, the less likely she is to make a mistake. :-)

Â-George

--

Andres Lagar-CavillaÂ|ÂGoogle Cloud PlatformÂ|Âandreslc@xxxxxxxxxxÂ|Â647-778-4380

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel