Integrgity Measurement Architecture(IMA) during kexec(kexec file load)
verifies the kernel signature and measures the signature of the kernel.
The signature in the measuremnt logs is used to verfiy the
authenticity of the kernel in the subsequent kexec'd session, however in
the current implementation IMA measurement logs are not carried over thus
remote attesation cannot verify the signature of the running kernel.
Adding support to arm64 to carry over the IMA measurement logs over kexec.
Add a new chosen node entry linux,ima-kexec-buffer to hold the address and
the size of the memory reserved to carry the IMA measurement log.
Refactor existing powerpc code to be used by amr64 as well.
Changelog:
v2:
Break patches into separate patches.
- Powerpc related Refactoring
- Updating the docuemntation for chosen node
- Updating arm64 to support IMA buffer pass
v1:
Refactoring carrying over IMA measuremnet logs over Kexec. This patch
moves the non-architecture specific code out of powerpc and adds to
security/ima.(Suggested by Thiago)
Add Documentation regarding the ima-kexec-buffer node in the chosen
node documentation
v0:
Add a layer of abstraction to use the memory reserved by device tree
for ima buffer pass.
Add support for ima buffer pass using reserved memory for arm64 kexec.
Update the arch sepcific code path in kexec file load to store the
ima buffer in the reserved memory. The same reserved memory is read
on kexec or cold boot.
Prakhar Srivastava (3):
Refactoring powerpc code for carrying over IMA measurement logs, to
move non architecture specific code to security/ima.
dt-bindings: chosen: Document ima-kexec-buffer carrying over IMA
measuremnt logs over kexec.
Add support for arm64 to carry over IMA measurement logs
Documentation/devicetree/bindings/chosen.txt | 17 +++
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/ima.h | 17 +++
arch/arm64/include/asm/kexec.h | 3 +
arch/arm64/kernel/machine_kexec_file.c | 47 +++++--
arch/powerpc/include/asm/ima.h | 10 --
arch/powerpc/kexec/ima.c | 126 ++-----------------
security/integrity/ima/ima_kexec.c | 116 +++++++++++++++++
8 files changed, 201 insertions(+), 136 deletions(-)
create mode 100644 arch/arm64/include/asm/ima.h
--
2.25.1
Integrity measurement architecture(IMA) validates if files
have been accidentally or maliciously altered, both remotely and
locally, appraise a file's measurement against a "good" value stored
as an extended attribute, and enforce local file integrity.
IMA also measures singatures of kernel and initrd during kexec along with
the command line used for kexec.
These measurements are critical to verify the seccurity posture of the OS.
Resering memory and adding the memory information to a device tree node
acts as the mechanism to carry over IMA measurement logs.
Update devicetree documentation to reflect the addition of new property
under the chosen node.
---
Documentation/devicetree/bindings/chosen.txt | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
index 45e79172a646..a15f70c007ef 100644
--- a/Documentation/devicetree/bindings/chosen.txt
+++ b/Documentation/devicetree/bindings/chosen.txt
@@ -135,3 +135,20 @@ e.g.
linux,initrd-end = <0x82800000>;
};
};
+
+linux,ima-kexec-buffer
+----------------------
+
+This property(currently used by powerpc, arm64) holds the memory range,
+the address and the size, of the IMA measurement logs that are being carried
+over to the kexec session.
+
+/ {
+ chosen {
+ linux,ima-kexec-buffer = <0x9 0x82000000 0x0 0x00008000>;
+ };
+};
+
+This porperty does not represent real hardware, but the memory allocated for
+carrying the IMA measurement logs. The address and the suze are expressed in
+#address-cells and #size-cells, respectively of the root node.
--
2.25.1
Add support for arm64 to carry over IMA measurement logs.
Update arm64 code to call into functions made available in patch 1/3.
---
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/ima.h | 17 ++++++++++
arch/arm64/include/asm/kexec.h | 3 ++
arch/arm64/kernel/machine_kexec_file.c | 47 +++++++++++++++++++++-----
4 files changed, 60 insertions(+), 8 deletions(-)
create mode 100644 arch/arm64/include/asm/ima.h
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5d513f461957..3d544e2e25e6 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1070,6 +1070,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/include/asm/ima.h b/arch/arm64/include/asm/ima.h
new file mode 100644
index 000000000000..70ac39b74607
--- /dev/null
+++ b/arch/arm64/include/asm/ima.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARCH_IMA_H
+#define _ASM_ARCH_IMA_H
+
+struct kimage;
+
+#ifdef CONFIG_IMA_KEXEC
+int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
+ size_t size);
+#else
+static inline int arch_ima_add_kexec_buffer(struct kimage *image,
+ unsigned long load_addr, size_t size)
+{
+ return 0;
+}
+#endif /* CONFIG_IMA_KEXEC */
+#endif /* _ASM_ARCH_IMA_H */
diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index d24b527e8c00..7bd60c185ad3 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -100,6 +100,9 @@ struct kimage_arch {
void *elf_headers;
unsigned long elf_headers_mem;
unsigned long elf_headers_sz;
+
+ phys_addr_t ima_buffer_addr;
+ size_t ima_buffer_size;
};
extern const struct kexec_file_ops kexec_image_ops;
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index b40c3b0def92..1e9007c926db 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -24,20 +24,37 @@
#include <asm/byteorder.h>
/* relevant device tree properties */
-#define FDT_PROP_KEXEC_ELFHDR "linux,elfcorehdr"
-#define FDT_PROP_MEM_RANGE "linux,usable-memory-range"
-#define FDT_PROP_INITRD_START "linux,initrd-start"
-#define FDT_PROP_INITRD_END "linux,initrd-end"
-#define FDT_PROP_BOOTARGS "bootargs"
-#define FDT_PROP_KASLR_SEED "kaslr-seed"
-#define FDT_PROP_RNG_SEED "rng-seed"
-#define RNG_SEED_SIZE 128
+#define FDT_PROP_KEXEC_ELFHDR "linux,elfcorehdr"
+#define FDT_PROP_MEM_RANGE "linux,usable-memory-range"
+#define FDT_PROP_INITRD_START "linux,initrd-start"
+#define FDT_PROP_INITRD_END "linux,initrd-end"
+#define FDT_PROP_BOOTARGS "bootargs"
+#define FDT_PROP_KASLR_SEED "kaslr-seed"
+#define FDT_PROP_RNG_SEED "rng-seed"
+#define FDT_PROP_IMA_KEXEC_BUFFER "linux,ima-kexec-buffer"
+#define RNG_SEED_SIZE 128
const struct kexec_file_ops * const kexec_file_loaders[] = {
&kexec_image_ops,
NULL
};
+/**
+ * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
+ *
+ * Architectures should use this function to pass on the IMA buffer
+ * information to the next kernel.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
+ size_t size)
+{
+ image->arch.ima_buffer_addr = load_addr;
+ image->arch.ima_buffer_size = size;
+ return 0;
+}
+
int arch_kimage_file_post_load_cleanup(struct kimage *image)
{
vfree(image->arch.dtb);
@@ -66,6 +83,9 @@ static int setup_dtb(struct kimage *image,
if (ret && ret != -FDT_ERR_NOTFOUND)
goto out;
ret = fdt_delprop(dtb, off, FDT_PROP_MEM_RANGE);
+ if (ret && ret != -FDT_ERR_NOTFOUND)
+ goto out;
+ ret = fdt_delprop(dtb, off, FDT_PROP_IMA_KEXEC_BUFFER);
if (ret && ret != -FDT_ERR_NOTFOUND)
goto out;
@@ -119,6 +139,17 @@ static int setup_dtb(struct kimage *image,
goto out;
}
+ 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);
+
+ }
+
/* add kaslr-seed */
ret = fdt_delprop(dtb, off, FDT_PROP_KASLR_SEED);
if (ret == -FDT_ERR_NOTFOUND)
--
2.25.1
Powerpc has support to carry over the IMA measurement logs. Refatoring the
non-architecture specific code out of arch/powerpc and into security/ima.
The code adds support for reserving and freeing up of memory for IMA measurement
logs.
---
arch/powerpc/include/asm/ima.h | 10 ---
arch/powerpc/kexec/ima.c | 126 ++---------------------------
security/integrity/ima/ima_kexec.c | 116 ++++++++++++++++++++++++++
3 files changed, 124 insertions(+), 128 deletions(-)
diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
index ead488cf3981..c29ec86498f8 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,6 @@ 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);
return 0;
}
#endif /* CONFIG_IMA_KEXEC */
diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c
index 720e50e490b6..6054ce91d2a6 100644
--- a/arch/powerpc/kexec/ima.c
+++ b/arch/powerpc/kexec/ima.c
@@ -12,121 +12,6 @@
#include <linux/memblock.h>
#include <linux/libfdt.h>
-static int get_addr_size_cells(int *addr_cells, int *size_cells)
-{
- struct device_node *root;
-
- root = of_find_node_by_path("/");
- if (!root)
- return -EINVAL;
-
- *addr_cells = of_n_addr_cells(root);
- *size_cells = of_n_size_cells(root);
-
- of_node_put(root);
-
- 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(&addr_cells, &size_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", &len);
- if (!prop)
- return -ENOENT;
-
- ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_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, &addr, &size);
- 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", &len);
- if (!prop)
- return;
-
- ret = do_get_kexec_buffer(prop, len, &addr, &size);
- 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");
-}
-
#ifdef CONFIG_IMA_KEXEC
/**
* arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
@@ -179,13 +64,18 @@ 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);
if (!image->arch.ima_buffer_size)
return 0;
- ret = get_addr_size_cells(&addr_cells, &size_cells);
- if (ret)
+ ret = fdt_address_cells(fdt, chosen_node);
+ if (ret < 0)
+ return ret;
+ addr_cells = ret;
+
+ ret = fdt_size_cells(fdt, chosen_node);
+ if (ret < 0)
return ret;
+ size_cells = ret;
entry_size = 4 * (addr_cells + size_cells);
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 121de3e04af2..e1e6d6154015 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -10,8 +10,124 @@
#include <linux/seq_file.h>
#include <linux/vmalloc.h>
#include <linux/kexec.h>
+#include <linux/of.h>
+#include <linux/memblock.h>
+#include <linux/libfdt.h>
#include "ima.h"
+static int get_addr_size_cells(int *addr_cells, int *size_cells)
+{
+ struct device_node *root;
+
+ root = of_find_node_by_path("/");
+ if (!root)
+ return -EINVAL;
+
+ *addr_cells = of_n_addr_cells(root);
+ *size_cells = of_n_size_cells(root);
+
+ of_node_put(root);
+
+ 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(&addr_cells, &size_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", &len);
+ if (!prop)
+ return -ENOENT;
+
+ ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_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, &addr, &size);
+ if (ret)
+ return ret;
+
+ ret = of_remove_property(of_chosen, prop);
+ if (ret)
+ return ret;
+
+ return memblock_free(__pa(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", &len);
+ if (!prop)
+ return;
+
+ do_get_kexec_buffer(prop, len, &addr, &size);
+ ret = fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
+ if (ret < 0)
+ return;
+
+ memblock_free(addr, size);
+}
+
#ifdef CONFIG_IMA_KEXEC
static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
unsigned long segment_size)
--
2.25.1
Prakhar Srivastava <[email protected]> writes:
> Powerpc has support to carry over the IMA measurement logs. Refatoring the
> non-architecture specific code out of arch/powerpc and into security/ima.
>
> The code adds support for reserving and freeing up of memory for IMA measurement
> logs.
Last week, Mimi provided this feedback:
"From your patch description, this patch should be broken up. Moving
the non-architecture specific code out of powerpc should be one patch.
Additional support should be in another patch. After each patch, the
code should work properly."
That's not what you do here. You move the code, but you also make other
changes at the same time. This has two problems:
1. It makes the patch harder to review, because it's very easy to miss a
change.
2. If in the future a git bisect later points to this patch, it's not
clear whether the problem is because of the code movement, or because
of the other changes.
When you move code, ideally the patch should only make the changes
necessary to make the code work at its new location. The patch which
does code movement should not cause any change in behavior.
Other changes should go in separate patches, either before or after the
one moving the code.
More comments below.
>
> ---
> arch/powerpc/include/asm/ima.h | 10 ---
> arch/powerpc/kexec/ima.c | 126 ++---------------------------
> security/integrity/ima/ima_kexec.c | 116 ++++++++++++++++++++++++++
> 3 files changed, 124 insertions(+), 128 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
> index ead488cf3981..c29ec86498f8 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,6 @@ 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);
> return 0;
> }
This is wrong. Even if the currently running kernel doesn't have
CONFIG_IMA_KEXEC, it should remove the IMA buffer property and memory
reservation from the FDT that is being prepared for the next kernel.
This is because the IMA kexec buffer is useless for the next kernel,
regardless of whether the current kernel supports CONFIG_IMA_KEXEC or
not. Keeping it around would be a waste of memory.
> @@ -179,13 +64,18 @@ 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);
This is wrong, for the same reason stated above.
> if (!image->arch.ima_buffer_size)
> return 0;
>
> - ret = get_addr_size_cells(&addr_cells, &size_cells);
> - if (ret)
> + ret = fdt_address_cells(fdt, chosen_node);
> + if (ret < 0)
> + return ret;
> + addr_cells = ret;
> +
> + ret = fdt_size_cells(fdt, chosen_node);
> + if (ret < 0)
> return ret;
> + size_cells = ret;
>
> entry_size = 4 * (addr_cells + size_cells);
>
I liked this change. Thanks! I agree it's better to use
fdt_address_cells() and fdt_size_cells() here.
But it should be in a separate patch. Either before or after the one
moving the code.
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 121de3e04af2..e1e6d6154015 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -10,8 +10,124 @@
> #include <linux/seq_file.h>
> #include <linux/vmalloc.h>
> #include <linux/kexec.h>
> +#include <linux/of.h>
> +#include <linux/memblock.h>
> +#include <linux/libfdt.h>
> #include "ima.h"
>
> +static int get_addr_size_cells(int *addr_cells, int *size_cells)
> +{
> + struct device_node *root;
> +
> + root = of_find_node_by_path("/");
> + if (!root)
> + return -EINVAL;
> +
> + *addr_cells = of_n_addr_cells(root);
> + *size_cells = of_n_size_cells(root);
> +
> + of_node_put(root);
> +
> + 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(&addr_cells, &size_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", &len);
> + if (!prop)
> + return -ENOENT;
> +
> + ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
> + if (ret)
> + return ret;
> +
> + *addr = __va(tmp_addr);
> + *size = tmp_size;
> +
> + return 0;
> +}
The functions above were moved without being changed. Good.
> +/**
> + * 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, &addr, &size);
> + if (ret)
> + return ret;
> +
> + ret = of_remove_property(of_chosen, prop);
> + if (ret)
> + return ret;
> +
> + return memblock_free(__pa(addr), size);
Here you added a __pa() call. Do you store a virtual address in
linux,ima-kexec-buffer property? Doesn't it make more sense to store a
physical address?
Even if making this change is the correct thing to do, it should be a
separate patch, unless it can't be avoided. And if that is the case,
then it should be explained in the commit message.
> +
> +}
> +
> +/**
> + * 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", &len);
> + if (!prop)
> + return;
> +
> + do_get_kexec_buffer(prop, len, &addr, &size);
> + ret = fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> + if (ret < 0)
> + return;
> +
> + memblock_free(addr, size);
> +}
Here is another function that changed when moved. This one I know to be
wrong. You're confusing the purposes of remove_ima_buffer() and
ima_free_kexec_buffer().
You did send me a question about them nearly three weeks ago which I
only answered today, so I apologize. Also, their names could more
clearly reflect their differences, so it's bad naming on my part.
With IMA kexec buffers, there are two kernels (and thus their two
respective, separate device trees) to be concerned about:
1. the currently running kernel, which uses the live device tree
(accessed with the of_* functions) and the memblock subsystem;
2. the kernel which is being loaded by kexec, which will use the FDT
blob being passed around as argument to these functions, and the memory
reservations in the memory reservation table of the FDT blob.
ima_free_kexec_buffer() is used by IMA in the currently running kernel.
Therefore the device tree it is concerned about is the live one, and
thus uses the of_* functions to access it. And uses memblock to change
the memory reservation.
remove_ima_buffer() on the other hand is used by the kexec code to
prepare an FDT blob for the kernel that is being loaded. It should not
make any changes to live device tree, nor to memblock allocations. It
should only make changes to the FDT blob.
--
Thiago Jung Bauermann
IBM Linux Technology Center
Prakhar Srivastava <[email protected]> writes:
> Integrity measurement architecture(IMA) validates if files
> have been accidentally or maliciously altered, both remotely and
> locally, appraise a file's measurement against a "good" value stored
> as an extended attribute, and enforce local file integrity.
>
> IMA also measures singatures of kernel and initrd during kexec along with
> the command line used for kexec.
> These measurements are critical to verify the seccurity posture of the OS.
>
> Resering memory and adding the memory information to a device tree node
> acts as the mechanism to carry over IMA measurement logs.
>
> Update devicetree documentation to reflect the addition of new property
> under the chosen node.
Thank you for writing this documentation patch. It's something I should
have done when I added the powerpc IMA kexec support.
You addressed Rob Herring's comments regarding the commit message, but
not the ones regarding the patch contents.
When posting a new version of the patches, make sure to address all
comments made so far. Addressing a comment doesn't necessarily mean
implementing the requested change. If you don't then you should at least
explain why you chose a different path.
I mention it because this has occurred before with this patch series,
and it's hard to make forward progress if review comments get ignored.
> ---
> Documentation/devicetree/bindings/chosen.txt | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> index 45e79172a646..a15f70c007ef 100644
> --- a/Documentation/devicetree/bindings/chosen.txt
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -135,3 +135,20 @@ e.g.
> linux,initrd-end = <0x82800000>;
> };
> };
> +
> +linux,ima-kexec-buffer
> +----------------------
> +
> +This property(currently used by powerpc, arm64) holds the memory range,
space before the parenthesis.
> +the address and the size, of the IMA measurement logs that are being carried
Maybe it's because English isn't my first language, but IMHO it's
clearer if "the address and the size" is between parentheses rather than
commas.
> +over to the kexec session.
I don't think there's a "kexec session", but I'm not sure what a good
term would be. "linux,booted-from-kexec" uses "new kernel" so perhaps
that's a good option to use instead of "kexec session".
> +
> +/ {
> + chosen {
> + linux,ima-kexec-buffer = <0x9 0x82000000 0x0 0x00008000>;
> + };
> +};
> +
> +This porperty does not represent real hardware, but the memory allocated for
> +carrying the IMA measurement logs. The address and the suze are expressed in
> +#address-cells and #size-cells, respectively of the root node.
--
Thiago Jung Bauermann
IBM Linux Technology Center
On 6/19/20 5:19 PM, Thiago Jung Bauermann wrote:
>
> Prakhar Srivastava <[email protected]> writes:
>
>> Powerpc has support to carry over the IMA measurement logs. Refatoring the
>> non-architecture specific code out of arch/powerpc and into security/ima.
>>
>> The code adds support for reserving and freeing up of memory for IMA measurement
>> logs.
>
> Last week, Mimi provided this feedback:
>
> "From your patch description, this patch should be broken up. Moving
> the non-architecture specific code out of powerpc should be one patch.
> Additional support should be in another patch. After each patch, the
> code should work properly."
>
> That's not what you do here. You move the code, but you also make other
> changes at the same time. This has two problems:
>
> 1. It makes the patch harder to review, because it's very easy to miss a
> change.
>
> 2. If in the future a git bisect later points to this patch, it's not
> clear whether the problem is because of the code movement, or because
> of the other changes.
>
> When you move code, ideally the patch should only make the changes
> necessary to make the code work at its new location. The patch which
> does code movement should not cause any change in behavior.
>
> Other changes should go in separate patches, either before or after the
> one moving the code.
>
> More comments below.
>
Hi Thiago,
Apologies for the delayed response i was away for a few days.
I am working on breaking up the changes so that its easier to review and
update as well.
Thanks,
Prakhar Srivastava
>>
>> ---
>> arch/powerpc/include/asm/ima.h | 10 ---
>> arch/powerpc/kexec/ima.c | 126 ++---------------------------
>> security/integrity/ima/ima_kexec.c | 116 ++++++++++++++++++++++++++
>> 3 files changed, 124 insertions(+), 128 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
>> index ead488cf3981..c29ec86498f8 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,6 @@ 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);
>> return 0;
>> }
>
> This is wrong. Even if the currently running kernel doesn't have
> CONFIG_IMA_KEXEC, it should remove the IMA buffer property and memory
> reservation from the FDT that is being prepared for the next kernel.
>
> This is because the IMA kexec buffer is useless for the next kernel,
> regardless of whether the current kernel supports CONFIG_IMA_KEXEC or
> not. Keeping it around would be a waste of memory.
>
I will keep it in my next revision.
My understanding was the reserved memory is freed and property removed
when IMA loads the logs on init. During setup_fdt in kexec, a duplicate
copy of the dt is used, but memory still needs to be allocated, thus the
property itself indicats presence of reserved memory.
>> @@ -179,13 +64,18 @@ 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);
>
> This is wrong, for the same reason stated above.
>
>> if (!image->arch.ima_buffer_size)
>> return 0;
>>
>> - ret = get_addr_size_cells(&addr_cells, &size_cells);
>> - if (ret)
>> + ret = fdt_address_cells(fdt, chosen_node);
>> + if (ret < 0)
>> + return ret;
>> + addr_cells = ret;
>> +
>> + ret = fdt_size_cells(fdt, chosen_node);
>> + if (ret < 0)
>> return ret;
>> + size_cells = ret;
>>
>> entry_size = 4 * (addr_cells + size_cells);
>>
>
> I liked this change. Thanks! I agree it's better to use
> fdt_address_cells() and fdt_size_cells() here.
>
> But it should be in a separate patch. Either before or after the one
> moving the code.
>
>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>> index 121de3e04af2..e1e6d6154015 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -10,8 +10,124 @@
>> #include <linux/seq_file.h>
>> #include <linux/vmalloc.h>
>> #include <linux/kexec.h>
>> +#include <linux/of.h>
>> +#include <linux/memblock.h>
>> +#include <linux/libfdt.h>
>> #include "ima.h"
>>
>> +static int get_addr_size_cells(int *addr_cells, int *size_cells)
>> +{
>> + struct device_node *root;
>> +
>> + root = of_find_node_by_path("/");
>> + if (!root)
>> + return -EINVAL;
>> +
>> + *addr_cells = of_n_addr_cells(root);
>> + *size_cells = of_n_size_cells(root);
>> +
>> + of_node_put(root);
>> +
>> + 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(&addr_cells, &size_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", &len);
>> + if (!prop)
>> + return -ENOENT;
>> +
>> + ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
>> + if (ret)
>> + return ret;
>> +
>> + *addr = __va(tmp_addr);
>> + *size = tmp_size;
>> +
>> + return 0;
>> +}
>
> The functions above were moved without being changed. Good.
>
>> +/**
>> + * 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, &addr, &size);
>> + if (ret)
>> + return ret;
>> +
>> + ret = of_remove_property(of_chosen, prop);
>> + if (ret)
>> + return ret;
>> +
>> + return memblock_free(__pa(addr), size);
>
> Here you added a __pa() call. Do you store a virtual address in
> linux,ima-kexec-buffer property? Doesn't it make more sense to store a
> physical address?
>
trying to minimize the changes here as do_get_kexec_buffer return the va.
I will refactor this to remove the double translation.
> Even if making this change is the correct thing to do, it should be a
> separate patch, unless it can't be avoided. And if that is the case,
> then it should be explained in the commit message.
>
>> +
>> +}
>> +
>> +/**
>> + * 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", &len);
>> + if (!prop)
>> + return;
>> +
>> + do_get_kexec_buffer(prop, len, &addr, &size);
>> + ret = fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
>> + if (ret < 0)
>> + return;
>> +
>> + memblock_free(addr, size);
>> +}
>
> Here is another function that changed when moved. This one I know to be
> wrong. You're confusing the purposes of remove_ima_buffer() and
> ima_free_kexec_buffer().
>
> You did send me a question about them nearly three weeks ago which I
> only answered today, so I apologize. Also, their names could more
> clearly reflect their differences, so it's bad naming on my part.
>
> With IMA kexec buffers, there are two kernels (and thus their two
> respective, separate device trees) to be concerned about:
>
> 1. the currently running kernel, which uses the live device tree
> (accessed with the of_* functions) and the memblock subsystem;
>
> 2. the kernel which is being loaded by kexec, which will use the FDT
> blob being passed around as argument to these functions, and the memory
> reservations in the memory reservation table of the FDT blob.
>
> ima_free_kexec_buffer() is used by IMA in the currently running kernel.
> Therefore the device tree it is concerned about is the live one, and
> thus uses the of_* functions to access it. And uses memblock to change
> the memory reservation.
>
> remove_ima_buffer() on the other hand is used by the kexec code to
> prepare an FDT blob for the kernel that is being loaded. It should not
> make any changes to live device tree, nor to memblock allocations. It
> should only make changes to the FDT blob.
>
Thank you for this, greatly appreciate clearing my misunderstandings.
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
>
On 6/19/20 5:41 PM, Thiago Jung Bauermann wrote:
>
> Prakhar Srivastava <[email protected]> writes:
>
>> Integrity measurement architecture(IMA) validates if files
>> have been accidentally or maliciously altered, both remotely and
>> locally, appraise a file's measurement against a "good" value stored
>> as an extended attribute, and enforce local file integrity.
>>
>> IMA also measures singatures of kernel and initrd during kexec along with
>> the command line used for kexec.
>> These measurements are critical to verify the seccurity posture of the OS.
>>
>> Resering memory and adding the memory information to a device tree node
>> acts as the mechanism to carry over IMA measurement logs.
>>
>> Update devicetree documentation to reflect the addition of new property
>> under the chosen node.
>
> Thank you for writing this documentation patch. It's something I should
> have done when I added the powerpc IMA kexec support.
>
> You addressed Rob Herring's comments regarding the commit message, but
> not the ones regarding the patch contents.
>
> When posting a new version of the patches, make sure to address all
> comments made so far. Addressing a comment doesn't necessarily mean
> implementing the requested change. If you don't then you should at least
> explain why you chose a different path.
>
> I mention it because this has occurred before with this patch series,
> and it's hard to make forward progress if review comments get ignored.
>
>> ---
>> Documentation/devicetree/bindings/chosen.txt | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
>> index 45e79172a646..a15f70c007ef 100644
>> --- a/Documentation/devicetree/bindings/chosen.txt
>> +++ b/Documentation/devicetree/bindings/chosen.txt
>> @@ -135,3 +135,20 @@ e.g.
>> linux,initrd-end = <0x82800000>;
>> };
>> };
>> +
>> +linux,ima-kexec-buffer
>> +----------------------
>> +
>> +This property(currently used by powerpc, arm64) holds the memory range,
>
> space before the parenthesis.
>
>> +the address and the size, of the IMA measurement logs that are being carried
>
> Maybe it's because English isn't my first language, but IMHO it's
> clearer if "the address and the size" is between parentheses rather than
> commas.
>
>> +over to the kexec session.
>
> I don't think there's a "kexec session", but I'm not sure what a good
> term would be. "linux,booted-from-kexec" uses "new kernel" so perhaps
> that's a good option to use instead of "kexec session".
>
>> +
>> +/ {
>> + chosen {
>> + linux,ima-kexec-buffer = <0x9 0x82000000 0x0 0x00008000>;
>> + };
>> +};
>> +
>> +This porperty does not represent real hardware, but the memory allocated for
>> +carrying the IMA measurement logs. The address and the suze are expressed in
>> +#address-cells and #size-cells, respectively of the root node.
>
>
I will update the descriptions and ack the comments/changes in the
patches as well.
Thankyou,
Prakhar Srivastava
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
>
Hello Prakhar,
Prakhar Srivastava <[email protected]> writes:
> On 6/19/20 5:19 PM, Thiago Jung Bauermann wrote:
>>
>> Prakhar Srivastava <[email protected]> writes:
>>
>>> Powerpc has support to carry over the IMA measurement logs. Refatoring the
>>> non-architecture specific code out of arch/powerpc and into security/ima.
>>>
>>> The code adds support for reserving and freeing up of memory for IMA measurement
>>> logs.
>>
>> Last week, Mimi provided this feedback:
>>
>> "From your patch description, this patch should be broken up. Moving
>> the non-architecture specific code out of powerpc should be one patch.
>> Additional support should be in another patch. After each patch, the
>> code should work properly."
>>
>> That's not what you do here. You move the code, but you also make other
>> changes at the same time. This has two problems:
>>
>> 1. It makes the patch harder to review, because it's very easy to miss a
>> change.
>>
>> 2. If in the future a git bisect later points to this patch, it's not
>> clear whether the problem is because of the code movement, or because
>> of the other changes.
>>
>> When you move code, ideally the patch should only make the changes
>> necessary to make the code work at its new location. The patch which
>> does code movement should not cause any change in behavior.
>>
>> Other changes should go in separate patches, either before or after the
>> one moving the code.
>>
>> More comments below.
>>
> Hi Thiago,
>
> Apologies for the delayed response i was away for a few days.
> I am working on breaking up the changes so that its easier to review and update
> as well.
No problem.
>
> Thanks,
> Prakhar Srivastava
>
>>>
>>> ---
>>> arch/powerpc/include/asm/ima.h | 10 ---
>>> arch/powerpc/kexec/ima.c | 126 ++---------------------------
>>> security/integrity/ima/ima_kexec.c | 116 ++++++++++++++++++++++++++
>>> 3 files changed, 124 insertions(+), 128 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
>>> index ead488cf3981..c29ec86498f8 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,6 @@ 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);
>>> return 0;
>>> }
>>
>> This is wrong. Even if the currently running kernel doesn't have
>> CONFIG_IMA_KEXEC, it should remove the IMA buffer property and memory
>> reservation from the FDT that is being prepared for the next kernel.
>>
>> This is because the IMA kexec buffer is useless for the next kernel,
>> regardless of whether the current kernel supports CONFIG_IMA_KEXEC or
>> not. Keeping it around would be a waste of memory.
>>
> I will keep it in my next revision.
> My understanding was the reserved memory is freed and property removed when IMA
> loads the logs on init.
If CONFIG_IMA_KEXEC is set, then yes. If it isn't then that needs to
happen in the function above.
> During setup_fdt in kexec, a duplicate copy of the dt is
> used, but memory still needs to be allocated, thus the property itself indicats
> presence of reserved memory.
>
>>> @@ -179,13 +64,18 @@ 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);
>>
>> This is wrong, for the same reason stated above.
>>
>>> if (!image->arch.ima_buffer_size)
>>> return 0;
>>>
>>> - ret = get_addr_size_cells(&addr_cells, &size_cells);
>>> - if (ret)
>>> + ret = fdt_address_cells(fdt, chosen_node);
>>> + if (ret < 0)
>>> + return ret;
>>> + addr_cells = ret;
>>> +
>>> + ret = fdt_size_cells(fdt, chosen_node);
>>> + if (ret < 0)
>>> return ret;
>>> + size_cells = ret;
>>>
>>> entry_size = 4 * (addr_cells + size_cells);
>>>
>>
>> I liked this change. Thanks! I agree it's better to use
>> fdt_address_cells() and fdt_size_cells() here.
>>
>> But it should be in a separate patch. Either before or after the one
>> moving the code.
>>
>>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>>> index 121de3e04af2..e1e6d6154015 100644
>>> --- a/security/integrity/ima/ima_kexec.c
>>> +++ b/security/integrity/ima/ima_kexec.c
>>> @@ -10,8 +10,124 @@
>>> #include <linux/seq_file.h>
>>> #include <linux/vmalloc.h>
>>> #include <linux/kexec.h>
>>> +#include <linux/of.h>
>>> +#include <linux/memblock.h>
>>> +#include <linux/libfdt.h>
>>> #include "ima.h"
>>>
>>> +static int get_addr_size_cells(int *addr_cells, int *size_cells)
>>> +{
>>> + struct device_node *root;
>>> +
>>> + root = of_find_node_by_path("/");
>>> + if (!root)
>>> + return -EINVAL;
>>> +
>>> + *addr_cells = of_n_addr_cells(root);
>>> + *size_cells = of_n_size_cells(root);
>>> +
>>> + of_node_put(root);
>>> +
>>> + 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(&addr_cells, &size_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", &len);
>>> + if (!prop)
>>> + return -ENOENT;
>>> +
>>> + ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + *addr = __va(tmp_addr);
>>> + *size = tmp_size;
>>> +
>>> + return 0;
>>> +}
>>
>> The functions above were moved without being changed. Good.
>>
>>> +/**
>>> + * 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, &addr, &size);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = of_remove_property(of_chosen, prop);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return memblock_free(__pa(addr), size);
>>
>> Here you added a __pa() call. Do you store a virtual address in
>> linux,ima-kexec-buffer property? Doesn't it make more sense to store a
>> physical address?
>>
> trying to minimize the changes here as do_get_kexec_buffer return the va.
> I will refactor this to remove the double translation.
In the powerpc version, do_get_kexec_buffer() returns the pa, and one of
its callers does the va translation. I think that worked well.
>> Even if making this change is the correct thing to do, it should be a
>> separate patch, unless it can't be avoided. And if that is the case,
>> then it should be explained in the commit message.
>>
>>> +
>>> +}
>>> +
>>> +/**
>>> + * 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", &len);
>>> + if (!prop)
>>> + return;
>>> +
>>> + do_get_kexec_buffer(prop, len, &addr, &size);
>>> + ret = fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
>>> + if (ret < 0)
>>> + return;
>>> +
>>> + memblock_free(addr, size);
>>> +}
>>
>> Here is another function that changed when moved. This one I know to be
>> wrong. You're confusing the purposes of remove_ima_buffer() and
>> ima_free_kexec_buffer().
>>
>> You did send me a question about them nearly three weeks ago which I
>> only answered today, so I apologize. Also, their names could more
>> clearly reflect their differences, so it's bad naming on my part.
>>
>> With IMA kexec buffers, there are two kernels (and thus their two
>> respective, separate device trees) to be concerned about:
>>
>> 1. the currently running kernel, which uses the live device tree
>> (accessed with the of_* functions) and the memblock subsystem;
>>
>> 2. the kernel which is being loaded by kexec, which will use the FDT
>> blob being passed around as argument to these functions, and the memory
>> reservations in the memory reservation table of the FDT blob.
>>
>> ima_free_kexec_buffer() is used by IMA in the currently running kernel.
>> Therefore the device tree it is concerned about is the live one, and
>> thus uses the of_* functions to access it. And uses memblock to change
>> the memory reservation.
>>
>> remove_ima_buffer() on the other hand is used by the kexec code to
>> prepare an FDT blob for the kernel that is being loaded. It should not
>> make any changes to live device tree, nor to memblock allocations. It
>> should only make changes to the FDT blob.
>>
> Thank you for this, greatly appreciate clearing my misunderstandings.
You're welcome. Sorry again for not answering your question before you
sent this patch series.
--
Thiago Jung Bauermann
IBM Linux Technology Center