linux-kernel - Re: [PATCH] KVM: SVM: don't apply host security mitigations to the guest

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: <f88f3f67-20af-0ed4-0227-f2f89d5bcd50@amd.com>
Date:   Thu, 29 Sep 2022 08:47:34 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Fares Mehanna <faresx@...zon.de>
Cc:     Benjamin Serebrin <serebrin@...zon.com>,
        Filippo Sironi <sironi@...zon.de>,
        Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        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>, Babu Moger <babu.moger@....com>,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: SVM: don't apply host security mitigations to the
 guest

On 9/12/22 06:58, Fares Mehanna wrote:
> Support of virtual SPEC_CTRL caused a new behavior in KVM which made host
> security mitigations applying by default to the guests.

Maybe expand on this to say that the effective mitigation is the host 
SPEC_CTRL value or'd with guest SPEC_CTRL value.

> 
> We noticed a regression after applying the patch, primarily because of the
> enablement of SSBD on AMD Milan.
> 
> This patch keeps the new behavior of applying host security mitigations to
> the guests, but adds an option to disable it so the guests are free to pick
> their own security mitigations.
> 
> Fixes: d00b99c514b3 ("KVM: SVM: Add support for Virtual SPEC_CTRL")
> Signed-off-by: Fares Mehanna <faresx@...zon.de>
> Reviewed-by: Benjamin Serebrin <serebrin@...zon.com>
> Reviewed-by: Filippo Sironi <sironi@...zon.de>
> ---
>   arch/x86/kvm/svm/svm.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index f3813dbacb9f..c6ea24685301 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -225,6 +225,10 @@ module_param(avic, bool, 0444);
>   bool __read_mostly dump_invalid_vmcb;
>   module_param(dump_invalid_vmcb, bool, 0644);
>   
> +/* enable/disable applying host security mitigations on the guest */
> +static bool host_mitigations_on_guest = true;
> +module_param(host_mitigations_on_guest, bool, 0444);
> +
>   
>   bool intercept_smi = true;
>   module_param(intercept_smi, bool, 0444);
> @@ -4000,6 +4004,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
>   	 */
>   	if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
>   		x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
> +	else if (!host_mitigations_on_guest)
> +		/*
> +		 * Clear the host MSR before vm-enter to avoid applying host
> +		 * security mitigations to the guest.
> +		 */
> +		x86_spec_ctrl_set_guest(0, 0);

If X86_FEATURE_V_SPEC_CTRL is active, won't svm->spec_ctrl and 
svm->virt_spec_ctrl always be zero, in which case you can do the if 
statement similar to the below one? Maybe just add a comment that those 
values will be zero in the case of X86_FEATURE_V_SPEC_CTRL, thus 
eliminating the host security mitigation effect.

Thanks,
Tom

>   
>   	svm_vcpu_enter_exit(vcpu);
>   
> @@ -4025,7 +4035,7 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
>   	if (!sev_es_guest(vcpu->kvm))
>   		reload_tss(vcpu);
>   
> -	if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
> +	if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL) || !host_mitigations_on_guest)
>   		x86_spec_ctrl_restore_host(svm->spec_ctrl, svm->virt_spec_ctrl);
>   
>   	if (!sev_es_guest(vcpu->kvm)) {