[PATCH 1/2] arm64: ARM_SMCCC_ARCH_WORKAROUND_1 doesn't return SMCCC_RET_NOT_REQUIRED

[Marc Zyngier]

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 at arm.com>
>> Cc: Steven Price <steven.price at arm.com>
>> Cc: Marc Zyngier <maz at kernel.org>
>> Cc: stable at 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 at 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

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