[PATCH V4 0/2] Add support for arm64 to carry ima measurement

[PATCH V4 0/2] Add support for arm64 to carry ima measurement

[Prakhar Srivastava]

Add support to carry ima measurement log
to the next kexec'ed session triggered via kexec_file_load.
- Top of Linux 5.3-rc6

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

Carrying forward the ima measurement log to the next kexec'ed session 
allows a verifying party to get the entire runtime event log since the
last full reboot, since that is when PCRs were last reset.

Tested for arm64 qemu and real hardware.
I have not been unable to test the patch for powerpc 64bit. Any testing
is greatly appretiated.

TODO: Add support for 32 bit in the of_ima.c
v4:
  - Fix issue with HAVE_* config wrongly used.

v3:
  - Fix build breaks due to bad config.

v2:
  - move common code to drivers/of/of_ima.c.
  - point arm64 to use of_ima implementation.
  - point powerpc to use of_ima implementation

v1:
  - add new fdt porperties to mark start and end for ima measurement
    log.
  - use fdt_* functions to add/remove fdt properties and memory
    allocations.
  - remove additional check for endian-ness as they are checked
    in fdt_* functions.

v0:
  - Add support to carry ima measurement log in arm64, 
   uses same code as powerpc.

Prakhar Srivastava (2):
  Add support for arm64 to carry ima measurement log in kexec_file_load
  update powerpc implementation to call into of_ima*

 arch/arm64/Kconfig                     |   1 +
 arch/arm64/include/asm/ima.h           |  24 +++
 arch/arm64/include/asm/kexec.h         |   5 +
 arch/arm64/kernel/Makefile             |   1 +
 arch/arm64/kernel/ima_kexec.c          |  78 ++++++++++
 arch/arm64/kernel/machine_kexec_file.c |   6 +
 arch/powerpc/include/asm/ima.h         |   5 -
 arch/powerpc/kernel/Makefile           |   3 -
 arch/powerpc/kernel/ima_kexec.c        | 170 ++-------------------
 drivers/of/Kconfig                     |   6 +
 drivers/of/Makefile                    |   1 +
 drivers/of/of_ima.c                    | 204 +++++++++++++++++++++++++
 include/linux/of.h                     |  31 ++++
 13 files changed, 371 insertions(+), 164 deletions(-)
 create mode 100644 arch/arm64/include/asm/ima.h
 create mode 100644 arch/arm64/kernel/ima_kexec.c
 create mode 100644 drivers/of/of_ima.c

-- 
2.17.1

[PATCH V4 1/2] Add support for arm64 to carry ima measurement log in kexec_file_load

[Prakhar Srivastava]

During kexec_file_load, carrying forward the ima measurement log allows
a verifying party to get the entire runtime event log since the last
full reboot since that is when PCRs were last reset.

Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
---
 arch/arm64/Kconfig                     |   1 +
 arch/arm64/include/asm/ima.h           |  24 +++
 arch/arm64/include/asm/kexec.h         |   5 +
 arch/arm64/kernel/Makefile             |   1 +
 arch/arm64/kernel/ima_kexec.c          |  78 ++++++++++
 arch/arm64/kernel/machine_kexec_file.c |   6 +
 drivers/of/Kconfig                     |   6 +
 drivers/of/Makefile                    |   1 +
 drivers/of/of_ima.c                    | 204 +++++++++++++++++++++++++
 include/linux/of.h                     |  31 ++++
 10 files changed, 357 insertions(+)
 create mode 100644 arch/arm64/include/asm/ima.h
 create mode 100644 arch/arm64/kernel/ima_kexec.c
 create mode 100644 drivers/of/of_ima.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 3adcec05b1f6..ae48869a131a 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -958,6 +958,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..287dbcefd15e
--- /dev/null
+++ b/arch/arm64/include/asm/ima.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARM64_IMA_H
+#define _ASM_ARM64_IMA_H
+
+struct kimage;
+
+int ima_get_kexec_buffer(void **addr, size_t *size);
+int ima_free_kexec_buffer(void);
+void remove_ima_buffer(void *fdt, int chosen_node);
+
+#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..e8d2412066e7 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -96,6 +96,11 @@ static inline void crash_post_resume(void) {}
 struct kimage_arch {
 	void *dtb;
 	unsigned long dtb_mem;
+
+#ifdef CONFIG_IMA_KEXEC
+	phys_addr_t ima_buffer_addr;
+	size_t ima_buffer_size;
+#endif
 };
 
 extern const struct kexec_file_ops kexec_image_ops;
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 478491f07b4f..8f3de48179c9 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_RANDOMIZE_BASE)		+= kaslr.o
 obj-$(CONFIG_HIBERNATION)		+= hibernate.o hibernate-asm.o
 obj-$(CONFIG_KEXEC_CORE)		+= machine_kexec.o relocate_kernel.o	\
 					   cpu-reset.o
+obj-$(CONFIG_HAVE_IMA_KEXEC)		+= ima_kexec.o
 obj-$(CONFIG_KEXEC_FILE)		+= machine_kexec_file.o kexec_image.o
 obj-$(CONFIG_ARM64_RELOC_TEST)		+= arm64-reloc-test.o
 arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c
new file mode 100644
index 000000000000..c2450aaa42c8
--- /dev/null
+++ b/arch/arm64/kernel/ima_kexec.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Authors:
+ * Prakhar Srivastava <prsriva@linux.microsoft.com>
+ */
+
+#include <linux/kexec.h>
+#include <linux/of.h>
+
+/**
+ * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
+ * The IMA measurement buffer once read needs to be freed.
+ */
+void remove_ima_buffer(void *fdt, int chosen_node)
+{
+	fdt_remove_ima_buffer(fdt, chosen_node);
+}
+
+/**
+ * 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)
+{
+	return of_get_ima_buffer(addr, size);
+}
+
+/**
+ * ima_free_kexec_buffer - free memory used by the IMA buffer
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int ima_free_kexec_buffer(void)
+{
+	return of_remove_ima_buffer();
+}
+
+#ifdef CONFIG_IMA_KEXEC
+/**
+ * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA
+ *	measurement log.
+ * @image: - pointer to the kimage, to store the address and size of the
+ *	IMA measurement log.
+ * @load_addr: - the address where the IMA measurement log is stored.
+ * @size - size of the IMA measurement log.
+ *
+ * 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;
+}
+
+/**
+ * setup_ima_buffer - update the fdt to contain the ima mesasurement log
+ * @image: - pointer to the kimage, containing the address and size of
+ *	     the IMA measurement log.
+ * @fdt: - pointer to the fdt.
+ * @chosen_node: - node under which property is to be defined.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node)
+{
+	return fdt_setup_ima_buffer(image->arch.ima_buffer_addr,
+				    image->arch.ima_buffer_size,
+				    fdt, chosen_node);
+
+}
+#endif /* CONFIG_IMA_KEXEC */
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 58871333737a..86d57198d739 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 measuremnet log buffer */
+	ret = setup_ima_buffer(image, dtb, off);
+	if (ret < 0)
+		goto out;
+
 	/* add kaslr-seed */
 	ret = fdt_delprop(dtb, off, FDT_PROP_KASLR_SEED);
 	if  (ret == -FDT_ERR_NOTFOUND)
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 37c2ccbefecd..167c6ecb64aa 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -103,4 +103,10 @@ config OF_OVERLAY
 config OF_NUMA
 	bool
 
+config OF_IMA
+	def_bool y
+	help
+	  IMA related wrapper functions to add/remove ima measurement logs during
+	  kexec_file_load call.
+
 endif # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 663a4af0cccd..b4caf083df4e 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -14,5 +14,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
 obj-$(CONFIG_OF_RESOLVE)  += resolver.o
 obj-$(CONFIG_OF_OVERLAY) += overlay.o
 obj-$(CONFIG_OF_NUMA) += of_numa.o
+obj-$(CONFIG_OF_IMA) += of_ima.o
 
 obj-$(CONFIG_OF_UNITTEST) += unittest-data/
diff --git a/drivers/of/of_ima.c b/drivers/of/of_ima.c
new file mode 100644
index 000000000000..cfc3cb4522b0
--- /dev/null
+++ b/drivers/of/of_ima.c
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ */
+
+#include <linux/slab.h>
+#include <linux/kexec.h>
+#include <linux/of.h>
+#include <linux/memblock.h>
+#include <linux/libfdt.h>
+
+/**
+ * delete_fdt_mem_rsv - delete memory reservation with given address and size
+ * @fdt - pointer to the fdt.
+ * @start - start address of the memory.
+ * @size - number of cells to be deletd.
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int fdt_delete_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 < 0) {
+			pr_err("Malformed device tree\n");
+			return ret;
+		}
+
+		if (rsv_start == start && rsv_size == size) {
+			ret = fdt_del_mem_rsv(fdt, i);
+			if (ret < 0) {
+				pr_err("Error deleting device tree reservation\n");
+				return ret;
+			}
+
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}
+
+/**
+ * of_get_ima_buffer_properties - get the properties for ima buffer
+ * @ima_buf_start - start of the ima buffer
+ * @ima_buf_end - end of the ima buffer
+ *	If any one of the properties is not found. The device tree
+ *	is malformed or something went wrong.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int of_get_ima_buffer_properties(void **ima_buf_start, void **ima_buf_end)
+{
+	struct property *pproperty;
+
+	pproperty = of_find_property(of_chosen, "linux,ima-kexec-buffer",
+				    NULL);
+	*ima_buf_start = pproperty ? pproperty->value : NULL;
+
+	pproperty = of_find_property(of_chosen, "linux,ima-kexec-buffer-end",
+				    NULL);
+	*ima_buf_end = pproperty ? pproperty->value : NULL;
+
+	if (!*ima_buf_start || !*ima_buf_end)
+		return -EINVAL;
+
+	return 0;
+}
+
+/**
+ * of_remove_ima_buffer - free memory used by the IMA buffer
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int of_remove_ima_buffer(void)
+{
+	int ret;
+	void *ima_buf_start, *ima_buf_end;
+	uint64_t buf_start, buf_end;
+
+	ret = of_get_ima_buffer_properties(&ima_buf_start, &ima_buf_end);
+	if (ret < 0)
+		return ret;
+
+	buf_start = fdt64_to_cpu(*((const fdt64_t *) ima_buf_start));
+	buf_end = fdt64_to_cpu(*((const fdt64_t *) ima_buf_end));
+
+	ret = of_remove_property(of_chosen, ima_buf_start);
+	if (ret < 0)
+		return ret;
+
+	ret = of_remove_property(of_chosen, ima_buf_end);
+	if (ret < 0)
+		return ret;
+
+	return memblock_free(buf_start, buf_end - buf_start);
+}
+
+/**
+ * of_get_ima_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 of_get_ima_buffer(void **addr, size_t *size)
+{
+	int ret;
+	void *ima_buf_start, *ima_buf_end;
+	uint64_t buf_start, buf_end;
+
+	ret = of_get_ima_buffer_properties(&ima_buf_start, &ima_buf_end);
+	if (ret < 0)
+		return ret;
+
+	buf_start = fdt64_to_cpu(*((const fdt64_t *) ima_buf_start));
+	buf_end = fdt64_to_cpu(*((const fdt64_t *) ima_buf_end));
+
+	*addr = __va(buf_start);
+	*size = buf_end - buf_start;
+
+	return 0;
+}
+
+/**
+ * fdt_remove_ima_buffer - remove the IMA buffer property and reservation
+ * @fdt - pointer the fdt.
+ * @chosen_node - node under which property can be found.
+ *
+ * The IMA measurement buffer is either read by now and freeed or a kexec call
+ * needs to replace the ima measurement buffer, clear the property and memory
+ * reservation.
+ */
+void fdt_remove_ima_buffer(void *fdt, int chosen_node)
+{
+	int ret, len;
+	const void *prop;
+	uint64_t tmp_start, tmp_end;
+
+	prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
+	if (prop) {
+		tmp_start = fdt64_to_cpu(*((const fdt64_t *) prop));
+
+		prop = fdt_getprop(fdt, chosen_node,
+				   "linux,ima-kexec-buffer-end", &len);
+		if (!prop)
+			return;
+
+		tmp_end = fdt64_to_cpu(*((const fdt64_t *) prop));
+
+		ret = fdt_delete_mem_rsv(fdt, tmp_start, tmp_end - tmp_start);
+
+		if (ret == 0)
+			pr_debug("Removed old IMA buffer reservation.\n");
+		else if (ret != -ENOENT)
+			return;
+
+		fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
+		fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer-end");
+	}
+}
+
+/**
+ * fdt_setup_ima_buffer - update the fdt to contain the ima mesasurement log
+ * @image: - pointer to the kimage, containing the address and size of
+ *	     the IMA measurement log.
+ * @fdt: - pointer to the fdt.
+ * @chosen_node: - node under which property is to be defined.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int fdt_setup_ima_buffer(const phys_addr_t ima_buffer_addr,
+	const size_t ima_buffer_size,
+	void *fdt, int chosen_node)
+{
+	int ret;
+
+	fdt_remove_ima_buffer(fdt, chosen_node);
+
+	if (!ima_buffer_addr)
+		return 0;
+
+	ret = fdt_setprop_u64(fdt, chosen_node, "linux,ima-kexec-buffer",
+			      ima_buffer_addr);
+	if (ret < 0)
+		return ret;
+
+	ret = fdt_setprop_u64(fdt, chosen_node, "linux,ima-kexec-buffer-end",
+			      ima_buffer_addr +
+			      ima_buffer_size);
+	if (ret < 0)
+		return ret;
+
+	ret = fdt_add_mem_rsv(fdt, ima_buffer_addr,
+			      ima_buffer_size);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
diff --git a/include/linux/of.h b/include/linux/of.h
index 844f89e1b039..c24baca43f18 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1477,4 +1477,35 @@ static inline int of_overlay_notifier_unregister(struct notifier_block *nb)
 
 #endif
 
+#ifdef CONFIG_OF_IMA
+int of_remove_ima_buffer(void);
+int of_get_ima_buffer(void **addr, size_t *size);
+void fdt_remove_ima_buffer(void *fdt, int chosen_node);
+int fdt_setup_ima_buffer(const phys_addr_t ima_buffer_addr,
+	const size_t ima_buffer_size,
+	void *fdt, int chosen_node);
+#else
+static inline int of_remove_ima_buffer(void)
+{
+	return -ENOTSUPP;
+};
+
+static inline int of_get_ima_buffer(void **addr, size_t *size)
+{
+	return -ENOTSUPP;
+};
+
+static inline void fdt_remove_ima_buffer(void *fdt, int chosen_node)
+{
+	return;
+};
+
+static inline int fdt_setup_ima_buffer(const phys_addr_t ima_buffer_addr,
+	const size_t ima_buffer_size, void *fdt, int chosen_node)
+{
+	return -ENOTSUPP;
+};
+
+#endif
+
 #endif /* _LINUX_OF_H */
-- 
2.17.1

[PATCH V4 2/2] update powerpc implementation to call into of_ima*

[Prakhar Srivastava]

update powerpc ima buffer pass implementationt to call into
of_ima* for a cross architecture support.

Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
---
 arch/powerpc/include/asm/ima.h  |   5 -
 arch/powerpc/kernel/Makefile    |   3 -
 arch/powerpc/kernel/ima_kexec.c | 170 +++-----------------------------
 3 files changed, 14 insertions(+), 164 deletions(-)

diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
index ead488cf3981..f50a4f622f3d 100644
--- a/arch/powerpc/include/asm/ima.h
+++ b/arch/powerpc/include/asm/ima.h
@@ -6,12 +6,7 @@ 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,
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 56dfa7a2a6f2..339aaae7ed3e 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -128,11 +128,8 @@ obj-$(CONFIG_KEXEC_CORE)	+= machine_kexec.o crash.o \
 				   machine_kexec_$(BITS).o
 obj-$(CONFIG_KEXEC_FILE)	+= machine_kexec_file_$(BITS).o kexec_elf_$(BITS).o
 ifdef CONFIG_HAVE_IMA_KEXEC
-ifdef CONFIG_IMA
 obj-y				+= ima_kexec.o
 endif
-endif
-
 obj-$(CONFIG_AUDIT)		+= audit.o
 obj64-$(CONFIG_AUDIT)		+= compat_audit.o
 
diff --git a/arch/powerpc/kernel/ima_kexec.c b/arch/powerpc/kernel/ima_kexec.c
index 720e50e490b6..41f52297de0c 100644
--- a/arch/powerpc/kernel/ima_kexec.c
+++ b/arch/powerpc/kernel/ima_kexec.c
@@ -6,45 +6,21 @@
  * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
  */
 
-#include <linux/slab.h>
 #include <linux/kexec.h>
 #include <linux/of.h>
-#include <linux/memblock.h>
-#include <linux/libfdt.h>
 
-static int get_addr_size_cells(int *addr_cells, int *size_cells)
+/**
+ * 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)
 {
-	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;
+	fdt_remove_ima_buffer(fdt, chosen_node);
+	return;
 }
 
-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
@@ -55,23 +31,7 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
  */
 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;
+	return of_get_ima_buffer(addr, size);
 }
 
 /**
@@ -79,52 +39,7 @@ int ima_get_kexec_buffer(void **addr, size_t *size)
  */
 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");
+	return of_remove_ima_buffer();
 }
 
 #ifdef CONFIG_IMA_KEXEC
@@ -145,27 +60,6 @@ int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
 	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.
@@ -176,44 +70,8 @@ static int write_number(void *p, u64 value, int cells)
  */
 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)
-		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(fdt, chosen_node, "linux,ima-kexec-buffer", value,
-			  entry_size);
-	if (ret < 0)
-		return -EINVAL;
-
-	ret = fdt_add_mem_rsv(fdt, image->arch.ima_buffer_addr,
-			      image->arch.ima_buffer_size);
-	if (ret)
-		return -EINVAL;
-
-	pr_debug("IMA buffer at 0x%llx, size = 0x%zx\n",
-		 image->arch.ima_buffer_addr, image->arch.ima_buffer_size);
-
-	return 0;
+	return fdt_setup_ima_buffer(image->arch.ima_buffer_addr,
+				    image->arch.ima_buffer_size,
+				    fdt, chosen_node);
 }
 #endif /* CONFIG_IMA_KEXEC */
-- 
2.17.1

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[James Morse]

Hi Prakhar,

(You've CC'd a few folk who work for 'arm.org'...)

On 11/10/2019 01:35, Prakhar Srivastava wrote:
> Add support to carry ima measurement log
> to the next kexec'ed session triggered via kexec_file_load.

I don't know much about 'ima', I'm assuming its the list of 'stuff' that has already been
fed into the TPM as part of SecureBoot. Please forgive the stupid questions,


> Currently during kexec the kernel file signatures are/can be validated
> prior to actual load, the information(PE/ima signature) is not carried
> to the next session. This lead to loss of information.
> 
> Carrying forward the ima measurement log to the next kexec'ed session 
> allows a verifying party to get the entire runtime event log since the
> last full reboot, since that is when PCRs were last reset.

Hmm, You're adding this as a linux-specific thing in the chosen node, which points at a
memreserve.

The question that normally needs answering when adding to the stuff we have to treat as
ABI over kexec is: how would this work from a bootloader that isn't kexec? Does it need to
work for non-linux OS?

Changing anything other than the chosen node of the DT isn't something the kernel should
be doing. I suspect if you need reserved memory for this stuff, it should be carved out by
the bootloader, and like all other memreserves: should not be moved or deleted.

('fdt_delete_mem_rsv()' is a terrifying idea, we depend on the memreserve nodes to tell
use which 'memory' we shouldn't touch!)


Sharing with powerpc is a great starting point ... but, how does this work for ACPI systems?
How does this work if I keep kexecing between ACPI and DT?

I'd prefer it we only had one way this works on arm64, so whatever we do has to cover both.

Does ima work without UEFI secure-boot?
If not, the Linux-specific UEFI 'memreserve' table might be a better fit, this would be
the same for both DT and ACPI systems. Given U-boot supports the UEFI API too, its
probably the right thing to do regardless of secure-boot.

It looks like x86 doesn't support this either yet. If we have to add something to support
ACPI, it would be good if it covers both firmware mechanisms for arm64, and works for x86
in the same way.

(How does this thing interact with EFI's existing efi_tpm_eventlog_init()?)


Thanks,

James

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[prsriva]

On 10/14/19 11:02 AM, James Morse wrote:
> Hi Prakhar,
> 
> (You've CC'd a few folk who work for 'arm.org'...)
> 
> On 11/10/2019 01:35, Prakhar Srivastava wrote:
>> Add support to carry ima measurement log
>> to the next kexec'ed session triggered via kexec_file_load.
> 
> I don't know much about 'ima', I'm assuming its the list of 'stuff' that has already been
> fed into the TPM as part of SecureBoot. Please forgive the stupid questions,
> 
The IMA logs are event logs for module load time signature 
validation(based on policies) which are backed by the TPM. No SecureBoot 
information is present in the log other than the boot aggregate.

>> Currently during kexec the kernel file signatures are/can be validated
>> prior to actual load, the information(PE/ima signature) is not carried
>> to the next session. This lead to loss of information.
>>
>> Carrying forward the ima measurement log to the next kexec'ed session
>> allows a verifying party to get the entire runtime event log since the
>> last full reboot, since that is when PCRs were last reset.
> 
> Hmm, You're adding this as a linux-specific thing in the chosen node, which points at a
> memreserve.
> 
> The question that normally needs answering when adding to the stuff we have to treat as
> ABI over kexec is: how would this work from a bootloader that isn't kexec? Does it need to
> work for non-linux OS?
> 
This change is only intended to be executed in the path of 
kexec_file_load and not intended to be executed by any boot loader.(Not 
very aware of boot loader calls.). The logs are non intended to be 
injected by the boot loader at all.
The change is configurable(CONFIG_IMA_KEXEC) under the IMA subsection 
and can be disabled if not needed.

> Changing anything other than the chosen node of the DT isn't something the kernel should
> be doing. I suspect if you need reserved memory for this stuff, it should be carved out by
> the bootloader, and like all other memreserves: should not be moved or deleted.
> 
> ('fdt_delete_mem_rsv()' is a terrifying idea, we depend on the memreserve nodes to tell
> use which 'memory' we shouldn't touch!)
> 
fdt_delete_mem_rsv - is to cleanup any memory that's been mistakenly 
still lying around in the same session while creating the fdt. 
memblock_free is actually used to free up the reserved memory.

Thiago may have more insight, This is primarily a code that's been 
ported from existing kernel for PowerPC.
https://github.com/torvalds/linux/blob/master/arch/powerpc/kernel/machine_kexec_file_64.c

> 
> Sharing with powerpc is a great starting point ... but, how does this work for ACPI systems?
> How does this work if I keep kexecing between ACPI and DT?
> 

I don't have an answer to this, just going through the call stack i dont 
believe it depends on ACPI as such. I am not the expert here, but more 
than willing to try out the scenario in question.(Can you point me to 
some documentation to setup some environment to test this.) 
Kexec_file_load call depends purely on DT implementation.


> I'd prefer it we only had one way this works on arm64, so whatever we do has to cover both.
I can move the code to be only part of arm64 arch if absolutely 
necessary. Thiago do you have any concerns on going back the path of 
multiple code paths?

>
> Does ima work without UEFI secure-boot?
Yes, IMA, the measurement is not dependent on any hardware capabilities.
TPM is needed to back the measurements but the IMA module will not fail 
if TPM is not available.
In short Secure-boot has no impact on IMA.

> If not, the Linux-specific UEFI 'memreserve' table might be a better fit, this would be
> the same for both DT and ACPI systems. Given U-boot supports the UEFI API too, its
> probably the right thing to do regardless of secure-boot.
> 
> It looks like x86 doesn't support this either yet. If we have to add something to support
> ACPI, it would be good if it covers both firmware mechanisms for arm64, and works for x86
> in the same way.
> 
> (How does this thing interact with EFI's existing efi_tpm_eventlog_init()?)
> 
IMA does not interact with the TPM event log.
Only one of the PCR's is extended but not logged in the TPM logs. The 
logging is done in IMA. The IMA measurement log in question is whats 
needed to be carried over to via kexec_file_load call.

I am not sure if i addressed all your concerns, please let me know
if i missed anything. To me most concerns look to be towards the kexec 
case and dependency on hardware(ACPI/TPM) during boot and early boot 
services, where as carrying the logs is only during the kexec_file_load 
sys call and does not interfere with that code path.
IMA documentation: https://sourceforge.net/p/linux-ima/wiki/Home/

Prakhar Srivastava
> 
> Thanks,
> 
> James
> 

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[James Morse]

Hi Prakhar,

(CC: +Ard : passing reserved memory between kernels using Kexec?)

On 15/10/2019 02:31, prsriva wrote:
> On 10/14/19 11:02 AM, James Morse wrote:
>> On 11/10/2019 01:35, Prakhar Srivastava wrote:
>>> Add support to carry ima measurement log
>>> to the next kexec'ed session triggered via kexec_file_load.
>>
>> I don't know much about 'ima', I'm assuming its the list of 'stuff' that has already been
>> fed into the TPM as part of SecureBoot. Please forgive the stupid questions,
>>
> The IMA logs are event logs for module load time signature validation(based on policies)
> which are backed by the TPM. No SecureBoot information is present in the log other than
> the boot aggregate.

Okay, so SecureBoot is optional with this thing.


>>> Currently during kexec the kernel file signatures are/can be validated
>>> prior to actual load, the information(PE/ima signature) is not carried
>>> to the next session. This lead to loss of information.
>>>
>>> Carrying forward the ima measurement log to the next kexec'ed session
>>> allows a verifying party to get the entire runtime event log since the
>>> last full reboot, since that is when PCRs were last reset.
>>
>> Hmm, You're adding this as a linux-specific thing in the chosen node, which points at a
>> memreserve.
>>
>> The question that normally needs answering when adding to the stuff we have to treat as
>> ABI over kexec is: how would this work from a bootloader that isn't kexec? Does it need to
>> work for non-linux OS?

> This change is only intended to be executed in the path of kexec_file_load and not
> intended to be executed by any boot loader.(Not very aware of boot loader calls.).

kexec_file_load only means something to the first kernel. If you boot something that isn't
linux, does it need to delete this stuff from the DT?
Even if you kexec_file_load linux, it could go on to regular-kexec something that is
not... what should that do with these things?

Other than the chosen node, the DT is treated as a cast-iron description of the platform,
we shouldn't be tinkering with it.

If its not describing hardware, it probably doesn't belong in the DT.


> The logs are non intended to be injected by the boot loader at all.

You're using linux as a bootloader with kexec. We have to treat the stuff that gets passed
between kernels as ABI, as people expect to be able to kexec to a newer kernel.

Is linux-as-a-bootloader special? Or should we work out what any bootloader should do here
first. This avoids having to change this when it turns out someone wants to log UEFI
DXE-drivers/modules in the TPM too.

From the git-log of the ima code it looks like this is some linux-specific format.
Are we certain it will never change, and nothing else ever needs to support it?
(e.g. the DXE driver example above. Is there another way that sort of thing would work?).


> The change is configurable(CONFIG_IMA_KEXEC) under the IMA subsection and can be disabled
> if not needed.

Sure, but not needed isn't the same as not supported.
If we support it at all, we need to cover everything that needs supporting. If its ABI (we
treat data passed between kernels as if it is), we need to get it right first time.

(my point? We need to get the ACPI story sorted before we add any support... otherwise we
end up with two incompatible ways of doing this).

[...]

>> Sharing with powerpc is a great starting point ... but, how does this work for ACPI
>> systems?
>> How does this work if I keep kexecing between ACPI and DT?

> I don't have an answer to this, just going through the call stack i dont believe it
> depends on ACPI as such. I am not the expert here, but more than willing to try out the
> scenario in question.(Can you point me to some documentation to setup some environment to
> test this.)

Yup: Documentation/arm64/arm-acpi.rst

Arm64's ACPI support depends on UEFI. As a starter:
https://wiki.ubuntu.com/ARM64/QEMU

You may need to pass 'acpi=on' on the commandline if your UEFI build supports both DT and
ACPI. The x86 name for UEFI-in-Qemu is OVMF, which helps when googling.


> Kexec_file_load call depends purely on DT implementation.

Heh. And it works with ACPI too! You'll note it only touches things in the chosen node...

An ACPI system boots without a DT. Linux's EFI-stub can make API calls and poke around in
the UEFI structures to find out about the system. When it finishes, the EFI-stub needs to
pass on a set of values to the kernel... we need some kind of key-value store ...

To avoid re-inventing the wheel, the EFI-stub creates an empty DT, and shoves the cmdline,
the initrd etc in there... just like a DT bootloader would have done.

From drivers/firmware/efi/libstub/arm-stub.c::efi_entry()
|	if (!fdt_addr)
|		pr_efi(sys_table, "Generating empty DTB\n");

(you will see this message on an ACPI-only system)

On an ACPI system there won't be anything else in the DT, other than the chosen node.

When booted with UEFI, the memory is described in the UEFI memory-map. An ACPI system
doesn't know to look for memreserve nodes in the DT. (it might work by accident, but I
wouldn't rely on it).


>> I'd prefer it we only had one way this works on arm64, so whatever we do has to cover both.

> I can move the code to be only part of arm64 arch if absolutely necessary. Thiago do you
> have any concerns on going back the path of multiple code paths?

Because arm64 needs to support ACPI too, I think its support for this will always look
different to PowerPCs.

I think the UEFI persistent-memory-reservations thing is a better fit for this [0][1].

As U-boot supports booting the kernel via the UEFI stub, I think this is all we need to
support this. (If your platform supports LPIs, you also need this UEFI table to kexec
safely anyway. See [1])


Removing something that was reserved memory might be tricky, I don't think we do this
anywhere else. Why do you need to remove the reservations?


>> Does ima work without UEFI secure-boot?

> Yes, IMA, the measurement is not dependent on any hardware capabilities.
> TPM is needed to back the measurements but the IMA module will not fail if TPM is not
> available.
> In short Secure-boot has no impact on IMA.

(thanks)


>> If not, the Linux-specific UEFI 'memreserve' table might be a better fit, this would be
>> the same for both DT and ACPI systems. Given U-boot supports the UEFI API too, its
>> probably the right thing to do regardless of secure-boot.
>>
>> It looks like x86 doesn't support this either yet. If we have to add something to support
>> ACPI, it would be good if it covers both firmware mechanisms for arm64, and works for x86
>> in the same way.
>>
>> (How does this thing interact with EFI's existing efi_tpm_eventlog_init()?)

> IMA does not interact with the TPM event log.
> Only one of the PCR's is extended but not logged in the TPM logs. The logging is done in
> IMA. The IMA measurement log in question is whats needed to be carried over to via
> kexec_file_load call.

If SecureBoot isn't relevant, I'm confused as to why kexec_file_load() is.

I thought kexec_file_load() only existed because SecureBoot systems need to validate the
new OS images's signature before loading it, and we can't trust user-space calling Kexec
to do this.

If there is no secure boot, why does this thing only work with kexec_file_load()?
(good news! With the UEFI memreseve table, it should work transparently with regular kexec
too)


> I am not sure if i addressed all your concerns, please let me know
> if i missed anything. To me most concerns look to be towards the kexec case and dependency
> on hardware(ACPI/TPM) during boot and early boot services, where as carrying the logs is
> only during the kexec_file_load sys call and does not interfere with that code path.
> IMA documentation: https://sourceforge.net/p/linux-ima/wiki/Home/

Supporting ACPI in the same way is something we need to do from day one. kexec_file_load()
already does this. I'm not sure "only kexec_file_load()" is a justifiable restriction...


Thanks,

James

[0] https://marc.info/?l=linux-efi&m=153754757208163&w=2
[1] https://lore.kernel.org/lkml/20180921195954.21574-1-marc.zyngier@arm.com/

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[Pavel Tatashin]

> I think the UEFI persistent-memory-reservations thing is a better fit for this [0][1].

Hi James,

Thank you for your thought. As I understand you propose the to use the
existing method as such:
1. Use the existing kexec ABI to pass reservation from kernel to
kernel using EFI the same as is done for GICv3 tables.
2. Allow this memory to be reservable only during first Linux boot via
EFI memory reserve
3. Allow to have this memory pre-reserved by firmware or to be
embedded into device tree.

A question I have is how to tell that a reserved region is reserved
for IMA use. With GICv3 it is done by reading the registers, finding
the interrupt tables memory, and check that the memory ranges are
indeed pre-reserved.

Is there a way to name memory with the current ABI that you think is acceptable?

Thank you,
Pasha

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[James Morris]

On Tue, 15 Oct 2019, James Morse wrote:

> > The IMA logs are event logs for module load time signature validation(based on policies)
> > which are backed by the TPM. No SecureBoot information is present in the log other than
> > the boot aggregate.
> 
> Okay, so SecureBoot is optional with this thing.

Correct. Verified boot is one alternative.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[Mimi Zohar]

Hi James,

On Tue, 2019-10-15 at 18:39 +0100, James Morse wrote:
> If SecureBoot isn't relevant, I'm confused as to why kexec_file_load() is.
> 
> I thought kexec_file_load() only existed because SecureBoot systems need to validate the
> new OS images's signature before loading it, and we can't trust user-space calling Kexec
> to do this.
> 
> If there is no secure boot, why does this thing only work with kexec_file_load()?
> (good news! With the UEFI memreseve table, it should work transparently with regular kexec
> too)

I'm so sorry for the confusion.  IMA was originally limited to
extending trusted boot concepts to the OS.  As of Linux 3.10, IMA
added support for extending secure boot concepts and auditing file
hashes (commit e7c568e0fd0cf).

True, kexec_file_load is required for verifying the kexec kernel
image, but it is also required for measuring the kexec kernel image as
well.

After reading the kernel image into memory (kernel_read_file_from_fd),
the hash is calculated and then added to the IMA measurement list and
used to extend the TPM.  All of this is based on the IMA policy,
including the TPM PCR.

> 
> > I am not sure if i addressed all your concerns, please let me know
> > if i missed anything. To me most concerns look to be towards the kexec case and dependency
> > on hardware(ACPI/TPM) during boot and early boot services, where as carrying the logs is
> > only during the kexec_file_load sys call and does not interfere with that code path.
> > IMA documentation: https://sourceforge.net/p/linux-ima/wiki/Home/
> 
> Supporting ACPI in the same way is something we need to do from day one. kexec_file_load()
> already does this. I'm not sure "only kexec_file_load()" is a justifiable restriction...

The TPM PCRs are not reset on a soft reboot.  As a result, in order to
validate the IMA measurement list against the TPM PCRs, the IMA
measurement list is saved on kexec load, restored on boot, and then
the memory allocated for carrying the measurement list across kexec is
freed.

Where/how to save the IMA measurement list is architecture dependent.
 Thiago Bauermann implemented allocating and freeing the measurement
list memory for Power.

Mimi

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[prsriva]

On 10/15/19 11:47 AM, Pavel Tatashin wrote:
>> I think the UEFI persistent-memory-reservations thing is a better fit for this [0][1].
> 
> Hi James,
> 
> Thank you for your thought. As I understand you propose the to use the
> existing method as such:
> 1. Use the existing kexec ABI to pass reservation from kernel to
> kernel using EFI the same as is done for GICv3 tables.
> 2. Allow this memory to be reservable only during first Linux boot via
> EFI memory reserve
> 3. Allow to have this memory pre-reserved by firmware or to be
> embedded into device tree.
> 
> A question I have is how to tell that a reserved region is reserved
> for IMA use. With GICv3 it is done by reading the registers, finding
> the interrupt tables memory, and check that the memory ranges are
> indeed pre-reserved.
> 
> Is there a way to name memory with the current ABI that you think is acceptable?
> 
> Thank you,
> Pasha
> 
Friendly ping.

Thanks,
Prakhar Srivastava

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[James Morse]

Hi Mimi,

On 16/10/2019 02:44, Mimi Zohar wrote:
> On Tue, 2019-10-15 at 18:39 +0100, James Morse wrote:
>> If SecureBoot isn't relevant, I'm confused as to why kexec_file_load() is.
>>
>> I thought kexec_file_load() only existed because SecureBoot systems need to validate the
>> new OS images's signature before loading it, and we can't trust user-space calling Kexec
>> to do this.
>>
>> If there is no secure boot, why does this thing only work with kexec_file_load()?
>> (good news! With the UEFI memreseve table, it should work transparently with regular kexec
>> too)

> I'm so sorry for the confusion.  IMA was originally limited to
> extending trusted boot concepts to the OS.  As of Linux 3.10, IMA
> added support for extending secure boot concepts and auditing file
> hashes (commit e7c568e0fd0cf).
> 
> True, kexec_file_load is required for verifying the kexec kernel
> image, but it is also required for measuring the kexec kernel image as
> well.
> 
> After reading the kernel image into memory (kernel_read_file_from_fd),
> the hash is calculated and then added to the IMA measurement list and
> used to extend the TPM.  All of this is based on the IMA policy,
> including the TPM PCR.

Don't we get a set of segments with the regular kexec syscall? These could equally be
hashed and measured, and logged via IMA and/or extending the TPMs measurements.

(obviously this would include the command-line and maybe purgatory, which makes it less
predictable, but these are still the binary blobs that were given privileged access to the
system).


>>> I am not sure if i addressed all your concerns, please let me know
>>> if i missed anything. To me most concerns look to be towards the kexec case and dependency
>>> on hardware(ACPI/TPM) during boot and early boot services, where as carrying the logs is
>>> only during the kexec_file_load sys call and does not interfere with that code path.
>>> IMA documentation: https://sourceforge.net/p/linux-ima/wiki/Home/
>>
>> Supporting ACPI in the same way is something we need to do from day one. kexec_file_load()
>> already does this. I'm not sure "only kexec_file_load()" is a justifiable restriction...

> The TPM PCRs are not reset on a soft reboot.  As a result, in order to
> validate the IMA measurement list against the TPM PCRs, the IMA
> measurement list is saved on kexec load, restored on boot, and then
> the memory allocated for carrying the measurement list across kexec is
> freed.

Hmm, this is why the reserved memory gets freed.

What happens to stuff that happens between kexec-load and boot?
There is a comment:
| /* segment size can't change between kexec load and execute */

But I can't see anywhere that enforces that. I guess those measurements will go missing,
and the TPM value will not match after kexec.



Thanks,

James

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[James Morse]

Hi Pavel,

On 15/10/2019 19:47, Pavel Tatashin wrote:
>> I think the UEFI persistent-memory-reservations thing is a better fit for this [0][1].

> Thank you for your thought. As I understand you propose the to use the
> existing method as such:
> 1. Use the existing kexec ABI to pass reservation from kernel to
> kernel using EFI the same as is done for GICv3 tables.
> 2. Allow this memory to be reservable only during first Linux boot via
> EFI memory reserve
> 3. Allow to have this memory pre-reserved by firmware or to be
> embedded into device tree.
> 
> A question I have is how to tell that a reserved region is reserved
> for IMA use. With GICv3 it is done by reading the registers, finding
> the interrupt tables memory, and check that the memory ranges are
> indeed pre-reserved.

Good point, efi_mem_reserve_persistent() has no way of describing what a region is for,
you have to know that from somewhere else.


> Is there a way to name memory with the current ABI that you think is acceptable?

This would need to go in the chosen node of the DT, like power-pc already does. This would
work on arm64:ACPI systems too (because they have a DT chosen node).


I'd like to understand why removing these entries is needed, it doesn't look like we have
an API call to remove them from the efi mem-reserve...

If it had a fixed position in memory its the sort of thing we'd expect firmware to
reserved during boot. (e.g. ramoops).

~

From ima_add_kexec_buffer() this really is a transient memory reservation over kexec.
I think the efi mem-reserve and a DT-chosen node entry with the PA is the only way to make
this work smoothly between DT<->ACPI systems.

We'd need a way of removing the efi mem-reserve in ima_free_kexec_buffer(), otherwise the
memory remains lost. The DT-chosen node entry should have its pointer zero'd out once
we've done this. (like we do for the KASLR seed).


Not considered is parsing the DT-chosen node entry as if it were a memreserve during early
boot. This wouldn't work if you kexec something that doesn't know what the node is, it
would overwrite the the memory and may not remove the node for the next kexec, which does.


Thanks,

James

Re: [PATCH V4 0/2] Add support for arm64 to carry ima measurement

[Mimi Zohar]

On Fri, 2019-10-25 at 18:07 +0100, James Morse wrote:
> Hi Mimi,
> 
> On 16/10/2019 02:44, Mimi Zohar wrote:
> > On Tue, 2019-10-15 at 18:39 +0100, James Morse wrote:
> >> If SecureBoot isn't relevant, I'm confused as to why kexec_file_load() is.
> >>
> >> I thought kexec_file_load() only existed because SecureBoot systems need to validate the
> >> new OS images's signature before loading it, and we can't trust user-space calling Kexec
> >> to do this.
> >>
> >> If there is no secure boot, why does this thing only work with kexec_file_load()?
> >> (good news! With the UEFI memreseve table, it should work transparently with regular kexec
> >> too)
> 
> > I'm so sorry for the confusion.  IMA was originally limited to
> > extending trusted boot concepts to the OS.  As of Linux 3.10, IMA
> > added support for extending secure boot concepts and auditing file
> > hashes (commit e7c568e0fd0cf).
> > 
> > True, kexec_file_load is required for verifying the kexec kernel
> > image, but it is also required for measuring the kexec kernel image as
> > well.
> > 
> > After reading the kernel image into memory (kernel_read_file_from_fd),
> > the hash is calculated and then added to the IMA measurement list and
> > used to extend the TPM.  All of this is based on the IMA policy,
> > including the TPM PCR.
> 
> Don't we get a set of segments with the regular kexec syscall? These could equally be
> hashed and measured, and logged via IMA and/or extending the TPMs measurements.

IMA works at the file level.  I'm not sure what it would mean to
measure "segments".

Originally, kexec_file_load read the KEXEC kernel image twice, once to
calculate the file hash, and again to verify the signature.  Now
kexec_file_load calls kernel_read_file_from_fd, which reads the file
into memory, before IMA calculates the file buffer hash.

> 
> (obviously this would include the command-line and maybe purgatory, which makes it less
> predictable, but these are still the binary blobs that were given privileged access to the
> system).
> 
> 
> >>> I am not sure if i addressed all your concerns, please let me know
> >>> if i missed anything. To me most concerns look to be towards the kexec case and dependency
> >>> on hardware(ACPI/TPM) during boot and early boot services, where as carrying the logs is
> >>> only during the kexec_file_load sys call and does not interfere with that code path.
> >>> IMA documentation: https://sourceforge.net/p/linux-ima/wiki/Home/
> >>
> >> Supporting ACPI in the same way is something we need to do from day one. kexec_file_load()
> >> already does this. I'm not sure "only kexec_file_load()" is a justifiable restriction...
> 
> > The TPM PCRs are not reset on a soft reboot.  As a result, in order to
> > validate the IMA measurement list against the TPM PCRs, the IMA
> > measurement list is saved on kexec load, restored on boot, and then
> > the memory allocated for carrying the measurement list across kexec is
> > freed.
> 
> Hmm, this is why the reserved memory gets freed.

Yes
> 
> What happens to stuff that happens between kexec-load and boot?
> There is a comment:
> | /* segment size can't change between kexec load and execute */

Right, the original version addressed this, but was nixed by Eric,
saying it was unnecessary.  The current version allocates more memory
than needed to hopefully compensate. 

> 
> But I can't see anywhere that enforces that. I guess those measurements will go missing,
> and the TPM value will not match after kexec.

No, the kexec load will succeed, but if there isn't enough memory to
store the measurement list, the exec should fail.

Mimi