Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

From: Tushar Sugandhi <tusharsu@linux.microsoft.com>
To: Mimi Zohar <zohar@linux.ibm.com>,
	ebiederm@xmission.com, noodles@fb.com, bauermann@kolabnow.com,
	kexec@lists.infradead.org, linux-integrity@vger.kernel.org
Cc: code@tyhicks.com, nramas@linux.microsoft.com, paul@paul-moore.com
Subject: Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function
Date: Tue, 14 Nov 2023 14:32:57 -0800	[thread overview]
Message-ID: <2c9e3b71-5416-4336-82f1-cd78e26dd62e@linux.microsoft.com> (raw)
In-Reply-To: <e4e92a1816475596b91bbbb2fdf33c11ce375cf1.camel@linux.ibm.com>


On 10/26/23 20:25, Mimi Zohar wrote:
> On Thu, 2023-10-26 at 16:16 -0400, Mimi Zohar wrote:
>> Hi Tushar,
>>
>> According to Documentation/process/submitting-patches.rst, the subject
>> line should be between 70-75 characters.
>>
>> Perhaps something like "ima: define and call ima_alloc_kexec_buffer()".
>>
>> On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote:
>>> IMA allocates memory and dumps the measurement during kexec soft reboot
>>> as a single function call ima_dump_measurement_list().  It gets called
>>> during kexec 'load' operation.  It results in the IMA measurements
>>> between the window of kexec 'load' and 'execute' getting dropped when the
>>> system boots into the new Kernel.  One of the kexec requirements is the
>>> segment size cannot change between the 'load' and the 'execute'.
>>> Therefore, to address this problem, ima_dump_measurement_list() needs
>>> to be refactored to allocate the memory at kexec 'load', and dump the
>>> measurements at kexec 'execute'.  The function that allocates the memory
>>> should handle the scenario where the kexec load is called multiple times
>>
>> The above pragraph is unnecessary.
>>
>>> Refactor ima_dump_measurement_list() to move the memory allocation part
>>> to a separate function ima_alloc_kexec_buf() to allocate buffer of size
>>> 'kexec_segment_size' at kexec 'load'.  Make the local variables in
>>> function ima_dump_measurement_list() global, so that they can be accessed
>>> from ima_alloc_kexec_buf().  Make necessary changes to the function
>>> ima_add_kexec_buffer() to call the above two functions.
>>
>> Fix the wording based on the suggested changes below.
>>
>>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>>
>> - Before re-posting this patch set, verify there aren't any
>> "checkpatch.pl --strict" issues.
>> - After applying each patch, compile the kernel and verify it still
>> works.
> 
> Doing this will detect whether or not the patch set is bisect safe.
> 
I usually just do checkpatch.pl <.patch file>.
I didn't know about --strict and it's benefits.
Will do it going forward.


>>> ---
>>>   security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
>>>   1 file changed, 93 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>>> index 419dc405c831..307e07991865 100644
>>> --- a/security/integrity/ima/ima_kexec.c
>>> +++ b/security/integrity/ima/ima_kexec.c
>>> @@ -15,61 +15,114 @@
>>>   #include "ima.h"
>>>   
>>>   #ifdef CONFIG_IMA_KEXEC
>>> +struct seq_file ima_kexec_file;
>>
>> Define "ima_kexec_file" as static since it only used in this file.
>> Since the variable does not need to be global, is there still a reason
>> for changing its name?   Minimize code change.
> 
> Adding "static" would make ima_kexec_file a global static variable.
> Please ignore my comment about reverting the variable name change.
> 
> Mimi
> 
Sure :)

~Tushar
...