From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Catterall Subject: Re: [RFC 4/4] HVM x86 deprivileged mode: Trap handlers for deprivileged mode Date: Mon, 17 Aug 2015 14:59:10 +0100 Message-ID: <55D1E8AE.1080005@citrix.com> References: <1438879519-564-1-git-send-email-Ben.Catterall@citrix.com> <1438879519-564-5-git-send-email-Ben.Catterall@citrix.com> <20150810100755.GD3094@deinos.phlegethon.org> <55C9CF7D.5070500@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55C9CF7D.5070500@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: xen-devel@lists.xensource.com, keir@xen.org, ian.campbell@citrix.com, george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org On 11/08/15 11:33, Ben Catterall wrote: > > > On 10/08/15 11:07, Tim Deegan wrote: >> Hi, >> >>> @@ -685,8 +685,17 @@ static int hap_page_fault(struct vcpu *v, >>> unsigned long va, >>> { >>> struct domain *d = v->domain; >>> >>> + /* If we get a page fault whilst in HVM security user mode */ >>> + if( v->user_mode == 1 ) >>> + { >>> + printk("HVM: #PF (%u:%u) whilst in user mode\n", >>> + d->domain_id, v->vcpu_id); >>> + domain_crash_synchronous(); >>> + } >>> + >> >> This should happen in paging_fault() so it can guard the >> shadow-pagetable paths too. Once it's there, it'll need a check for >> is_hvm_vcpu() as well as for user_mode. Maybe have a helper function >> 'is_hvm_deprivileged_vcpu()' to do both checks, also used in >> hvm_deprivileged_check_trap() &c. I've moved this and now need to add shadow page table support as this currently only supports HAP. >> > Ok, I'll make this change. >>> HAP_ERROR("Intercepted a guest #PF (%u:%u) with HAP enabled.\n", >>> d->domain_id, v->vcpu_id); >>> + >>> domain_crash(d); >>> return 0; >>> } >>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c >>> index 9f5a6c6..19d465f 100644 >>> --- a/xen/arch/x86/traps.c >>> +++ b/xen/arch/x86/traps.c >>> @@ -74,6 +74,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> /* >>> * opt_nmi: one of 'ignore', 'dom0', or 'fatal'. >>> @@ -500,6 +501,11 @@ static void do_guest_trap( >>> struct trap_bounce *tb; >>> const struct trap_info *ti; >>> >>> + /* If we take the trap whilst in HVM deprivileged mode >>> + * then we should crash the domain. >>> + */ >>> + hvm_deprivileged_check_trap(__FUNCTION__); >> >> I wonder whether it would be better to switch to an IDT with all >> unacceptable traps stubbed out, rather than have to blacklist them all >> separately. Probably not - this check is cheap, and maintaining the >> parallel tables would be a pain. >> >> Or maybe there's some single point upstream of here, in the asm >> handlers, that would catch all the cases where this check is needed? >> > Yep, I think this can be done. Had a deeper look at this. There is a point where all exceptions come in in the asm (handle_exception in entry.S) and we could branch out at this point. It does mean that we'd treat _all_ exceptions that occur in this mode the same way whereas the current way means that we can treat them differently (e.g. get __func__). So, should I make all exceptions go to the same point or keep as is? Thanks! >> In any case, the check needs to return an error code so the caller >> knows to return without running the rest of the handler (and likewise >> elsewhere). >> > understood. >> Cheers, >> >> Tim. >>