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

[Jim Mattson]

On Thu, Sep 29, 2022 at 6:47 AM Tom Lendacky <thomas.lendacky@xxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> > Reviewed-by: Benjamin Serebrin <serebrin@xxxxxxxxxx>
> > Reviewed-by: Filippo Sironi <sironi@xxxxxxxxx>
> > ---
> >   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);

This is much too late to restore the host's value of IA32_SPEC_CTRL on
hosts that are using RETBLEED_MITIGATION_UNRET. The host's
IA32_SPEC_CTRL value should be restored prior to the UNTRAIN_RET in
vmenter.S.

> >       if (!sev_es_guest(vcpu->kvm)) {