Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on ffa init

[Sebastian Ene]

On Wed, Mar 05, 2025 at 12:38:08AM +0000, Will Deacon wrote:
> On Tue, Mar 04, 2025 at 05:38:02PM +0000, Sebastian Ene wrote:
> > On Tue, Mar 04, 2025 at 01:56:35AM +0000, Will Deacon wrote:
> > > On Tue, Mar 04, 2025 at 12:53:25AM +0000, Sebastian Ene wrote:
> > > > On Mon, Mar 03, 2025 at 11:43:03PM +0000, Will Deacon wrote:
> > > > > On Thu, Feb 27, 2025 at 06:17:48PM +0000, Sebastian Ene wrote:
> > > > > > Map the hypervisor's buffers irrespective to the host and return
> > > > > > a linux error code from the FF-A error code on failure. Remove
> > > > > > the unmap ff-a buffers calls from the hypervisor as it will
> > > > > > never be called.
> > > > > > Prevent the host from using FF-A directly with Trustzone
> > > > > > if the hypervisor could not map its own buffers.
> > > > > > 
> > > > > > Signed-off-by: Sebastian Ene <sebastianene@xxxxxxxxxx>
> > > > > > ---
> > > > > >  arch/arm64/kvm/hyp/nvhe/ffa.c | 46 +++++++++++++----------------------
> > > > > >  1 file changed, 17 insertions(+), 29 deletions(-)
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > @@ -861,6 +842,7 @@ int hyp_ffa_init(void *pages)
> > > > > >  {
> > > > > >  	struct arm_smccc_res res;
> > > > > >  	void *tx, *rx;
> > > > > > +	int ret;
> > > > > >  
> > > > > >  	if (kvm_host_psci_config.smccc_version < ARM_SMCCC_VERSION_1_2)
> > > > > >  		return 0;
> > > > > > @@ -911,5 +893,11 @@ int hyp_ffa_init(void *pages)
> > > > > >  		.lock	= __HYP_SPIN_LOCK_UNLOCKED,
> > > > > >  	};
> > > > > >  
> > > > > > +	/* Map our hypervisor buffers into the SPMD */
> > > > > > +	ret = ffa_map_hyp_buffers();
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > 
> > > > > Doesn't calling RXTX_MAP here undo the fix from c9c012625e12 ("KVM:
> > > > > arm64: Trap FFA_VERSION host call in pKVM") where we want to allow for
> > > > > the host to negotiate the version lazily?
> > > > 
> > > > We still have the same behaviour where we don't allow memory
> > > > sharing to happen until the version is negotiated but this
> > > > separates the hypervisor buffer mapping part from the host.
> > > 
> > > Sadly, the spec doesn't restrict this to the memory sharing calls:
> > > 
> > >   | [...] negotiation of the version must happen before an invocation of
> > >   | any other FF-A ABI
> > > 
> > 
> > We do that, as the hypervisor negotiates its own version in
> > hyp_ffa_init.
> 
> hyp_ffa_init() only issues FFA_VERSION afaict, which is the one call
> that you're allowed to make during negotiation. So the existing code is
> fine.
> 
> > I think the host shouldn't be allowed to overwrite the
> > hyp_ffa_version obtained from _init, this feels wrong as you
> > can have a driver that forcefully downgrades the hypervisor to an old
> > version.
> 
> I think that's also fine. The FFA code in the hypervisor exists solely
> to proxy requests from the host; it's not used for anything else and so,
> from the host's persective, FFA should behave identically to the case in
> which the proxy is not present (e.g. if we were just using VHE). That
> means that we're doing the right thing by deferring to the host for
> version negotation.
> 
> Are you saying there's a bug in the current code if the host negotiates
> the downgrade?

It is an issue *only* for doing guest-ffa (which isn't posted here yet).
If we allow the host to dictate the version & there is an issue with TZ
FF-A dispatcher in that version => the guests will be affected by this
as well.

> 
> > We need to do three things, Sudeep & Will please correct me if I am
> > wrong, but this is how I see it:
> > 
> > - the hypervisor should act as a separate entity (it has a different ID and
> > in the current implementation we don't do a distinction between host/hyp) and
> > it should be able to lock its own version from init.
> 
> I strongly disagree with that. The hypervisor isn't using FFA for
> anything other than proxying the host and so we don't need to negotiate
> a separate version.
> 
> What would we gain by doing this? Is there a bug with what we're doing
> at the moment?

I think we need to make a distinction between the host and the
hypervisor when we are adding support for guest-ffa. We currently have
the same id (== 0) for both of them.

> 
> > - keep a separate version negotiated for the host
> > - trap FFA_ID_GET from the host and return ID=1 because
> >   currently we forward the call to the TZ and it returns the same ID
> >   as the (hypervisor == 0).
> 
> Why is this beneficial? It just looks like complexity at EL2 for no gain
> to me, but maybe I'm missing something.
>

Because the host can impersonate the hypervisor using ff-a direct calls atm.
and we are in a position to restrict the host from playing nasty games
with TZ.

> Will

Thanks,
Sebastian