Was there any failure caused by reading this MSR when you run current tboot on a bare-metal machine TXT enabled machine?
Or you just found this issue when running tboot in KVM guest? This is very helpful for us to identify the impact of this issue. -----Original Message----- From: Bandan Das [mailto:b...@redhat.com] Sent: Tuesday, May 10, 2016 2:37 PM To: Sun, Ning <ning....@intel.com> Cc: Tony Camuso <tcam...@redhat.com>; tboot-devel@lists.sourceforge.net Subject: Re: [tboot-devel] [PATCH] Check for VMX support before reading feature control MSR "Sun, Ning" <ning....@intel.com> writes: > Tboot should run on bare-metal TXT enabled platform, it is not supposed to > run in a virtual machine environment. Umm... how is that got anything to do with following the spec ? > -ning > > -----Original Message----- > From: Bandan Das [mailto:b...@redhat.com] > Sent: Tuesday, May 10, 2016 12:24 PM > To: Sun, Ning <ning....@intel.com> > Cc: Tony Camuso <tcam...@redhat.com>; > tboot-devel@lists.sourceforge.net > Subject: Re: [tboot-devel] [PATCH] Check for VMX support before > reading feature control MSR > > "Sun, Ning" <ning....@intel.com> writes: > >>> BTW, table 35-2 in the spec says the msr is present if >>> cpuid.01h.ecx(bit 5 or 6) is 1. I think we should check for both vmx >>> and smx before trying the read ? >> >> "msr is present if cpuid.01h.ecx(bit 5 or 6) is 1" , that means you >> can check one of those bits to determine presence of the MSR. As you >> want to see VMX register is exposed to KVM in Kernel and Guest VM, it > > The bug is that KVM isn't exposing feature control msr unless nested=1 and if > tboot tries to read it, gets injected with a GP. > > However, with real hardware, feature control msr is valid if either bit 5 or > 6 is 1. > > So, your patch should be: > + /* read feature control msr if processor supports VMX or SMX > instructions */ > + if ( (g_cpuid_ext_feat_info & CPUID_X86_FEATURE_VMX) || > + (g_cpuid_ext_feat_info & CPUID_X86_FEATURE_SMX) ) { > + g_feat_ctrl_msr = rdmsr(MSR_IA32_FEATURE_CONTROL); > + printk(TBOOT_DETA"IA32_FEATURE_CONTROL_MSR: %08lx\n", > g_feat_ctrl_msr); > + } > >> is enough to check vmx in tboot. >> >> -Ning >> >> -----Original Message----- From: Bandan Das [mailto:b...@redhat.com] >> Sent: Tuesday, May 10, 2016 10:53 AM To: Sun, Ning >> <ning....@intel.com> Cc: Tony Camuso <tcam...@redhat.com>; >> tboot-devel@lists.sourceforge.net Subject: Re: [tboot-devel] [PATCH] >> Check for VMX support before reading feature control MSR >> >> "Sun, Ning" <ning....@intel.com> writes: >> >>> BanDan, Tony, >>> We tried out your patch, unfortunately it did not work on our >>> machines, did you test your patch before submitting it? >> >> Sorry, my bad! I didn't realize we need the msr read even before we >>> get to checking for vmx. >> >> BTW, table 35-2 in the spec says the msr is present if >>> cpuid.01h.ecx(bit 5 or 6) is 1. I think we should check for both vmx >>> and smx before trying the read ? >> >> Bandan >> >>> -ning >>> -----Original Message----- From: Bandan Das [mailto:b...@redhat.com] >>> Sent: Thursday, May 05, 2016 10:02 AM To: Sun, Ning >>> <ning....@intel.com> Cc: Tony Camuso <tcam...@redhat.com>; >>> tboot-devel@lists.sourceforge.net Subject: Re: [tboot-devel] [PATCH] >>> Check for VMX support before reading feature control MSR "Sun, Ning" >>> <ning....@intel.com> writes: >>> >>>> Hi Tony, Bandan, Thanks very much for pointing out the bug and >>>> providing the fix. Here is an alternative patch, in which just one >>>> function was modified, let me know if there is any question about >>>> it: >>> I still prefer the first version because we already have a function >>> supports_vmx() to check for support and this check below results in >>>> duplicate code. Or probably just fold supports_vmx() in here if you >>>> want to go with this version. >>> Thanks for the review! Bandan >>> >>>> diff -r 1ed81e157733 tboot/txt/verify.c --- a/tboot/txt/verify.c >>>> Wed Apr 20 16:31:18 2016 -0700 +++ b/tboot/txt/verify.c Wed May 04 >>>> 17:46:30 2016 -0700 @@ -109,8 +109,13 @@ } g_cpuid_ext_feat_info = >>>> cpuid_ecx(1); - g_feat_ctrl_msr = rdmsr(MSR_IA32_FEATURE_CONTROL); >>>> - printk(TBOOT_DETA"IA32_FEATURE_CONTROL_MSR: %08lx\n", >>>> g_feat_ctrl_msr); + /* read feature control msr if processor >>>> supports VMX instructions */ + if ( (g_cpuid_ext_feat_info & >>>> CPUID_X86_FEATURE_VMX) ) { + g_feat_ctrl_msr = >>>> rdmsr(MSR_IA32_FEATURE_CONTROL); + >>>> printk(TBOOT_DETA"IA32_FEATURE_CONTROL_MSR: %08lx\n", >>>> g_feat_ctrl_msr); + } + else + printk(TBOOT_DETA"CPU does not >>>> support VMX, + IA32_FEATURE_CONTROL_MSR is non-existent.\n"); >>>> return true; } Regards, -Ning >>>> >>>> -----Original Message----- From: Tony Camuso >>>> [mailto:tcam...@redhat.com] Sent: Wednesday, May 04, 2016 10:32 AM >>>> To: tboot-devel@lists.sourceforge.net Cc: Bandan Das >>>> <b...@redhat.com> Subject: [tboot-devel] [PATCH] Check for VMX >>>> support before reading feature control MSR We found this problem >>>> when booting a KVM guest through tboot from a host OS where the VMX >>>> register is not exposed to the guest, even when the guest has >>>> cloned the host CPU. Attempting to read MSR_IA32_FEATURE_CONTROL >>>> before checking whether it exists, on CPUs where it does not exist, >>>> sends the BSP into an infinite loop. #GP is asserted when trying to >>>> read the non-existent MSR, which resets the IP, only to again >>>> encounter the attempted read of the non-existent MSR. Postponing >>>> the read of MSR_IA32_FEATURE_CONTROL until the existence of VMX has >>>> been ascertained prevents this problem. Signed-off-by: Bandan Das >>>> <b...@redhat.com> Signed-off-by: Tony Camuso <tcam...@redhat.com> >>>> --- tboot/txt/verify.c.orig 2016-05-02 13:32:25.144003225 -0400 +++ >>>> tboot/txt/verify.c 2016-05-04 13:25:27.614166207 -0400 @@ -109,8 >>>> +109,6 @@ static bool read_processor_info(void) } >>>> g_cpuid_ext_feat_info = cpuid_ecx(1); >>>> >>>> - g_feat_ctrl_msr = rdmsr(MSR_IA32_FEATURE_CONTROL); - >>>> printk(TBOOT_DETA"IA32_FEATURE_CONTROL_MSR: %08lx\n", >>>> g_feat_ctrl_msr); return true; } >>>> >>>> @@ -123,7 +121,13 @@ static bool supports_vmx(void) } >>>> printk(TBOOT_INFO"CPU is VMX-capable\n"); >>>> >>>> - /* and that VMX is enabled in the feature control MSR */ + /* Now >>>> that we know we support VMX, it is safe to read the feature + * >>>> control MSR. + */ + g_feat_ctrl_msr = >>>> rdmsr(MSR_IA32_FEATURE_CONTROL); + >>>> printk(TBOOT_DETA"IA32_FEATURE_CONTROL_MSR: %08lx\n", + >>>> g_feat_ctrl_msr); + + /* check that VMX is enabled in the feature >>>> control MSR */ if ( !(g_feat_ctrl_msr & >>>> IA32_FEATURE_CONTROL_MSR_ENABLE_VMX_IN_SMX) ) { >>>> printk(TBOOT_ERR"ERR: VMXON disabled by feature control MSR >>>> (%lx)\n", g_feat_ctrl_msr); >>>> ------------------------------------------------------------------- >>>> - >>>> - >>>> - -------- Find and fix application performance issues faster with >>>> Applications Manager Applications Manager provides deep performance >>>> insights into multiple tiers of your business applications. It >>>> resolves application problems quickly and reduces your MTTR. Get >>>> your free trial! >>>> https://ad.doubleclick.net/ddm/clk/302982198;130105516;z >>>> _______________________________________________ tboot-devel mailing >>>> list tboot-devel@lists.sourceforge.net >>>> https://lists.sourceforge.net/lists/listinfo/tboot-devel ------------------------------------------------------------------------------ Mobile security can be enabling, not merely restricting. Employees who bring their own devices (BYOD) to work are irked by the imposition of MDM restrictions. Mobile Device Manager Plus allows you to control only the apps on BYO-devices by containerizing them, leaving personal data untouched! https://ad.doubleclick.net/ddm/clk/304595813;131938128;j _______________________________________________ tboot-devel mailing list tboot-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tboot-devel
