linux-kernel - Re: [PATCH 1/3] KVM: arm64: Restrict FF-A host version renegotiation

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: <aBnNXyJn818ZEKOS@google.com>
Date: Tue,  6 May 2025 02:29:41 -0700
From: Per Larsen <perl@...unant.com>
To: maz@...nel.org
Cc: armellel@...gle.com,
	arve@...roid.com,
	catalin.marinas@....com,
	kernel-team@...roid.com,
	kvmarm@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	perl@...unant.com,
	qperret@...gle.com,
	sebastianene@...gle.com,
	sudeep.holla@....com,
	will@...nel.org,
	yuzenghui@...wei.com,
	Per Larsen <perlarsen@...gle.com>
Subject: Re: [PATCH 1/3] KVM: arm64: Restrict FF-A host version renegotiation

From: Per Larsen <perlarsen@...gle.com>

On Fri, May 02, 2025 at 09:47:45AM +0100, Marc Zyngier wrote:
> On Fri, 02 May 2025 04:52:39 +0100,
> Per Larsen <perl@...unant.com> wrote:
> > 
> > FF-A implementations with the same major version must interoperate with
> > earlier minor versions per DEN0077A 1.2 REL0 13.2.1 but FF-A version 1.1
> > broke the ABI on several structures and 1.2 relies on SMCCC 1.2 is not
> > backwards compatible with SMCCC 1.2 (see DEN0028 1.6 G BET0 Appendix F).
> > 
> > If we return the negotiated hypervisor version when the host requests a
> > lesser minor version, the host will rely on the FF-A interoperability
> > rules. Since the hypervisor does not currently have the necessary
> > compatibility paths (e.g. to handle breaking changes to the SMC calling
> > convention), return NOT_SUPPORTED.
> > 
> > Signed-off-by: Per Larsen <perlarsen@...gle.com>
> > Signed-off-by: Per Larsen <perl@...unant.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/ffa.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > index 3369dd0c4009..10e88207b78e 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c
> > @@ -712,7 +712,24 @@ static void do_ffa_version(struct arm_smccc_res *res,
> > 
> >   hyp_spin_lock(&version_lock);
> >   if (has_version_negotiated) {
> > - res->a0 = hyp_ffa_version;
> > + /*
> > + * FF-A implementations with the same major version must
> > + * interoperate with earlier minor versions per DEN0077A 1.2
> > + * REL0 13.2.1 but FF-A version 1.1 broke the ABI on several
> > + * structures and 1.2 relies on SMCCC 1.2 is not backwards
> > + * compatible with SMCCC 1.2 (see DEN0028 1.6 G BET0 Appendix F).
> 
> I can't parse this sentence. Missing words?
> 

Yes, I will fix this in v2.

> > + *
> > + * If we return the negotiated hypervisor version when the host
> > + * requests a lesser minor version, the host will rely on the
> > + * aforementioned FF-A interoperability rules. Since the
> > + * hypervisor does not currently have the necessary compatibility
> > + * paths (e.g. to paper over the above-mentioned calling
> > + * convention changes), return NOT_SUPPORTED.
> > + */
> > + if (FFA_MINOR_VERSION(ffa_req_version) < FFA_MINOR_VERSION(hyp_ffa_version))
> > + res->a0 = FFA_RET_NOT_SUPPORTED;
> > + else
> > + res->a0 = hyp_ffa_version;
> >   goto unlock;
> >   }
> > 
> 
> Something has gone seriously wrong with your email, and the patches
> are badly mangled and unusable. They are also sent as individual
> patches and not as a thread, which is a sign that you didn't send them
> using git. Please fix this for your next posting.
> 

Yes, my apologies. I will use git send-email to post v2.

> More to the meat of the patches: why should the hypervisor paper over
> anything if the spec is broken? Why can't the host just as well decide
> for itself what to do?
> 

Asssuming we drop this patch from the series and apply the rest, the
hypervisor and host can negotiate FF-A 1.2. If the host then calls
FFA_VERSION a second time to request FF-A 1.1, the hypervisor would
return version 1.2 (without this patch). Per the spec, that means the
host is can use the compatibility rules (DEN0077A Sec 13.2.1) to go
ahead and use FF-A 1.1 (every function in 1.A must work in a compatible
way in 1.B if B>A). 

However, the hypervisor negotiated version stays at 1.2 so it will use
SMCCC 1.2 for 64-bit interfaces. The host has no way of knowing this and
might as well assume that the hypervisor was implemented to fall back to
SMCCC 1.1 in this particular case. 

I don't even know that the host will ever try to renegotiate as it is
explicitly not allowed by the FF-A spec. There is no way for the
hypervisor to say, "stay at the negotiated version" so we must return
NOT_SUPPORTED. 

> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
> 
Thanks,
Per