From: Stephen Boyd <swboyd@chromium.org>
To: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
Andre Przywara <andre.przywara@arm.com>,
Steven Price <steven.price@arm.com>,
Marc Zyngier <maz@kernel.org>,
stable@vger.kernel.org
Subject: Re: [PATCH 1/2] arm64: ARM_SMCCC_ARCH_WORKAROUND_1 doesn't return SMCCC_RET_NOT_REQUIRED
Date: Wed, 21 Oct 2020 08:23:54 -0700 [thread overview]
Message-ID: <160329383454.884498.3396883189907056188@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20201021075722.GA17230@willie-the-truck>
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
next prev parent reply other threads:[~2020-10-21 15:24 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-20 21:45 [PATCH 0/2] arm64: Fixes for spectre-v2 detection in guest kernels Stephen Boyd
2020-10-20 21:45 ` [PATCH 1/2] arm64: ARM_SMCCC_ARCH_WORKAROUND_1 doesn't return SMCCC_RET_NOT_REQUIRED Stephen Boyd
2020-10-21 7:57 ` Will Deacon
2020-10-21 10:23 ` Marc Zyngier
2020-10-21 12:43 ` Will Deacon
2020-10-21 15:23 ` Stephen Boyd [this message]
2020-10-21 15:49 ` Will Deacon
2020-10-21 16:12 ` Stephen Boyd
2020-10-21 21:13 ` Will Deacon
2020-10-21 22:06 ` Stephen Boyd
2020-10-20 21:45 ` [PATCH 2/2] arm64: proton-pack: Update comment to reflect new function name Stephen Boyd
2020-10-21 15:44 ` [PATCH 0/2] arm64: Fixes for spectre-v2 detection in guest kernels Will Deacon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=160329383454.884498.3396883189907056188@swboyd.mtv.corp.google.com \
--to=swboyd@chromium.org \
--cc=andre.przywara@arm.com \
--cc=catalin.marinas@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=stable@vger.kernel.org \
--cc=steven.price@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).