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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 09/10/14 21:28, Andres Lagar Cavilla wrote:
>     On Wed, Sep 10, 2014 at 5:12 PM, Razvan Cojocaru
>     <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
>     > On 09/10/2014 07:03 PM, George Dunlap wrote:
>     >> On Tue, Sep 9, 2014 at 11:28 AM, Razvan Cojocaru
>     >> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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@bitdefender.com
>     <mailto:rcojocaru@bitdefender.com>>
>     >>> Acked-by: Kevin Tian <kevin.tian@intel.com
>     <mailto:kevin.tian@intel.com>>
>     >> [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.

As far as I understand, George is asking about why events that have
npfec.kind != npfec_kind_with_gla are being emulated instead of being
sent out like the rest, and if that's a requirement that all memory
introspection clients might have.

To answer that question, _our_ application is not interested in events
other than npfec_kind_with_gla, and because of that it seemed worthwhile
to save a HV <-> dom0 roundtrip for events that would need to be ignored
by the application anyway, and thus keep the guest as responsive as
possible. I can't, of course, state that no other introspection client
will be interested in the other types of events.  But I can add another
parameter to xc_mem_access_enable_introspection() (please see patch 3/5
in the series) to specify whether non-npfec_kind_with_gla events should
be ignored or not (is this what the ..._EMULATE_SILENT suggestion refers
to?).


Thanks,
Razvan Cojocaru