| Message ID | 20180522174254.27551-5-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: SSBD (aka Spectre-v4) mitigation (XSA-263) | expand |
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@arm.com> > --- > 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. > + 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). 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. > +static inline bool cpu_require_ssbd_mitigation(void) > +{ > + return this_cpu(ssbd_callback_required); > +} > + > +#else > + > +static inline bool cpu_require_ssbd_mitigation(void) > +{ > + return false; > +} > > +#endif > + > #endif /* __ARM_CPUERRATA_H__ */ > /* > * Local variables: > diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h > index e557a095af..2a5c075d3b 100644 > --- a/xen/include/asm-arm/cpufeature.h > +++ b/xen/include/asm-arm/cpufeature.h > @@ -43,8 +43,9 @@ > #define SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT 5 > #define SKIP_CTXT_SWITCH_SERROR_SYNC 6 > #define ARM_HARDEN_BRANCH_PREDICTOR 7 > +#define ARM_SSBD 8 > > -#define ARM_NCAPS 8 > +#define ARM_NCAPS 9 > > #ifndef __ASSEMBLY__ > > diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h > index 8342cc33fe..650744d28b 100644 > --- a/xen/include/asm-arm/smccc.h > +++ b/xen/include/asm-arm/smccc.h > @@ -258,6 +258,12 @@ struct arm_smccc_res { > ARM_SMCCC_OWNER_ARCH, \ > 0x8000) > > +#define ARM_SMCCC_ARCH_WORKAROUND_2_FID \ > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ > + ARM_SMCCC_CONV_32, \ > + ARM_SMCCC_OWNER_ARCH, \ > + 0x7FFF) > + > /* SMCCC error codes */ > #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1) > #define ARM_SMCCC_NOT_SUPPORTED (-1) > -- > 2.11.0 >
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@arm.com> >> --- >> 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. >> + 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). Cheers,
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@arm.com> > > > --- > > > 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.
On 25/05/2018 21:51, Stefano Stabellini wrote: > 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@arm.com> >>>> --- >>>> 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. "Speculative Store Bypass" is the agreed vendor-neutral name for the issue. This is why all the mitigation is SSBD, where the D on the end is Disable. Google SP4 is a common name (but only covers one reporter of the issue), whereas Spectre has nothing to do with this issue, and is definitely wrong to use. If in doubt, use SSB(D). ~Andrew
On Sat, 26 May 2018, Andrew Cooper wrote: > On 25/05/2018 21:51, Stefano Stabellini wrote: > > 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@arm.com> > >>>> --- > >>>> 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. > > "Speculative Store Bypass" is the agreed vendor-neutral name for the > issue. This is why all the mitigation is SSBD, where the D on the end > is Disable. > > Google SP4 is a common name (but only covers one reporter of the issue), > whereas Spectre has nothing to do with this issue, and is definitely > wrong to use. > > If in doubt, use SSB(D). I think we should definitely call it SSBD, I was just saying that it might be helpful to include also "Variant 4" in the description, such as: This is also known as Variant 4. to help users find the right results on Google. Anyway, given that you are certainly better informed than me about it, I won't insist on this point, I am OK without mentioning "Variant 4".
Hi, On 05/29/2018 10:35 PM, Stefano Stabellini wrote: > On Sat, 26 May 2018, Andrew Cooper wrote: >> On 25/05/2018 21:51, Stefano Stabellini wrote: >>> 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@arm.com> >>>>>> --- >>>>>> 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. >> >> "Speculative Store Bypass" is the agreed vendor-neutral name for the >> issue. This is why all the mitigation is SSBD, where the D on the end >> is Disable. >> >> Google SP4 is a common name (but only covers one reporter of the issue), >> whereas Spectre has nothing to do with this issue, and is definitely >> wrong to use. >> >> If in doubt, use SSB(D). > > I think we should definitely call it SSBD, I was just saying that it > might be helpful to include also "Variant 4" in the description, such > as: > > This is also known as Variant 4. > > to help users find the right results on Google. There are enough hit with "Speculative Store Bypass Disable" for a user to find what's going on. > Anyway, given that you > are certainly better informed than me about it, I won't insist on this > point, I am OK without mentioning "Variant 4". I would prefer to not mention it in the Kconfig. Cheers,
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. + + 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); + +static inline bool cpu_require_ssbd_mitigation(void) +{ + return this_cpu(ssbd_callback_required); +} + +#else + +static inline bool cpu_require_ssbd_mitigation(void) +{ + return false; +} + +#endif + #endif /* __ARM_CPUERRATA_H__ */ /* * Local variables: diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h index e557a095af..2a5c075d3b 100644 --- a/xen/include/asm-arm/cpufeature.h +++ b/xen/include/asm-arm/cpufeature.h @@ -43,8 +43,9 @@ #define SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT 5 #define SKIP_CTXT_SWITCH_SERROR_SYNC 6 #define ARM_HARDEN_BRANCH_PREDICTOR 7 +#define ARM_SSBD 8 -#define ARM_NCAPS 8 +#define ARM_NCAPS 9 #ifndef __ASSEMBLY__ diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h index 8342cc33fe..650744d28b 100644 --- a/xen/include/asm-arm/smccc.h +++ b/xen/include/asm-arm/smccc.h @@ -258,6 +258,12 @@ struct arm_smccc_res { ARM_SMCCC_OWNER_ARCH, \ 0x8000) +#define ARM_SMCCC_ARCH_WORKAROUND_2_FID \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + ARM_SMCCC_CONV_32, \ + ARM_SMCCC_OWNER_ARCH, \ + 0x7FFF) + /* SMCCC error codes */ #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1) #define ARM_SMCCC_NOT_SUPPORTED (-1)
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@arm.com> --- 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(-)