| Message ID | 1594168797-29444-3-git-send-email-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nSVM: Check MBZ bits in CR3 and CR4 on vmrun of nested guests | expand |
On 08/07/20 02:39, Krish Sadhukhan wrote: > +extern int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4); > + This should be added in x86.h, not here. > +static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb) > { > if ((vmcb->save.efer & EFER_SVME) == 0) > return false; > @@ -231,6 +233,22 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) > (vmcb->save.cr0 & X86_CR0_NW)) > return false; > > + if (!is_long_mode(&(svm->vcpu))) { > + if (vmcb->save.cr4 & X86_CR4_PAE) { > + if (vmcb->save.cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK) > + return false; > + } else { > + if (vmcb->save.cr3 & MSR_CR3_LEGACY_RESERVED_MASK) > + return false; > + } > + } else { > + if ((vmcb->save.cr4 & X86_CR4_PAE) && > + (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK)) > + return false; > + } is_long_mode here is wrong, as it refers to the host. You need to do something like this: diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 385461496cf5..cbbab83f19cc 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -222,8 +222,9 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control) return true; } -static bool nested_vmcb_checks(struct vmcb *vmcb) +static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb) { + bool nested_vmcb_lma; if ((vmcb->save.efer & EFER_SVME) == 0) return false; @@ -234,6 +237,27 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) if (!kvm_dr6_valid(vmcb->save.dr6) || !kvm_dr7_valid(vmcb->save.dr7)) return false; + nested_vmcb_lma = + (vmcb->save.efer & EFER_LME) && + (vmcb->save.cr0 & X86_CR0_PG); + + if (!nested_vmcb_lma) { + if (vmcb->save.cr4 & X86_CR4_PAE) { + if (vmcb->save.cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK) + return false; + } else { + if (vmcb->save.cr3 & MSR_CR3_LEGACY_RESERVED_MASK) + return false; + } + } else { + if (!(vmcb->save.cr4 & X86_CR4_PAE) || + !(vmcb->save.cr0 & X86_CR0_PE) || + (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK)) + return false; + } + if (kvm_valid_cr4(&(svm->vcpu), vmcb->save.cr4)) + return false; + return nested_vmcb_check_controls(&vmcb->control); } which also takes care of other CR0/CR4 checks in the APM. I'll test this a bit more and queue it. Are you also going to add more checks in svm_set_nested_state? Paolo
On 7/8/20 3:03 AM, Paolo Bonzini wrote: > On 08/07/20 02:39, Krish Sadhukhan wrote: >> +extern int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4); >> + > This should be added in x86.h, not here. > >> +static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb) >> { >> if ((vmcb->save.efer & EFER_SVME) == 0) >> return false; >> @@ -231,6 +233,22 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) >> (vmcb->save.cr0 & X86_CR0_NW)) >> return false; >> >> + if (!is_long_mode(&(svm->vcpu))) { >> + if (vmcb->save.cr4 & X86_CR4_PAE) { >> + if (vmcb->save.cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK) >> + return false; >> + } else { >> + if (vmcb->save.cr3 & MSR_CR3_LEGACY_RESERVED_MASK) >> + return false; >> + } >> + } else { >> + if ((vmcb->save.cr4 & X86_CR4_PAE) && >> + (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK)) >> + return false; >> + } > is_long_mode here is wrong, as it refers to the host. > > You need to do something like this: > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index 385461496cf5..cbbab83f19cc 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -222,8 +222,9 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control) > return true; > } > > -static bool nested_vmcb_checks(struct vmcb *vmcb) > +static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb) > { > + bool nested_vmcb_lma; > if ((vmcb->save.efer & EFER_SVME) == 0) > return false; > > @@ -234,6 +237,27 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) > if (!kvm_dr6_valid(vmcb->save.dr6) || !kvm_dr7_valid(vmcb->save.dr7)) > return false; > > + nested_vmcb_lma = > + (vmcb->save.efer & EFER_LME) && > + (vmcb->save.cr0 & X86_CR0_PG); > + > + if (!nested_vmcb_lma) { > + if (vmcb->save.cr4 & X86_CR4_PAE) { > + if (vmcb->save.cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK) > + return false; > + } else { > + if (vmcb->save.cr3 & MSR_CR3_LEGACY_RESERVED_MASK) > + return false; > + } > + } else { > + if (!(vmcb->save.cr4 & X86_CR4_PAE) || > + !(vmcb->save.cr0 & X86_CR0_PE) || > + (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK)) > + return false; > + } > + if (kvm_valid_cr4(&(svm->vcpu), vmcb->save.cr4)) > + return false; > + > return nested_vmcb_check_controls(&vmcb->control); > } > > which also takes care of other CR0/CR4 checks in the APM. Thanks for adding additional other checks and fixing the long-mode check. > > I'll test this a bit more and queue it. Are you also going to add > more checks in svm_set_nested_state? I will send a patch to cover the missing checks in svm_set_nested_state(). > > Paolo >
On 7/8/20 3:03 AM, Paolo Bonzini wrote: > On 08/07/20 02:39, Krish Sadhukhan wrote: >> +extern int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4); >> + > This should be added in x86.h, not here. > >> +static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb) >> { >> if ((vmcb->save.efer & EFER_SVME) == 0) >> return false; >> @@ -231,6 +233,22 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) >> (vmcb->save.cr0 & X86_CR0_NW)) >> return false; >> >> + if (!is_long_mode(&(svm->vcpu))) { >> + if (vmcb->save.cr4 & X86_CR4_PAE) { >> + if (vmcb->save.cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK) >> + return false; >> + } else { >> + if (vmcb->save.cr3 & MSR_CR3_LEGACY_RESERVED_MASK) >> + return false; >> + } >> + } else { >> + if ((vmcb->save.cr4 & X86_CR4_PAE) && >> + (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK)) >> + return false; >> + } > is_long_mode here is wrong, as it refers to the host. > > You need to do something like this: > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index 385461496cf5..cbbab83f19cc 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -222,8 +222,9 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control) > return true; > } > > -static bool nested_vmcb_checks(struct vmcb *vmcb) > +static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb) > { > + bool nested_vmcb_lma; > if ((vmcb->save.efer & EFER_SVME) == 0) > return false; > > @@ -234,6 +237,27 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) > if (!kvm_dr6_valid(vmcb->save.dr6) || !kvm_dr7_valid(vmcb->save.dr7)) > return false; > > + nested_vmcb_lma = > + (vmcb->save.efer & EFER_LME) && Just curious about using LME instead of LMA. According to APM, " The processor behaves as a 32-bit x86 processor in all respects until long mode is activated, even if long mode is enabled. None of the new 64-bit data sizes, addressing, or system aspects available in long mode can be used until EFER.LMA=1." Is it possible that L1 sets LME, but not LMA, in L2's VMCS and this code will execute even if the processor is not in long-mode ? > + (vmcb->save.cr0 & X86_CR0_PG); > + > + if (!nested_vmcb_lma) { > + if (vmcb->save.cr4 & X86_CR4_PAE) { > + if (vmcb->save.cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK) > + return false; > + } else { > + if (vmcb->save.cr3 & MSR_CR3_LEGACY_RESERVED_MASK) > + return false; > + } > + } else { > + if (!(vmcb->save.cr4 & X86_CR4_PAE) || > + !(vmcb->save.cr0 & X86_CR0_PE) || > + (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK)) > + return false; > + } > + if (kvm_valid_cr4(&(svm->vcpu), vmcb->save.cr4)) > + return false; > + > return nested_vmcb_check_controls(&vmcb->control); > } > > which also takes care of other CR0/CR4 checks in the APM. > > I'll test this a bit more and queue it. Are you also going to add > more checks in svm_set_nested_state? > > Paolo >
On Wed, Jul 8, 2020 at 3:51 PM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > Just curious about using LME instead of LMA. According to APM, > > " The processor behaves as a 32-bit x86 processor in all respects > until long mode is activated, even if long mode is enabled. None of the > new 64-bit data sizes, addressing, or system aspects available in long > mode can be used until EFER.LMA=1." > > > Is it possible that L1 sets LME, but not LMA, in L2's VMCS and this > code will execute even if the processor is not in long-mode ? No. EFER.LMA is not modifiable through software. It is always "EFER.LME != 0 && CR0.PG != 0."
On 09/07/20 01:07, Jim Mattson wrote: >> Just curious about using LME instead of LMA. According to APM, >> >> " The processor behaves as a 32-bit x86 processor in all respects >> until long mode is activated, even if long mode is enabled. None of the >> new 64-bit data sizes, addressing, or system aspects available in long >> mode can be used until EFER.LMA=1." >> >> >> Is it possible that L1 sets LME, but not LMA, in L2's VMCS and this >> code will execute even if the processor is not in long-mode ? > > No. EFER.LMA is not modifiable through software. It is always > "EFER.LME != 0 && CR0.PG != 0." In fact, AMD doesn't specify (unlike Intel) that EFER.LME, CR0.PG and EFER.LMA must be consistent, and for SMM state restore they say that "The EFER.LMA register bit is set to the value obtained by logically ANDing the SMRAM values of EFER.LME, CR0.PG, and CR4.PAE". So it is plausible that they ignore completely EFER.LMA in the VMCB. I quickly tried hacking svm_set_efer to set or reset it, and it works either way. EFLAGS.VM from the VMCB is also ignored if the processor is in long mode just like the APM says in "10.4 Leaving SMM"! Paolo
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 6bceafb..6d2ac5a 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -222,7 +222,9 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control) return true; } -static bool nested_vmcb_checks(struct vmcb *vmcb) +extern int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4); + +static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb) { if ((vmcb->save.efer & EFER_SVME) == 0) return false; @@ -231,6 +233,22 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) (vmcb->save.cr0 & X86_CR0_NW)) return false; + if (!is_long_mode(&(svm->vcpu))) { + if (vmcb->save.cr4 & X86_CR4_PAE) { + if (vmcb->save.cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK) + return false; + } else { + if (vmcb->save.cr3 & MSR_CR3_LEGACY_RESERVED_MASK) + return false; + } + } else { + if ((vmcb->save.cr4 & X86_CR4_PAE) && + (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK)) + return false; + } + if (kvm_valid_cr4(&(svm->vcpu), vmcb->save.cr4)) + return false; + return nested_vmcb_check_controls(&vmcb->control); } @@ -416,7 +434,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm) nested_vmcb = map.hva; - if (!nested_vmcb_checks(nested_vmcb)) { + if (!nested_vmcb_checks(svm, nested_vmcb)) { nested_vmcb->control.exit_code = SVM_EXIT_ERR; nested_vmcb->control.exit_code_hi = 0; nested_vmcb->control.exit_info_1 = 0; diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 6ac4c00..26b14ec 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -346,7 +346,10 @@ static inline bool gif_set(struct vcpu_svm *svm) } /* svm.c */ -#define MSR_INVALID 0xffffffffU +#define MSR_CR3_LEGACY_RESERVED_MASK 0xfe7U +#define MSR_CR3_LEGACY_PAE_RESERVED_MASK 0x7U +#define MSR_CR3_LONG_RESERVED_MASK 0xfff0000000000fe7U +#define MSR_INVALID 0xffffffffU u32 svm_msrpm_offset(u32 msr); void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index f0335bc..732ae6a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -932,7 +932,7 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr) } EXPORT_SYMBOL_GPL(kvm_set_xcr); -static int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) +int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) { if (cr4 & cr4_reserved_bits) return -EINVAL; @@ -942,6 +942,7 @@ static int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) return 0; } +EXPORT_SYMBOL_GPL(kvm_valid_cr4); int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) {
According to section "Canonicalization and Consistency Checks" in APM vol. 2 the following guest state is illegal: "Any MBZ bit of CR3 is set." "Any MBZ bit of CR4 is set." Suggeted-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> --- arch/x86/kvm/svm/nested.c | 22 ++++++++++++++++++++-- arch/x86/kvm/svm/svm.h | 5 ++++- arch/x86/kvm/x86.c | 3 ++- 3 files changed, 26 insertions(+), 4 deletions(-)