Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[Tushar Sugandhi <tusharsu@linux.microsoft.com>]

On 12/20/23 12:15, Mimi Zohar wrote:
> Hi Tushar,
> 
> The Subject line should include the word "extra".   The use of the
> extra memory isn't limited to the measurements between the kexec load
> and exec.  Additional records could be added as a result of the kexec
> load itself.  Let's simplify the title to "ima: make the kexec extra
> memory configurable".
> 
> Please remove any references to measurements between kexec load and
> execute.
> 
Thanks Mimi. I will make these changes.

> On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
>> IMA currently allocates half a PAGE_SIZE for the extra events that would
>> be measured between kexec 'load' and 'execute'.  Depending on the IMA
>> policy and the system state, that memory may not be sufficient to hold
>> the extra IMA events measured after kexec 'load'.  The memory
>> requirements vary from system to system and they should be configurable.
> 
> The extra memory allocated for carrying the IMA measurement list across
> kexec is hardcoded as a half a PAGE.   Make it configurable.
> 
Will do.
>> Define a Kconfig option, IMA_KEXEC_EXTRA_MEMORY_KB, to configure the
>> extra memory (in kb) to be allocated for IMA measurements added in the
>> window from kexec 'load' to kexec 'execute'.
> 
>> Update ima_add_kexec_buffer() function to allocate memory based on the
>> Kconfig option value, rather than the currently hardcoded one.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/Kconfig     |  9 +++++++++
>>   security/integrity/ima/ima_kexec.c | 13 ++++++++-----
>>   2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
>> index 60a511c6b583..8792b7aab768 100644
>> --- a/security/integrity/ima/Kconfig
>> +++ b/security/integrity/ima/Kconfig
>> @@ -338,3 +338,12 @@ config IMA_DISABLE_HTABLE
>>   	default n
>>   	help
>>   	   This option disables htable to allow measurement of duplicate records.
>> +
>> +config IMA_KEXEC_EXTRA_MEMORY_KB
>> +	int
>> +	depends on IMA && IMA_KEXEC
>> +	default 64
> 
> Since this isn't optional, the default should remain as a half page.
> Since a page is architecture specific, the default will need to be arch
>   specific.
> 
> thanks,
> 
> Mimih
> 
It was a feedback from Stefan in the V2 of this series to convert it 
from number of PAGES to KB.[1]

But I can revert it to number of pages again.

Also, making the default value as a fraction (1/2 page) feels weird for 
a CONFIG variable.

Is it ok to make the default value as one page rather than half page?

[1]
https://lore.kernel.org/all/15a12e79-2e90-28f7-ffa3-ff470c673099@linux.ibm.com/
 >>> +config IMA_KEXEC_EXTRA_PAGES
 >>> +    int
 >>> +    depends on IMA && IMA_KEXEC
 >>> +    default 16
 >>> +    help
 >>> +      IMA_KEXEC_EXTRA_PAGES determines the number of extra
 >>> +      pages to be allocated for IMA measurements added in the
 >>> +      window from kexec 'load' to kexec 'execute'.
 >>
 >>
 >> On ppc64 a page is 64kb. I would ask for additional kb here.
 >>
 >>
 > Not sure I understand.  Do you mean I should make the default value of
 > the config IMA_KEXEC_EXTRA_PAGES 64 or something?


No, what I mean is you should ask the user for how many extra kilobytes
(kb) to allocate - not ask for pages.


     Stefan

>> +	help
>> +	  IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be
>> +	  allocated (in kb) for IMA measurements added in the window
>> +	  from kexec 'load' to kexec 'execute'.
>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>> index 55bd5362262e..063da9c834a0 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -128,15 +128,18 @@ void ima_add_kexec_buffer(struct kimage *image)
>>   	int ret;
>>   
>>   	/*
>> -	 * Reserve an extra half page of memory for additional measurements
>> -	 * added during the kexec load.
>> +	 * Reserve extra memory for measurements added in the window from
>> +	 * kexec 'load' to kexec 'execute'.
>>   	 */
>> -	binary_runtime_size = ima_get_binary_runtime_size();
>> +	binary_runtime_size = ima_get_binary_runtime_size() +
>> +			      sizeof(struct ima_kexec_hdr) +
>> +			      (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024);
>> +
>>   	if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE)
>>   		kexec_segment_size = ULONG_MAX;
>>   	else
>> -		kexec_segment_size = ALIGN(ima_get_binary_runtime_size() +
>> -					   PAGE_SIZE / 2, PAGE_SIZE);
>> +		kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE);
>> +
>>   	if ((kexec_segment_size == ULONG_MAX) ||
>>   	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
>>   		pr_err("Binary measurement list too large.\n");
>