From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] VMX: don't crash processing 'd' debug key Date: Fri, 8 Nov 2013 16:09:59 +0000 Message-ID: <527D0CD7.6050701@citrix.com> References: <527B7D3802000078001008A4@nat28.tlf.novell.com> <20131107190855.GB27766@deinos.phlegethon.org> <527D199A02000078001014C6@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VeodH-0004s0-RD for xen-devel@lists.xenproject.org; Fri, 08 Nov 2013 16:10:04 +0000 In-Reply-To: <527D199A02000078001014C6@nat28.tlf.novell.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: Jan Beulich Cc: xen-devel , Tim Deegan , Eddie Dong , Jun Nakajima List-Id: xen-devel@lists.xenproject.org On 08/11/13 16:04, Jan Beulich wrote: >>>> On 07.11.13 at 20:08, Tim Deegan wrote: >> At 10:44 +0000 on 07 Nov (1383817496), Jan Beulich wrote: >>> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp >>> { >>> unsigned long attr = 0, sel = 0, limit; >>> >>> - vmx_vmcs_enter(v); >>> + /* >>> + * We may get here in the context of dump_execstate(), which may have >>> + * interrupted context switching between setting "current" and >>> + * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make >>> + * all the VMREADs below fail if we don't bail right away. >>> + */ >>> + if ( unlikely(!vmx_vmcs_enter(v)) ) >>> + { >>> + memset(reg, 0, sizeof(*reg)); >>> + return; >> It would be nice to print something here, at least on the first >> instance. Otherwise someone looking at bizarre debugkey output would >> have to know (and remember) about this path. > Did this. > >> I'd also be inclined to ASSERT that, e.g. interrupts are disabled here >> -- if for any reason this function ever starts corrupting register >> state on other paths, we'll want to know about it quickly! > But I'm rather hesitant to do this. If anything, we'd need per-CPU > state tracking whether we're in do_invalid_op()'s main switch. > > Jan > I agree - the debug keys are hardly normal operation, and we don't want to ASSERT() in a debugkey. Perhaps an alternative would be a short printk indicating that if this is debugkey then the caller was unlucky and should try again, as we know there is a short vulnerable window? > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel