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]

Message-ID: <7ef28201-4a14-fdc6-4a28-153378c1efc2@amd.com>
Date:   Thu, 11 Feb 2021 16:42:46 -0600
From:   Babu Moger <babu.moger@....com>
To:     Paolo Bonzini <pbonzini@...hat.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 2/11/21 2:56 AM, Paolo Bonzini wrote:
> 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.

Makes sense. I have tested removing these two hunks and it worked fine.

> 
> 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,

Yes. 5.13 is fine.
thanks
Babu

> those have been tested already but I need to send them out for review
> before "officially" getting them in kvm.git.
> 
> Paolo
>