linux-kernel - Re: [PATCH v3 3/4] KVM: nVMX: relax canonical checks on some x86 registers in vmx host state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zr_Ms-7IpzINzmc7@google.com>
Date: Fri, 16 Aug 2024 15:03:31 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: mlevitsk@...hat.com
Cc: kvm@...r.kernel.org, Ingo Molnar <mingo@...hat.com>, x86@...nel.org, 
	Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Borislav Petkov <bp@...en8.de>, linux-kernel@...r.kernel.org, 
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v3 3/4] KVM: nVMX: relax canonical checks on some x86
 registers in vmx host state

On Fri, Aug 16, 2024, mlevitsk@...hat.com wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 30 +++++++++++++++++++++++-------
> >  1 file changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 2392a7ef254d..3f18edff80ac 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -2969,6 +2969,22 @@ static int nested_vmx_check_address_space_size(struct kvm_vcpu *vcpu,
> >         return 0;
> >  }
> >  
> > +static bool is_l1_noncanonical_address_static(u64 la, struct kvm_vcpu *vcpu)
> > +{
> > +       u8 max_guest_address_bits = guest_can_use(vcpu, X86_FEATURE_LA57) ? 57 : 48;

I don't see any reason to use LA57 support from guest CPUID for the VMCS checks.
The virtualization hole exists can't be safely plugged for all cases, so why
bother trying to plug it only for some cases?

It'd be very odd that an L1 could set a "bad" value via WRMSR, but then couldn't
load that same value on VM-Exit, e.g. if L1 gets the VMCS value by doing RDMSR.

> > +       /*
> > +        * Most x86 arch registers which contain linear addresses like
> > +        * segment bases, addresses that are used in instructions (e.g SYSENTER),
> > +        * have static canonicality checks,
> > +        * size of whose depends only on CPU's support for 5-level
> > +        * paging, rather than state of CR4.LA57.
> > +        *
> > +        * In other words the check only depends on the CPU model,
> > +        * rather than on runtime state.
> > +        */
> > +       return !__is_canonical_address(la, max_guest_address_bits);
> > +}
> > +
> >  static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> >                                        struct vmcs12 *vmcs12)
> >  {
> > @@ -2979,8 +2995,8 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> >             CC(!kvm_vcpu_is_legal_cr3(vcpu, vmcs12->host_cr3)))
> >                 return -EINVAL;
> >  
> > -       if (CC(is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu)) ||
> > -           CC(is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu)))
> > +       if (CC(is_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_esp, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_ia32_sysenter_eip, vcpu)))
> >                 return -EINVAL;
> >  
> >         if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
> > @@ -3014,11 +3030,11 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
> >             CC(vmcs12->host_ss_selector == 0 && !ia32e))
> >                 return -EINVAL;
> >  
> > -       if (CC(is_noncanonical_address(vmcs12->host_fs_base, vcpu)) ||
> > -           CC(is_noncanonical_address(vmcs12->host_gs_base, vcpu)) ||
> > -           CC(is_noncanonical_address(vmcs12->host_gdtr_base, vcpu)) ||
> > -           CC(is_noncanonical_address(vmcs12->host_idtr_base, vcpu)) ||
> > -           CC(is_noncanonical_address(vmcs12->host_tr_base, vcpu)) ||
> > +       if (CC(is_l1_noncanonical_address_static(vmcs12->host_fs_base, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_gs_base, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_gdtr_base, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_idtr_base, vcpu)) ||
> > +           CC(is_l1_noncanonical_address_static(vmcs12->host_tr_base, vcpu)) ||

If loads via LTR, LLDT, and LGDT are indeed exempt, then we need to update
emul_is_noncanonical_address() too.

The best idea I have is to have a separate flow for system registers (not a great
name, but I can't think of anything better), and the

E.g. s/is_host_noncanonical_msr_value/is_non_canonical_system_reg, and then
wire that up to the emulator.

> >             CC(is_noncanonical_address(vmcs12->host_rip, vcpu)))
> >                 return -EINVAL;
> >  
>