Quoting Will Deacon (2020-10-21 00:57:23)
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@arm.com Cc: Steven Price steven.price@arm.com Cc: Marc Zyngier maz@kernel.org Cc: stable@vger.kernel.org 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@chromium.org
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.
If I'm reading the TF-A code correctly it looks like this will return SMC_UNK if the platform decides that "This flag can be set to 0 by the platform if none of the PEs in the system need the workaround." Where the flag is WORKAROUND_CVE_2017_5715 and the call handler returns 1 if the errata doesn't apply but the config is enabled, 0 if the errata applies and the config is enabled, or SMC_UNK (I guess this is NOT_SUPPORTED?) if the config is disabled[2].
So TF-A could disable this config and then the kernel would think it is vulnerable when it actually isn't? The spec is a pile of ectoplasma here.
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?
Does the kernel implement a workaround in the case that no guest PE is affected? If so then returning 1 sounds OK to me, but otherwise NOT_SUPPORTED should work per the spec.
[1] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/docs/design... [2] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/services/ar...