| Message ID | 20230319084927.29607-3-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: >get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() >to check 64-bit mode. Should use is_64_bit_mode() instead. > >Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") >Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") It is better to split this patch into two: one for nested and one for SGX. It is possible that there is a kernel release which has just one of above two flawed commits, then this fix patch cannot be applied cleanly to the release. >Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> >--- > arch/x86/kvm/vmx/nested.c | 2 +- > arch/x86/kvm/vmx/sgx.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > >diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >index 557b9c468734..0f84cc05f57c 100644 >--- a/arch/x86/kvm/vmx/nested.c >+++ b/arch/x86/kvm/vmx/nested.c >@@ -4959,7 +4959,7 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification, > > /* Checks for #GP/#SS exceptions. */ > exn = false; >- if (is_long_mode(vcpu)) { >+ if (is_64_bit_mode(vcpu)) { > /* > * The virtual/linear address is never truncated in 64-bit > * mode, e.g. a 32-bit address size can yield a 64-bit virtual >diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c >index aa53c98034bf..0574030b071f 100644 >--- a/arch/x86/kvm/vmx/sgx.c >+++ b/arch/x86/kvm/vmx/sgx.c >@@ -29,14 +29,14 @@ static int sgx_get_encls_gva(struct kvm_vcpu *vcpu, unsigned long offset, > > /* Skip vmcs.GUEST_DS retrieval for 64-bit mode to avoid VMREADs. */ > *gva = offset; >- if (!is_long_mode(vcpu)) { >+ if (!is_64_bit_mode(vcpu)) { > vmx_get_segment(vcpu, &s, VCPU_SREG_DS); > *gva += s.base; > } > > if (!IS_ALIGNED(*gva, alignment)) { > fault = true; >- } else if (likely(is_long_mode(vcpu))) { >+ } else if (likely(is_64_bit_mode(vcpu))) { > fault = is_noncanonical_address(*gva, vcpu); > } else { > *gva &= 0xffffffff; >-- >2.25.1 >
On 3/20/2023 8:36 PM, Chao Gao wrote: > On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: >> get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() >> to check 64-bit mode. Should use is_64_bit_mode() instead. >> >> Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") >> Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") > It is better to split this patch into two: one for nested and one for > SGX. > > It is possible that there is a kernel release which has just one of > above two flawed commits, then this fix patch cannot be applied cleanly > to the release. OK. > >> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> >> --- >> arch/x86/kvm/vmx/nested.c | 2 +- >> arch/x86/kvm/vmx/sgx.c | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >> index 557b9c468734..0f84cc05f57c 100644 >> --- a/arch/x86/kvm/vmx/nested.c >> +++ b/arch/x86/kvm/vmx/nested.c >> @@ -4959,7 +4959,7 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification, >> >> /* Checks for #GP/#SS exceptions. */ >> exn = false; >> - if (is_long_mode(vcpu)) { >> + if (is_64_bit_mode(vcpu)) { >> /* >> * The virtual/linear address is never truncated in 64-bit >> * mode, e.g. a 32-bit address size can yield a 64-bit virtual >> diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c >> index aa53c98034bf..0574030b071f 100644 >> --- a/arch/x86/kvm/vmx/sgx.c >> +++ b/arch/x86/kvm/vmx/sgx.c >> @@ -29,14 +29,14 @@ static int sgx_get_encls_gva(struct kvm_vcpu *vcpu, unsigned long offset, >> >> /* Skip vmcs.GUEST_DS retrieval for 64-bit mode to avoid VMREADs. */ >> *gva = offset; >> - if (!is_long_mode(vcpu)) { >> + if (!is_64_bit_mode(vcpu)) { >> vmx_get_segment(vcpu, &s, VCPU_SREG_DS); >> *gva += s.base; >> } >> >> if (!IS_ALIGNED(*gva, alignment)) { >> fault = true; >> - } else if (likely(is_long_mode(vcpu))) { >> + } else if (likely(is_64_bit_mode(vcpu))) { >> fault = is_noncanonical_address(*gva, vcpu); >> } else { >> *gva &= 0xffffffff; >> -- >> 2.25.1 >>
On Sun, 2023-03-19 at 16:49 +0800, Binbin Wu wrote: > get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() > to check 64-bit mode. Should use is_64_bit_mode() instead. > > Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") > Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> > --- > arch/x86/kvm/vmx/nested.c | 2 +- > arch/x86/kvm/vmx/sgx.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 557b9c468734..0f84cc05f57c 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4959,7 +4959,7 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification, > > /* Checks for #GP/#SS exceptions. */ > exn = false; > - if (is_long_mode(vcpu)) { > + if (is_64_bit_mode(vcpu)) { > /* > * The virtual/linear address is never truncated in 64-bit > * mode, e.g. a 32-bit address size can yield a 64-bit virtual > diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c > index aa53c98034bf..0574030b071f 100644 > --- a/arch/x86/kvm/vmx/sgx.c > +++ b/arch/x86/kvm/vmx/sgx.c > @@ -29,14 +29,14 @@ static int sgx_get_encls_gva(struct kvm_vcpu *vcpu, unsigned long offset, > > /* Skip vmcs.GUEST_DS retrieval for 64-bit mode to avoid VMREADs. */ > *gva = offset; > - if (!is_long_mode(vcpu)) { > + if (!is_64_bit_mode(vcpu)) { > vmx_get_segment(vcpu, &s, VCPU_SREG_DS); > *gva += s.base; > } > > if (!IS_ALIGNED(*gva, alignment)) { > fault = true; > - } else if (likely(is_long_mode(vcpu))) { > + } else if (likely(is_64_bit_mode(vcpu))) { > fault = is_noncanonical_address(*gva, vcpu); > } else { > *gva &= 0xffffffff; For SGX part, Reviewed-by: Kai Huang <kai.huang@intel.com>
On Mon, Mar 20, 2023, Chao Gao wrote: > On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: > >get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() > >to check 64-bit mode. Should use is_64_bit_mode() instead. > > > >Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") > >Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") > > It is better to split this patch into two: one for nested and one for > SGX. > > It is possible that there is a kernel release which has just one of > above two flawed commits, then this fix patch cannot be applied cleanly > to the release. The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say just drop the nVMX side of things. I could have sworn ENCLS had the same behavior, but the SDM disagrees. Though why on earth ENCLS is allowed in compatibility mode is beyond me. ENCLU I can kinda sorta understand, but ENCLS?!?!!
On 3/22/2023 5:35 AM, Sean Christopherson wrote: > On Mon, Mar 20, 2023, Chao Gao wrote: >> On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: >>> get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() >>> to check 64-bit mode. Should use is_64_bit_mode() instead. >>> >>> Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") >>> Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") >> It is better to split this patch into two: one for nested and one for >> SGX. >> >> It is possible that there is a kernel release which has just one of >> above two flawed commits, then this fix patch cannot be applied cleanly >> to the release. > The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except > for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say > just drop the nVMX side of things. Got it. Do you mind if I add a comment about it in code? > > I could have sworn ENCLS had the same behavior, but the SDM disagrees. Though why > on earth ENCLS is allowed in compatibility mode is beyond me. ENCLU I can kinda > sorta understand, but ENCLS?!?!! Yes, the SDM does have definition about the behavior "outside 64-bit mode (IA32_EFER.LAM = 0 || CS.L = 0)". IMO, the change has very little impact on performance since the two intercepted ENCLS leaf EINIT & ECREATE will only be called once per lifecycle of a SGX Enclave.
On Tue, 2023-03-21 at 14:35 -0700, Sean Christopherson wrote: > On Mon, Mar 20, 2023, Chao Gao wrote: > > On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: > > > get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() > > > to check 64-bit mode. Should use is_64_bit_mode() instead. > > > > > > Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") > > > Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") > > > > It is better to split this patch into two: one for nested and one for > > SGX. > > > > It is possible that there is a kernel release which has just one of > > above two flawed commits, then this fix patch cannot be applied cleanly > > to the release. > > The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except > for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say > just drop the nVMX side of things. But it looks the old code doesn't unconditionally inject #UD when in compatibility mode? /* Checks for #GP/#SS exceptions. */ exn = false; if (is_long_mode(vcpu)) { /* * The virtual/linear address is never truncated in 64-bit * mode, e.g. a 32-bit address size can yield a 64-bit virtual * address when using FS/GS with a non-zero base. */ if (seg_reg == VCPU_SREG_FS || seg_reg == VCPU_SREG_GS) *ret = s.base + off; else *ret = off; /* Long mode: #GP(0)/#SS(0) if the memory address is in a * non-canonical form. This is the only check on the memory * destination for long mode! */ exn = is_noncanonical_address(*ret, vcpu); } ... The logic of only adding seg.base for FS/GS to linear address (and ignoring seg.base for all other segs) only applies to 64 bit mode, but the code only checks _long_ mode. Am I missing something? > > I could have sworn ENCLS had the same behavior, but the SDM disagrees. Though why > on earth ENCLS is allowed in compatibility mode is beyond me. ENCLU I can kinda > sorta understand, but ENCLS?!?!! I can reach out to Intel guys to (try to) find the answer if you want me to? :)
On 3/29/2023 7:33 AM, Huang, Kai wrote: > On Tue, 2023-03-21 at 14:35 -0700, Sean Christopherson wrote: >> On Mon, Mar 20, 2023, Chao Gao wrote: >>> On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: >>>> get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() >>>> to check 64-bit mode. Should use is_64_bit_mode() instead. >>>> >>>> Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") >>>> Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") >>> It is better to split this patch into two: one for nested and one for >>> SGX. >>> >>> It is possible that there is a kernel release which has just one of >>> above two flawed commits, then this fix patch cannot be applied cleanly >>> to the release. >> The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except >> for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say >> just drop the nVMX side of things. > But it looks the old code doesn't unconditionally inject #UD when in > compatibility mode? I think Sean means VMX instructions is not valid in compatibility mode and it triggers #UD, which has higher priority than VM-Exit, by the processor in non-root mode. So if there is a VM-Exit due to VMX instruction , it is in 64-bit mode for sure if it is in long mode. > > /* Checks for #GP/#SS exceptions. */ > exn = false; > if (is_long_mode(vcpu)) { > /* > * The virtual/linear address is never truncated in 64-bit > * mode, e.g. a 32-bit address size can yield a 64-bit virtual > * address when using FS/GS with a non-zero base. > */ > if (seg_reg == VCPU_SREG_FS || seg_reg == VCPU_SREG_GS) > *ret = s.base + off; > else > *ret = off; > > /* Long mode: #GP(0)/#SS(0) if the memory address is in a > * non-canonical form. This is the only check on the memory > * destination for long mode! > */ > exn = is_noncanonical_address(*ret, vcpu); > } > ... > > The logic of only adding seg.base for FS/GS to linear address (and ignoring > seg.base for all other segs) only applies to 64 bit mode, but the code only > checks _long_ mode. > > Am I missing something? > >> I could have sworn ENCLS had the same behavior, but the SDM disagrees. Though why >> on earth ENCLS is allowed in compatibility mode is beyond me. ENCLU I can kinda >> sorta understand, but ENCLS?!?!! > I can reach out to Intel guys to (try to) find the answer if you want me to? :)
On Wed, 2023-03-29 at 09:27 +0800, Binbin Wu wrote: > On 3/29/2023 7:33 AM, Huang, Kai wrote: > > On Tue, 2023-03-21 at 14:35 -0700, Sean Christopherson wrote: > > > On Mon, Mar 20, 2023, Chao Gao wrote: > > > > On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: > > > > > get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() > > > > > to check 64-bit mode. Should use is_64_bit_mode() instead. > > > > > > > > > > Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") > > > > > Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") > > > > It is better to split this patch into two: one for nested and one for > > > > SGX. > > > > > > > > It is possible that there is a kernel release which has just one of > > > > above two flawed commits, then this fix patch cannot be applied cleanly > > > > to the release. > > > The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except > > > for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say > > > just drop the nVMX side of things. > > But it looks the old code doesn't unconditionally inject #UD when in > > compatibility mode? > > I think Sean means VMX instructions is not valid in compatibility mode > and it triggers #UD, which has higher priority than VM-Exit, by the > processor in non-root mode. > > So if there is a VM-Exit due to VMX instruction , it is in 64-bit mode > for sure if it is in long mode. Oh I see thanks. Then is it better to add some comment to explain, or add a WARN() if it's not in 64-bit mode?
On 3/29/2023 10:04 AM, Huang, Kai wrote: > On Wed, 2023-03-29 at 09:27 +0800, Binbin Wu wrote: >> On 3/29/2023 7:33 AM, Huang, Kai wrote: >>> On Tue, 2023-03-21 at 14:35 -0700, Sean Christopherson wrote: >>>> On Mon, Mar 20, 2023, Chao Gao wrote: >>>>> On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: >>>>>> get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() >>>>>> to check 64-bit mode. Should use is_64_bit_mode() instead. >>>>>> >>>>>> Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") >>>>>> Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") >>>>> It is better to split this patch into two: one for nested and one for >>>>> SGX. >>>>> >>>>> It is possible that there is a kernel release which has just one of >>>>> above two flawed commits, then this fix patch cannot be applied cleanly >>>>> to the release. >>>> The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except >>>> for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say >>>> just drop the nVMX side of things. >>> But it looks the old code doesn't unconditionally inject #UD when in >>> compatibility mode? >> I think Sean means VMX instructions is not valid in compatibility mode >> and it triggers #UD, which has higher priority than VM-Exit, by the >> processor in non-root mode. >> >> So if there is a VM-Exit due to VMX instruction , it is in 64-bit mode >> for sure if it is in long mode. > Oh I see thanks. > > Then is it better to add some comment to explain, or add a WARN() if it's not in > 64-bit mode? I also prefer to add a comment if no objection. Seems I am not the only one who didn't get it : ) >
On Wed, Mar 29, 2023, Binbin Wu wrote: > > On 3/29/2023 10:04 AM, Huang, Kai wrote: > > On Wed, 2023-03-29 at 09:27 +0800, Binbin Wu wrote: > > > On 3/29/2023 7:33 AM, Huang, Kai wrote: > > > > On Tue, 2023-03-21 at 14:35 -0700, Sean Christopherson wrote: > > > > > On Mon, Mar 20, 2023, Chao Gao wrote: > > > > > > On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: > > > > > > > get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() > > > > > > > to check 64-bit mode. Should use is_64_bit_mode() instead. > > > > > > > > > > > > > > Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") > > > > > > > Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") > > > > > > It is better to split this patch into two: one for nested and one for > > > > > > SGX. > > > > > > > > > > > > It is possible that there is a kernel release which has just one of > > > > > > above two flawed commits, then this fix patch cannot be applied cleanly > > > > > > to the release. > > > > > The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except > > > > > for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say > > > > > just drop the nVMX side of things. > > > > But it looks the old code doesn't unconditionally inject #UD when in > > > > compatibility mode? > > > I think Sean means VMX instructions is not valid in compatibility mode > > > and it triggers #UD, which has higher priority than VM-Exit, by the > > > processor in non-root mode. > > > > > > So if there is a VM-Exit due to VMX instruction , it is in 64-bit mode > > > for sure if it is in long mode. > > Oh I see thanks. > > > > Then is it better to add some comment to explain, or add a WARN() if it's not in > > 64-bit mode? > > I also prefer to add a comment if no objection. > > Seems I am not the only one who didn't get it� : ) I would rather have a code change than a comment, e.g. diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index f63b28f46a71..0460ca219f96 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4931,7 +4931,8 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification, int base_reg = (vmx_instruction_info >> 23) & 0xf; bool base_is_valid = !(vmx_instruction_info & (1u << 27)); - if (is_reg) { + if (is_reg || + WARN_ON_ONCE(is_long_mode(vcpu) && !is_64_bit_mode(vcpu))) { kvm_queue_exception(vcpu, UD_VECTOR); return 1; } The only downside is that querying is_64_bit_mode() could unnecessarily trigger a VMREAD to get the current CS.L bit, but a measurable performance regressions is extremely unlikely because is_64_bit_mode() all but guaranteed to be called in these paths anyways (and KVM caches segment info), e.g. by kvm_register_read(). And then in a follow-up, we should also be able to do: @@ -5402,7 +5403,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu) if (instr_info & BIT(10)) { kvm_register_write(vcpu, (((instr_info) >> 3) & 0xf), value); } else { - len = is_64_bit_mode(vcpu) ? 8 : 4; + len = is_long_mode(vcpu) ? 8 : 4; if (get_vmx_mem_address(vcpu, exit_qualification, instr_info, true, len, &gva)) return 1; @@ -5476,7 +5477,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu) if (instr_info & BIT(10)) value = kvm_register_read(vcpu, (((instr_info) >> 3) & 0xf)); else { - len = is_64_bit_mode(vcpu) ? 8 : 4; + len = is_long_mode(vcpu) ? 8 : 4; if (get_vmx_mem_address(vcpu, exit_qualification, instr_info, false, len, &gva)) return 1;
On Wed, 2023-03-29 at 10:34 -0700, Sean Christopherson wrote: > On Wed, Mar 29, 2023, Binbin Wu wrote: > > > > On 3/29/2023 10:04 AM, Huang, Kai wrote: > > > On Wed, 2023-03-29 at 09:27 +0800, Binbin Wu wrote: > > > > On 3/29/2023 7:33 AM, Huang, Kai wrote: > > > > > On Tue, 2023-03-21 at 14:35 -0700, Sean Christopherson wrote: > > > > > > On Mon, Mar 20, 2023, Chao Gao wrote: > > > > > > > On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: > > > > > > > > get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() > > > > > > > > to check 64-bit mode. Should use is_64_bit_mode() instead. > > > > > > > > > > > > > > > > Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") > > > > > > > > Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") > > > > > > > It is better to split this patch into two: one for nested and one for > > > > > > > SGX. > > > > > > > > > > > > > > It is possible that there is a kernel release which has just one of > > > > > > > above two flawed commits, then this fix patch cannot be applied cleanly > > > > > > > to the release. > > > > > > The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except > > > > > > for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say > > > > > > just drop the nVMX side of things. > > > > > But it looks the old code doesn't unconditionally inject #UD when in > > > > > compatibility mode? > > > > I think Sean means VMX instructions is not valid in compatibility mode > > > > and it triggers #UD, which has higher priority than VM-Exit, by the > > > > processor in non-root mode. > > > > > > > > So if there is a VM-Exit due to VMX instruction , it is in 64-bit mode > > > > for sure if it is in long mode. > > > Oh I see thanks. > > > > > > Then is it better to add some comment to explain, or add a WARN() if it's not in > > > 64-bit mode? > > > > I also prefer to add a comment if no objection. > > > > Seems I am not the only one who didn't get it� : ) > > I would rather have a code change than a comment, e.g. > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index f63b28f46a71..0460ca219f96 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4931,7 +4931,8 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification, > int base_reg = (vmx_instruction_info >> 23) & 0xf; > bool base_is_valid = !(vmx_instruction_info & (1u << 27)); > > - if (is_reg) { > + if (is_reg || > + WARN_ON_ONCE(is_long_mode(vcpu) && !is_64_bit_mode(vcpu))) { > kvm_queue_exception(vcpu, UD_VECTOR); > return 1; > } > > Looks good to me. > The only downside is that querying is_64_bit_mode() could unnecessarily trigger a > VMREAD to get the current CS.L bit, but a measurable performance regressions is > extremely unlikely because is_64_bit_mode() all but guaranteed to be called in > these paths anyways (and KVM caches segment info), e.g. by kvm_register_read(). Agreed. > > And then in a follow-up, we should also be able to do: > > @@ -5402,7 +5403,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu) > if (instr_info & BIT(10)) { > kvm_register_write(vcpu, (((instr_info) >> 3) & 0xf), value); > } else { > - len = is_64_bit_mode(vcpu) ? 8 : 4; > + len = is_long_mode(vcpu) ? 8 : 4; > if (get_vmx_mem_address(vcpu, exit_qualification, > instr_info, true, len, &gva)) > return 1; > @@ -5476,7 +5477,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu) > if (instr_info & BIT(10)) > value = kvm_register_read(vcpu, (((instr_info) >> 3) & 0xf)); > else { > - len = is_64_bit_mode(vcpu) ? 8 : 4; > + len = is_long_mode(vcpu) ? 8 : 4; > if (get_vmx_mem_address(vcpu, exit_qualification, > instr_info, false, len, &gva)) > return 1; > Yeah, although it's a little bit wired the actual WARN() happens after above code change. But I don't know how to make the code better. Maybe we should put the WARN() at the very beginning but this would require duplicated code in each handle_xxx() for VMX instructions.
On 3/30/2023 6:46 AM, Huang, Kai wrote: > On Wed, 2023-03-29 at 10:34 -0700, Sean Christopherson wrote: >> On Wed, Mar 29, 2023, Binbin Wu wrote: >>> On 3/29/2023 10:04 AM, Huang, Kai wrote: >>>> On Wed, 2023-03-29 at 09:27 +0800, Binbin Wu wrote: >>>>> On 3/29/2023 7:33 AM, Huang, Kai wrote: >>>>>> On Tue, 2023-03-21 at 14:35 -0700, Sean Christopherson wrote: >>>>>>> On Mon, Mar 20, 2023, Chao Gao wrote: >>>>>>>> On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: >>>>>>>>> get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() >>>>>>>>> to check 64-bit mode. Should use is_64_bit_mode() instead. >>>>>>>>> >>>>>>>>> Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") >>>>>>>>> Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") >>>>>>>> It is better to split this patch into two: one for nested and one for >>>>>>>> SGX. >>>>>>>> >>>>>>>> It is possible that there is a kernel release which has just one of >>>>>>>> above two flawed commits, then this fix patch cannot be applied cleanly >>>>>>>> to the release. >>>>>>> The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except >>>>>>> for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say >>>>>>> just drop the nVMX side of things. >>>>>> But it looks the old code doesn't unconditionally inject #UD when in >>>>>> compatibility mode? >>>>> I think Sean means VMX instructions is not valid in compatibility mode >>>>> and it triggers #UD, which has higher priority than VM-Exit, by the >>>>> processor in non-root mode. >>>>> >>>>> So if there is a VM-Exit due to VMX instruction , it is in 64-bit mode >>>>> for sure if it is in long mode. >>>> Oh I see thanks. >>>> >>>> Then is it better to add some comment to explain, or add a WARN() if it's not in >>>> 64-bit mode? >>> I also prefer to add a comment if no objection. >>> >>> Seems I am not the only one who didn't get it� : ) >> I would rather have a code change than a comment, e.g. >> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >> index f63b28f46a71..0460ca219f96 100644 >> --- a/arch/x86/kvm/vmx/nested.c >> +++ b/arch/x86/kvm/vmx/nested.c >> @@ -4931,7 +4931,8 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification, >> int base_reg = (vmx_instruction_info >> 23) & 0xf; >> bool base_is_valid = !(vmx_instruction_info & (1u << 27)); >> >> - if (is_reg) { >> + if (is_reg || >> + WARN_ON_ONCE(is_long_mode(vcpu) && !is_64_bit_mode(vcpu))) { >> kvm_queue_exception(vcpu, UD_VECTOR); >> return 1; >> } >> >> > Looks good to me. > >> The only downside is that querying is_64_bit_mode() could unnecessarily trigger a >> VMREAD to get the current CS.L bit, but a measurable performance regressions is >> extremely unlikely because is_64_bit_mode() all but guaranteed to be called in >> these paths anyways (and KVM caches segment info), e.g. by kvm_register_read(). > Agreed. > >> And then in a follow-up, we should also be able to do: >> >> @@ -5402,7 +5403,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu) >> if (instr_info & BIT(10)) { >> kvm_register_write(vcpu, (((instr_info) >> 3) & 0xf), value); >> } else { >> - len = is_64_bit_mode(vcpu) ? 8 : 4; >> + len = is_long_mode(vcpu) ? 8 : 4; >> if (get_vmx_mem_address(vcpu, exit_qualification, >> instr_info, true, len, &gva)) >> return 1; >> @@ -5476,7 +5477,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu) >> if (instr_info & BIT(10)) >> value = kvm_register_read(vcpu, (((instr_info) >> 3) & 0xf)); >> else { >> - len = is_64_bit_mode(vcpu) ? 8 : 4; >> + len = is_long_mode(vcpu) ? 8 : 4; >> if (get_vmx_mem_address(vcpu, exit_qualification, >> instr_info, false, len, &gva)) >> return 1; >> > Yeah, although it's a little bit wired the actual WARN() happens after above > code change. But I don't know how to make the code better. Maybe we should put > the WARN() at the very beginning but this would require duplicated code in each > handle_xxx() for VMX instructions. I checked the code again and find the comment of nested_vmx_check_permission(). "/* * Intel's VMX Instruction Reference specifies a common set of prerequisites * for running VMX instructions (except VMXON, whose prerequisites are * slightly different). It also specifies what exception to inject otherwise. * Note that many of these exceptions have priority over VM exits, so they * don't have to be checked again here. */" I think the Note part in the comment has tried to callout why the check for compatibility mode is unnecessary. But I have a question here, nested_vmx_check_permission() checks that the vcpu is vmxon, otherwise it will inject a #UD. Why this #UD is handled in the VMExit handler specifically? Not all #UDs have higher priority than VM exits? According to SDM Section "Relative Priority of Faults and VM Exits": "Certain exceptions have priority over VM exits. These include invalid-opcode exceptions, ..." Seems not further classifications of #UDs. Anyway, I will seperate this patch from the LAM KVM enabling patch. And send a patch seperately if needed later.
> > I checked the code again and find the comment of > nested_vmx_check_permission(). > > "/* > * Intel's VMX Instruction Reference specifies a common set of > prerequisites > * for running VMX instructions (except VMXON, whose prerequisites are > * slightly different). It also specifies what exception to inject > otherwise. > * Note that many of these exceptions have priority over VM exits, so they > * don't have to be checked again here. > */" > > I think the Note part in the comment has tried to callout why the check > for compatibility mode is unnecessary. > > But I have a question here, nested_vmx_check_permission() checks that > the vcpu is vmxon, > otherwise it will inject a #UD. Why this #UD is handled in the VMExit > handler specifically? > Not all #UDs have higher priority than VM exits? > > According to SDM Section "Relative Priority of Faults and VM Exits": > "Certain exceptions have priority over VM exits. These include > invalid-opcode exceptions, ..." > Seems not further classifications of #UDs. This is clarified in the pseudo code of VMX instructions in the SDM. If you look at the pseudo code, all VMX instructions except VMXON (obviously) have something like below: IF (not in VMX operation) ... THEN #UD; ELSIF in VMX non-root operation THEN VMexit; So to me "this particular" #UD has higher priority over VM exits (while other #UDs may not). But IIUC above #UD won't happen when running VMX instruction in the guest, because if there's any live guest, the CPU must already have been in VMX operation. So below check in nested_vmx_check_permission(): if (!to_vmx(vcpu)->nested.vmxon) { kvm_queue_exception(vcpu, UD_VECTOR); return 0; } is needed to emulate the case that guest runs any other VMX instructions before VMXON. > > Anyway, I will seperate this patch from the LAM KVM enabling patch. And > send a patch seperately if > needed later. > I think your change for SGX is still needed based on the pseudo code of ENCLS.
On Mon, Apr 03, 2023, Huang, Kai wrote: > > > > I checked the code again and find the comment of > > nested_vmx_check_permission(). > > > > "/* > > �* Intel's VMX Instruction Reference specifies a common set of > > prerequisites > > �* for running VMX instructions (except VMXON, whose prerequisites are > > �* slightly different). It also specifies what exception to inject > > otherwise. > > �* Note that many of these exceptions have priority over VM exits, so they > > �* don't have to be checked again here. > > �*/" > > > > I think the Note part in the comment has tried to callout why the check > > for compatibility mode is unnecessary. > > > > But I have a question here, nested_vmx_check_permission() checks that the > > vcpu is vmxon, otherwise it will inject a #UD. Why this #UD is handled in > > the VMExit handler specifically? Not all #UDs have higher priority than VM > > exits? > > > > According to SDM Section "Relative Priority of Faults and VM Exits": > > "Certain exceptions have priority over VM exits. These include > > invalid-opcode exceptions, ..." > > Seems not further classifications of #UDs. > > This is clarified in the pseudo code of VMX instructions in the SDM. If you > look at the pseudo code, all VMX instructions except VMXON (obviously) have > something like below: > > IF (not in VMX operation) ... > THEN #UD; > ELSIF in VMX non-root operation > THEN VMexit; > > So to me "this particular" #UD has higher priority over VM exits (while other > #UDs may not). > But IIUC above #UD won't happen when running VMX instruction in the guest, > because if there's any live guest, the CPU must already have been in VMX > operation. So below check in nested_vmx_check_permission(): > > if (!to_vmx(vcpu)->nested.vmxon) { > kvm_queue_exception(vcpu, UD_VECTOR); > return 0; > } > > is needed to emulate the case that guest runs any other VMX instructions before > VMXON. Yep. IMO, the pseucode is misleading/confusing, the "in VMX non-root operation" check should really come first. The VMXON pseudocode has the same awkward sequence: IF (register operand) or (CR0.PE = 0) or (CR4.VMXE = 0) or ... THEN #UD; ELSIF not in VMX operation THEN IF (CPL > 0) or (in A20M mode) or (the values of CR0 and CR4 are not supported in VMX operation) THEN #GP(0); ELSIF in VMX non-root operation THEN VMexit; ELSIF CPL > 0 THEN #GP(0); ELSE VMfail("VMXON executed in VMX root operation"); FI; whereas I find this sequence for VMXON more representative of what actually happens: IF (register operand) or (CR0.PE = 0) or (CR4.VMXE = 0) or ... THEN #UD IF in VMX non-root operation THEN VMexit; IF CPL > 0 THEN #GP(0) IF in VMX operation THEN VMfail("VMXON executed in VMX root operation"); IF (in A20M mode) or (the values of CR0 and CR4 are not supported in VMX operation) THEN #GP(0); > > Anyway, I will seperate this patch from the LAM KVM enabling patch. And > > send a patch seperately if needed later. > > I think your change for SGX is still needed based on the pseudo code of ENCLS. Agreed.
On Mon, 2023-04-03 at 08:02 -0700, Sean Christopherson wrote: > On Mon, Apr 03, 2023, Huang, Kai wrote: > > > > > > I checked the code again and find the comment of > > > nested_vmx_check_permission(). > > > > > > "/* > > > �* Intel's VMX Instruction Reference specifies a common set of > > > prerequisites > > > �* for running VMX instructions (except VMXON, whose prerequisites are > > > �* slightly different). It also specifies what exception to inject > > > otherwise. > > > �* Note that many of these exceptions have priority over VM exits, so they > > > �* don't have to be checked again here. > > > �*/" > > > > > > I think the Note part in the comment has tried to callout why the check > > > for compatibility mode is unnecessary. > > > > > > But I have a question here, nested_vmx_check_permission() checks that the > > > vcpu is vmxon, otherwise it will inject a #UD. Why this #UD is handled in > > > the VMExit handler specifically? Not all #UDs have higher priority than VM > > > exits? > > > > > > According to SDM Section "Relative Priority of Faults and VM Exits": > > > "Certain exceptions have priority over VM exits. These include > > > invalid-opcode exceptions, ..." > > > Seems not further classifications of #UDs. > > > > This is clarified in the pseudo code of VMX instructions in the SDM. If you > > look at the pseudo code, all VMX instructions except VMXON (obviously) have > > something like below: > > > > IF (not in VMX operation) ... > > THEN #UD; > > ELSIF in VMX non-root operation > > THEN VMexit; > > > > So to me "this particular" #UD has higher priority over VM exits (while other > > #UDs may not). > > > But IIUC above #UD won't happen when running VMX instruction in the guest, > > because if there's any live guest, the CPU must already have been in VMX > > operation. So below check in nested_vmx_check_permission(): > > > > if (!to_vmx(vcpu)->nested.vmxon) { > > kvm_queue_exception(vcpu, UD_VECTOR); > > return 0; > > } > > > > is needed to emulate the case that guest runs any other VMX instructions before > > VMXON. > > Yep. IMO, the pseucode is misleading/confusing, the "in VMX non-root operation" > check should really come first. The VMXON pseudocode has the same awkward > sequence: > > IF (register operand) or (CR0.PE = 0) or (CR4.VMXE = 0) or ... > THEN #UD; > ELSIF not in VMX operation > THEN > IF (CPL > 0) or (in A20M mode) or > (the values of CR0 and CR4 are not supported in VMX operation) > THEN #GP(0); > ELSIF in VMX non-root operation > THEN VMexit; > ELSIF CPL > 0 > THEN #GP(0); > ELSE VMfail("VMXON executed in VMX root operation"); > FI; > > > whereas I find this sequence for VMXON more representative of what actually happens: > > IF (register operand) or (CR0.PE = 0) or (CR4.VMXE = 0) or ... > THEN #UD > > IF in VMX non-root operation > THEN VMexit; > > IF CPL > 0 > THEN #GP(0) > > IF in VMX operation > THEN VMfail("VMXON executed in VMX root operation"); > > IF (in A20M mode) or > (the values of CR0 and CR4 are not supported in VMX operation) > THEN #GP(0); Perhaps we need to live with the fact that the pseudo code in the SDM can be buggy :)
On 4/3/2023 7:24 PM, Huang, Kai wrote: >> >> >> Anyway, I will seperate this patch from the LAM KVM enabling patch. And >> send a patch seperately if >> needed later. >> > I think your change for SGX is still needed based on the pseudo code of ENCLS. Yes, I meant I would seperate VMX part since it is not a bug after all, SGX will still be in the patchset. >
On 4/3/2023 7:24 PM, Huang, Kai wrote: >> I checked the code again and find the comment of >> nested_vmx_check_permission(). >> >> "/* >> * Intel's VMX Instruction Reference specifies a common set of >> prerequisites >> * for running VMX instructions (except VMXON, whose prerequisites are >> * slightly different). It also specifies what exception to inject >> otherwise. >> * Note that many of these exceptions have priority over VM exits, so they >> * don't have to be checked again here. >> */" >> >> I think the Note part in the comment has tried to callout why the check >> for compatibility mode is unnecessary. >> >> But I have a question here, nested_vmx_check_permission() checks that >> the vcpu is vmxon, >> otherwise it will inject a #UD. Why this #UD is handled in the VMExit >> handler specifically? >> Not all #UDs have higher priority than VM exits? >> >> According to SDM Section "Relative Priority of Faults and VM Exits": >> "Certain exceptions have priority over VM exits. These include >> invalid-opcode exceptions, ..." >> Seems not further classifications of #UDs. > This is clarified in the pseudo code of VMX instructions in the SDM. If you > look at the pseudo code, all VMX instructions except VMXON (obviously) have > something like below: > > IF (not in VMX operation) ... > THEN #UD; > ELSIF in VMX non-root operation > THEN VMexit; > > So to me "this particular" #UD has higher priority over VM exits (while other > #UDs may not). > > But IIUC above #UD won't happen when running VMX instruction in the guest, > because if there's any live guest, the CPU must already have been in VMX > operation. So below check in nested_vmx_check_permission(): > > if (!to_vmx(vcpu)->nested.vmxon) { > kvm_queue_exception(vcpu, UD_VECTOR); > return 0; > } > > is needed to emulate the case that guest runs any other VMX instructions before > VMXON. > Yes, you are right. Get the point now, thanks.
On Tue, 2023-04-04 at 09:21 +0800, Binbin Wu wrote: > On 4/3/2023 7:24 PM, Huang, Kai wrote: > > > > > > > > > Anyway, I will seperate this patch from the LAM KVM enabling patch. And > > > send a patch seperately if > > > needed later. > > > > > I think your change for SGX is still needed based on the pseudo code of ENCLS. > > Yes, I meant I would seperate VMX part since it is not a bug after all, > SGX will still be in the patchset. > > Shouldn't SGX part be also split out as a bug fix patch? Does it have anything to do with this LAM support series?
On 4/4/2023 9:53 AM, Huang, Kai wrote: > On Tue, 2023-04-04 at 09:21 +0800, Binbin Wu wrote: >> On 4/3/2023 7:24 PM, Huang, Kai wrote: >>>> >>>> Anyway, I will seperate this patch from the LAM KVM enabling patch. And >>>> send a patch seperately if >>>> needed later. >>>> >>> I think your change for SGX is still needed based on the pseudo code of ENCLS. >> Yes, I meant I would seperate VMX part since it is not a bug after all, >> SGX will still be in the patchset. >> >> > Shouldn't SGX part be also split out as a bug fix patch? > > Does it have anything to do with this LAM support series? It is related to LAM support because LAM only effective in 64-bit mode, so the untag action should only be done in 64-bit mode. If the SGX fix patch is not included, that means LAM untag could be called in compatiblity mode in SGX ENCLS handler.
On Tue, 2023-04-04 at 10:45 +0800, Binbin Wu wrote: > On 4/4/2023 9:53 AM, Huang, Kai wrote: > > On Tue, 2023-04-04 at 09:21 +0800, Binbin Wu wrote: > > > On 4/3/2023 7:24 PM, Huang, Kai wrote: > > > > > > > > > > Anyway, I will seperate this patch from the LAM KVM enabling patch. And > > > > > send a patch seperately if > > > > > needed later. > > > > > > > > > I think your change for SGX is still needed based on the pseudo code of ENCLS. > > > Yes, I meant I would seperate VMX part since it is not a bug after all, > > > SGX will still be in the patchset. > > > > > > > > Shouldn't SGX part be also split out as a bug fix patch? > > > > Does it have anything to do with this LAM support series? > > It is related to LAM support because LAM only effective in 64-bit mode, > so the untag action should only be done in 64-bit mode. > > If the SGX fix patch is not included, that means LAM untag could be > called in compatiblity mode in SGX ENCLS handler. > > Yes I got this point, and your patch 6/7 depends on it. But my point is this fix is needed anyway regardless the LAM support, and it should be merged, for instance, asap as a bug fix (and CC stable perhaps) -- while the LAM support is a feature, and can be merged at a different time frame. Of course just my 2cents and this is up to maintainers.
On 4/4/2023 11:09 AM, Huang, Kai wrote: > On Tue, 2023-04-04 at 10:45 +0800, Binbin Wu wrote: >> On 4/4/2023 9:53 AM, Huang, Kai wrote: >>> On Tue, 2023-04-04 at 09:21 +0800, Binbin Wu wrote: >>>> On 4/3/2023 7:24 PM, Huang, Kai wrote: >>>>>> Anyway, I will seperate this patch from the LAM KVM enabling patch. And >>>>>> send a patch seperately if >>>>>> needed later. >>>>>> >>>>> I think your change for SGX is still needed based on the pseudo code of ENCLS. >>>> Yes, I meant I would seperate VMX part since it is not a bug after all, >>>> SGX will still be in the patchset. >>>> >>>> >>> Shouldn't SGX part be also split out as a bug fix patch? >>> >>> Does it have anything to do with this LAM support series? >> It is related to LAM support because LAM only effective in 64-bit mode, >> so the untag action should only be done in 64-bit mode. >> >> If the SGX fix patch is not included, that means LAM untag could be >> called in compatiblity mode in SGX ENCLS handler. >> >> > Yes I got this point, and your patch 6/7 depends on it. > > But my point is this fix is needed anyway regardless the LAM support, and it > should be merged, for instance, asap as a bug fix (and CC stable perhaps) -- > while the LAM support is a feature, and can be merged at a different time frame. OK, I can seperate the patch. > > Of course just my 2cents and this is up to maintainers.
On 4/4/2023 11:15 AM, Binbin Wu wrote: > > On 4/4/2023 11:09 AM, Huang, Kai wrote: >> On Tue, 2023-04-04 at 10:45 +0800, Binbin Wu wrote: >>> On 4/4/2023 9:53 AM, Huang, Kai wrote: >>>> On Tue, 2023-04-04 at 09:21 +0800, Binbin Wu wrote: >>>>> On 4/3/2023 7:24 PM, Huang, Kai wrote: >>>>>>> Anyway, I will seperate this patch from the LAM KVM enabling >>>>>>> patch. And >>>>>>> send a patch seperately if >>>>>>> needed later. >>>>>>> >>>>>> I think your change for SGX is still needed based on the pseudo >>>>>> code of ENCLS. >>>>> Yes, I meant I would seperate VMX part since it is not a bug after >>>>> all, >>>>> SGX will still be in the patchset. >>>>> >>>>> >>>> Shouldn't SGX part be also split out as a bug fix patch? >>>> >>>> Does it have anything to do with this LAM support series? >>> It is related to LAM support because LAM only effective in 64-bit mode, >>> so the untag action should only be done in 64-bit mode. >>> >>> If the SGX fix patch is not included, that means LAM untag could be >>> called in compatiblity mode in SGX ENCLS handler. >>> >>> >> Yes I got this point, and your patch 6/7 depends on it. >> >> But my point is this fix is needed anyway regardless the LAM support, >> and it >> should be merged, for instance, asap as a bug fix (and CC stable >> perhaps) -- >> while the LAM support is a feature, and can be merged at a different >> time frame. > > OK, I can seperate the patch. Kai, I sent the patch seperately and added your reivewed-by from the previous review. https://lore.kernel.org/kvm/20230404032502.27798-1-binbin.wu@linux.intel.com/T/#u > > >> >> Of course just my 2cents and this is up to maintainers.
On 3/30/2023 1:34 AM, Sean Christopherson wrote: > On Wed, Mar 29, 2023, Binbin Wu wrote: >> On 3/29/2023 10:04 AM, Huang, Kai wrote: >>> On Wed, 2023-03-29 at 09:27 +0800, Binbin Wu wrote: >>>> On 3/29/2023 7:33 AM, Huang, Kai wrote: >>>>> On Tue, 2023-03-21 at 14:35 -0700, Sean Christopherson wrote: >>>>>> On Mon, Mar 20, 2023, Chao Gao wrote: >>>>>>> On Sun, Mar 19, 2023 at 04:49:22PM +0800, Binbin Wu wrote: >>>>>>>> get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() >>>>>>>> to check 64-bit mode. Should use is_64_bit_mode() instead. >>>>>>>> >>>>>>>> Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") >>>>>>>> Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") >>>>>>> It is better to split this patch into two: one for nested and one for >>>>>>> SGX. >>>>>>> >>>>>>> It is possible that there is a kernel release which has just one of >>>>>>> above two flawed commits, then this fix patch cannot be applied cleanly >>>>>>> to the release. >>>>>> The nVMX code isn't buggy, VMX instructions #UD in compatibility mode, and except >>>>>> for VMCALL, that #UD has higher priority than VM-Exit interception. So I'd say >>>>>> just drop the nVMX side of things. >>>>> But it looks the old code doesn't unconditionally inject #UD when in >>>>> compatibility mode? >>>> I think Sean means VMX instructions is not valid in compatibility mode >>>> and it triggers #UD, which has higher priority than VM-Exit, by the >>>> processor in non-root mode. >>>> >>>> So if there is a VM-Exit due to VMX instruction , it is in 64-bit mode >>>> for sure if it is in long mode. >>> Oh I see thanks. >>> >>> Then is it better to add some comment to explain, or add a WARN() if it's not in >>> 64-bit mode? >> I also prefer to add a comment if no objection. >> >> Seems I am not the only one who didn't get it� : ) > I would rather have a code change than a comment, e.g. > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index f63b28f46a71..0460ca219f96 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4931,7 +4931,8 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification, > int base_reg = (vmx_instruction_info >> 23) & 0xf; > bool base_is_valid = !(vmx_instruction_info & (1u << 27)); > > - if (is_reg) { > + if (is_reg || > + WARN_ON_ONCE(is_long_mode(vcpu) && !is_64_bit_mode(vcpu))) { > kvm_queue_exception(vcpu, UD_VECTOR); > return 1; > } > > > The only downside is that querying is_64_bit_mode() could unnecessarily trigger a > VMREAD to get the current CS.L bit, but a measurable performance regressions is > extremely unlikely because is_64_bit_mode() all but guaranteed to be called in > these paths anyways (and KVM caches segment info), e.g. by kvm_register_read(). > > And then in a follow-up, we should also be able to do: > > @@ -5402,7 +5403,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu) > if (instr_info & BIT(10)) { > kvm_register_write(vcpu, (((instr_info) >> 3) & 0xf), value); > } else { > - len = is_64_bit_mode(vcpu) ? 8 : 4; > + len = is_long_mode(vcpu) ? 8 : 4; > if (get_vmx_mem_address(vcpu, exit_qualification, > instr_info, true, len, &gva)) > return 1; > @@ -5476,7 +5477,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu) > if (instr_info & BIT(10)) > value = kvm_register_read(vcpu, (((instr_info) >> 3) & 0xf)); > else { > - len = is_64_bit_mode(vcpu) ? 8 : 4; > + len = is_long_mode(vcpu) ? 8 : 4; > if (get_vmx_mem_address(vcpu, exit_qualification, > instr_info, false, len, &gva)) > return 1; Agree to replace is_64_bit_mode() with is_long_mode(). But, based on the implementation and comment of nested_vmx_check_permission(), do you think it still needs to add the check for compatibility mode?
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 557b9c468734..0f84cc05f57c 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4959,7 +4959,7 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification, /* Checks for #GP/#SS exceptions. */ exn = false; - if (is_long_mode(vcpu)) { + if (is_64_bit_mode(vcpu)) { /* * The virtual/linear address is never truncated in 64-bit * mode, e.g. a 32-bit address size can yield a 64-bit virtual diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c index aa53c98034bf..0574030b071f 100644 --- a/arch/x86/kvm/vmx/sgx.c +++ b/arch/x86/kvm/vmx/sgx.c @@ -29,14 +29,14 @@ static int sgx_get_encls_gva(struct kvm_vcpu *vcpu, unsigned long offset, /* Skip vmcs.GUEST_DS retrieval for 64-bit mode to avoid VMREADs. */ *gva = offset; - if (!is_long_mode(vcpu)) { + if (!is_64_bit_mode(vcpu)) { vmx_get_segment(vcpu, &s, VCPU_SREG_DS); *gva += s.base; } if (!IS_ALIGNED(*gva, alignment)) { fault = true; - } else if (likely(is_long_mode(vcpu))) { + } else if (likely(is_64_bit_mode(vcpu))) { fault = is_noncanonical_address(*gva, vcpu); } else { *gva &= 0xffffffff;
get_vmx_mem_address() and sgx_get_encls_gva() use is_long_mode() to check 64-bit mode. Should use is_64_bit_mode() instead. Fixes: f9eb4af67c9d ("KVM: nVMX: VMX instructions: add checks for #GP/#SS exceptions") Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions") Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> --- arch/x86/kvm/vmx/nested.c | 2 +- arch/x86/kvm/vmx/sgx.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)