Message-ID: <b43a443a-7884-c8ef-dff2-5563069937aa@linux.microsoft.com>
Date:   Tue, 11 Jul 2023 11:31:43 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 03/10] ima: allocate buffer at kexec load to hold ima
 measurements
Content-Language: en-US
To:     Mimi Zohar <zohar@linux.ibm.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,
        Eric Biederman <ebiederm@xmission.com>
References: <20230703215709.1195644-1-tusharsu@linux.microsoft.com>
 <20230703215709.1195644-4-tusharsu@linux.microsoft.com>
 <ef2f8f1f9ab806e010cac02aab1b64fa51ce4c2c.camel@linux.ibm.com>
From:   Tushar Sugandhi <tusharsu@linux.microsoft.com>
In-Reply-To: <ef2f8f1f9ab806e010cac02aab1b64fa51ce4c2c.camel@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Adding Eric to cc.

On 7/7/23 06:01, Mimi Zohar wrote:
> 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.
>
Will do.
>> 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.
Thanks. :)
>> 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?
>
You are right.  As you suggested below, defining the extra amount in
Kconfig is a cleaner approach.

>> 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.
Sure.  Will do.
>>   /* 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.
Agreed.  Will take care of this based on if we end up using just
ima_dump_measurement_list(), or combination of
ima_allocate_buf_at_kexec_load() and ima_populate_buf_at_kexec_execute().

~Tushar
>> -	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;