On 14/11/19 19:34, Sean Christopherson wrote: > On Tue, Oct 22, 2019 at 08:16:22AM -0700, Sean Christopherson wrote: >> On Tue, Oct 22, 2019 at 12:51:01PM +0200, Paolo Bonzini wrote: >>> On 22/10/19 02:08, Sean Christopherson wrote: >>>> Remove the code to initialize IA32_FEATURE_CONTROL MSR when KVM is >>>> loaded now that the MSR is initialized during boot on all CPUs that >>>> support VMX, i.e. can possibly load kvm_intel. >>>> >>>> Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx> >>>> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx> >>>> --- >>>> arch/x86/kvm/vmx/vmx.c | 48 +++++++++++++++++------------------------- >>>> 1 file changed, 19 insertions(+), 29 deletions(-) >>> >>> I am still not sure about this... Enabling VMX is adding a possible >>> attack vector for the kernel, we should not do it unless we plan to do a >>> VMXON. >> >> An attacker would need arbitrary cpl0 access to toggle CR4.VMXE and do >> VMXON (and VMLAUNCH), would an extra WRMSR really slow them down? >> >> And practically speaking, how often do you encounter systems whose >> firmware leaves IA32_FEATURE_CONTROL unlocked? I honestly don't know... always on nested virtualization probably doesn't count as an answer. :) >From a totally abstract point of view I like the idea of KVM being an independent driver and thus the only place that touches VMX stuff (including the relevant bit in IA32_FEATURE_CONTROL). But I understand that this doesn't really make any concrete difference, so I guess you can go ahead with this. >>> Why is it so important to operate with locked >>> IA32_FEATURE_CONTROL (so that KVM can enable VMX and the kernel can >>> still enable SGX if desired). >> >> For simplicity. The alternative that comes to mind is to compute the >> desired MSR value and write/lock the MSR on demand, e.g. add a sequence >> similar to KVM's hardware_enable_all() for SGX, but that's a fair amount >> of complexity for marginal benefit (IMO). >> >> If a user really doesn't want VMX enabled, they can clear the feature bit >> via the clearcpuid kernel param. >> >> That being said, enabling VMX in IA32_FEATURE_CONTROL if and only if >> IS_ENABLED(CONFIG_KVM) is true would be an easy enhancement. > > Paolo, any follow up thoughts on this approach? Yes, that would be a simple enhancement, useful at least for documentation purpose. Paolo
