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: <151a612b-198a-4f7e-94e7-10426831ab94@arm.com>
Date: Wed, 2 Jul 2025 22:58:56 -0500
From: Prachotan Bathi <prachotan.bathi@....com>
To: Jarkko Sakkinen <jarkko@...nel.org>
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 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.

Best
Prachotan.
Sorry for the spam, Thunderbird keeps defaulting to html emails.


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