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] [thread-next>] [day] [month] [year] [list]
Message-ID: <aGdAMg43nHPwgeKn@kernel.org>
Date: Fri, 4 Jul 2025 05:45:06 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Prachotan Bathi <prachotan.bathi@....com>
Cc: 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 Wed, Jul 02, 2025 at 10:58:56PM -0500, Prachotan Bathi wrote:
> On 7/2/25 5:16 PM, Jarkko Sakkinen wrote:
> 
> > On Thu, Jun 26, 2025 at 01:45:20PM -0500, Prachotan Bathi wrote:
> > > Add a memzero macro to simplify and standardize zeroing
> > > FF-A data args, replacing direct uses of memset for
> > > improved readability and maintainability.
> > > 
> > > Signed-off-by: Prachotan Bathi <prachotan.bathi@....com>
> > > ---
> > >   drivers/char/tpm/tpm_crb_ffa.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> > > index 089d1e54bb46..397cc3b0a478 100644
> > > --- a/drivers/char/tpm/tpm_crb_ffa.c
> > > +++ b/drivers/char/tpm/tpm_crb_ffa.c
> > > @@ -12,6 +12,8 @@
> > >   #include <linux/arm_ffa.h>
> > >   #include "tpm_crb_ffa.h"
> > > +#define memzero(s, n) memset((s), 0, (n))
> > > +
> > >   /* TPM service function status codes */
> > >   #define CRB_FFA_OK			0x05000001
> > >   #define CRB_FFA_OK_RESULTS_RETURNED	0x05000002
> > > @@ -192,7 +194,7 @@ static int __tpm_crb_ffa_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)) {
> > > -		memset(&tpm_crb_ffa->direct_msg_data2, 0x00,
> > > +		memzero(&tpm_crb_ffa->direct_msg_data2,
> > >   		       sizeof(struct ffa_send_direct_data2));
> > >   		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> > > @@ -205,7 +207,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
> > >   		if (!ret)
> > >   			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
> > >   	} else {
> > > -		memset(&tpm_crb_ffa->direct_msg_data, 0x00,
> > > +		memzero(&tpm_crb_ffa->direct_msg_data,
> > >   		       sizeof(struct ffa_send_direct_data));
> > >   		tpm_crb_ffa->direct_msg_data.data1 = func_id;
> > > -- 
> > > 2.43.0
> > > 
> > It adds a ross-reference to the source code, meaning that you have to
> > jump back and forth in the source code just to see that there is a
> > function that wraps up a single memset() call.
> > 
> > How does that map to "readability"?
> > 
> > BR, Jarkko
> 
> Hi Jarkko
> 
> I've implemented this change post your feedback on v4 of the initial patch
> series, maybe this should've been a question at that point, but what was the
> reasoning for recommending that I use memzero instead? I'll use the same
> reasoning to rephrase the commit message.

OK I found what you were referring to:

https://lore.kernel.org/linux-integrity/aFF-WNSolTdV9PZG@kernel.org/

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 },
+		};
 
 		ret = msg_ops->sync_send_receive2(tpm_crb_ffa->ffa_dev,
 				&tpm_crb_ffa->direct_msg_data2);
 		if (!ret)
 			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
 	} else {
-		memzero(&tpm_crb_ffa->direct_msg_data,
-		       sizeof(struct ffa_send_direct_data));
-
-		tpm_crb_ffa->direct_msg_data.data1 = func_id;
-		tpm_crb_ffa->direct_msg_data.data2 = a0;
-		tpm_crb_ffa->direct_msg_data.data3 = a1;
-		tpm_crb_ffa->direct_msg_data.data4 = a2;
+		tpm_crb_ffa->direct_msg_data = (struct ffa_send_direct_data){
+			.data1 = func_id,
+			.data2 = a0,
+			.data3 = a1,
+		};
 
 		ret = msg_ops->sync_send_receive(tpm_crb_ffa->ffa_dev,
 				&tpm_crb_ffa->direct_msg_data);

I tested the compilation with:

make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64 tinyconfig && ./scripts/config --file .config -e CONFIG_KEYS -e CONFIG_TCG_TPM -e CONFIG_64BIT -e CONFIG_TRUSTED_KEYS -e CONFIG_TTY -e CONFIG_PROCFS -e CONFIG_SYSFS -e CONFIG_TCG_VTPM_PROXY -e CONFIG_EFI -e CONFIG_ACPI -e CONFIG_ARM_FFA_TRANSPORT -e CONFIG_TCG_CRB && yes '' | make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64 oldconfig && make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64 -j$(nproc)

BR, Jarkko