Re: [Xen-devel] [PATCH 04/13] xen/arm: Add ARCH_WORKAROUND_2 probing

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 23 May 2018, Julien Grall wrote:
> Hi,
> 
> On 05/23/2018 10:57 PM, Stefano Stabellini wrote:
> > On Tue, 22 May 2018, Julien Grall wrote:
> > > As for Spectre variant-2, we rely on SMCCC 1.1 to provide the discovery
> > > mechanism for detecting the SSBD mitigation.
> > > 
> > > A new capability is also allocated for that purpose, and a config
> > > option.
> > > 
> > > This is part of XSA-263.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > ---
> > >   xen/arch/arm/Kconfig             | 10 ++++++++++
> > >   xen/arch/arm/cpuerrata.c         | 39
> > > +++++++++++++++++++++++++++++++++++++++
> > >   xen/include/asm-arm/cpuerrata.h  | 21 +++++++++++++++++++++
> > >   xen/include/asm-arm/cpufeature.h |  3 ++-
> > >   xen/include/asm-arm/smccc.h      |  6 ++++++
> > >   5 files changed, 78 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > index 8174c0c635..0e2d027060 100644
> > > --- a/xen/arch/arm/Kconfig
> > > +++ b/xen/arch/arm/Kconfig
> > > @@ -73,6 +73,16 @@ config SBSA_VUART_CONSOLE
> > >             Allows a guest to use SBSA Generic UART as a console. The
> > >             SBSA Generic UART implements a subset of ARM PL011 UART.
> > >   +config ARM_SSBD
> > > + bool "Speculative Store Bypass Disable" if EXPERT = "y"
> > > + depends on HAS_ALTERNATIVE
> > > + default y
> > > + help
> > > +   This enables mitigation of bypassing of previous stores by
> > > speculative
> > > +   loads.
> > 
> > I would add a reference to spectre v4. What do you think of:
> > 
> >    This enables the mitigation of Spectre v4 attacks based on bypassing
> >    of previous memory stores by speculative loads.
> 
> Well, the real name is SSBD (Speculative Store Bypass Disable). AFAIK, Spectre
> only refers to variant 1 and 2 so far. This one has no fancy name and the
> specifications is using SSBD.

Googling for Spectre Variant 4 returns twice as many results as Googling
for Speculative Store Bypass Disable. It doesn't matter what is the
official name for the security issue, I think we need to include a
reference to the most common name for it.


> > > +   If unsure, say Y.
> > > +
> > >   endmenu
> > >     menu "ARM errata workaround via the alternative framework"
> > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> > > index 1baa20654b..bcea2eb6e5 100644
> > > --- a/xen/arch/arm/cpuerrata.c
> > > +++ b/xen/arch/arm/cpuerrata.c
> > > @@ -235,6 +235,39 @@ static int enable_ic_inv_hardening(void *data)
> > >     #endif
> > >   +#ifdef CONFIG_ARM_SSBD
> > > +
> > > +/*
> > > + * Assembly code may use the variable directly, so we need to make sure
> > > + * it fits in a register.
> > > + */
> > > +DEFINE_PER_CPU_READ_MOSTLY(register_t, ssbd_callback_required);
> > > +
> > > +static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
> > > +{
> > > +    struct arm_smccc_res res;
> > > +    bool supported = true;
> > > +
> > > +    if ( smccc_ver < SMCCC_VERSION(1, 1) )
> > > +        return false;
> > > +
> > > +    /*
> > > +     * The probe function return value is either negative (unsupported
> > > +     * or mitigated), positive (unaffected), or zero (requires
> > > +     * mitigation). We only need to do anything in the last case.
> > > +     */
> > > +    arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
> > > +                      ARM_SMCCC_ARCH_WORKAROUND_2_FID, &res);
> > > +    if ( (int)res.a0 != 0 )
> > > +        supported = false;
> > > +
> > > +    if ( supported )
> > > +        this_cpu(ssbd_callback_required) = 1;
> > > +
> > > +    return supported;
> > > +}
> > > +#endif
> > > +
> > >   #define MIDR_RANGE(model, min, max)     \
> > >       .matches = is_affected_midr_range,  \
> > >       .midr_model = model,                \
> > > @@ -336,6 +369,12 @@ static const struct arm_cpu_capabilities arm_errata[]
> > > = {
> > >           .enable = enable_ic_inv_hardening,
> > >       },
> > >   #endif
> > > +#ifdef CONFIG_ARM_SSBD
> > > +    {
> > > +        .capability = ARM_SSBD,
> > > +        .matches = has_ssbd_mitigation,
> > > +    },
> > > +#endif
> > >       {},
> > >   };
> > >   diff --git a/xen/include/asm-arm/cpuerrata.h
> > > b/xen/include/asm-arm/cpuerrata.h
> > > index 4e45b237c8..e628d3ff56 100644
> > > --- a/xen/include/asm-arm/cpuerrata.h
> > > +++ b/xen/include/asm-arm/cpuerrata.h
> > > @@ -27,9 +27,30 @@ static inline bool check_workaround_##erratum(void)
> > > \
> > >     CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422,
> > > CONFIG_ARM_32)
> > >   CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64)
> > > +CHECK_WORKAROUND_HELPER(ssbd, ARM_SSBD, CONFIG_ARM_SSBD)
> > >     #undef CHECK_WORKAROUND_HELPER
> > >   +#ifdef CONFIG_ARM_SSBD
> > > +
> > > +#include <asm/current.h>
> > > +
> > > +DECLARE_PER_CPU(register_t, ssbd_callback_required);
> > 
> > It is becoming more common to have per-cpu capabilities and workarounds
> > (or at least per MPIDR).
> 
> Really? This is the first place where we need an ad-hoc boolean per-CPU. For
> the hardening branch predictor, we have to store the vector pointer.
>
> > Instead of adding this add-hoc variable, should
> > we make cpu_hwcaps per-cpu, then implement this check with
> > cpus_have_cap (that would become per-cpu as well)?
> > 
> > It looks like the code would be simpler.
> 
> I don't see any benefits for that. Most of the workaround/features are
> platform wide because they either use alternative or set/clear a bit in the
> system registers.
> 
> Furthermore, as I wrote above the declaration, this is going to be used in
> assembly code and we need something that can be tested in the less possible
> number of instructions because The smccc function ARM_ARCH_WORKAROUND_2 is
> going to be called very often.
> 
> Lastly, after the next patch, ssbd_callback_required and ARM_SSBD have
> different meaning. The former indicates that runtime mitigation is required,
> while the latter just indicate that the mitigation is present (either runtime
> or forced enable).

You are right. I was following up from the big.LITTLE series where not
all CPUs need the same workaround. On the call you pointed out that the
cpufeature framework is mostly to enable specific code workarounds in
Xen for some erratas. These dynamic code fixes have to be enabled on all
cores regardless on whether they are affected because they rely on
dynamic code patching. I see your point, and I agree we should keep
cpufeature as-is for now. I think I have a better suggestion in my
reply to patch #5.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel