linux-kernel - Re: [PATCH v10 1/8] ima: rename variable the ser_file "file" to "ima_kexec

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: <4340a887-f0d1-481f-b47f-cd0226adf869@linux.microsoft.com>
Date: Fri, 21 Mar 2025 09:16:50 -0700
From: steven chen <chenste@...ux.microsoft.com>
To: Mimi Zohar <zohar@...ux.ibm.com>, stefanb@...ux.ibm.com,
 roberto.sassu@...weicloud.com, roberto.sassu@...wei.com,
 eric.snowberg@...cle.com, ebiederm@...ssion.com, paul@...l-moore.com,
 code@...icks.com, bauermann@...abnow.com, linux-integrity@...r.kernel.org,
 kexec@...ts.infradead.org, linux-security-module@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: madvenka@...ux.microsoft.com, nramas@...ux.microsoft.com,
 James.Bottomley@...senPartnership.com, bhe@...hat.com, vgoyal@...hat.com,
 dyoung@...hat.com
Subject: Re: [PATCH v10 1/8] ima: rename variable the ser_file "file" to
 "ima_kexec_file"

On 3/19/2025 6:42 AM, Mimi Zohar wrote:
> Fix spelling: set_file
>
>
> On Mon, 2025-03-17 at 18:04 -0700, steven chen wrote:
>> The name of the local variable "file" of type seq_file defined in the
>> ima_dump_measurement_list function is too generic. To better reflect the
>> purpose of the variable, rename it to "ima_kexec_file". This change will
>> help improve code readability and maintainability by making the variable's
>> role more explicit.
> The reason for making the variable name change is the variable scope.
>
> -> Before making the function local seq_file "file" variable global, rename it
> to ima_kexec_file.
>
>> The variable ima_kexec_file is indeed the memory allocated for copying IMA
>> measurement records. The ima_dump_measurement_list function calculates the
>> actual memory occupied by the IMA logs and compares it with the allocated
>> memory. If there is enough memory, it copies all IMA measurement records;
>> otherwise, it does not copy any records, which would result in a failure
>> of remote attestation.
> This paragraph is not applicable to the patch change.
>
>> Suggested-by: Mimi Zohar <zohar@...ux.ibm.com>
>> Signed-off-by: steven chen <chenste@...ux.microsoft.com>
>> ---
>>   security/integrity/ima/ima_kexec.c | 39 ++++++++++++++++++------------
>>   1 file changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>> index 9d45f4d26f73..8567619889d1 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -15,33 +15,41 @@
>>   #include "ima.h"
>>   
>>   #ifdef CONFIG_IMA_KEXEC
>> +/*
>> + * Copy the measurement list to the allocated memory
>> + * compare the size of IMA measurement list with the size of the allocated memory
>> + *    if the size of the allocated memory is not less than the size of IMA measurement list
>> + *        copy the measurement list to the allocated memory.
>> + *    else
>> + *        return error
>> + */
> Please minimize patch changes.  Before posting, please look at the patch(s) and
> remove anything not applicable to it.  In this case, the comment is not
> applicable to the variable name change.
>
> thanks,
>
> Mimi
>    
>>   static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
>>   				     unsigned long segment_size)
>>   {
>> +	struct seq_file ima_kexec_file;
>>   	struct ima_queue_entry *qe;
>> -	struct seq_file file;
>>   	struct ima_kexec_hdr khdr;
>>   	int ret = 0;
>>   
>>   	/* segment size can't change between kexec load and execute */
>> -	file.buf = vmalloc(segment_size);
>> -	if (!file.buf) {
>> +	ima_kexec_file.buf = vmalloc(segment_size);
>> +	if (!ima_kexec_file.buf) {
>>   		ret = -ENOMEM;
>>   		goto out;
>>   	}
>>   
>> -	file.file = NULL;
>> -	file.size = segment_size;
>> -	file.read_pos = 0;
>> -	file.count = sizeof(khdr);	/* reserved space */
>> +	ima_kexec_file.file = NULL;
>> +	ima_kexec_file.size = segment_size;
>> +	ima_kexec_file.read_pos = 0;
>> +	ima_kexec_file.count = sizeof(khdr);	/* reserved space */
>>   
>>   	memset(&khdr, 0, sizeof(khdr));
>>   	khdr.version = 1;
>>   	/* This is an append-only list, no need to hold the RCU read lock */
>>   	list_for_each_entry_rcu(qe, &ima_measurements, later, true) {
>> -		if (file.count < file.size) {
>> +		if (ima_kexec_file.count < ima_kexec_file.size) {
>>   			khdr.count++;
>> -			ima_measurements_show(&file, qe);
>> +			ima_measurements_show(&ima_kexec_file, qe);
>>   		} else {
>>   			ret = -EINVAL;
>>   			break;
>> @@ -55,23 +63,24 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
>>   	 * fill in reserved space with some buffer details
>>   	 * (eg. version, buffer size, number of measurements)
>>   	 */
>> -	khdr.buffer_size = file.count;
>> +	khdr.buffer_size = ima_kexec_file.count;
>>   	if (ima_canonical_fmt) {
>>   		khdr.version = cpu_to_le16(khdr.version);
>>   		khdr.count = cpu_to_le64(khdr.count);
>>   		khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
>>   	}
>> -	memcpy(file.buf, &khdr, sizeof(khdr));
>> +	memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr));
>>   
>>   	print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
>> -			     file.buf, file.count < 100 ? file.count : 100,
>> +			     ima_kexec_file.buf, ima_kexec_file.count < 100 ?
>> +			     ima_kexec_file.count : 100,
>>   			     true);
>>   
>> -	*buffer_size = file.count;
>> -	*buffer = file.buf;
>> +	*buffer_size = ima_kexec_file.count;
>> +	*buffer = ima_kexec_file.buf;
>>   out:
>>   	if (ret == -EINVAL)
>> -		vfree(file.buf);
>> +		vfree(ima_kexec_file.buf);
>>   	return ret;
>>   }
>>   

Hi Mimi,

I will update in next version.

Thanks,

Steven