Re: [PATCH 1/2] arm64: ARM_SMCCC_ARCH_WORKAROUND_1 doesn't return SMCCC_RET_NOT_REQUIRED
From: Marc Zyngier
Date: Wed Oct 21 2020 - 06:23:40 EST
On 2020-10-21 08:57, Will Deacon wrote:
On Tue, Oct 20, 2020 at 02:45:43PM -0700, Stephen Boyd wrote:
According to the SMCCC spec (7.5.2 Discovery) the
ARM_SMCCC_ARCH_WORKAROUND_1 function id only returns 0, 1, and
SMCCC_RET_NOT_SUPPORTED corresponding to "workaround required",
"workaround not required but implemented", and "who knows, you're on
your own" respectively. For kvm hypercalls (hvc), we've implemented
this
function id to return SMCCC_RET_NOT_SUPPORTED, 1, and
SMCCC_RET_NOT_REQUIRED. The SMCCC_RET_NOT_REQUIRED return value is not
a
thing for this function id, and is probably copy/pasted from the
SMCCC_ARCH_WORKAROUND_2 function id that does support it.
Clean this up by returning 0, 1, and SMCCC_RET_NOT_SUPPORTED
appropriately. Changing this exposes the problem that
spectre_v2_get_cpu_fw_mitigation_state() assumes a
SMCCC_RET_NOT_SUPPORTED return value means we are vulnerable, but
really
it means we have no idea and should assume we can't do anything about
mitigation. Put another way, it better be unaffected because it can't
be
mitigated in the firmware (in this case kvm) as the call isn't
implemented!
Cc: Andre Przywara <andre.przywara@xxxxxxx>
Cc: Steven Price <steven.price@xxxxxxx>
Cc: Marc Zyngier <maz@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Fixes: c118bbb52743 ("arm64: KVM: Propagate full Spectre v2 workaround
state to KVM guests")
Fixes: 73f381660959 ("arm64: Advertise mitigation of Spectre-v2, or
lack thereof")
Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
---
This will require a slightly different backport to stable kernels, but
at least it looks like this is a problem given that this return value
isn't valid per the spec and we've been going around it by returning
something invalid for some time.
arch/arm64/kernel/proton-pack.c | 3 +--
arch/arm64/kvm/hypercalls.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kernel/proton-pack.c
b/arch/arm64/kernel/proton-pack.c
index 68b710f1b43f..00bd54f63f4f 100644
--- a/arch/arm64/kernel/proton-pack.c
+++ b/arch/arm64/kernel/proton-pack.c
@@ -149,10 +149,9 @@ static enum mitigation_state
spectre_v2_get_cpu_fw_mitigation_state(void)
case SMCCC_RET_SUCCESS:
return SPECTRE_MITIGATED;
case SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED:
+ case SMCCC_RET_NOT_SUPPORTED: /* Good luck w/ the Gatekeeper of
Gozer */
return SPECTRE_UNAFFECTED;
Hmm, I'm not sure this is correct. The SMCCC spec is terrifically
unhelpful:
NOT_SUPPORTED:
Either:
* None of the PEs in the system require firmware mitigation for
CVE-2017-5715.
* The system contains at least 1 PE affected by CVE-2017-5715 that
has no firmware
mitigation available.
* The firmware does not provide any information about whether
firmware mitigation is
required.
so we can't tell whether the thing is vulnerable or not in this case,
and
have to assume that it is.
default:
- fallthrough;
- case SMCCC_RET_NOT_SUPPORTED:
return SPECTRE_VULNERABLE;
}
}
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 9824025ccc5c..868486957808 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -31,7 +31,7 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
val = SMCCC_RET_SUCCESS;
break;
case SPECTRE_UNAFFECTED:
- val = SMCCC_RET_NOT_REQUIRED;
+ val = SMCCC_RET_NOT_SUPPORTED;
Which means we need to return SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED
here, I
suppose?
Gahh, I keep mixing Spectre-v2 and WA2. Not good. I *think* the patch
below is enough, but I need to give it a go...
M.
diff --git a/arch/arm64/kernel/proton-pack.c
b/arch/arm64/kernel/proton-pack.c
index 68b710f1b43f..3f417d6305ef 100644
--- a/arch/arm64/kernel/proton-pack.c
+++ b/arch/arm64/kernel/proton-pack.c
@@ -134,8 +134,6 @@ static enum mitigation_state
spectre_v2_get_cpu_hw_mitigation_state(void)
return SPECTRE_VULNERABLE;
}
-#define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED (1)
-
static enum mitigation_state
spectre_v2_get_cpu_fw_mitigation_state(void)
{
int ret;
@@ -148,7 +146,7 @@ static enum mitigation_state
spectre_v2_get_cpu_fw_mitigation_state(void)
switch (ret) {
case SMCCC_RET_SUCCESS:
return SPECTRE_MITIGATED;
- case SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED:
+ case SMCCC_RET_UNAFFECTED:
return SPECTRE_UNAFFECTED;
default:
fallthrough;
@@ -474,7 +472,7 @@ static enum mitigation_state
spectre_v4_get_cpu_fw_mitigation_state(void)
switch (ret) {
case SMCCC_RET_SUCCESS:
return SPECTRE_MITIGATED;
- case SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED:
+ case SMCCC_RET_UNAFFECTED:
fallthrough;
case SMCCC_RET_NOT_REQUIRED:
return SPECTRE_UNAFFECTED;
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 9824025ccc5c..792824de5d27 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -31,7 +31,7 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
val = SMCCC_RET_SUCCESS;
break;
case SPECTRE_UNAFFECTED:
- val = SMCCC_RET_NOT_REQUIRED;
+ val = SMCCC_RET_UNAFFECTED;
break;
}
break;
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 885c9ffc835c..6b4902dde822 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -104,6 +104,7 @@
* Return codes defined in ARM DEN 0070A
* ARM DEN 0070A is now merged/consolidated into ARM DEN 0028 C
*/
+#define SMCCC_RET_UNAFFECTED 1
#define SMCCC_RET_SUCCESS 0
#define SMCCC_RET_NOT_SUPPORTED -1
#define SMCCC_RET_NOT_REQUIRED -2
--
Jazz is not dead. It just smells funny...