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