| Message ID | 20201020214544.3206838-2-swboyd@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] arm64: ARM_SMCCC_ARCH_WORKAROUND_1 doesn't return SMCCC_RET_NOT_REQUIRED | expand |
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. > 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? Will
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@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. > >> 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
On Wed, Oct 21, 2020 at 11:23:34AM +0100, Marc Zyngier wrote: > 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@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. > > > > > 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... Yeah, and me. We should've named them 2 and 4 back in the day. > 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) Unfortunately, I think this value _is_ specific to the ARCH_WORKAROUND calls, so it should stay like it is (i.e. other calls in SMCCC can return 1 to indicate other things) But the semantic bit here: > 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; Looks correct to me. Will
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/cpu-specific-build-macros.rst#n14 [2] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/services/arm_arch_svc/arm_arch_svc_setup.c#n30
On Wed, Oct 21, 2020 at 08:23:54AM -0700, Stephen Boyd wrote: > 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. Yes, but there's not a lot we can do in that case as we rely on the firmware to tell us whether or not we're affected. We do have the "safelist" as a last resort, but that's about it. > > > 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. I don't follow you here. The spec says that "SMCCC_RET_NOT_SUPPORTED" is valid return code in the case that "The system contains at least 1 PE affected by CVE-2017-5715 that has no firmware mitigation available." and do the guest would end up in the "vulnerable" state. Will
Quoting Will Deacon (2020-10-21 08:49:09) > On Wed, Oct 21, 2020 at 08:23:54AM -0700, Stephen Boyd wrote: > > > > 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. > > Yes, but there's not a lot we can do in that case as we rely on the > firmware to tell us whether or not we're affected. We do have the > "safelist" as a last resort, but that's about it. There are quite a few platforms that set this config to 0. Should they be setting it to 1? tf-a $ git grep WORKAROUND_CVE_2017_5715 -- **/platform.mk | wc -l 17 This looks like a disconnect between kernel and TF-A but I'm not aware of all the details here. > > > > > 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. > > I don't follow you here. The spec says that "SMCCC_RET_NOT_SUPPORTED" is > valid return code in the case that "The system contains at least 1 PE > affected by CVE-2017-5715 that has no firmware mitigation available." > and do the guest would end up in the "vulnerable" state. > Returning 1 says "SMCCC_ARCH_WORKAROUND_1 can be invoked safely on all PEs in the system" so I am not sure that invoking it is from a guest is safe on systems that don't require the workaround? If it is always safe to invoke the call from guest to host then returning 1 should be fine here. My read of the spec was that the intent is to remove the call at some point and have the removal of the call mean that it isn't vulnerable. Because NOT_SUPPORTED per the spec means "not needed", "maybe needed", or "firmware doesn't know". Aha maybe they wanted us to make the call on each CPU (i.e. PE) and then if any of them return 0 we should consider it vulnerable and if they return NOT_SUPPORTED we should keep calling for each CPU until we are sure we don't see a 0 and only see a 1 or NOT_SUPPORTED? Looks like a saturating value sort of thing, across CPUs that we care/know about.
On Wed, Oct 21, 2020 at 09:12:02AM -0700, Stephen Boyd wrote: > Quoting Will Deacon (2020-10-21 08:49:09) > > On Wed, Oct 21, 2020 at 08:23:54AM -0700, Stephen Boyd wrote: > > > > > > 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. > > > > Yes, but there's not a lot we can do in that case as we rely on the > > firmware to tell us whether or not we're affected. We do have the > > "safelist" as a last resort, but that's about it. > > There are quite a few platforms that set this config to 0. Should they > be setting it to 1? > > tf-a $ git grep WORKAROUND_CVE_2017_5715 -- **/platform.mk | wc -l > 17 A quick skim suggests that most (all?) of these are A53-based, so that's on the safelist and will be fine. > This looks like a disconnect between kernel and TF-A but I'm not aware > of all the details here. I think it's alright, as it's just a legacy problem (newer cores should have CSV2 set) and older cores are safelisted. > > > 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. > > > > I don't follow you here. The spec says that "SMCCC_RET_NOT_SUPPORTED" is > > valid return code in the case that "The system contains at least 1 PE > > affected by CVE-2017-5715 that has no firmware mitigation available." > > and do the guest would end up in the "vulnerable" state. > > > > Returning 1 says "SMCCC_ARCH_WORKAROUND_1 can be invoked safely on all > PEs in the system" so I am not sure that invoking it is from a guest is > safe on systems that don't require the workaround? If it is always safe > to invoke the call from guest to host then returning 1 should be fine > here. I think it's fine, as KVM will pick that up. > My read of the spec was that the intent is to remove the call at some > point and have the removal of the call mean that it isn't vulnerable. No, the CSV2 field in whichever ID register is for that. We check that in spectre_v2_get_cpu_hw_mitigation_state(). > Because NOT_SUPPORTED per the spec means "not needed", "maybe needed", > or "firmware doesn't know". Aha maybe they wanted us to make the call on > each CPU (i.e. PE) and then if any of them return 0 we should consider > it vulnerable and if they return NOT_SUPPORTED we should keep calling > for each CPU until we are sure we don't see a 0 and only see a 1 or > NOT_SUPPORTED? Looks like a saturating value sort of thing, across CPUs > that we care/know about. The mitigation state is always per-cpu because of big/little systems, there just isn't a short-cut for the firmware to say "all CPUs are unaffected" like there is for SMCCC_ARCH_WORKAROUND_2 with its "NOT_REQUIRED" return code. Will
Quoting Will Deacon (2020-10-21 14:13:26) > On Wed, Oct 21, 2020 at 09:12:02AM -0700, Stephen Boyd wrote: > > > My read of the spec was that the intent is to remove the call at some > > point and have the removal of the call mean that it isn't vulnerable. > > No, the CSV2 field in whichever ID register is for that. We check that in > spectre_v2_get_cpu_hw_mitigation_state(). Alright, makes sense! > > > Because NOT_SUPPORTED per the spec means "not needed", "maybe needed", > > or "firmware doesn't know". Aha maybe they wanted us to make the call on > > each CPU (i.e. PE) and then if any of them return 0 we should consider > > it vulnerable and if they return NOT_SUPPORTED we should keep calling > > for each CPU until we are sure we don't see a 0 and only see a 1 or > > NOT_SUPPORTED? Looks like a saturating value sort of thing, across CPUs > > that we care/know about. > > The mitigation state is always per-cpu because of big/little systems, there > just isn't a short-cut for the firmware to say "all CPUs are unaffected" > like there is for SMCCC_ARCH_WORKAROUND_2 with its "NOT_REQUIRED" return > code. > Ok. Can/should kvm be emulating the CSV2 bit that the guest sees? Just wondering why I'm falling into this (ghost) trap in the first place.
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; 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; break; } break;
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(-)