linux-kernel - Re: [PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx buffer to Trustzone

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] [day] [month] [year] [list]

Message-ID: <Z9Lk9QwAGhYxk8lC@bogus>
Date: Thu, 13 Mar 2025 14:00:21 +0000
From: Sudeep Holla <sudeep.holla@....com>
To: Will Deacon <will@...nel.org>
Cc: Sebastian Ene <sebastianene@...gle.com>, catalin.marinas@....com,
	Sudeep Holla <sudeep.holla@....com>, joey.gouly@....com,
	maz@...nel.org, oliver.upton@...ux.dev, snehalreddy@...gle.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,
	Andrei Homescu <ahomescu@...gle.com>
Subject: Re: [PATCH v2 4/4] KVM: arm64: Release the ownership of the hyp rx
 buffer to Trustzone

On Thu, Mar 13, 2025 at 12:15:59PM +0000, Will Deacon wrote:
> On Thu, Mar 06, 2025 at 09:40:43AM +0000, Sudeep Holla wrote:
> > On Wed, Mar 05, 2025 at 07:34:26PM +0000, Will Deacon wrote:
> > > On Wed, Mar 05, 2025 at 09:41:04AM +0000, Sudeep Holla wrote:
> > > > On Wed, Mar 05, 2025 at 12:45:23AM +0000, Will Deacon wrote:
> > > > > Hmm, the FFA spec is characteristically unclear as to whether or not we
> > > > > need to release the rx buffer in the case that the flags indicate use of
> > > > > the rx buffer but the returned partition count is 0.
> > > > >
> > > > > Sudeep -- do you know what we should be doing in that case?
> > > > >
> > > >
> > > > We need to call RX_RELEASE here. I went back to the spec to confirm the
> > > > same again.
> > > >
> > > > v1.2 EAC0 spec Section 7.2.2.4.2 Transfer of buffer ownership
> > > > (Or just look for the section title in any version of the spec)
> > > > "
> > > > 2. Ownership transfer for the RX buffer takes place as follows.
> > > >     2. For a framework message,
> > > >        1. Completion of the FFA_PARTITION_INFO_GET ABI transfers the ownership
> > > >        of the caller’s RX buffer from the Producer to the Consumer.
> > > > 3. For both types of messages, an invocation of the following FF-A ABIs
> > > >     transfers the ownership from the Consumer to the Producer.
> > > >        1. FFA_MSG_WAIT ...
> > > >        2. FFA_RX_RELEASE.
> > > > "
> > > >
> > > > Hope that helps, can dig deeper if there are any ambiguities around this.
> > >
> > > Thanks Sudeep, but that also makes it sound like we need the RX_RELEASE
> > > even if we're not using the RX buffer per the input flags. :/
> > >
> >
> > Good spot, I had forgotten about the input flags that can avoid using the
> > buffer. I will see if we can improve the spec in that regards.
>
> Thanks. In the meantime, what do you think is the correct behaviour in that
> case? I guess _not_ doing the release when the flags don't request the RX
> buffer? In other words:
>
>
> 	if (flags & PARTITION_INFO_GET_RETURN_COUNT_ONLY)
> 		goto out_unlock;
>
> 	if (!count)
> 		goto release_rx;
>
> 	[...]
>
> 	if (copy_sz > KVM_FFA_MBOX_NR_PAGES * PAGE_SIZE) {
> 		ffa_to_smccc_res(res, FFA_RET_ABORTED);
> 		goto release_rx;
> 	}
>
> 	memcpy(host_buffers.rx, hyp_buffers.rx, copy_sz);
> release_rx:
> 	ffa_rx_release(&_res);
> out_unlock:
> 	hyp_spin_unlock(&host_buffers.lock);
> }
>
>
> What do you reckon?

Yes matches my understanding. I also cross checked with FF-A spec authors
to be sure. Now I got to fix that in the driver, currently it releases
buffer unconditionally which is wrong 🙁.

-- 
Regards,
Sudeep