Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
Looks good. Just minor comments below. Nevertheless:
Reviewed-by: Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxx>
> ---
> 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.
--
Thiago Jung Bauermann
IBM Linux Technology Center
