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

[Tom Lendacky <thomas.lendacky@amd.com>]

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@amazon.de>
> Reviewed-by: Benjamin Serebrin <serebrin@amazon.com>
> Reviewed-by: Filippo Sironi <sironi@amazon.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)) {