linux-kernel - Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

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: <aGlc2I9YGgPyc3lO@kernel.org>
Date: Sat, 5 Jul 2025 20:11:52 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: David Laight <david.laight.linux@...il.com>
Cc: Prachotan Bathi <prachotan.bathi@....com>,
	Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>,
	Stuart Yoder <stuart.yoder@....com>,
	linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace
 memset

On Sat, Jul 05, 2025 at 08:10:03AM +0100, David Laight wrote:
> On Fri, 4 Jul 2025 17:04:02 +0300
> Jarkko Sakkinen <jarkko@...nel.org> wrote:
> 
> > On Fri, Jul 04, 2025 at 11:40:10AM +0100, David Laight wrote:
> > > On Fri, 4 Jul 2025 05:56:50 +0300
> > > Jarkko Sakkinen <jarkko@...nel.org> wrote:
> > >   
> > > > On Fri, Jul 04, 2025 at 05:45:11AM +0300, Jarkko Sakkinen wrote:  
> > > ...  
> > > > > Well, that was some truly misguided advice from my side so all the shame
> > > > > here is on me :-) There's no global memzero() and neither explicit
> > > > > version makes much sense here. Sorry about that.
> > > > > 
> > > > > I gave it now (actual) thought, and here's what I'd propose:
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> > > > > index 96746d5b03e3..e769f6143a7c 100644
> > > > > --- a/drivers/char/tpm/tpm_crb_ffa.c
> > > > > +++ b/drivers/char/tpm/tpm_crb_ffa.c
> > > > > @@ -203,26 +203,20 @@ static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
> > > > >  	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
> > > > >  
> > > > >  	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> > > > > -		memzero(&tpm_crb_ffa->direct_msg_data2,
> > > > > -		       sizeof(struct ffa_send_direct_data2));
> > > > > -
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[1] = a0;
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[2] = a1;
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[3] = a2;
> > > > > +		tpm_crb_ffa->direct_msg_data2 = (struct ffa_send_direct_data2){
> > > > > +			.data = { func_id, a0, a1, a2 },
> > > > > +		};  
> > > 
> > > clang has a habit of compiling that as an un-named on-stack structure that
> > > is initialised and then memcpy() used to copy it into place.
> > > Often not was intended and blows the stack when the structure is large.
> > > 
> > > So probably not a pattern that should be encouraged.  
> > 
> > This is interesting observation so I had to do some compilation tests to
> > verify the claim just to see how it plays out (and for the sake of
> > learning while doing it).
> > 
> > Note that I use GCC for the examples but I have high doubts that clang
> > would do worse. Please share the insight if that is a wrong assumption.
> 
> It is a clang issue and may only affect builds with some of the 'memory
> sanitiser' run-time checks.
> Search through the mail archives for issues with overlarge stack frames.

If clang really did that, it would cost at worst 40 bytes of stack (aka
the size of struct ffa_send_direct_data. And if there is an issue, it
absolutely will get fixed.

That does not weight over making a code change that makes the most sense
for Linux in the long-term. And to add, I did show both the code and
the figures to support my claim.
 

> 
> 	David

BR, Jarkko