| Message ID | 20191206231302.3466-2-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests | expand |
On 07/12/19 00:12, Krish Sadhukhan wrote: > According to section "Checks on Guest Control Registers, Debug Registers, and > and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry > of nested guests: > > "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each > contain a canonical address." > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > --- > arch/x86/kvm/vmx/nested.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 0e7c9301fe86..a2d1c305a7d8 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, > CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))) > return -EINVAL; > > + if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) || > + CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip))) > + return -EINVAL; This should not be negated. That said, the new tests pass even without this check, and that's not surprising since the MSRs are passed through to the vmcs02 directly. Paolo > if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && > CC(!kvm_pat_valid(vmcs12->guest_ia32_pat))) > return -EINVAL; >
On Fri, Dec 6, 2019 at 3:49 PM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > According to section "Checks on Guest Control Registers, Debug Registers, and > and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry > of nested guests: > > "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each > contain a canonical address." > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > --- > arch/x86/kvm/vmx/nested.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 0e7c9301fe86..a2d1c305a7d8 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, > CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))) > return -EINVAL; > > + if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) || > + CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip))) > + return -EINVAL; > + Don't the hardware checks on the corresponding vmcs02 fields suffice in this case? > if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && > CC(!kvm_pat_valid(vmcs12->guest_ia32_pat))) > return -EINVAL; > -- > 2.20.1 >
On 12/10/19 9:57 AM, Jim Mattson wrote: > On Fri, Dec 6, 2019 at 3:49 PM Krish Sadhukhan > <krish.sadhukhan@oracle.com> wrote: >> According to section "Checks on Guest Control Registers, Debug Registers, and >> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry >> of nested guests: >> >> "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each >> contain a canonical address." >> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> >> --- >> arch/x86/kvm/vmx/nested.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >> index 0e7c9301fe86..a2d1c305a7d8 100644 >> --- a/arch/x86/kvm/vmx/nested.c >> +++ b/arch/x86/kvm/vmx/nested.c >> @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, >> CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))) >> return -EINVAL; >> >> + if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) || >> + CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip))) >> + return -EINVAL; >> + > Don't the hardware checks on the corresponding vmcs02 fields suffice > in this case? In prepare_vmcs02(), we have the following code: if (vmx->nested.dirty_vmcs12 || hv_evmcs) { prepare_vmcs02_rare(vmx, vmcs12); If vmcs12 is dirty, we are setting these two fields from vmcs12 and I thought the values needed to be checked in software. Did I miss something ? > >> if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && >> CC(!kvm_pat_valid(vmcs12->guest_ia32_pat))) >> return -EINVAL; >> -- >> 2.20.1 >>
On Tue, Dec 10, 2019 at 11:36 AM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > > On 12/10/19 9:57 AM, Jim Mattson wrote: > > On Fri, Dec 6, 2019 at 3:49 PM Krish Sadhukhan > > <krish.sadhukhan@oracle.com> wrote: > >> According to section "Checks on Guest Control Registers, Debug Registers, and > >> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry > >> of nested guests: > >> > >> "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each > >> contain a canonical address." > >> > >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > >> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > >> --- > >> arch/x86/kvm/vmx/nested.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > >> index 0e7c9301fe86..a2d1c305a7d8 100644 > >> --- a/arch/x86/kvm/vmx/nested.c > >> +++ b/arch/x86/kvm/vmx/nested.c > >> @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, > >> CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))) > >> return -EINVAL; > >> > >> + if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) || > >> + CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip))) > >> + return -EINVAL; > >> + > > Don't the hardware checks on the corresponding vmcs02 fields suffice > > in this case? > > In prepare_vmcs02(), we have the following code: > > if (vmx->nested.dirty_vmcs12 || hv_evmcs) { > prepare_vmcs02_rare(vmx, vmcs12); > > If vmcs12 is dirty, we are setting these two fields from vmcs12 and I > thought the values needed to be checked in software. Did I miss something ? Typically, "guest state" doesn't have to be checked in software, as long as (a) the vmcs12 field is copied unmodified to the corresponding vmcs02 field, and (b) the virtual CPU enforces the same constraints as the physical CPU. In this case, if there is a problem with the guest state, the VM-entry to vmcs02 will immediately VM-exit with "VM-entry failure due to invalid guest state," and L0 will reflect this exit reason to L1. > > > >> if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && > >> CC(!kvm_pat_valid(vmcs12->guest_ia32_pat))) > >> return -EINVAL; > >> -- > >> 2.20.1 > >>
On 12/10/19 11:48 AM, Jim Mattson wrote: > On Tue, Dec 10, 2019 at 11:36 AM Krish Sadhukhan > <krish.sadhukhan@oracle.com> wrote: >> >> On 12/10/19 9:57 AM, Jim Mattson wrote: >>> On Fri, Dec 6, 2019 at 3:49 PM Krish Sadhukhan >>> <krish.sadhukhan@oracle.com> wrote: >>>> According to section "Checks on Guest Control Registers, Debug Registers, and >>>> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry >>>> of nested guests: >>>> >>>> "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each >>>> contain a canonical address." >>>> >>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >>>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> >>>> --- >>>> arch/x86/kvm/vmx/nested.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >>>> index 0e7c9301fe86..a2d1c305a7d8 100644 >>>> --- a/arch/x86/kvm/vmx/nested.c >>>> +++ b/arch/x86/kvm/vmx/nested.c >>>> @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, >>>> CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))) >>>> return -EINVAL; >>>> >>>> + if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) || >>>> + CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip))) >>>> + return -EINVAL; >>>> + >>> Don't the hardware checks on the corresponding vmcs02 fields suffice >>> in this case? >> In prepare_vmcs02(), we have the following code: >> >> if (vmx->nested.dirty_vmcs12 || hv_evmcs) { >> prepare_vmcs02_rare(vmx, vmcs12); >> >> If vmcs12 is dirty, we are setting these two fields from vmcs12 and I >> thought the values needed to be checked in software. Did I miss something ? > Typically, "guest state" doesn't have to be checked in software, as > long as (a) the vmcs12 field is copied unmodified to the corresponding > vmcs02 field, and (b) the virtual CPU enforces the same constraints as > the physical CPU. In this case, if there is a problem with the guest > state, the VM-entry to vmcs02 will immediately VM-exit with "VM-entry > failure due to invalid guest state," and L0 will reflect this exit > reason to L1. Thanks for the explanation ! So the kvm-unit-test is still needed to verify that hardware does the check. Right ? > >>>> if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && >>>> CC(!kvm_pat_valid(vmcs12->guest_ia32_pat))) >>>> return -EINVAL; >>>> -- >>>> 2.20.1 >>>>
On 10/12/19 21:29, Krish Sadhukhan wrote: > > Thanks for the explanation ! > > So the kvm-unit-test is still needed to verify that hardware does the > check. Right ? Yes, and I've queued that part. Paolo
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 0e7c9301fe86..a2d1c305a7d8 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu, CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4))) return -EINVAL; + if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) || + CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip))) + return -EINVAL; + if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) && CC(!kvm_pat_valid(vmcs12->guest_ia32_pat))) return -EINVAL;
According to section "Checks on Guest Control Registers, Debug Registers, and and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry of nested guests: "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each contain a canonical address." Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> --- arch/x86/kvm/vmx/nested.c | 4 ++++ 1 file changed, 4 insertions(+)