Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c002e44a-826a-ea1a-8720-e8dea05a26ad@linux.ibm.com>
Date:   Wed, 24 May 2023 20:23:50 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v9 3/4] of: kexec: Refactor IMA buffer related functions
 to make them reusable
Content-Language: en-US
To:     Jerry Snitselaar <jsnitsel@redhat.com>
Cc:     kexec@lists.infradead.org, devicetree@vger.kernel.org,
        linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org, nayna@linux.ibm.com,
        nasastry@in.ibm.com, mpe@ellerman.id.au,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Mimi Zohar <zohar@linux.ibm.com>,
        Rob Herring <robh@kernel.org>,
        Nageswara R Sastry <rnsastry@linux.ibm.com>,
        Coiby Xu <coxu@redhat.com>
References: <20230418134409.177485-1-stefanb@linux.ibm.com>
 <20230418134409.177485-4-stefanb@linux.ibm.com>
 <m3gasjgsxlggzsayurbzjaeiallxeobtggtzifnivqfyvexqn6@7rv7oumuq46x>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <m3gasjgsxlggzsayurbzjaeiallxeobtggtzifnivqfyvexqn6@7rv7oumuq46x>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk


On 5/24/23 19:16, Jerry Snitselaar wrote:
> On Tue, Apr 18, 2023 at 09:44:08AM -0400, Stefan Berger wrote:
>> Refactor IMA buffer related functions to make them reusable for carrying
>> TPM logs across kexec.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Frank Rowand <frowand.list@gmail.com>
>> Cc: Mimi Zohar <zohar@linux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
>> Tested-by: Coiby Xu <coxu@redhat.com>
>>
> 
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Thanks a lot for looking at the patches. Unfortunately this series only resolves the issue with the newer kexec call but we have seen systems where the older one is used for setting a crash kernel and when the crash happens we're back to square one. I have been trying to embed the log into the memory of the flattened of-device tree but that also turns out to be not so straight forward.

     Stefan
>> ---
>> v6:
>>   - Add __init to get_kexec_buffer as suggested by Jonathan
>>
>> v5:
>>   - Rebased on Jonathan McDowell's commit "b69a2afd5afc x86/kexec: Carry
>>     forward IMA measurement log on kexec"
>> v4:
>>   - Move debug output into setup_buffer()
>> ---
>>   drivers/of/kexec.c | 126 ++++++++++++++++++++++++++-------------------
>>   1 file changed, 74 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
>> index 1373d7e0a9b3..fa8c0c75adf9 100644
>> --- a/drivers/of/kexec.c
>> +++ b/drivers/of/kexec.c
>> @@ -117,45 +117,57 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
>>   }
>>   
>>   #ifdef CONFIG_HAVE_IMA_KEXEC
>> -/**
>> - * ima_get_kexec_buffer - get IMA buffer from the previous kernel
>> - * @addr:	On successful return, set to point to the buffer contents.
>> - * @size:	On successful return, set to the buffer size.
>> - *
>> - * Return: 0 on success, negative errno on error.
>> - */
>> -int __init ima_get_kexec_buffer(void **addr, size_t *size)
>> +static int __init get_kexec_buffer(const char *name, unsigned long *addr,
>> +				   size_t *size)
>>   {
>>   	int ret, len;
>> -	unsigned long tmp_addr;
>>   	unsigned long start_pfn, end_pfn;
>> -	size_t tmp_size;
>>   	const void *prop;
>>   
>> -	prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len);
>> +	prop = of_get_property(of_chosen, name, &len);
>>   	if (!prop)
>>   		return -ENOENT;
>>   
>> -	ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
>> +	ret = do_get_kexec_buffer(prop, len, addr, size);
>>   	if (ret)
>>   		return ret;
>>   
>> -	/* Do some sanity on the returned size for the ima-kexec buffer */
>> -	if (!tmp_size)
>> +	/* Do some sanity on the returned size for the kexec buffer */
>> +	if (!*size)
>>   		return -ENOENT;
>>   
>>   	/*
>>   	 * Calculate the PFNs for the buffer and ensure
>>   	 * they are with in addressable memory.
>>   	 */
>> -	start_pfn = PHYS_PFN(tmp_addr);
>> -	end_pfn = PHYS_PFN(tmp_addr + tmp_size - 1);
>> +	start_pfn = PHYS_PFN(*addr);
>> +	end_pfn = PHYS_PFN(*addr + *size - 1);
>>   	if (!page_is_ram(start_pfn) || !page_is_ram(end_pfn)) {
>> -		pr_warn("IMA buffer at 0x%lx, size = 0x%zx beyond memory\n",
>> -			tmp_addr, tmp_size);
>> +		pr_warn("%s buffer at 0x%lx, size = 0x%zx beyond memory\n",
>> +			name, *addr, *size);
>>   		return -EINVAL;
>>   	}
>>   
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ima_get_kexec_buffer - get IMA buffer from the previous kernel
>> + * @addr:	On successful return, set to point to the buffer contents.
>> + * @size:	On successful return, set to the buffer size.
>> + *
>> + * Return: 0 on success, negative errno on error.
>> + */
>> +int __init ima_get_kexec_buffer(void **addr, size_t *size)
>> +{
>> +	int ret;
>> +	unsigned long tmp_addr;
>> +	size_t tmp_size;
>> +
>> +	ret = get_kexec_buffer("linux,ima-kexec-buffer", &tmp_addr, &tmp_size);
>> +	if (ret)
>> +		return ret;
>> +
>>   	*addr = __va(tmp_addr);
>>   	*size = tmp_size;
>>   
>> @@ -188,72 +200,82 @@ int __init ima_free_kexec_buffer(void)
>>   }
>>   #endif
>>   
>> -/**
>> - * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
>> - *
>> - * @fdt: Flattened Device Tree to update
>> - * @chosen_node: Offset to the chosen node in the device tree
>> - *
>> - * The IMA measurement buffer is of no use to a subsequent kernel, so we always
>> - * remove it from the device tree.
>> - */
>> -static void remove_ima_buffer(void *fdt, int chosen_node)
>> +static int remove_buffer(void *fdt, int chosen_node, const char *name)
>>   {
>>   	int ret, len;
>>   	unsigned long addr;
>>   	size_t size;
>>   	const void *prop;
>>   
>> -	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
>> -		return;
>> -
>> -	prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
>> +	prop = fdt_getprop(fdt, chosen_node, name, &len);
>>   	if (!prop)
>> -		return;
>> +		return -ENOENT;
>>   
>>   	ret = do_get_kexec_buffer(prop, len, &addr, &size);
>> -	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
>> +	fdt_delprop(fdt, chosen_node, name);
>>   	if (ret)
>> -		return;
>> +		return ret;
>>   
>>   	ret = fdt_find_and_del_mem_rsv(fdt, addr, size);
>>   	if (!ret)
>> -		pr_debug("Removed old IMA buffer reservation.\n");
>> +		pr_debug("Remove old %s buffer reserveration", name);
>> +	return ret;
>>   }
>>   
>> -#ifdef CONFIG_IMA_KEXEC
>>   /**
>> - * setup_ima_buffer - add IMA buffer information to the fdt
>> - * @image:		kexec image being loaded.
>> - * @fdt:		Flattened device tree for the next kernel.
>> - * @chosen_node:	Offset to the chosen node.
>> + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
>>    *
>> - * Return: 0 on success, or negative errno on error.
>> + * @fdt: Flattened Device Tree to update
>> + * @chosen_node: Offset to the chosen node in the device tree
>> + *
>> + * The IMA measurement buffer is of no use to a subsequent kernel, so we always
>> + * remove it from the device tree.
>>    */
>> -static int setup_ima_buffer(const struct kimage *image, void *fdt,
>> -			    int chosen_node)
>> +static void remove_ima_buffer(void *fdt, int chosen_node)
>> +{
>> +	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
>> +		return;
>> +
>> +	remove_buffer(fdt, chosen_node, "linux,ima-kexec-buffer");
>> +}
>> +
>> +#ifdef CONFIG_IMA_KEXEC
>> +static int setup_buffer(void *fdt, int chosen_node, const char *name,
>> +			phys_addr_t addr, size_t size)
>>   {
>>   	int ret;
>>   
>> -	if (!image->ima_buffer_size)
>> +	if (!size)
>>   		return 0;
>>   
>>   	ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
>> -				       "linux,ima-kexec-buffer",
>> -				       image->ima_buffer_addr,
>> -				       image->ima_buffer_size);
>> +				       name, addr, size);
>>   	if (ret < 0)
>>   		return -EINVAL;
>>   
>> -	ret = fdt_add_mem_rsv(fdt, image->ima_buffer_addr,
>> -			      image->ima_buffer_size);
>> +	ret = fdt_add_mem_rsv(fdt, addr, size);
>>   	if (ret)
>>   		return -EINVAL;
>>   
>> -	pr_debug("IMA buffer at 0x%pa, size = 0x%zx\n",
>> -		 &image->ima_buffer_addr, image->ima_buffer_size);
>> +	pr_debug("%s at 0x%pa, size = 0x%zx\n", name, &addr, size);
>>   
>>   	return 0;
>> +
>> +}
>> +
>> +/**
>> + * setup_ima_buffer - add IMA buffer information to the fdt
>> + * @image:		kexec image being loaded.
>> + * @fdt:		Flattened device tree for the next kernel.
>> + * @chosen_node:	Offset to the chosen node.
>> + *
>> + * Return: 0 on success, or negative errno on error.
>> + */
>> +static int setup_ima_buffer(const struct kimage *image, void *fdt,
>> +			    int chosen_node)
>> +{
>> +	return setup_buffer(fdt, chosen_node, "linux,ima-kexec-buffer",
>> +			    image->ima_buffer_addr, image->ima_buffer_size);
>>   }
>>   #else /* CONFIG_IMA_KEXEC */
>>   static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
>> -- 
>> 2.38.1
>>
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec