[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240503143937.GA18656@willie-the-truck>
Date: Fri, 3 May 2024 15:39:38 +0100
From: Will Deacon <will@...nel.org>
To: Sebastian Ene <sebastianene@...gle.com>
Cc: catalin.marinas@....com, james.morse@....com, jean-philippe@...aro.org,
maz@...nel.org, oliver.upton@...ux.dev, qperret@...gle.com,
qwandor@...gle.com, sudeep.holla@....com, suzuki.poulose@....com,
tabba@...gle.com, yuzenghui@...wei.com, lpieralisi@...nel.org,
kvmarm@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH 1/4] KVM: arm64: Trap FFA_VERSION host call in pKVM
Hi Seb,
On Thu, Apr 18, 2024 at 04:30:23PM +0000, Sebastian Ene wrote:
> The pKVM hypervisor initializes with FF-A version 1.0. Keep the
> supported version inside the host structure and prevent the host
> drivers from overwriting the FF-A version with an increased version.
> Without trapping the call, the host drivers can negotiate a higher
> version number with TEE which can result in a different memory layout
> described during the memory sharing calls.
>
> Signed-off-by: Sebastian Ene <sebastianene@...gle.com>
> ---
> arch/arm64/kvm/hyp/nvhe/ffa.c | 43 ++++++++++++++++++++++++++++++++---
> 1 file changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> index 320f2eaa14a9..023712e8beeb 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> @@ -58,6 +58,7 @@ struct kvm_ffa_buffers {
> hyp_spinlock_t lock;
> void *tx;
> void *rx;
> + u32 ffa_version;
> };
Why should this be part of 'struct kvm_ffa_buffers'? The host, proxy and
Secure side will end up using the same version, so a simple global
variable would suffice, no?
> /*
> @@ -640,6 +641,39 @@ static bool do_ffa_features(struct arm_smccc_res *res,
> return true;
> }
>
> +static void do_ffa_version(struct arm_smccc_res *res,
> + struct kvm_cpu_context *ctxt)
> +{
> + DECLARE_REG(u32, ffa_req_version, ctxt, 1);
> + u32 current_version;
> +
> + hyp_spin_lock(&host_buffers.lock);
Why do you need to take the lock for this?
> + current_version = host_buffers.ffa_version;
> + if (FFA_MAJOR_VERSION(ffa_req_version) != FFA_MAJOR_VERSION(current_version)) {
> + res->a0 = FFA_RET_NOT_SUPPORTED;
> + goto unlock;
> + }
We won't have probed the proxy if the Secure side doesn't support 1.x
so I think you should just do:
if (FFA_MAJOR_VERSION(ffa_req_version) != 1)
...
> + /*
> + * If the client driver tries to downgrade the version, we need to ask
> + * first if TEE supports it.
> + */
> + if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(current_version)) {
Similarly here, I don't think 'current_version' is what we should expose.
Rather, we should be returning the highest version that the proxy
supports in the host, which is 1.0 at this point in the patch series.
> + arm_smccc_1_1_smc(FFA_VERSION, ffa_req_version, 0,
> + 0, 0, 0, 0, 0,
> + res);
Hmm, I'm struggling to see how this is supposed to work per the spec.
The FF-A spec says:
| ... negotiation of the version must happen before an invocation of
| any other FF-A ABI.
and:
| Once the caller invokes any FF-A ABI other than FFA_VERSION, the
| version negotiation phase is complete.
|
| Once an FF-A version has been negotiated between a caller and a
| callee, the version may not be changed for the lifetime of the
| calling component. The callee must treat the negotiated version as
| the only supported version for any subsequent interactions with the
| caller.
So by the time we get here, we've already settled on our version with
the Secure side and the host cannot downgrade.
That's a bit rubbish if you ask me, but I think it means we'll have to
defer some of the proxy initialisation until the host calls FFA_VERSION,
at which point we'll need to negotiate a common version between the host,
the proxy and Secure. Once we've done that, our FFA_VERSION handler will
just return that negotiated version.
Will
Powered by blists - more mailing lists