Message-ID: <645E4ED5-F6EE-4F8F-A990-81F19ED82BFA@nutanix.com>
Date:   Fri, 29 Apr 2022 17:31:16 +0000
From:   Jon Kohler <jon@...anix.com>
To:     Borislav Petkov <bp@...en8.de>
CC:     Jon Kohler <jon@...anix.com>, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Balbir Singh <sblbir@...zon.com>,
        Kim Phillips <kim.phillips@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Kees Cook <keescook@...omium.org>,
        Waiman Long <longman@...hat.com>
Subject: Re: [PATCH v3] x86/speculation, KVM: only IBPB for
 switch_mm_always_ibpb on vCPU load



> On Apr 29, 2022, at 12:59 PM, Borislav Petkov <bp@...en8.de> wrote:
> 
> On Fri, Apr 22, 2022 at 12:21:01PM -0400, Jon Kohler wrote:
>> Both switch_mm_always_ibpb and switch_mm_cond_ibpb are handled by
>> switch_mm() -> cond_mitigation(), which works well in cases where
>> switching vCPUs (i.e. switching tasks) also switches mm_struct;
>> however, this misses a paranoid case where user space may be running
>> multiple guests in a single process (i.e. single mm_struct).
> 
> You lost me here. I admit I'm no virt guy so you'll have to explain in
> more detail what use case that is that you want to support.
> 
> What guests share mm_struct?

Selftests IIUC, but there may be other VMMs that do funny stuff. Said
another way, I don’t think we actively restrict user space from doing
this as far as I know.

> 
> What is the paranoid aspect here? You want to protect a single guest
> from all the others sharing a mm_struct?

The paranoid aspect here is KVM is issuing an *additional* IBPB on
top of what already happens in switch_mm(). 

IMHO KVM side IBPB for most use cases isn’t really necessarily but 
the general concept is that you want to protect vCPU from guest A
from guest B, so you issue a prediction barrier on vCPU switch.

*however* that protection already happens in switch_mm(), because
guest A and B are likely to use different mm_struct, so the only point
of having this support in KVM seems to be to “kill it with fire” for 
paranoid users who might be doing some tomfoolery that would 
somehow bypass switch_mm() protection (such as somehow 
sharing a struct).

One argument could just say, let’s KISS principle and rip this out of
KVM entirely, and users who do this shared mm_struct stuff can just
know that their security profile is possibly less than it could be. That
would certainly simplify the heck out of all of this, but you could probably
set your watch to the next email saying that we broke someones use
case and they’d be all grumpy.

That’s why we’ve proposed keeping this to the always_ibpb path only,
so that if you intentionally configure always_ibpb, you’re going to get
barriers in both KVM and in switch_mm.

Sean feel free to chime in if I missed the mark with this explanation,
you’ve got a way with words for these things :)

> 
>> +/*
>> + * Issue IBPB when switching guest vCPUs IFF switch_mm_always_ibpb.
>> + * For the more common case of running VMs in their own dedicated process,
>> + * switching vCPUs that belong to different VMs, i.e. switching tasks,
>> + * will also switch mm_structs and thus do IPBP via cond_mitigation();
>> + * however, in the always_ibpb case, take a paranoid approach and issue
>> + * IBPB on both switch_mm() and vCPU switch.
>> + */
>> +static inline void x86_virt_guest_switch_ibpb(void)
>> +{
>> +	if (static_branch_unlikely(&switch_mm_always_ibpb))
>> +		indirect_branch_prediction_barrier();
> 
> If this switch is going to be conditional, then make it so:
> 
> static void x86_do_cond_ibpb(void)
> {
> 	if (static_branch_likely(&switch_mm_cond_ibpb))
> 		indirect_branch_prediction_barrier();
> }

In the context of this change, we only want to do this barrier on the always_ibpb
path, so we wouldn’t want to do cont_ibpb here, but you bring up a good point, 
we could change it to x86_do_always_ibpb() and move it to bugs.c, that would
simplify things. Thanks.

> 
> and there's nothing "virt" about it - might as well call the function
> what it does. And I'd put that function in bugs.c...
> 
>> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
>> index 6296e1ebed1d..6aafb0279cbc 100644
>> --- a/arch/x86/kernel/cpu/bugs.c
>> +++ b/arch/x86/kernel/cpu/bugs.c
>> @@ -68,8 +68,12 @@ u64 __ro_after_init x86_amd_ls_cfg_ssbd_mask;
>> DEFINE_STATIC_KEY_FALSE(switch_to_cond_stibp);
>> /* Control conditional IBPB in switch_mm() */
>> DEFINE_STATIC_KEY_FALSE(switch_mm_cond_ibpb);
>> -/* Control unconditional IBPB in switch_mm() */
>> +/* Control unconditional IBPB in both switch_mm() and
>> + * x86_virt_guest_switch_ibpb().
>> + * See notes on x86_virt_guest_switch_ibpb() for KVM use case details.
>> + */
>> DEFINE_STATIC_KEY_FALSE(switch_mm_always_ibpb);
>> +EXPORT_SYMBOL_GPL(switch_mm_always_ibpb);
> 
> ... so that I don't export that key to modules.
> 
> I'd like to have the big picture clarified first, why we need this, etc.

No problem, and I appreciate the help and review! Let me know if the
above makes sense and I’m happy to issue a v4 patch for this.

Cheers,
Jon


> Thx.
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__people.kernel.org_tglx_notes-2Dabout-2Dnetiquette&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=NGPRGGo37mQiSXgHKm5rCQ&m=ZqhgyVXM5xkqhxRyZoFn5ed8_yDhRAKNqjt4Jthv1UJx7NCnlZAkK5_jJs4dN0WA&s=h_0nhBch2znONU8N13GxQyyvSuSSq2Kr7YFpjjR9tko&e= 

Powered by blists - more mailing lists