From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V6 5/5] xen: Handle resumed instruction based on previous mem_event reply Date: Thu, 11 Sep 2014 00:28:06 +0300 Message-ID: <5410C266.1060007@bitdefender.com> References: <1410258512-2955-1-git-send-email-rcojocaru@bitdefender.com> <1410258512-2955-6-git-send-email-rcojocaru@bitdefender.com> <54107873.1040001@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XRpQy-0008Lf-6f for xen-devel@lists.xenproject.org; Wed, 10 Sep 2014 21:28:12 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andres Lagar Cavilla Cc: "Tian, Kevin" , Keir Fraser , Ian Campbell , Stefano Stabellini , Jun Nakajima , "Dong, Eddie" , Ian Jackson , Tim Deegan , Jan Beulich , Andrew Cooper , xen-devel List-Id: xen-devel@lists.xenproject.org On 09/10/14 21:28, Andres Lagar Cavilla wrote: > On Wed, Sep 10, 2014 at 5:12 PM, Razvan Cojocaru > > wrote: > > On 09/10/2014 07:03 PM, George Dunlap wrote: > >> On Tue, Sep 9, 2014 at 11:28 AM, Razvan Cojocaru > >> > 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 > > >>> Acked-by: Kevin Tian > > >> [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