Re: [PATCH v6 1/3] powerpc: Refactor kexec functions to move arch independent code to IMA
Hello Lakshmi,
The code itself is good, I have just some minor comments about it.
But movement of powerpc code is unfortunately creating some issues. More
details below.
Lakshmi Ramasubramanian writes:
> diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c
> index 720e50e490b6..467647886064 100644
> --- a/arch/powerpc/kexec/ima.c
> +++ b/arch/powerpc/kexec/ima.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -28,105 +29,6 @@ static int get_addr_size_cells(int *addr_cells, int
> *size_cells)
> return 0;
> }
>
> -static int do_get_kexec_buffer(const void *prop, int len, unsigned long
> *addr,
> -size_t *size)
> -{
> - int ret, addr_cells, size_cells;
> -
> - ret = get_addr_size_cells(_cells, _cells);
> - if (ret)
> - return ret;
> -
> - if (len < 4 * (addr_cells + size_cells))
> - return -ENOENT;
> -
> - *addr = of_read_number(prop, addr_cells);
> - *size = of_read_number(prop + 4 * addr_cells, size_cells);
> -
> - 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 ima_get_kexec_buffer(void **addr, size_t *size)
> -{
> - int ret, len;
> - unsigned long tmp_addr;
> - size_t tmp_size;
> - const void *prop;
> -
> - prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", );
> - if (!prop)
> - return -ENOENT;
> -
> - ret = do_get_kexec_buffer(prop, len, _addr, _size);
> - if (ret)
> - return ret;
> -
> - *addr = __va(tmp_addr);
> - *size = tmp_size;
> -
> - return 0;
> -}
> -
> -/**
> - * ima_free_kexec_buffer - free memory used by the IMA buffer
> - */
> -int ima_free_kexec_buffer(void)
> -{
> - int ret;
> - unsigned long addr;
> - size_t size;
> - struct property *prop;
> -
> - prop = of_find_property(of_chosen, "linux,ima-kexec-buffer", NULL);
> - if (!prop)
> - return -ENOENT;
> -
> - ret = do_get_kexec_buffer(prop->value, prop->length, , );
> - if (ret)
> - return ret;
> -
> - ret = of_remove_property(of_chosen, prop);
> - if (ret)
> - return ret;
> -
> - return memblock_free(addr, size);
> -
> -}
> -
> -/**
> - * remove_ima_buffer - remove the IMA buffer property and reservation from
> @fdt
> - *
> - * 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, len;
> - unsigned long addr;
> - size_t size;
> - const void *prop;
> -
> - prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", );
> - if (!prop)
> - return;
> -
> - ret = do_get_kexec_buffer(prop, len, , );
> - fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> - if (ret)
> - return;
> -
> - ret = delete_fdt_mem_rsv(fdt, addr, size);
> - if (!ret)
> - pr_debug("Removed old IMA buffer reservation.\n");
> -}
> -
With this change if CONFIG_IMA=y but CONFIG_IMA_KEXEC=n, this file will
only have static int get_addr_size_cells() as its contents, which isn't
useful (and will print a warning about the function not being used). You
should change the Makefile to only build this file if CONFIG_IMA_KEXEC=y.
Then you can also remove the #ifdef CONFIG_IMA_KEXEC/#endif within it as
well.
> #ifdef CONFIG_IMA_KEXEC
> /**
> * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
> @@ -179,7 +81,7 @@ int setup_ima_buffer(const struct kimage *image, void
> *fdt, int chosen_node)
> int ret, addr_cells, size_cells, entry_size;
> u8 value[16];
>
> - remove_ima_buffer(fdt, chosen_node);
> + ima_remove_kexec_buffer(fdt, chosen_node);
> if (!image->arch.ima_buffer_size)
> return 0;
>
> @@ -201,7 +103,7 @@ int setup_ima_buffer(const struct kimage *image, void
> *fdt, int chosen_node)
> if (ret)
> return ret;
>
> - ret = fdt_setprop(fdt, chosen_node, "linux,ima-kexec-buffer", value,
> + ret = fdt_setprop(fdt, chosen_node, FDT_PROP_IMA_KEXEC_BUFFER, value,
> entry_size);
> if (ret < 0)
> return -EINVAL;
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 9e93bef52968..00a60dcc7075 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -223,8 +223,19 @@ extern int crash_exclude_mem_range(struct crash_mem *mem,
> unsigned long long mend);
> extern int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
> void **addr,
[PATCH v6 1/3] powerpc: Refactor kexec functions to move arch independent code to IMA
The functions ima_get_kexec_buffer(), ima_free_kexec_buffer(),
remove_ima_buffer(), and delete_fdt_mem_rsv() that handle carrying
forward the IMA measurement logs on kexec for powerpc do not have
architecture specific code, but they are currently defined for
powerpc only.
Move these functions to IMA subsystem so that it can be used for other
architectures as well. A later patch in this series will use these
functions for carrying forward the IMA measurement log for ARM64.
Rename remove_ima_buffer() to ima_remove_kexec_buffer().
Define FDT_PROP_IMA_KEXEC_BUFFER for the chosen node, namely
"linux,ima-kexec-buffer", that is added to the DTB to hold
the address and the size of the memory reserved to carry
the IMA measurement log.
Co-developed-by: Prakhar Srivastava
Signed-off-by: Prakhar Srivastava
Signed-off-by: Lakshmi Ramasubramanian
---
arch/powerpc/include/asm/ima.h | 11 +--
arch/powerpc/include/asm/kexec.h| 1 -
arch/powerpc/kexec/file_load.c | 33 +---
arch/powerpc/kexec/ima.c| 104 +---
include/linux/ima.h | 2 +
include/linux/kexec.h | 11 +++
include/linux/libfdt.h | 3 +
security/integrity/ima/Makefile | 3 +-
security/integrity/ima/ima.h| 2 +
security/integrity/ima/ima_fdt.c| 80 ++
security/integrity/ima/ima_kexec.c | 58 +
security/integrity/ima/ima_kexec_file.c | 51
12 files changed, 214 insertions(+), 145 deletions(-)
create mode 100644 security/integrity/ima/ima_fdt.c
create mode 100644 security/integrity/ima/ima_kexec_file.c
diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
index ead488cf3981..188146ecbfaf 100644
--- a/arch/powerpc/include/asm/ima.h
+++ b/arch/powerpc/include/asm/ima.h
@@ -4,15 +4,6 @@
struct kimage;
-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
-
#ifdef CONFIG_IMA_KEXEC
int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
size_t size);
@@ -22,7 +13,7 @@ int setup_ima_buffer(const struct kimage *image, void *fdt,
int chosen_node);
static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
int chosen_node)
{
- remove_ima_buffer(fdt, chosen_node);
+ ima_remove_kexec_buffer(fdt, chosen_node);
return 0;
}
#endif /* CONFIG_IMA_KEXEC */
diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
index 55d6ede30c19..7c223031ecdd 100644
--- a/arch/powerpc/include/asm/kexec.h
+++ b/arch/powerpc/include/asm/kexec.h
@@ -126,7 +126,6 @@ int setup_purgatory(struct kimage *image, const void
*slave_code,
int setup_new_fdt(const struct kimage *image, void *fdt,
unsigned long initrd_load_addr, unsigned long initrd_len,
const char *cmdline);
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size);
#ifdef CONFIG_PPC64
struct kexec_buf;
diff --git a/arch/powerpc/kexec/file_load.c b/arch/powerpc/kexec/file_load.c
index 9a232bc36c8f..7a17655c530e 100644
--- a/arch/powerpc/kexec/file_load.c
+++ b/arch/powerpc/kexec/file_load.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -109,38 +110,6 @@ int setup_purgatory(struct kimage *image, const void
*slave_code,
return 0;
}
-/**
- * delete_fdt_mem_rsv - delete memory reservation with given address and size
- *
- * Return: 0 on success, or negative errno on error.
- */
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)
-{
- int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
-
- for (i = 0; i < num_rsvs; i++) {
- uint64_t rsv_start, rsv_size;
-
- ret = fdt_get_mem_rsv(fdt, i, _start, _size);
- if (ret) {
- pr_err("Malformed device tree.\n");
- return -EINVAL;
- }
-
- if (rsv_start == start && rsv_size == size) {
- ret = fdt_del_mem_rsv(fdt, i);
- if (ret) {
- pr_err("Error deleting device tree
reservation.\n");
- return -EINVAL;
- }
-
- return 0;
- }
- }
-
- return -ENOENT;
-}
-
/*
* setup_new_fdt - modify /chosen and memory reservation for the next kernel
* @image: kexec image being loaded.
diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c
index 720e50e490b6..467647886064 100644
--- a/arch/powerpc/kexec/ima.c
+++ b/arch/powerpc/kexec/ima.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
