| Message ID | 20180510154638.GA7217@flask (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Radim, You're right: this commit is unnecessary. I do think it's helpful, though, after initializing exec_control to "L0's desires," to explicitly clear those desires which are not relevant. I like your version. On Thu, May 10, 2018 at 8:46 AM, Radim Krčmář <rkrcmar@redhat.com> wrote: > 2018-05-07 10:55-0700, Jim Mattson: >> If vmcs12 doesn't specify the "use MSR bitmaps" VM-execution control, >> then vmcs02 should not specify this control either. When the MSR >> bitmaps are not used, all executions of RDMSR and WRMSR cause >> VM-exits. > > We already clear it at the end of nested_get_vmcs12_pages() in this > case. > > I don't think that adding it here improves readability of the code. > Maybe if we made sure that vmcs02 always begins with disabled MSR > bitmaps and only set it afterwards, i.e. > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index c7668806163f..3938573cfb19 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -10435,9 +10435,6 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu, > if (nested_vmx_prepare_msr_bitmap(vcpu, vmcs12)) > vmcs_set_bits(CPU_BASED_VM_EXEC_CONTROL, > CPU_BASED_USE_MSR_BITMAPS); > - else > - vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL, > - CPU_BASED_USE_MSR_BITMAPS); > } > > static void vmx_start_preemption_timer(struct kvm_vcpu *vcpu) > @@ -11139,6 +11136,9 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, > exec_control &= ~CPU_BASED_USE_IO_BITMAPS; > exec_control |= CPU_BASED_UNCOND_IO_EXITING; > > + /* MSR bitmaps are potentially enabled after recomputing the bitmap. */ > + exec_control &= ~CPU_BASED_USE_MSR_BITMAPS; > + > vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, exec_control); > > /* EXCEPTION_BITMAP and CR0_GUEST_HOST_MASK should basically be the
It looks like this was fixed as part of commit d4667ca142610 ("Merge
branch 'x86-pti-for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip"). Until then,
it was possible to leak the control from L0's desires.
On Thu, May 10, 2018 at 9:39 AM, Jim Mattson <jmattson@google.com> wrote:
> Hi Radim,
>
> You're right: this commit is unnecessary. I do think it's helpful,
> though, after initializing exec_control to "L0's desires," to
> explicitly clear those desires which are not relevant.
>
> I like your version.
>
>
>
> On Thu, May 10, 2018 at 8:46 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> 2018-05-07 10:55-0700, Jim Mattson:
>>> If vmcs12 doesn't specify the "use MSR bitmaps" VM-execution control,
>>> then vmcs02 should not specify this control either. When the MSR
>>> bitmaps are not used, all executions of RDMSR and WRMSR cause
>>> VM-exits.
>>
>> We already clear it at the end of nested_get_vmcs12_pages() in this
>> case.
>>
>> I don't think that adding it here improves readability of the code.
>> Maybe if we made sure that vmcs02 always begins with disabled MSR
>> bitmaps and only set it afterwards, i.e.
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index c7668806163f..3938573cfb19 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -10435,9 +10435,6 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu,
>> if (nested_vmx_prepare_msr_bitmap(vcpu, vmcs12))
>> vmcs_set_bits(CPU_BASED_VM_EXEC_CONTROL,
>> CPU_BASED_USE_MSR_BITMAPS);
>> - else
>> - vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL,
>> - CPU_BASED_USE_MSR_BITMAPS);
>> }
>>
>> static void vmx_start_preemption_timer(struct kvm_vcpu *vcpu)
>> @@ -11139,6 +11136,9 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>> exec_control &= ~CPU_BASED_USE_IO_BITMAPS;
>> exec_control |= CPU_BASED_UNCOND_IO_EXITING;
>>
>> + /* MSR bitmaps are potentially enabled after recomputing the bitmap. */
>> + exec_control &= ~CPU_BASED_USE_MSR_BITMAPS;
>> +
>> vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, exec_control);
>>
>> /* EXCEPTION_BITMAP and CR0_GUEST_HOST_MASK should basically be the
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index c7668806163f..3938573cfb19 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10435,9 +10435,6 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu, if (nested_vmx_prepare_msr_bitmap(vcpu, vmcs12)) vmcs_set_bits(CPU_BASED_VM_EXEC_CONTROL, CPU_BASED_USE_MSR_BITMAPS); - else - vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL, - CPU_BASED_USE_MSR_BITMAPS); } static void vmx_start_preemption_timer(struct kvm_vcpu *vcpu) @@ -11139,6 +11136,9 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, exec_control &= ~CPU_BASED_USE_IO_BITMAPS; exec_control |= CPU_BASED_UNCOND_IO_EXITING; + /* MSR bitmaps are potentially enabled after recomputing the bitmap. */ + exec_control &= ~CPU_BASED_USE_MSR_BITMAPS; + vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, exec_control); /* EXCEPTION_BITMAP and CR0_GUEST_HOST_MASK should basically be the