Re: [PATCH] arm64: spectre-v2: Favour CPU-specific mitigation at EL2

[Marc Zyngier <maz@kernel.org>]

Hi Will,

$subject seems slightly off. Don't we actually want to favor 
CPU-specific
mitigation at *EL1*?

On 2020-10-21 11:52, Will Deacon wrote:
> Spectre-v2 can be mitigated on Falkor CPUs either by calling into
> firmware or by issuing a magic, CPU-specific sequence of branches.
> Although the latter is faster, the size of the code sequence means that
> it cannot be used in the EL2 vectors, and so there is a need for both
> mitigations to co-exist in order to achieve optimal performance.
> 
> Change the mitigation selection logic for Spectre-v2 so that the
> CPU-specific mitigation is used only when the firmware mitigation is
> also available, rather than when a firmware mitigation is unavailable.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/proton-pack.c | 36 ++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/kernel/proton-pack.c 
> b/arch/arm64/kernel/proton-pack.c
> index 68b710f1b43f..5029ef14fb27 100644
> --- a/arch/arm64/kernel/proton-pack.c
> +++ b/arch/arm64/kernel/proton-pack.c
> @@ -67,7 +67,8 @@ ssize_t cpu_show_spectre_v1(struct device *dev,
> struct device_attribute *attr,
>   * - Mitigated in hardware and advertised by ID_AA64PFR0_EL1.CSV2.
>   * - Mitigated in hardware and listed in our "safe list".
>   * - Mitigated in software by firmware.
> - * - Mitigated in software by a CPU-specific dance in the kernel.
> + * - Mitigated in software by a CPU-specific dance in the kernel and a
> + *   firmware call at EL2.
>   * - Vulnerable.
>   *
>   * It's not unlikely for different CPUs in a big.LITTLE system to fall 
> into
> @@ -259,6 +260,16 @@ static void qcom_link_stack_sanitisation(void)
>  		     : "=&r" (tmp));
>  }
> 
> +static bp_hardening_cb_t spectre_v2_get_sw_mitigation_cb(void)
> +{
> +	u32 midr = read_cpuid_id();
> +	if (((midr & MIDR_CPU_MODEL_MASK) != MIDR_QCOM_FALKOR) &&
> +	    ((midr & MIDR_CPU_MODEL_MASK) != MIDR_QCOM_FALKOR_V1))
> +		return NULL;
> +
> +	return qcom_link_stack_sanitisation;
> +}
> +
>  static enum mitigation_state spectre_v2_enable_fw_mitigation(void)
>  {
>  	bp_hardening_cb_t cb;
> @@ -284,26 +295,15 @@ static enum mitigation_state
> spectre_v2_enable_fw_mitigation(void)
>  		return SPECTRE_VULNERABLE;
>  	}
> 
> +	/*
> +	 * Prefer a CPU-specific workaround if it exists. Note that we
> +	 * still rely on firmware for the mitigation at EL2.
> +	 */
> +	cb = spectre_v2_get_sw_mitigation_cb() ?: cb;
>  	install_bp_hardening_cb(cb);
>  	return SPECTRE_MITIGATED;
>  }
> 
> -static enum mitigation_state spectre_v2_enable_sw_mitigation(void)
> -{
> -	u32 midr;
> -
> -	if (spectre_v2_mitigations_off())
> -		return SPECTRE_VULNERABLE;
> -
> -	midr = read_cpuid_id();
> -	if (((midr & MIDR_CPU_MODEL_MASK) != MIDR_QCOM_FALKOR) &&
> -	    ((midr & MIDR_CPU_MODEL_MASK) != MIDR_QCOM_FALKOR_V1))
> -		return SPECTRE_VULNERABLE;
> -
> -	install_bp_hardening_cb(qcom_link_stack_sanitisation);
> -	return SPECTRE_MITIGATED;
> -}
> -
>  void spectre_v2_enable_mitigation(const struct arm64_cpu_capabilities
> *__unused)
>  {
>  	enum mitigation_state state;
> @@ -313,8 +313,6 @@ void spectre_v2_enable_mitigation(const struct
> arm64_cpu_capabilities *__unused)
>  	state = spectre_v2_get_cpu_hw_mitigation_state();
>  	if (state == SPECTRE_VULNERABLE)
>  		state = spectre_v2_enable_fw_mitigation();
> -	if (state == SPECTRE_VULNERABLE)
> -		state = spectre_v2_enable_sw_mitigation();
> 
>  	update_mitigation_state(&spectre_v2_state, state);
>  }

This mostly works on QDF2400, except for one odd case:

- I boot a host with mitigations disabled
- I boot a guest with mitigations enabled

The host says "Vulnerable", which is expected. The guest says 
"Vulnerable",
despite being able to mitigate things at EL1 on its own. I'm inclined to
say that it should say "Mitigated", but it isn't clear whether that's
always safe (the lack of FW mitigation is a hint, but not a certainty).

Anyway, not a big deal, as it only affects a system that hardly anyone
has access to, for a pretty nonsensical setup.

Tested-by: Marc Zyngier <maz@kernel.org>

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel