[RFC][PATCH v1 0/1] Carry ima measurement log for arm64 via kexec_file_load

[RFC][PATCH v1 0/1] Carry ima measurement log for arm64 via kexec_file_load

[Prakhar Srivastava]

The patch adds support for arm64 to carry ima measurement log
to the next soft boot session triggered via kexec_file_load.
- Top of Linux 5.3-rc6

Currently during kexec the kernel file signatures are validated
prior to actual load, the information(PE/ima signature) 
is not carried to the next session. 
This lead to loss of information.

This patch addresses the same by carrying forward the ima measurement log
to the next kexec'ed session. This just allows a verifying party to get
the entire runtime event log since the last full reboot since that is
when PCRs were last reset.

The code is in most part same as powerpc, i want to get feedback as to
how/correct way to refactor the code so that cross architecture 
partial helpers can be put in a common place.

Prakhar Srivastava (1):
  Carry ima measurement log for arm64 via kexec_file_load

 arch/arm64/Kconfig                     |   7 +
 arch/arm64/include/asm/ima.h           |  31 ++++
 arch/arm64/include/asm/kexec.h         |   4 +
 arch/arm64/kernel/Makefile             |   1 +
 arch/arm64/kernel/ima_kexec.c          | 219 +++++++++++++++++++++++++
 arch/arm64/kernel/machine_kexec_file.c |  39 +++++
 6 files changed, 301 insertions(+)
 create mode 100644 arch/arm64/include/asm/ima.h
 create mode 100644 arch/arm64/kernel/ima_kexec.c

-- 
2.17.1

[RFC][PATCH 1/1] Carry ima measurement log for arm64 via kexec_file_load

[Prakhar Srivastava]

Carry ima measurement log for arm64 via kexec_file_load.
add support to kexec_file_load to pass the ima measurement log

This patch adds entry for the ima measurement log in the
dtb which is then used in the kexec'ed session to fetch the
segment and then load the ima measurement log.

Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
---
 arch/arm64/Kconfig                     |   7 +
 arch/arm64/include/asm/ima.h           |  31 ++++
 arch/arm64/include/asm/kexec.h         |   4 +
 arch/arm64/kernel/Makefile             |   1 +
 arch/arm64/kernel/ima_kexec.c          | 219 +++++++++++++++++++++++++
 arch/arm64/kernel/machine_kexec_file.c |  39 +++++
 6 files changed, 301 insertions(+)
 create mode 100644 arch/arm64/include/asm/ima.h
 create mode 100644 arch/arm64/kernel/ima_kexec.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 3adcec05b1f6..9e1b831e7baa 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -964,6 +964,13 @@ config KEXEC_FILE
 	  for kernel and initramfs as opposed to list of segments as
 	  accepted by previous system call.
 
+config HAVE_IMA_KEXEC
+	bool "enable arch specific ima buffer pass"
+	depends on KEXEC_FILE
+	help
+		This adds support to carry ima log to the next kernel in case
+		of kexec_file_load
+
 config KEXEC_VERIFY_SIG
 	bool "Verify kernel signature during kexec_file_load() syscall"
 	depends on KEXEC_FILE
diff --git a/arch/arm64/include/asm/ima.h b/arch/arm64/include/asm/ima.h
new file mode 100644
index 000000000000..2c504281028d
--- /dev/null
+++ b/arch/arm64/include/asm/ima.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARM64_IMA_H
+#define _ASM_ARM64_IMA_H
+
+#define FDT_PROP_KEXEC_BUFFER	"linux,ima-kexec-buffer"
+
+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);
+
+int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node);
+#else
+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 */
+#endif /* _ASM_ARM64_IMA_H */
diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 12a561a54128..ca1f9ad5c4d4 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -96,6 +96,8 @@ static inline void crash_post_resume(void) {}
 struct kimage_arch {
 	void *dtb;
 	unsigned long dtb_mem;
+	phys_addr_t ima_buffer_addr;
+	size_t ima_buffer_size;
 };
 
 extern const struct kexec_file_ops kexec_image_ops;
@@ -107,6 +109,8 @@ extern int load_other_segments(struct kimage *image,
 		unsigned long kernel_load_addr, unsigned long kernel_size,
 		char *initrd, unsigned long initrd_len,
 		char *cmdline);
+extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
+		unsigned long size);
 #endif
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 478491f07b4f..9620f90bd0e1 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_CRASH_CORE)		+= crash_core.o
 obj-$(CONFIG_ARM_SDE_INTERFACE)		+= sdei.o
 obj-$(CONFIG_ARM64_SSBD)		+= ssbd.o
 obj-$(CONFIG_ARM64_PTR_AUTH)		+= pointer_auth.o
+obj-$(CONFIG_HAVE_IMA_KEXEC)		+= ima_kexec.o
 
 obj-y					+= vdso/ probes/
 obj-$(CONFIG_COMPAT_VDSO)		+= vdso32/
diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c
new file mode 100644
index 000000000000..5ae0d776ec42
--- /dev/null
+++ b/arch/arm64/kernel/ima_kexec.c
@@ -0,0 +1,219 @@
+/*
+ * Copyright (C) 2016 IBM Corporation
+ *
+ * Authors:
+ * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/slab.h>
+#include <linux/kexec.h>
+#include <linux/of.h>
+#include <linux/memblock.h>
+#include <linux/libfdt.h>
+#include <asm/kexec.h>
+#include <asm/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, FDT_PROP_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, FDT_PROP_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);
+}
+
+#ifdef CONFIG_IMA
+/**
+ * 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, FDT_PROP_KEXEC_BUFFER, &len);
+	if (!prop)
+		return;
+
+	ret = do_get_kexec_buffer(prop, len, &addr, &size);
+	fdt_delprop(fdt, chosen_node, FDT_PROP_KEXEC_BUFFER);
+	if (ret)
+		return;
+
+	ret = delete_fdt_mem_rsv(fdt, addr, size);
+	if (!ret)
+		pr_debug("Removed old IMA buffer reservation.\n");
+}
+#endif /* CONFIG_IMA */
+
+#ifdef CONFIG_IMA_KEXEC
+/**
+ * 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;
+}
+
+static int write_number(void *p, u64 value, int cells)
+{
+	if (cells == 1) {
+		u32 tmp;
+
+		if (value > U32_MAX)
+			return -EINVAL;
+
+		tmp = cpu_to_be32(value);
+		memcpy(p, &tmp, sizeof(tmp));
+	} else if (cells == 2) {
+		u64 tmp;
+
+		tmp = cpu_to_be64(value);
+		memcpy(p, &tmp, sizeof(tmp));
+	} else
+		return -EINVAL;
+	return 0;
+}
+
+/**
+ * setup_ima_buffer - add IMA buffer information to the fdt
+ * @image:		kexec image being loaded.
+ * @dtb:		Flattened device tree for the next kernel.
+ * @chosen_node:	Offset to the chosen node.
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int setup_ima_buffer(const struct kimage *image, void *dtb, int chosen_node)
+{
+	int ret, addr_cells, size_cells, entry_size;
+	u8 value[16];
+
+	remove_ima_buffer(dtb, chosen_node);
+
+	ret = get_addr_size_cells(&addr_cells, &size_cells);
+	if (ret)
+		return ret;
+
+	entry_size = 4 * (addr_cells + size_cells);
+
+	if (entry_size > sizeof(value))
+		return -EINVAL;
+
+	ret = write_number(value, image->arch.ima_buffer_addr, addr_cells);
+	if (ret)
+		return ret;
+
+	ret = write_number(value + 4 * addr_cells, image->arch.ima_buffer_size,
+			size_cells);
+	if (ret)
+		return ret;
+
+	ret = fdt_setprop(dtb, chosen_node, FDT_PROP_KEXEC_BUFFER, value,
+			  entry_size);
+	if (ret < 0)
+		return -EINVAL;
+
+	ret = fdt_add_mem_rsv(dtb, image->segment[0].mem,
+			      image->segment[0].memsz);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+#endif /* CONFIG_IMA_KEXEC */
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 58871333737a..c05ad6b74b62 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -21,6 +21,7 @@
 #include <linux/types.h>
 #include <linux/vmalloc.h>
 #include <asm/byteorder.h>
+#include <asm/ima.h>
 
 /* relevant device tree properties */
 #define FDT_PROP_INITRD_START	"linux,initrd-start"
@@ -85,6 +86,11 @@ static int setup_dtb(struct kimage *image,
 			goto out;
 	}
 
+	/* add ima_buffer */
+	ret = setup_ima_buffer(image, dtb, off);
+	if (ret)
+		goto out;
+
 	/* add kaslr-seed */
 	ret = fdt_delprop(dtb, off, FDT_PROP_KASLR_SEED);
 	if  (ret == -FDT_ERR_NOTFOUND)
@@ -114,6 +120,39 @@ static int setup_dtb(struct kimage *image,
  */
 #define DTB_EXTRA_SPACE 0x1000
 
+
+/**
+ * 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, &rsv_start, &rsv_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;
+}
+
 static int create_dtb(struct kimage *image,
 		      unsigned long initrd_load_addr, unsigned long initrd_len,
 		      char *cmdline, void **dtb)
-- 
2.17.1

[RFC][PATCH v1 0/1] Carry ima measurement log for arm64 via kexec_file_load

[Prakhar Srivastava]

The patch adds support for arm64 to carry ima measurement log
to the next soft boot session triggered via kexec_file_load.
- Top of Linux 5.3-rc6

Currently during kexec the kernel file signatures are validated
prior to actual load, the information(PE/ima signature) 
is not carried to the next session. 
This lead to loss of information.

This patch addresses the same by carrying forward the ima measurement log
to the next kexec'ed session. This just allows a verifying party to get
the entire runtime event log since the last full reboot since that is
when PCRs were last reset.

The code is in most part same as powerpc, i want to get feedback as to
how/correct way to refactor the code so that cross architecture 
partial helpers can be put in a common place.

Prakhar Srivastava (1):
  Carry ima measurement log for arm64 via kexec_file_load

 arch/arm64/Kconfig                     |   7 +
 arch/arm64/include/asm/ima.h           |  31 ++++
 arch/arm64/include/asm/kexec.h         |   4 +
 arch/arm64/kernel/Makefile             |   1 +
 arch/arm64/kernel/ima_kexec.c          | 219 +++++++++++++++++++++++++
 arch/arm64/kernel/machine_kexec_file.c |  39 +++++
 6 files changed, 301 insertions(+)
 create mode 100644 arch/arm64/include/asm/ima.h
 create mode 100644 arch/arm64/kernel/ima_kexec.c

-- 
2.17.1

[RFC][PATCH 1/1] Carry ima measurement log for arm64 via kexec_file_load

[Prakhar Srivastava]

Carry ima measurement log for arm64 via kexec_file_load.
add support to kexec_file_load to pass the ima measurement log

This patch adds entry for the ima measurement log in the
dtb which is then used in the kexec'ed session to fetch the
segment and then load the ima measurement log.

Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
---
 arch/arm64/Kconfig                     |   7 +
 arch/arm64/include/asm/ima.h           |  31 ++++
 arch/arm64/include/asm/kexec.h         |   4 +
 arch/arm64/kernel/Makefile             |   1 +
 arch/arm64/kernel/ima_kexec.c          | 219 +++++++++++++++++++++++++
 arch/arm64/kernel/machine_kexec_file.c |  39 +++++
 6 files changed, 301 insertions(+)
 create mode 100644 arch/arm64/include/asm/ima.h
 create mode 100644 arch/arm64/kernel/ima_kexec.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 3adcec05b1f6..9e1b831e7baa 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -964,6 +964,13 @@ config KEXEC_FILE
 	  for kernel and initramfs as opposed to list of segments as
 	  accepted by previous system call.
 
+config HAVE_IMA_KEXEC
+	bool "enable arch specific ima buffer pass"
+	depends on KEXEC_FILE
+	help
+		This adds support to carry ima log to the next kernel in case
+		of kexec_file_load
+
 config KEXEC_VERIFY_SIG
 	bool "Verify kernel signature during kexec_file_load() syscall"
 	depends on KEXEC_FILE
diff --git a/arch/arm64/include/asm/ima.h b/arch/arm64/include/asm/ima.h
new file mode 100644
index 000000000000..2c504281028d
--- /dev/null
+++ b/arch/arm64/include/asm/ima.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARM64_IMA_H
+#define _ASM_ARM64_IMA_H
+
+#define FDT_PROP_KEXEC_BUFFER	"linux,ima-kexec-buffer"
+
+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);
+
+int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node);
+#else
+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 */
+#endif /* _ASM_ARM64_IMA_H */
diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 12a561a54128..ca1f9ad5c4d4 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -96,6 +96,8 @@ static inline void crash_post_resume(void) {}
 struct kimage_arch {
 	void *dtb;
 	unsigned long dtb_mem;
+	phys_addr_t ima_buffer_addr;
+	size_t ima_buffer_size;
 };
 
 extern const struct kexec_file_ops kexec_image_ops;
@@ -107,6 +109,8 @@ extern int load_other_segments(struct kimage *image,
 		unsigned long kernel_load_addr, unsigned long kernel_size,
 		char *initrd, unsigned long initrd_len,
 		char *cmdline);
+extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
+		unsigned long size);
 #endif
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 478491f07b4f..9620f90bd0e1 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_CRASH_CORE)		+= crash_core.o
 obj-$(CONFIG_ARM_SDE_INTERFACE)		+= sdei.o
 obj-$(CONFIG_ARM64_SSBD)		+= ssbd.o
 obj-$(CONFIG_ARM64_PTR_AUTH)		+= pointer_auth.o
+obj-$(CONFIG_HAVE_IMA_KEXEC)		+= ima_kexec.o
 
 obj-y					+= vdso/ probes/
 obj-$(CONFIG_COMPAT_VDSO)		+= vdso32/
diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c
new file mode 100644
index 000000000000..5ae0d776ec42
--- /dev/null
+++ b/arch/arm64/kernel/ima_kexec.c
@@ -0,0 +1,219 @@
+/*
+ * Copyright (C) 2016 IBM Corporation
+ *
+ * Authors:
+ * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/slab.h>
+#include <linux/kexec.h>
+#include <linux/of.h>
+#include <linux/memblock.h>
+#include <linux/libfdt.h>
+#include <asm/kexec.h>
+#include <asm/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, FDT_PROP_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, FDT_PROP_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);
+}
+
+#ifdef CONFIG_IMA
+/**
+ * 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, FDT_PROP_KEXEC_BUFFER, &len);
+	if (!prop)
+		return;
+
+	ret = do_get_kexec_buffer(prop, len, &addr, &size);
+	fdt_delprop(fdt, chosen_node, FDT_PROP_KEXEC_BUFFER);
+	if (ret)
+		return;
+
+	ret = delete_fdt_mem_rsv(fdt, addr, size);
+	if (!ret)
+		pr_debug("Removed old IMA buffer reservation.\n");
+}
+#endif /* CONFIG_IMA */
+
+#ifdef CONFIG_IMA_KEXEC
+/**
+ * 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;
+}
+
+static int write_number(void *p, u64 value, int cells)
+{
+	if (cells == 1) {
+		u32 tmp;
+
+		if (value > U32_MAX)
+			return -EINVAL;
+
+		tmp = cpu_to_be32(value);
+		memcpy(p, &tmp, sizeof(tmp));
+	} else if (cells == 2) {
+		u64 tmp;
+
+		tmp = cpu_to_be64(value);
+		memcpy(p, &tmp, sizeof(tmp));
+	} else
+		return -EINVAL;
+	return 0;
+}
+
+/**
+ * setup_ima_buffer - add IMA buffer information to the fdt
+ * @image:		kexec image being loaded.
+ * @dtb:		Flattened device tree for the next kernel.
+ * @chosen_node:	Offset to the chosen node.
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int setup_ima_buffer(const struct kimage *image, void *dtb, int chosen_node)
+{
+	int ret, addr_cells, size_cells, entry_size;
+	u8 value[16];
+
+	remove_ima_buffer(dtb, chosen_node);
+
+	ret = get_addr_size_cells(&addr_cells, &size_cells);
+	if (ret)
+		return ret;
+
+	entry_size = 4 * (addr_cells + size_cells);
+
+	if (entry_size > sizeof(value))
+		return -EINVAL;
+
+	ret = write_number(value, image->arch.ima_buffer_addr, addr_cells);
+	if (ret)
+		return ret;
+
+	ret = write_number(value + 4 * addr_cells, image->arch.ima_buffer_size,
+			size_cells);
+	if (ret)
+		return ret;
+
+	ret = fdt_setprop(dtb, chosen_node, FDT_PROP_KEXEC_BUFFER, value,
+			  entry_size);
+	if (ret < 0)
+		return -EINVAL;
+
+	ret = fdt_add_mem_rsv(dtb, image->segment[0].mem,
+			      image->segment[0].memsz);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+#endif /* CONFIG_IMA_KEXEC */
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 58871333737a..c05ad6b74b62 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -21,6 +21,7 @@
 #include <linux/types.h>
 #include <linux/vmalloc.h>
 #include <asm/byteorder.h>
+#include <asm/ima.h>
 
 /* relevant device tree properties */
 #define FDT_PROP_INITRD_START	"linux,initrd-start"
@@ -85,6 +86,11 @@ static int setup_dtb(struct kimage *image,
 			goto out;
 	}
 
+	/* add ima_buffer */
+	ret = setup_ima_buffer(image, dtb, off);
+	if (ret)
+		goto out;
+
 	/* add kaslr-seed */
 	ret = fdt_delprop(dtb, off, FDT_PROP_KASLR_SEED);
 	if  (ret == -FDT_ERR_NOTFOUND)
@@ -114,6 +120,39 @@ static int setup_dtb(struct kimage *image,
  */
 #define DTB_EXTRA_SPACE 0x1000
 
+
+/**
+ * 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, &rsv_start, &rsv_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;
+}
+
 static int create_dtb(struct kimage *image,
 		      unsigned long initrd_load_addr, unsigned long initrd_len,
 		      char *cmdline, void **dtb)
-- 
2.17.1

Re: [RFC][PATCH 1/1] Carry ima measurement log for arm64 via kexec_file_load

[Stephen Boyd]

Why is linux-arm-msm list CCed on this topic?

Quoting Prakhar Srivastava (2019-08-29 13:05:32)
> Carry ima measurement log for arm64 via kexec_file_load.
> add support to kexec_file_load to pass the ima measurement log

These first two sentences look sort of odd for a commit text.

> 
> This patch adds entry for the ima measurement log in the

Don't use 'this patch' in commit text. It's in the wrong voice.

> dtb which is then used in the kexec'ed session to fetch the
> segment and then load the ima measurement log.
> 
> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 3adcec05b1f6..9e1b831e7baa 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -964,6 +964,13 @@ config KEXEC_FILE
>           for kernel and initramfs as opposed to list of segments as
>           accepted by previous system call.
>  
> +config HAVE_IMA_KEXEC
> +       bool "enable arch specific ima buffer pass"
> +       depends on KEXEC_FILE
> +       help
> +               This adds support to carry ima log to the next kernel in case

Should ima be all caps here?

> +               of kexec_file_load

Add a full-stop?

> +
>  config KEXEC_VERIFY_SIG
>         bool "Verify kernel signature during kexec_file_load() syscall"
>         depends on KEXEC_FILE
> diff --git a/arch/arm64/include/asm/ima.h b/arch/arm64/include/asm/ima.h
> new file mode 100644
> index 000000000000..2c504281028d
> --- /dev/null
> +++ b/arch/arm64/include/asm/ima.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_ARM64_IMA_H
> +#define _ASM_ARM64_IMA_H
> +
> +#define FDT_PROP_KEXEC_BUFFER  "linux,ima-kexec-buffer"

Is this documented somewhere in the DT bindings?

> +
> +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);
> +
> +int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node);
> +#else
> +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 */
> +#endif /* _ASM_ARM64_IMA_H */
> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> index 12a561a54128..ca1f9ad5c4d4 100644
> --- a/arch/arm64/include/asm/kexec.h
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -96,6 +96,8 @@ static inline void crash_post_resume(void) {}
>  struct kimage_arch {
>         void *dtb;
>         unsigned long dtb_mem;
> +       phys_addr_t ima_buffer_addr;
> +       size_t ima_buffer_size;

Should this be in an ifdef?

>  };
>  
>  extern const struct kexec_file_ops kexec_image_ops;
> @@ -107,6 +109,8 @@ extern int load_other_segments(struct kimage *image,
>                 unsigned long kernel_load_addr, unsigned long kernel_size,
>                 char *initrd, unsigned long initrd_len,
>                 char *cmdline);
> +extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
> +               unsigned long size);
>  #endif
>  
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c
> new file mode 100644
> index 000000000000..5ae0d776ec42
> --- /dev/null
> +++ b/arch/arm64/kernel/ima_kexec.c
> @@ -0,0 +1,219 @@
> +/*
> + * Copyright (C) 2016 IBM Corporation
> + *
> + * Authors:
> + * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Please use a SPDX tag instead of this boiler plate.

> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kexec.h>
> +#include <linux/of.h>
> +#include <linux/memblock.h>
> +#include <linux/libfdt.h>
> +#include <asm/kexec.h>
> +#include <asm/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, FDT_PROP_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, FDT_PROP_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);
> +}
> +
> +#ifdef CONFIG_IMA
> +/**
> + * 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, FDT_PROP_KEXEC_BUFFER, &len);
> +       if (!prop)
> +               return;
> +
> +       ret = do_get_kexec_buffer(prop, len, &addr, &size);
> +       fdt_delprop(fdt, chosen_node, FDT_PROP_KEXEC_BUFFER);
> +       if (ret)
> +               return;
> +
> +       ret = delete_fdt_mem_rsv(fdt, addr, size);
> +       if (!ret)
> +               pr_debug("Removed old IMA buffer reservation.\n");
> +}
> +#endif /* CONFIG_IMA */
> +
> +#ifdef CONFIG_IMA_KEXEC
> +/**
> + * 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;
> +}
> +
> +static int write_number(void *p, u64 value, int cells)

Maybe this should be an of_write_number() API exposed by the DT parsing
code?

> +{
> +       if (cells == 1) {
> +               u32 tmp;
> +
> +               if (value > U32_MAX)
> +                       return -EINVAL;
> +
> +               tmp = cpu_to_be32(value);
> +               memcpy(p, &tmp, sizeof(tmp));
> +       } else if (cells == 2) {
> +               u64 tmp;
> +
> +               tmp = cpu_to_be64(value);
> +               memcpy(p, &tmp, sizeof(tmp));
> +       } else
> +               return -EINVAL;

Put braces around this else please.

> +       return 0;
> +}
> +
> +/**
> + * setup_ima_buffer - add IMA buffer information to the fdt
> + * @image:             kexec image being loaded.
> + * @dtb:               Flattened device tree for the next kernel.
> + * @chosen_node:       Offset to the chosen node.

Why capitalize Flattened and Offset?

> + *
> + * Return: 0 on success, or negative errno on error.
> + */
> +int setup_ima_buffer(const struct kimage *image, void *dtb, int chosen_node)
> +{
> +       int ret, addr_cells, size_cells, entry_size;
> +       u8 value[16];
> +
> +       remove_ima_buffer(dtb, chosen_node);
> +
> +       ret = get_addr_size_cells(&addr_cells, &size_cells);
> +       if (ret)
> +               return ret;
> +
> +       entry_size = 4 * (addr_cells + size_cells);
> +
> +       if (entry_size > sizeof(value))
> +               return -EINVAL;
> +
> +       ret = write_number(value, image->arch.ima_buffer_addr, addr_cells);
> +       if (ret)
> +               return ret;
> +
> +       ret = write_number(value + 4 * addr_cells, image->arch.ima_buffer_size,
> +                       size_cells);
> +       if (ret)
> +               return ret;
> +
> +       ret = fdt_setprop(dtb, chosen_node, FDT_PROP_KEXEC_BUFFER, value,
> +                         entry_size);
> +       if (ret < 0)
> +               return -EINVAL;
> +
> +       ret = fdt_add_mem_rsv(dtb, image->segment[0].mem,
> +                             image->segment[0].memsz);
> +       if (ret)
> +               return -EINVAL;
> +
> +       return 0;
> +}
> +#endif /* CONFIG_IMA_KEXEC */
> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
> index 58871333737a..c05ad6b74b62 100644
> --- a/arch/arm64/kernel/machine_kexec_file.c
> +++ b/arch/arm64/kernel/machine_kexec_file.c
> @@ -21,6 +21,7 @@
>  #include <linux/types.h>
>  #include <linux/vmalloc.h>
>  #include <asm/byteorder.h>
> +#include <asm/ima.h>
>  
>  /* relevant device tree properties */
>  #define FDT_PROP_INITRD_START  "linux,initrd-start"
> @@ -85,6 +86,11 @@ static int setup_dtb(struct kimage *image,
>                         goto out;
>         }
>  
> +       /* add ima_buffer */
> +       ret = setup_ima_buffer(image, dtb, off);
> +       if (ret)
> +               goto out;
> +
>         /* add kaslr-seed */
>         ret = fdt_delprop(dtb, off, FDT_PROP_KASLR_SEED);
>         if  (ret == -FDT_ERR_NOTFOUND)
> @@ -114,6 +120,39 @@ static int setup_dtb(struct kimage *image,
>   */
>  #define DTB_EXTRA_SPACE 0x1000
>  
> +
> +/**
> + * delete_fdt_mem_rsv - delete memory reservation with given address and size
> + *

Can you document the arguments too?

> + * 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, &rsv_start, &rsv_size);
> +               if (ret) {
> +                       pr_err("Malformed device tree.\n");

Please drop the full-stop on this printk.

> +                       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");

Same comment.

> +                               return -EINVAL;
> +                       }
> +
> +                       return 0;
> +               }
> +       }
> +
> +       return -ENOENT;
> +}

A lot of this code looks DT generic. Can it be moved out of the arch
layer to drivers/of/?

Re: [RFC][PATCH 1/1] Carry ima measurement log for arm64 via kexec_file_load

[Thiago Jung Bauermann]

Stephen Boyd <sboyd@kernel.org> writes:
> A lot of this code looks DT generic. Can it be moved out of the arch
> layer to drivers/of/?

Yes, if this code could be in drivers/of/ it would be great! Perhaps the
 DT generic functions could go in drivers/of/fdt.c, and ones dealing
 with IMA nodes/properties could go in drivers/of/fdt_ima.c?

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFC][PATCH 1/1] Carry ima measurement log for arm64 via kexec_file_load

[Thiago Jung Bauermann]

Hello Prakhar,

Answering this part from the cover letter:

> The code is in most part same as powerpc, i want to get feedback as to
> how/correct way to refactor the code so that cross architecture
> partial helpers can be put in a common place.

That's a great idea. If it could go to drivers/of/ as Stephen Boyd
mentioned in the other email that would be great.

More comments below.

Prakhar Srivastava <prsriva@linux.microsoft.com> writes:

> Carry ima measurement log for arm64 via kexec_file_load.
> add support to kexec_file_load to pass the ima measurement log
>
> This patch adds entry for the ima measurement log in the
> dtb which is then used in the kexec'ed session to fetch the
> segment and then load the ima measurement log.
>
> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> ---
>  arch/arm64/Kconfig                     |   7 +
>  arch/arm64/include/asm/ima.h           |  31 ++++
>  arch/arm64/include/asm/kexec.h         |   4 +
>  arch/arm64/kernel/Makefile             |   1 +
>  arch/arm64/kernel/ima_kexec.c          | 219 +++++++++++++++++++++++++
>  arch/arm64/kernel/machine_kexec_file.c |  39 +++++
>  6 files changed, 301 insertions(+)
>  create mode 100644 arch/arm64/include/asm/ima.h
>  create mode 100644 arch/arm64/kernel/ima_kexec.c
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 3adcec05b1f6..9e1b831e7baa 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -964,6 +964,13 @@ config KEXEC_FILE
>  	  for kernel and initramfs as opposed to list of segments as
>  	  accepted by previous system call.
>
> +config HAVE_IMA_KEXEC
> +	bool "enable arch specific ima buffer pass"
> +	depends on KEXEC_FILE
> +	help
> +		This adds support to carry ima log to the next kernel in case
> +		of kexec_file_load
> +
>  config KEXEC_VERIFY_SIG
>  	bool "Verify kernel signature during kexec_file_load() syscall"
>  	depends on KEXEC_FILE

This Kconfig should be defined in arch/Kconfig so that both arm64 and
powerpc can select it.

> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 478491f07b4f..9620f90bd0e1 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_CRASH_CORE)		+= crash_core.o
>  obj-$(CONFIG_ARM_SDE_INTERFACE)		+= sdei.o
>  obj-$(CONFIG_ARM64_SSBD)		+= ssbd.o
>  obj-$(CONFIG_ARM64_PTR_AUTH)		+= pointer_auth.o
> +obj-$(CONFIG_HAVE_IMA_KEXEC)		+= ima_kexec.o
>
>  obj-y					+= vdso/ probes/
>  obj-$(CONFIG_COMPAT_VDSO)		+= vdso32/
> diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c
> new file mode 100644
> index 000000000000..5ae0d776ec42
> --- /dev/null
> +++ b/arch/arm64/kernel/ima_kexec.c
> @@ -0,0 +1,219 @@
> +/*
> + * Copyright (C) 2016 IBM Corporation
> + *
> + * Authors:
> + * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */

The powerpc file was updated to use the SPDX tag and remove the
paragraph above. Please update your file to match.

> +#include <linux/slab.h>
> +#include <linux/kexec.h>
> +#include <linux/of.h>
> +#include <linux/memblock.h>
> +#include <linux/libfdt.h>
> +#include <asm/kexec.h>
> +#include <asm/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)
> +{
> +

This spurious blank line only exists in the arm64 version. Should be
removed.

> +	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, FDT_PROP_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, FDT_PROP_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);

On the other hand you removed the spurious blank line that exists in
powerpc. Thanks. :-)

> +}
> +
> +#ifdef CONFIG_IMA

remove_ima_buffer() should exist even if CONFIG_IMA isn't set. If I
recall correctly, Mimi requested that kernels that are booted via kexec
should remove the IMA buffer even if they won't use it.

> +/**
> + * 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, FDT_PROP_KEXEC_BUFFER, &len);
> +	if (!prop)
> +		return;
> +
> +	ret = do_get_kexec_buffer(prop, len, &addr, &size);
> +	fdt_delprop(fdt, chosen_node, FDT_PROP_KEXEC_BUFFER);
> +	if (ret)
> +		return;
> +
> +	ret = delete_fdt_mem_rsv(fdt, addr, size);
> +	if (!ret)
> +		pr_debug("Removed old IMA buffer reservation.\n");
> +}
> +#endif /* CONFIG_IMA */
> +
> +#ifdef CONFIG_IMA_KEXEC
> +/**
> + * 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;
> +}
> +
> +static int write_number(void *p, u64 value, int cells)
> +{
> +	if (cells == 1) {
> +		u32 tmp;
> +
> +		if (value > U32_MAX)
> +			return -EINVAL;
> +
> +		tmp = cpu_to_be32(value);
> +		memcpy(p, &tmp, sizeof(tmp));
> +	} else if (cells == 2) {
> +		u64 tmp;
> +
> +		tmp = cpu_to_be64(value);
> +		memcpy(p, &tmp, sizeof(tmp));
> +	} else
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +/**
> + * setup_ima_buffer - add IMA buffer information to the fdt
> + * @image:		kexec image being loaded.
> + * @dtb:		Flattened device tree for the next kernel.
> + * @chosen_node:	Offset to the chosen node.
> + *
> + * Return: 0 on success, or negative errno on error.
> + */
> +int setup_ima_buffer(const struct kimage *image, void *dtb, int chosen_node)
> +{

Is there any particular reason to rename the fdt parameter to dtb?

> +	int ret, addr_cells, size_cells, entry_size;
> +	u8 value[16];
> +
> +	remove_ima_buffer(dtb, chosen_node);

In the powerpc version, there's an if here to return early if
image->arch.ima_buffer_size == 0. Is there a reason why you removed it?

> +
> +	ret = get_addr_size_cells(&addr_cells, &size_cells);
> +	if (ret)
> +		return ret;
> +
> +	entry_size = 4 * (addr_cells + size_cells);
> +
> +	if (entry_size > sizeof(value))
> +		return -EINVAL;
> +
> +	ret = write_number(value, image->arch.ima_buffer_addr, addr_cells);
> +	if (ret)
> +		return ret;
> +
> +	ret = write_number(value + 4 * addr_cells, image->arch.ima_buffer_size,
> +			size_cells);
> +	if (ret)
> +		return ret;
> +
> +	ret = fdt_setprop(dtb, chosen_node, FDT_PROP_KEXEC_BUFFER, value,
> +			  entry_size);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	ret = fdt_add_mem_rsv(dtb, image->segment[0].mem,
> +			      image->segment[0].memsz);

powerpc uses image->arch.ima_buffer_addr and image->arch.ima_buffer_size
here. Why do you use segment[0] above?

> +	if (ret)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +#endif /* CONFIG_IMA_KEXEC */
> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
> index 58871333737a..c05ad6b74b62 100644
> --- a/arch/arm64/kernel/machine_kexec_file.c
> +++ b/arch/arm64/kernel/machine_kexec_file.c
> @@ -21,6 +21,7 @@
>  #include <linux/types.h>
>  #include <linux/vmalloc.h>
>  #include <asm/byteorder.h>
> +#include <asm/ima.h>
>
>  /* relevant device tree properties */
>  #define FDT_PROP_INITRD_START	"linux,initrd-start"
> @@ -85,6 +86,11 @@ static int setup_dtb(struct kimage *image,
>  			goto out;
>  	}
>
> +	/* add ima_buffer */
> +	ret = setup_ima_buffer(image, dtb, off);
> +	if (ret)
> +		goto out;
> +
>  	/* add kaslr-seed */
>  	ret = fdt_delprop(dtb, off, FDT_PROP_KASLR_SEED);
>  	if  (ret == -FDT_ERR_NOTFOUND)
> @@ -114,6 +120,39 @@ static int setup_dtb(struct kimage *image,
>   */
>  #define DTB_EXTRA_SPACE 0x1000
>
> +
> +/**
> + * 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, &rsv_start, &rsv_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;
> +}

This functions is the same as the one in powerpc, and should be shared too.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [RFC][PATCH 1/1] Carry ima measurement log for arm64 via kexec_file_load

[prsriva]

On 8/30/19 5:11 PM, Thiago Jung Bauermann wrote:
> Hello Prakhar,
>
> Answering this part from the cover letter:
>
>> The code is in most part same as powerpc, i want to get feedback as to
>> how/correct way to refactor the code so that cross architecture
>> partial helpers can be put in a common place.

I started refactoring code to bring helpers under drivers/of, but

i soon reliazed the current implementation can be changed a bit

so that some of the additional functions can be sourced from

existing fdt_*/of_* functions since the fdt_ima was seeming to be

an overkill. I have done so in the V1 patch and also addressed

comments you have.

Hopefully its(v1) is a cleaner approach.

- Thanks for the review, and guidance.

Thanks,

Prakhar Srivastava

> That's a great idea. If it could go to drivers/of/ as Stephen Boyd
> mentioned in the other email that would be great.
>
> More comments below.
> -Addressed those in the v1 patch
> Prakhar Srivastava <prsriva@linux.microsoft.com> writes:
>

Re: [RFC][PATCH 1/1] Carry ima measurement log for arm64 via kexec_file_load

[Mimi Zohar]

Hi Prakhar,

On Fri, 2019-09-06 at 16:56 -0700, prsriva wrote:
> On 8/30/19 5:11 PM, Thiago Jung Bauermann wrote:
> > Hello Prakhar,
> >
> > Answering this part from the cover letter:
> >
> >> The code is in most part same as powerpc, i want to get feedback as to
> >> how/correct way to refactor the code so that cross architecture
> >> partial helpers can be put in a common place.
> 
> I started refactoring code to bring helpers under drivers/of, but
> 
> i soon reliazed the current implementation can be changed a bit
> 
> so that some of the additional functions can be sourced from
> 
> existing fdt_*/of_* functions since the fdt_ima was seeming to be
> 
> an overkill. I have done so in the V1 patch and also addressed
> 
> comments you have.
> 
> Hopefully its(v1) is a cleaner approach.
> 
> - Thanks for the review, and guidance.

"Carrying over the ima log during kexec_file_load" was originally
posted on 5/10 and 5/31 without a cover letter. On 8/29 it was
reposted as an RFC with a cover letter.  The cover letter was v1, but
the patch itself was not.  In the future, please use the "git format-
patch "-subject-prefix" option to add the version number to both the
cover letter and the patches.

The comments you received were based on the 8/29 version.  I haven't
seen anything after that. 

Mimi


> > That's a great idea. If it could go to drivers/of/ as Stephen Boyd
> > mentioned in the other email that would be great.
> >
> > More comments below.
> > -Addressed those in the v1 patch
> > Prakhar Srivastava <prsriva@linux.microsoft.com> writes:
> >

Re: [RFC][PATCH 1/1] Carry ima measurement log for arm64 via kexec_file_load

[prsriva]

On 9/8/19 4:31 PM, Mimi Zohar wrote:
> Hi Prakhar,
> "Carrying over the ima log during kexec_file_load" was originally
> posted on 5/10 and 5/31 without a cover letter. On 8/29 it was
> reposted as an RFC with a cover letter.  The cover letter was v1, but
> the patch itself was not.  In the future, please use the "git format-
> patch "-subject-prefix" option to add the version number to both the
> cover letter and the patches.
>
> The comments you received were based on the 8/29 version.  I haven't
> seen anything after that.
>
> Mimi

Added the correct subject prefix, resent patches.

I resent the patches sent at 5/31, since i had no response.

v1 version addresses the comments in the RFC patch.

I have cc-ed you to the email, hopefully its not missed again.

Thankyou for pointing it out.

Thanks,

Prakhar Srivastava