[v10,5/8] powerpc: Move remove_ima_buffer() to drivers/of/kexec.c

remove_ima_buffer() removes the chosen node "linux,ima-kexec-buffer"
from the device tree and frees the memory reserved for carrying forward
the IMA measurement logs on kexec. This function does not have
architecture specific code, but is currently limited to powerpc.

Move remove_ima_buffer() to "drivers/of/ima_kexec.c" so that it is
accessible for other architectures as well.

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/powerpc/include/asm/ima.h |  8 ++------
 arch/powerpc/kexec/ima.c       | 26 --------------------------
 drivers/of/kexec.c             | 29 +++++++++++++++++++++++++++++
 include/linux/kexec.h          |  1 +
 4 files changed, 32 insertions(+), 32 deletions(-)

Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:

> remove_ima_buffer() removes the chosen node "linux,ima-kexec-buffer"
> from the device tree and frees the memory reserved for carrying forward
> the IMA measurement logs on kexec. This function does not have
> architecture specific code, but is currently limited to powerpc.
>
> Move remove_ima_buffer() to "drivers/of/ima_kexec.c" so that it is

s/ima_kexec./kexec.c/

> accessible for other architectures as well.
>
> 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>

Looks good. Just minor comments below. Nevertheless:

Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

> ---
>  arch/powerpc/include/asm/ima.h |  8 ++------
>  arch/powerpc/kexec/ima.c       | 26 --------------------------
>  drivers/of/kexec.c             | 29 +++++++++++++++++++++++++++++
>  include/linux/kexec.h          |  1 +
>  4 files changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
> index ead488cf3981..a2fc71bc3b23 100644
> --- a/arch/powerpc/include/asm/ima.h
> +++ b/arch/powerpc/include/asm/ima.h
> @@ -2,17 +2,13 @@
>  #ifndef _ASM_POWERPC_IMA_H
>  #define _ASM_POWERPC_IMA_H
>  
> +#include <linux/kexec.h>
> +
>  struct kimage;

When you include <linux/kexec.h>, there's no need anymore for the
`struct kimage` forward declaration so you can remove it.

>  
>  int ima_get_kexec_buffer(void **addr, size_t *size);
>  int ima_free_kexec_buffer(void);
>  
> -#ifdef CONFIG_IMA
> -void remove_ima_buffer(void *fdt, int chosen_node);
> -#else
> -static inline void remove_ima_buffer(void *fdt, int chosen_node) {}
> -#endif
> -

This patch introduces a slight behaviour change on powerpc when
CONFIG_IMA isn't set: since remove_ima_buffer() is now defined,
kexec_file_load() will remove the IMA kexec buffer when creating the FDT
for the next kernel. Before this patch, it will leave it there.

I think it's actually an improvement since a stale IMA kexec buffer
isn't useful and just wastes memory. This should be mentioned on the
commit message.

>  #ifdef CONFIG_IMA_KEXEC
>  int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
>  			      size_t size);
> diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c
> index 906e8212435d..68017123b07d 100644
> --- a/arch/powerpc/kexec/ima.c
> +++ b/arch/powerpc/kexec/ima.c
> @@ -61,32 +61,6 @@ int ima_free_kexec_buffer(void)
>  	return memblock_free(addr, size);
>  }
>  
> -/**
> - * 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.
> - */
> -void remove_ima_buffer(void *fdt, int chosen_node)
> -{
> -	int ret;
> -	unsigned long addr;
> -	size_t size;
> -
> -	ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
> -	if (ret)
> -		return;
> -
> -	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> -
> -	ret = delete_fdt_mem_rsv(fdt, addr, size);
> -	if (!ret)
> -		pr_debug("Removed old IMA buffer reservation.\n");
> -}
> -
>  #ifdef CONFIG_IMA_KEXEC
>  /**
>   * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> index 516b86f7113a..42d16dfff78d 100644
> --- a/drivers/of/kexec.c
> +++ b/drivers/of/kexec.c
> @@ -129,3 +129,32 @@ int get_ima_kexec_buffer(void *fdt, int chosen_node,
>  
>  	return 0;
>  }
> +
> +/**
> + * 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.
> + */
> +void remove_ima_buffer(void *fdt, int chosen_node)
> +{
> +	int ret;
> +	unsigned long addr;
> +	size_t size;
> +
> +	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
> +		return;
> +
> +	ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
> +	if (ret)
> +		return;
> +
> +	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> +
> +	ret = delete_fdt_mem_rsv(fdt, addr, size);
> +	if (!ret)
> +		pr_debug("Removed old IMA buffer reservation.\n");
> +}
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 10ff704ab670..52a0efff184d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -411,6 +411,7 @@ static inline int kexec_crash_loaded(void) { return 0; }
>  extern int get_root_addr_size_cells(int *addr_cells, int *size_cells);
>  extern int get_ima_kexec_buffer(void *fdt, int chosen_node,
>  				unsigned long *addr, size_t *size);
> +extern void remove_ima_buffer(void *fdt, int chosen_node);
>  extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
>  			      unsigned long size);
>  #endif /* CONFIG_OF_FLATTREE */

Same comment as before: remove the `extern` keyword.

On 12/5/20 12:14 PM, Thiago Jung Bauermann wrote:
> 
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> 
>> remove_ima_buffer() removes the chosen node "linux,ima-kexec-buffer"
>> from the device tree and frees the memory reserved for carrying forward
>> the IMA measurement logs on kexec. This function does not have
>> architecture specific code, but is currently limited to powerpc.
>>
>> Move remove_ima_buffer() to "drivers/of/ima_kexec.c" so that it is
> 
> s/ima_kexec./kexec.c/
Will fix it.

> 
>> accessible for other architectures as well.
>>
>> 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>
> 
> Looks good. Just minor comments below. Nevertheless:
> 
> Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
>> ---
>>   arch/powerpc/include/asm/ima.h |  8 ++------
>>   arch/powerpc/kexec/ima.c       | 26 --------------------------
>>   drivers/of/kexec.c             | 29 +++++++++++++++++++++++++++++
>>   include/linux/kexec.h          |  1 +
>>   4 files changed, 32 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
>> index ead488cf3981..a2fc71bc3b23 100644
>> --- a/arch/powerpc/include/asm/ima.h
>> +++ b/arch/powerpc/include/asm/ima.h
>> @@ -2,17 +2,13 @@
>>   #ifndef _ASM_POWERPC_IMA_H
>>   #define _ASM_POWERPC_IMA_H
>>   
>> +#include <linux/kexec.h>
>> +
>>   struct kimage;
> 
> When you include <linux/kexec.h>, there's no need anymore for the
> `struct kimage` forward declaration so you can remove it.

Sure - i'll remove the forward declaration.

> 
>>   
>>   int ima_get_kexec_buffer(void **addr, size_t *size);
>>   int ima_free_kexec_buffer(void);
>>   
>> -#ifdef CONFIG_IMA
>> -void remove_ima_buffer(void *fdt, int chosen_node);
>> -#else
>> -static inline void remove_ima_buffer(void *fdt, int chosen_node) {}
>> -#endif
>> -
> 
> This patch introduces a slight behaviour change on powerpc when
> CONFIG_IMA isn't set: since remove_ima_buffer() is now defined,
> kexec_file_load() will remove the IMA kexec buffer when creating the FDT
> for the next kernel. Before this patch, it will leave it there.
> 
> I think it's actually an improvement since a stale IMA kexec buffer
> isn't useful and just wastes memory. This should be mentioned on the
> commit message.

Thanks Thiago.

Appreciate if you could test this change for powerpc and let me know if 
you see any issues.

>>   #ifdef CONFIG_IMA_KEXEC
>>   int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
>>   			      size_t size);
>> diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c
>> index 906e8212435d..68017123b07d 100644
>> --- a/arch/powerpc/kexec/ima.c
>> +++ b/arch/powerpc/kexec/ima.c
>> @@ -61,32 +61,6 @@ int ima_free_kexec_buffer(void)
>>   	return memblock_free(addr, size);
>>   }
>>   
>> -/**
>> - * 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.
>> - */
>> -void remove_ima_buffer(void *fdt, int chosen_node)
>> -{
>> -	int ret;
>> -	unsigned long addr;
>> -	size_t size;
>> -
>> -	ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
>> -	if (ret)
>> -		return;
>> -
>> -	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
>> -
>> -	ret = delete_fdt_mem_rsv(fdt, addr, size);
>> -	if (!ret)
>> -		pr_debug("Removed old IMA buffer reservation.\n");
>> -}
>> -
>>   #ifdef CONFIG_IMA_KEXEC
>>   /**
>>    * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
>> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
>> index 516b86f7113a..42d16dfff78d 100644
>> --- a/drivers/of/kexec.c
>> +++ b/drivers/of/kexec.c
>> @@ -129,3 +129,32 @@ int get_ima_kexec_buffer(void *fdt, int chosen_node,
>>   
>>   	return 0;
>>   }
>> +
>> +/**
>> + * 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.
>> + */
>> +void remove_ima_buffer(void *fdt, int chosen_node)
>> +{
>> +	int ret;
>> +	unsigned long addr;
>> +	size_t size;
>> +
>> +	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
>> +		return;
>> +
>> +	ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
>> +	if (ret)
>> +		return;
>> +
>> +	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
>> +
>> +	ret = delete_fdt_mem_rsv(fdt, addr, size);
>> +	if (!ret)
>> +		pr_debug("Removed old IMA buffer reservation.\n");
>> +}
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 10ff704ab670..52a0efff184d 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -411,6 +411,7 @@ static inline int kexec_crash_loaded(void) { return 0; }
>>   extern int get_root_addr_size_cells(int *addr_cells, int *size_cells);
>>   extern int get_ima_kexec_buffer(void *fdt, int chosen_node,
>>   				unsigned long *addr, size_t *size);
>> +extern void remove_ima_buffer(void *fdt, int chosen_node);
>>   extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
>>   			      unsigned long size);
>>   #endif /* CONFIG_OF_FLATTREE */
> 
> Same comment as before: remove the `extern` keyword.
> 

Will remove "extern" keyword.

  -lakshmi