[PATCH 0/2] arm64: Fixes for spectre-v2 detection in guest kernels

[PATCH 0/2] arm64: Fixes for spectre-v2 detection in guest kernels

[Stephen Boyd]

The first patch fixes a problem with spectre-v2 detection in guest
kernels found on v5.4 and the second patch fixes an outdated comment.

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

Stephen Boyd (2):
  arm64: ARM_SMCCC_ARCH_WORKAROUND_1 doesn't return
    SMCCC_RET_NOT_REQUIRED
  arm64: proton-pack: Update comment to reflect new function name

 arch/arm64/kernel/proton-pack.c | 7 +++----
 arch/arm64/kvm/hypercalls.c     | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)


base-commit: 38525c6919e2f6b27c1855905f342a0def3cbdcf
-- 
Sent by a computer, using git, on the internet

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

[Stephen Boyd]

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;
 	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;
-- 
Sent by a computer, using git, on the internet

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

[Will Deacon]

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

Re: [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@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

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

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

[Will Deacon]

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

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

[Stephen Boyd]

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

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

[Will Deacon]

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

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

[Stephen Boyd]

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.

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

[Will Deacon]

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

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

[Stephen Boyd]

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.

[PATCH 2/2] arm64: proton-pack: Update comment to reflect new function name

[Stephen Boyd]

The function detect_harden_bp_fw() is gone after commit d4647f0a2ad7
("arm64: Rewrite Spectre-v2 mitigation code"). Update this comment to
reflect the new state of affairs.

Cc: Marc Zyngier <maz@kernel.org>
Fixes: d4647f0a2ad7 ("arm64: Rewrite Spectre-v2 mitigation code")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/kernel/proton-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/proton-pack.c b/arch/arm64/kernel/proton-pack.c
index 00bd54f63f4f..106857685145 100644
--- a/arch/arm64/kernel/proton-pack.c
+++ b/arch/arm64/kernel/proton-pack.c
@@ -203,8 +203,8 @@ static void install_bp_hardening_cb(bp_hardening_cb_t fn)
 				   __SMCCC_WORKAROUND_1_SMC_SZ;
 
 	/*
-	 * detect_harden_bp_fw() passes NULL for the hyp_vecs start/end if
-	 * we're a guest. Skip the hyp-vectors work.
+	 * Vinz Clortho takes the hyp_vecs start/end "keys" at
+	 * the door when we're a guest. Skip the hyp-vectors work.
 	 */
 	if (!is_hyp_mode_available()) {
 		__this_cpu_write(bp_hardening_data.fn, fn);
-- 
Sent by a computer, using git, on the internet

Re: [PATCH 0/2] arm64: Fixes for spectre-v2 detection in guest kernels

[Will Deacon]

On Tue, 20 Oct 2020 14:45:42 -0700, Stephen Boyd wrote:
> The first patch fixes a problem with spectre-v2 detection in guest
> kernels found on v5.4 and the second patch fixes an outdated comment.
> 
> 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
> 
> [...]

Applied just the second patch to arm64 (for-next/core), thanks!

[2/2] arm64: proton-pack: Update comment to reflect new function name
      https://git.kernel.org/arm64/c/66dd3474702a

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev