Re: [PATCH 03/10] ima: allocate buffer at kexec load to hold ima measurements

[Mimi Zohar <zohar@linux.ibm.com>]

Hi Tushar,

The function to "ima: allocate buffer at kexec load to hold ima
measurements" already exists.  Please update the Subject line to
indicate increasing the IMA kexec buffer size.

On Mon, 2023-07-03 at 14:57 -0700, Tushar Sugandhi wrote:
> The IMA subsystem needs a dedicated mechanism to reserve extra memory for
> measurements added between the kexec 'load' and kexec 'execute'.

What is a "dedicated mechanism?".  Either start the sentence with
"reserve ..." or completely remove it.

> Update ima_add_kexec_buffer to allocate a buffer of a sufficient size
> taking ima binary runtime measurements size, size of ima_kexec_hdr, and
> IMA_KEXEC_EXTRA_SIZE into account.  Adjust the kexec_segment_size to align
> to the PAGE_SIZE.  Call ima_allocate_buf_at_kexec_load() to allocate the
> buffer.

The size of the ima_kexec_hdr kind of gets lost in the amount of
additional memory being allocated, but sure it's a nice clean up.

> This patch assumes the extra space defined (IMA_KEXEC_EXTRA_SIZE) is
> sufficient to handle the additional measurements.  This should be as per
> the system requirements and based on the number of additional measurements
> expected during the window from kexec 'load' to kexec 'execute'.

Should the extra amount of memory be hard coded?

> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  security/integrity/ima/ima.h       |  2 ++
>  security/integrity/ima/ima_kexec.c | 21 ++++++++++-----------
>  2 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c29db699c996..2ffda9449b9b 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -43,6 +43,8 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
>  
>  #define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
>  
> +#define IMA_KEXEC_EXTRA_SIZE (16 * PAGE_SIZE)
> 

Not all IMA policies require extra memory.  Define and use a new IMA
Kconfig to set the amount of extra memory.

>  /* current content of the policy */
>  extern int ima_policy_flag;
>  
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 858b67689701..7deb8df31485 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -188,31 +188,30 @@ void ima_add_kexec_buffer(struct kimage *image)
>  	/* use more understandable variable names than defined in kbuf */
>  	void *kexec_buffer = NULL;
>  	size_t kexec_buffer_size;
> -	size_t kexec_segment_size;
>  	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) +
> +			      IMA_KEXEC_EXTRA_SIZE;
> +
>  	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");
>  		return;
>  	}
>  
> -	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
> -				  kexec_segment_size);

Removing the call to ima_dump_measurement_list here is not bisect safe.

> -	if (!kexec_buffer) {
> -		pr_err("Not enough memory for the kexec measurement buffer.\n");
> +	ret = ima_allocate_buf_at_kexec_load();
> +	if (ret < 0)
>  		return;
> -	}
>  
>  	kbuf.buffer = kexec_buffer;
>  	kbuf.bufsz = kexec_buffer_size;

-- 
thanks,

Mimi