| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c0591bdc2983167f00d002a731cba82e@kernel.org>
Date: Wed, 21 Oct 2020 11:23:34 +0100
From: Marc Zyngier <maz@...nel.org>
To: Will Deacon <will@...nel.org>
Cc: Stephen Boyd <swboyd@...omium.org>,
Catalin Marinas <catalin.marinas@....com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
Andre Przywara <andre.przywara@....com>,
Steven Price <steven.price@....com>, stable@...r.kernel.org
Subject: Re: [PATCH 1/2] arm64: ARM_SMCCC_ARCH_WORKAROUND_1 doesn't return
SMCCC_RET_NOT_REQUIRED
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@....com>
>> Cc: Steven Price <steven.price@....com>
>> Cc: Marc Zyngier <maz@...nel.org>
>> Cc: stable@...r.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@...omium.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
--
Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists