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: <YqimcQTl9ia+ov/I@anrayabh-desk>
Date:   Tue, 14 Jun 2022 20:47:05 +0530
From:   Anirudh Rayabharam <anrayabh@...ux.microsoft.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        Ilias Stamatis <ilstam@...zon.com>,
        Maxim Levitsky <mlevitsk@...hat.com>, mail@...rudhrb.com,
        kumarpraveen@...ux.microsoft.com, wei.liu@...nel.org,
        robert.bradford@...el.com, liuwe@...rosoft.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: nVMX: Don't expose TSC scaling to L1 when on Hyper-V

On Tue, Jun 14, 2022 at 10:25:52AM +0530, Anirudh Rayabharam wrote:
> On Mon, Jun 13, 2022 at 06:49:17PM +0200, Paolo Bonzini wrote:
> > On 6/13/22 18:16, Anirudh Rayabharam wrote:
> > > +	if (!kvm_has_tsc_control)
> > > +		msrs->secondary_ctls_high &= ~SECONDARY_EXEC_TSC_SCALING;
> > > +
> > >   	msrs->secondary_ctls_low = 0;
> > >   	msrs->secondary_ctls_high &=
> > >   		SECONDARY_EXEC_DESC |
> > > @@ -6667,8 +6670,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
> > >   		SECONDARY_EXEC_RDRAND_EXITING |
> > >   		SECONDARY_EXEC_ENABLE_INVPCID |
> > >   		SECONDARY_EXEC_RDSEED_EXITING |
> > > -		SECONDARY_EXEC_XSAVES |
> > > -		SECONDARY_EXEC_TSC_SCALING;
> > > +		SECONDARY_EXEC_XSAVES;
> > >   	/*
> > 
> > This is wrong because it _always_ disables SECONDARY_EXEC_TSC_SCALING,
> > even if kvm_has_tsc_control == true.
> 
> The MSR actually allows 1-setting of the "use TSC scaling" control. So this
> line is redundant anyway.
> 
> > 
> > That said, I think a better implementation of this patch is to just add
> > a version of evmcs_sanitize_exec_ctrls that takes a struct
> > nested_vmx_msrs *, and call it at the end of nested_vmx_setup_ctl_msrs like
> > 
> > 	evmcs_sanitize_nested_vmx_vsrs(msrs);
> 
> Sanitize at the end might not work because I see some cases in
> nested_vmx_setup_ctls_msrs() where we want to expose some things to L1
> even though the hardware doesn't support it.
> 
> > 
> > Even better (but I cannot "mentally test it" offhand) would be just
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index e802f71a9e8d..b3425ce835c5 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -1862,7 +1862,7 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >  		 * sanity checking and refuse to boot. Filter all unsupported
> >  		 * features out.
> >  		 */
> > -		if (!msr_info->host_initiated &&
> > +		if (static_branch_unlikely(&enable_evmcs) ||
> >  		    vmx->nested.enlightened_vmcs_enabled)
> >  			nested_evmcs_filter_control_msr(msr_info->index,
> >  							&msr_info->data);
> 
> I will try this.

This patch fixed the issue for me. But again, this might filter out
things that we wan't to expose to L1 even if not available in hardware.

Thanks,

	Anirudh.

> 
> Thanks,
> 
> 	Anirudh.
> 
> > 
> > I cannot quite understand the host_initiated check, so I'll defer to
> > Vitaly on why it is needed.  Most likely, removing it would cause some
> > warnings in QEMU with e.g. "-cpu Haswell,+vmx"; but I think it's a
> > userspace bug and we should remove that part of the condition.  You
> > don't need to worry about that part, we'll cross that bridge if the
> > above patch works for your case.
> > 
> > Thanks,
> > 
> > Paolo

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ