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:11:28 +0000 Message-ID: <527D0D30.8060303@citrix.com> References: <527B7D3802000078001008A4@nat28.tlf.novell.com> <20131107190855.GB27766@deinos.phlegethon.org> <527D199A02000078001014C6@nat28.tlf.novell.com> <527D0CD7.6050701@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Veoej-00056B-8y for xen-devel@lists.xenproject.org; Fri, 08 Nov 2013 16:11:33 +0000 In-Reply-To: <527D0CD7.6050701@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: Jan Beulich Cc: xen-devel , Tim Deegan , Eddie Dong , Jun Nakajima List-Id: xen-devel@lists.xenproject.org On 08/11/13 16:09, Andrew Cooper wrote: > 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? I should have waited and read v2 - that looks fine to me. Sorry for the noise. ~Andrew