linux-kernel - Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC

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]

Date:   Thu, 11 Feb 2021 09:56:39 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Babu Moger <babu.moger@....com>, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de
Cc:     fenghua.yu@...el.com, tony.luck@...el.com, wanpengli@...cent.com,
        kvm@...r.kernel.org, thomas.lendacky@....com, peterz@...radead.org,
        seanjc@...gle.com, joro@...tes.org, x86@...nel.org,
        kyung.min.park@...el.com, linux-kernel@...r.kernel.org,
        krish.sadhukhan@...cle.com, hpa@...or.com, mgross@...ux.intel.com,
        vkuznets@...hat.com, kim.phillips@....com, wei.huang2@....com,
        jmattson@...gle.com
Subject: Re: [PATCH v4 2/2] KVM: SVM: Add support for Virtual SPEC_CTRL

On 29/01/21 01:43, Babu Moger wrote:
> This support also fixes an issue where a guest may sometimes see an 
> inconsistent value for the SPEC_CTRL MSR on processors that support this 
> feature. With the current SPEC_CTRL support, the first write to 
> SPEC_CTRL is intercepted and the virtualized version of the SPEC_CTRL 
> MSR is not updated.

This is a bit ugly, new features should always be enabled manually (AMD 
did it right for vVMLOAD/vVMSAVE for example, even though _in theory_ 
assuming that all hypervisors were intercepting VMLOAD/VMSAVE would have 
been fine).

Also regarding nested virtualization:

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 7a605ad8254d..9e51f9e4f631 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -534,6 +534,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm)
>  		hsave->save.cr3    = vmcb->save.cr3;
>  	else
>  		hsave->save.cr3    = kvm_read_cr3(&svm->vcpu);
> +	hsave->save.spec_ctrl = vmcb->save.spec_ctrl;
>  
>  	copy_vmcb_control_area(&hsave->control, &vmcb->control);
>  
> @@ -675,6 +676,7 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>  	kvm_rip_write(&svm->vcpu, hsave->save.rip);
>  	svm->vmcb->save.dr7 = DR7_FIXED_1;
>  	svm->vmcb->save.cpl = 0;
> +	svm->vmcb->save.spec_ctrl = hsave->save.spec_ctrl;
>  	svm->vmcb->control.exit_int_info = 0;
>  
>  	vmcb_mark_all_dirty(svm->vmcb);

I think this is incorrect.  Since we don't support this feature in the 
nested hypervisor, any writes to the SPEC_CTRL MSR while L2 (nested 
guest) runs have to be reflected to L1 (nested hypervisor).  In other 
words, this new field is more like VMLOAD/VMSAVE state, in that it 
doesn't change across VMRUN and VMEXIT.  These two hunks can be removed.

If we want to do it, exposing this feature to the nested hypervisor will 
be a bit complicated, because one has to write host SPEC_CTRL | 
vmcb01.GuestSpecCtrl in the host MSR, in order to free the vmcb02 
GuestSpecCtrl for the vmcb12 GuestSpecCtrl.

It would also be possible to emulate it on processors that don't have 
it.  However I'm not sure it's a good idea because of the problem that 
you mentioned with running old kernels on new processors.

I have queued the patches with the small fix above.  However I plan to 
only include them in 5.13 because I have a bunch of other SVM patches, 
those have been tested already but I need to send them out for review 
before "officially" getting them in kvm.git.

Paolo