Message-ID: <ZjUCyGoptCcIoGpU@google.com>
Date: Fri, 3 May 2024 15:29:12 +0000
From: Sebastian Ene <sebastianene@...gle.com>
To: Will Deacon <will@...nel.org>
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

On Fri, May 03, 2024 at 03:39:38PM +0100, Will Deacon wrote:

Hello Will,

> 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?
> 

I prefer keeping it here as we will have more clients in the future /
different VMs and each one of them will have its own version and its own
pair of buffers.

> >  /*
> > @@ -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?
> 

Because we interpret the host buffer content based on the version that we
end up setting here and each time we are accessing these buffers we are
protected by this lock.

> > +	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)
> 	...
>

Ack.

> > +	/*
> > +	 * 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.

We already report the highest version that the proxy supports on line:
`res->a0 = current_version;`

'current_version' is assigned during proxy initialization.
This check allows us to downgrade the supported ffa_version if the Host
requested it and only if TF-A supports it.

> 
> > +		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.

I think that is a bit vague in my opinion but what I get is that the first call
executed should always be the get version ff-a call.

> 
> 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.

At this stage I think the spec didn't take into account that there can be a hypervisor
in between.

> 
> 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.

We are already doing this when the ARM driver is built as an external
module. If it is not as an external module and is builtin we have a
bigger issue because it loads before pKVM at subsys_initcall. This means
that we won't trap FFA_MAP* and other setup calls.

> 
> Will
> 

Thank you,
Seb


> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@...roid.com.
> 

Powered by blists - more mailing lists