Re: [PATCH v5 3/3] arm64: Add IMA kexec buffer to DTB

[Lakshmi Ramasubramanian <nramas@linux.microsoft.com>]

On 9/3/20 3:11 PM, Thiago Jung Bauermann wrote:
> 
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> 
>> The address and size of the current kernel's IMA measurement log
>> need to be added to the device tree's IMA kexec buffer node for
>> the log to be carried over to the next kernel on the kexec call.
>>
>> Add the IMA measurement log buffer properties to the device tree for
>> ARM64 and reserve the memory for storing the IMA log.
>> Update CONFIG_KEXEC_FILE to select CONFIG_HAVE_IMA_KEXEC to
>> indicate that the IMA measurement log information is present in
>> the device tree.
>>
>> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
>> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>> ---
>>   arch/arm64/Kconfig                     |  1 +
>>   arch/arm64/kernel/machine_kexec_file.c | 15 +++++++++++++++
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 6d232837cbee..9f03c8245e5b 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -1077,6 +1077,7 @@ config KEXEC
>>   config KEXEC_FILE
>>   	bool "kexec file based system call"
>>   	select KEXEC_CORE
>> +	select HAVE_IMA_KEXEC
>>   	help
>>   	  This is new version of kexec system call. This system call is
>>   	  file based and takes file descriptors as system call argument
>> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
>> index 361a1143e09e..0fe3d629eefe 100644
>> --- a/arch/arm64/kernel/machine_kexec_file.c
>> +++ b/arch/arm64/kernel/machine_kexec_file.c
>> @@ -136,6 +136,21 @@ static int setup_dtb(struct kimage *image,
>>   				FDT_PROP_KASLR_SEED);
>>   	}
>>   
>> +	/* add ima-kexec-buffer */
>> +	if (image->arch.ima_buffer_size > 0) {
>> +		ret = fdt_appendprop_addrrange(dtb, 0, off,
>> +				FDT_PROP_IMA_KEXEC_BUFFER,
>> +				image->arch.ima_buffer_addr,
>> +				image->arch.ima_buffer_size);
>> +		if (ret)
>> +			return (ret == -FDT_ERR_NOSPACE ? -ENOMEM : -EINVAL);
>> +
>> +		ret = fdt_add_mem_rsv(dtb, image->arch.ima_buffer_addr,
>> +				      image->arch.ima_buffer_size);
>> +		if (ret)
>> +			goto out;
>> +	}
>> +
>>   	/* add rng-seed */
>>   	if (rng_is_initialized()) {
>>   		void *rng_seed;
> 
> I just noticed one more thing this code isn't doing compared to the
> powerpc version (sorry to bring these issues piecemeal, I didn't realize
> this before):
> 
> You're not checking whether there already is a device tree property and
> corresponding memory reservation for an IMA kexec buffer that the
> currently running kernel might have received from a previous kernel. In
> that case, this code will do the wrong thing because
> fdt_appendprop_addrrange() will append the range to the existing
> property, which is not what you want. You'll also have a memory
> reservation entry for a stale IMA kexec buffer, which just wastes
> memory.
> 
> So one thing you need to do, whether or not there's an IMA kexec buffer
> to be passed to the next kernel, is to remove any existing
> FDT_PROP_IMA_KEXEC_BUFFER property and also its corresponding memory
> reservation, so that you avoid accumulating stale memory reservations
> for non-existing IMA kexec buffers from previous kexecs.
> 

That's good catch Thiago. Thanks for the feedback. I'll make this change 
and update.

thanks,
  -lakshmi