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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8c6enoolJe7Zeqk@google.com>
Date: Tue, 4 Mar 2025 17:38:02 +0000
From: Sebastian Ene <sebastianene@...gle.com>
To: Will Deacon <will@...nel.org>
Cc: catalin.marinas@....com, joey.gouly@....com, maz@...nel.org,
	oliver.upton@...ux.dev, snehalreddy@...gle.com,
	sudeep.holla@....com, suzuki.poulose@....com, vdonnefort@...gle.com,
	yuzenghui@...wei.com, kvmarm@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	kernel-team@...roid.com
Subject: Re: [PATCH v2 3/4] KVM: arm64: Map the hypervisor FF-A buffers on
 ffa init

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@...gle.com>
> > > > ---
> > > >  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. 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.

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

> We're also probing the minimum rxtx size in hyp_ffa_post_init() so doing
> this here is doubly wrong.
> 

Those operations should happen before the current ffa_map_hyp_buffers()
call, I agree.

Thanks,
Sebastian

> So I think we should probably just drop this patch.
> 
> Will