[PATCH] x86/spec-ctrl: Use IST RSB protection for !SVM systems

[PATCH] x86/spec-ctrl: Use IST RSB protection for !SVM systems

[Andrew Cooper]

There is a corner case where a VT-x guest which manages to reliably trigger
non-fatal #MC's could evade the rogue RSB speculation protections that were
supposed to be in place.

This is a lack of defence in depth; Xen does not architecturally execute more
RET than CALL instructions, so an attacker would have to locate a different
gadget (e.g. SpectreRSB) first to execute a transient path of excess RET
instructions.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/spec_ctrl.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 44e86f3d674d..d2cd5459739f 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -1327,8 +1327,24 @@ void __init init_speculation_mitigations(void)
      * mappings.
      */
     if ( opt_rsb_hvm )
+    {
         setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);
 
+        /*
+         * For SVM, Xen's RSB safety actions are performed before STGI, so
+         * behave atomically with respect to IST sources.
+         *
+         * For VT-x, NMIs are atomic with VMExit (the NMI gets queued but not
+         * delivered) whereas other IST sources are not atomic.  Specifically,
+         * #MC can hit ahead the RSB safety action in the vmexit path.
+         *
+         * Therefore, it is necessary for the IST logic to protect Xen against
+         * possible rogue RSB speculation.
+         */
+        if ( !cpu_has_svm )
+            default_spec_ctrl_flags |= SCF_ist_rsb;
+    }
+
     ibpb_calculations();
 
     /* Check whether Eager FPU should be enabled by default. */
-- 
2.11.0

Re: [PATCH] x86/spec-ctrl: Use IST RSB protection for !SVM systems

[Jan Beulich]

On 05.08.2022 12:38, Andrew Cooper wrote:
> There is a corner case where a VT-x guest which manages to reliably trigger
> non-fatal #MC's could evade the rogue RSB speculation protections that were
> supposed to be in place.
> 
> This is a lack of defence in depth; Xen does not architecturally execute more
> RET than CALL instructions, so an attacker would have to locate a different
> gadget (e.g. SpectreRSB) first to execute a transient path of excess RET
> instructions.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH] x86/spec-ctrl: Use IST RSB protection for !SVM systems

[Jan Beulich]

On 05.08.2022 12:38, Andrew Cooper wrote:
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -1327,8 +1327,24 @@ void __init init_speculation_mitigations(void)
>       * mappings.
>       */
>      if ( opt_rsb_hvm )
> +    {
>          setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);
>  
> +        /*
> +         * For SVM, Xen's RSB safety actions are performed before STGI, so
> +         * behave atomically with respect to IST sources.
> +         *
> +         * For VT-x, NMIs are atomic with VMExit (the NMI gets queued but not
> +         * delivered) whereas other IST sources are not atomic.  Specifically,
> +         * #MC can hit ahead the RSB safety action in the vmexit path.
> +         *
> +         * Therefore, it is necessary for the IST logic to protect Xen against
> +         * possible rogue RSB speculation.
> +         */
> +        if ( !cpu_has_svm )
> +            default_spec_ctrl_flags |= SCF_ist_rsb;

Only now, when I'm about to backport this, it occurs to me to ask: Why
is this !cpu_has_svm rather than cpu_has_vmx? Plus shouldn't this further
be gated upon HVM actually being in use (i.e. in particular CONFIG_HVM=y
in the first place)?

Jan

Re: [PATCH] x86/spec-ctrl: Use IST RSB protection for !SVM systems

[Andrew Cooper]

On 15/08/2022 09:26, Jan Beulich wrote:
> On 05.08.2022 12:38, Andrew Cooper wrote:
>> --- a/xen/arch/x86/spec_ctrl.c
>> +++ b/xen/arch/x86/spec_ctrl.c
>> @@ -1327,8 +1327,24 @@ void __init init_speculation_mitigations(void)
>>       * mappings.
>>       */
>>      if ( opt_rsb_hvm )
>> +    {
>>          setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);
>>  
>> +        /*
>> +         * For SVM, Xen's RSB safety actions are performed before STGI, so
>> +         * behave atomically with respect to IST sources.
>> +         *
>> +         * For VT-x, NMIs are atomic with VMExit (the NMI gets queued but not
>> +         * delivered) whereas other IST sources are not atomic.  Specifically,
>> +         * #MC can hit ahead the RSB safety action in the vmexit path.
>> +         *
>> +         * Therefore, it is necessary for the IST logic to protect Xen against
>> +         * possible rogue RSB speculation.
>> +         */
>> +        if ( !cpu_has_svm )
>> +            default_spec_ctrl_flags |= SCF_ist_rsb;
> Only now, when I'm about to backport this, it occurs to me to ask: Why
> is this !cpu_has_svm rather than cpu_has_vmx?

Because it is only SVM known to be safe.

> Plus shouldn't this further
> be gated upon HVM actually being in use (i.e. in particular CONFIG_HVM=y
> in the first place)?

Perhaps, but not locally here.  All of init_speculation_mitigations()
wants reconsidering if you're going down that route.

~Andrew

Re: [PATCH] x86/spec-ctrl: Use IST RSB protection for !SVM systems

[Jan Beulich]

On 15.08.2022 11:33, Andrew Cooper wrote:
> On 15/08/2022 09:26, Jan Beulich wrote:
>> On 05.08.2022 12:38, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/spec_ctrl.c
>>> +++ b/xen/arch/x86/spec_ctrl.c
>>> @@ -1327,8 +1327,24 @@ void __init init_speculation_mitigations(void)
>>>       * mappings.
>>>       */
>>>      if ( opt_rsb_hvm )
>>> +    {
>>>          setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);
>>>  
>>> +        /*
>>> +         * For SVM, Xen's RSB safety actions are performed before STGI, so
>>> +         * behave atomically with respect to IST sources.
>>> +         *
>>> +         * For VT-x, NMIs are atomic with VMExit (the NMI gets queued but not
>>> +         * delivered) whereas other IST sources are not atomic.  Specifically,
>>> +         * #MC can hit ahead the RSB safety action in the vmexit path.
>>> +         *
>>> +         * Therefore, it is necessary for the IST logic to protect Xen against
>>> +         * possible rogue RSB speculation.
>>> +         */
>>> +        if ( !cpu_has_svm )
>>> +            default_spec_ctrl_flags |= SCF_ist_rsb;
>> Only now, when I'm about to backport this, it occurs to me to ask: Why
>> is this !cpu_has_svm rather than cpu_has_vmx?
> 
> Because it is only SVM known to be safe.

Yes. Which amounts to only VT-x being unsafe. And in particular PV alone
(e.g. shim, from the perspective of the shim itself) is safe as well, no
matter what CPU we're on.

>> Plus shouldn't this further
>> be gated upon HVM actually being in use (i.e. in particular CONFIG_HVM=y
>> in the first place)?
> 
> Perhaps, but not locally here.  All of init_speculation_mitigations()
> wants reconsidering if you're going down that route.

Not sure - many of the settings (like X86_FEATURE_SC_RSB_HVM also being
set in the enclosing if()) only affect HVM-specific code paths, so which
way they are set wouldn't matter when !CONFIG_HVM. But the one here
clearly affects a common code path, for no gains at all. It's not an
overly hot code path, sure, but it still strikes me as odd.

Jan