[PATCH v4 0/5] kexec_file: Add buffer hand-over for the next kernel

[PATCH v4 0/5] kexec_file: Add buffer hand-over for the next kernel

[Thiago Jung Bauermann]

Hello,

The purpose of this new version of the series is to fix a small issue that
I found, which is that the kernel doesn't remove the memory reservation
for the hand-over buffer it received from the previous kernel in the
device tree it sets up for the next kernel. The result is that for each
successive kexec, a stale hand-over buffer is left behind, wasting memory.

This is fixed by changes to kexec_free_handover_buffer and
setup_handover_buffer in patch 2. The other change is to fix checkpatch
warnings in the last patch.

Original cover letter:

This patch series implements a mechanism which allows the kernel to pass
on a buffer to the kernel that will be kexec'd. This buffer is passed
as a segment which is added to the kimage when it is being prepared
by kexec_file_load.

How the second kernel is informed of this buffer is architecture-specific.
On powerpc, this is done via the device tree, by checking
the properties /chosen/linux,kexec-handover-buffer-start and
/chosen/linux,kexec-handover-buffer-end, which is analogous to how the
kernel finds the initrd.

This is needed because the Integrity Measurement Architecture subsystem
needs to preserve its measurement list accross the kexec reboot. The
following patch series for the IMA subsystem uses this feature for that
purpose:

https://lists.infradead.org/pipermail/kexec/2016-August/016745.html

This is so that IMA can implement trusted boot support on the OpenPower
platform, because on such systems an intermediary Linux instance running
as part of the firmware is used to boot the target operating system via
kexec. Using this mechanism, IMA on this intermediary instance can
hand over to the target OS the measurements of the components that were
used to boot it.

Because there could be additional measurement events between the
kexec_file_load call and the actual reboot, IMA needs a way to update the
buffer with those additional events before rebooting. One can minimize
the interval between the kexec_file_load and the reboot syscalls, but as
small as it can be, there is always the possibility that the measurement
list will be out of date at the time of reboot.

To address this issue, this patch series also introduces
kexec_update_segment, which allows a reboot notifier to change the
contents of the image segment during the reboot process.

The last patch is not intended to be merged, it just demonstrates how
this feature can be used.

This series applies on top of v6 of the "kexec_file_load implementation
for PowerPC" patch series (which applies on top of v4.8-rc1):

https://lists.infradead.org/pipermail/kexec/2016-August/016960.html

Changes for v4:
- Rebased series on kexec_file_load patch series v7.
- Patch "powerpc: kexec_file: Add buffer hand-over support for the next kernel"
  - Convert hand-over buffer address to physical address when calling
    memblock_free in kexec_free_handover_buffer.
  - Delete hand-over buffer properties from the live device tree in
    kexec_free_handover_buffer.
  - Remove the memory reservation and the properties for the hand-over
    buffer received from the previous kernel in setup_handover_buffer.
- Patch "IMA: Demonstration code for kexec buffer passing."
  - Fix checkpatch warnings. (Andrew Morton)

Changes for v3:
- Rebased series on kexec_file_load patch series v6.
  Both patch series apply cleanly on todays' Linus master branch, except
  for a few lines of fuzz in arch/powerpc/Makefile and arch/powerpc/Kconfig.
- Patch "kexec_file: Add buffer hand-over support for the next kernel"
  - Fix compilation warning in <linux/kexec.h> by adding a struct kexec_buf
    forward declaration when CONFIG_KEXEC_FILE=n. (Fenguang Wu)
- Patch "kexec_file: Allow skipping checksum calculation for some segments."
  - Substitute checksum argument in kexec_add_buffer with skip_checksum
    member in struct kexec_buf, as suggested by Dave Young.
- Patch "kexec_file: Add mechanism to update kexec segments."
  - Use kmap_atomic in kexec_update_segment, as suggested by Andrew Morton.
  - Fix build warning on m68k by passing unsigned long value to __va instead
    of void *. (Fenguang Wu)
  - Change bufsz and memsz arguments of kexec_update_segment to size_t to fix
    compilation warning. (Fenguang Wu)
- Patch "kexec: Share logic to copy segment page contents."
  - Dropped this patch.
- Patch "IMA: Demonstration code for kexec buffer passing."
  - Update to use kexec_buf.skip_checksum instead of passing it in
    kexec_add_buffer.

Changes for v2:
- Rebased on v5 of kexec_file_load implementation for PowerPC patch series.
- Patch "kexec_file: Add buffer hand-over support for the next kernel"
  - Changed kexec_add_handover_buffer to receive a struct kexec_buf, as
    suggested by Dave Young.
- Patch "powerpc: kexec_file: Add buffer hand-over support for the next kernel"
  - Moved setup_handover_buffer from kexec_elf_64.c to machine_kexec_64.c.
  - Call setup_handover_buffer from setup_new_fdt instead of elf64_load.
  - Changed kexec_get_handover_buffer to read from the expanded device tree
    instead of the flattened device tree.
- Patch "kexec_file: Add mechanism to update kexec segments.":
  - Removed unnecessary "#include <linux/highmem.h>" in kexec_file.c.
  - Round up memsz argument to PAGE_SIZE.
  - Check if kexec_image is NULL in kexec_update_segment.
- Patch "IMA: Demonstration code for kexec buffer passing."
  - Avoid registering reboot notifier again if kexec_file_load is called
    more than once.


Thiago Jung Bauermann (5):
  kexec_file: Add buffer hand-over support for the next kernel
  powerpc: kexec_file: Add buffer hand-over support for the next kernel
  kexec_file: Allow skipping checksum calculation for some segments.
  kexec_file: Add mechanism to update kexec segments.
  IMA: Demonstration code for kexec buffer passing.

 arch/powerpc/include/asm/kexec.h       |  12 +-
 arch/powerpc/kernel/kexec_elf_64.c     |   2 +-
 arch/powerpc/kernel/machine_kexec_64.c | 274 +++++++++++++++++++++++++++------
 include/linux/ima.h                    |  11 ++
 include/linux/kexec.h                  |  56 +++++--
 kernel/kexec_core.c                    |  98 ++++++++++++
 kernel/kexec_file.c                    |  87 ++++++++++-
 security/integrity/ima/ima.h           |   5 +
 security/integrity/ima/ima_init.c      |  26 ++++
 security/integrity/ima/ima_template.c  |  85 ++++++++++
 10 files changed, 591 insertions(+), 65 deletions(-)

-- 
1.9.1

[PATCH v4 1/5] kexec_file: Add buffer hand-over support for the next kernel

[Thiago Jung Bauermann]

The buffer hand-over mechanism allows the currently running kernel to pass
data to kernel that will be kexec'd via a kexec segment. The second kernel
can check whether the previous kernel sent data and retrieve it.

This is the architecture-independent part of the feature.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/kexec.h | 31 +++++++++++++++++++++++
 kernel/kexec_file.c   | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index d419d0e51fe5..16561e96a6d7 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -383,6 +383,37 @@ static inline void *boot_phys_to_virt(unsigned long entry)
 	return phys_to_virt(boot_phys_to_phys(entry));
 }
 
+#ifdef CONFIG_KEXEC_FILE
+bool __weak kexec_can_hand_over_buffer(void);
+int __weak arch_kexec_add_handover_buffer(struct kimage *image,
+					  unsigned long load_addr,
+					  unsigned long size);
+int kexec_add_handover_buffer(struct kexec_buf *kbuf);
+int __weak kexec_get_handover_buffer(void **addr, unsigned long *size);
+int __weak kexec_free_handover_buffer(void);
+#else
+struct kexec_buf;
+
+static inline bool kexec_can_hand_over_buffer(void)
+{
+	return false;
+}
+
+static inline int kexec_add_handover_buffer(struct kexec_buf *kbuf)
+{
+	return -ENOTSUPP;
+}
+
+static inline int kexec_get_handover_buffer(void **addr, unsigned long *size)
+{
+	return -ENOTSUPP;
+}
+
+static inline int kexec_free_handover_buffer(void)
+{
+	return -ENOTSUPP;
+}
+#endif /* CONFIG_KEXEC_FILE */
 #else /* !CONFIG_KEXEC_CORE */
 struct pt_regs;
 struct task_struct;
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 3401816700f3..f5684adfad07 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -113,6 +113,74 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	image->image_loader_data = NULL;
 }
 
+/**
+ * kexec_can_hand_over_buffer - can we pass data to the kexec'd kernel?
+ */
+bool __weak kexec_can_hand_over_buffer(void)
+{
+	return false;
+}
+
+/**
+ * arch_kexec_add_handover_buffer - do arch-specific steps to handover buffer
+ *
+ * Architectures should use this function to pass on the handover buffer
+ * information to the next kernel.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int __weak arch_kexec_add_handover_buffer(struct kimage *image,
+					  unsigned long load_addr,
+					  unsigned long size)
+{
+	return -ENOTSUPP;
+}
+
+/**
+ * kexec_add_handover_buffer - add buffer to be used by the next kernel
+ * @kbuf:	Buffer contents and memory parameters.
+ *
+ * This function assumes that kexec_mutex is held.
+ * On successful return, @kbuf->mem will have the physical address of
+ * the buffer in the next kernel.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int kexec_add_handover_buffer(struct kexec_buf *kbuf)
+{
+	int ret;
+
+	if (!kexec_can_hand_over_buffer())
+		return -ENOTSUPP;
+
+	ret = kexec_add_buffer(kbuf);
+	if (ret)
+		return ret;
+
+	return arch_kexec_add_handover_buffer(kbuf->image, kbuf->mem,
+					      kbuf->memsz);
+}
+
+/**
+ * kexec_get_handover_buffer - get the handover 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 __weak kexec_get_handover_buffer(void **addr, unsigned long *size)
+{
+	return -ENOTSUPP;
+}
+
+/**
+ * kexec_free_handover_buffer - free memory used by the handover buffer
+ */
+int __weak kexec_free_handover_buffer(void)
+{
+	return -ENOTSUPP;
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
-- 
1.9.1

[PATCH v4 2/5] powerpc: kexec_file: Add buffer hand-over support for the next kernel

[Thiago Jung Bauermann]

The buffer hand-over mechanism allows the currently running kernel to pass
data to kernel that will be kexec'd via a kexec segment. The second kernel
can check whether the previous kernel sent data and retrieve it.

This is the architecture-specific part.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/kexec.h       |  12 +-
 arch/powerpc/kernel/kexec_elf_64.c     |   2 +-
 arch/powerpc/kernel/machine_kexec_64.c | 274 +++++++++++++++++++++++++++------
 3 files changed, 240 insertions(+), 48 deletions(-)

diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
index 73f88b5f9bd1..b8e32194ce63 100644
--- a/arch/powerpc/include/asm/kexec.h
+++ b/arch/powerpc/include/asm/kexec.h
@@ -92,12 +92,20 @@ static inline bool kdump_in_progress(void)
 }
 
 #ifdef CONFIG_KEXEC_FILE
+#define ARCH_HAS_KIMAGE_ARCH
+
+struct kimage_arch {
+	phys_addr_t handover_buffer_addr;
+	unsigned long handover_buffer_size;
+};
+
 int setup_purgatory(struct kimage *image, const void *slave_code,
 		    const void *fdt, unsigned long kernel_load_addr,
 		    unsigned long fdt_load_addr, unsigned long stack_top,
 		    int debug);
-int setup_new_fdt(void *fdt, unsigned long initrd_load_addr,
-		  unsigned long initrd_len, const char *cmdline);
+int setup_new_fdt(const struct kimage *image, void *fdt,
+		  unsigned long initrd_load_addr, unsigned long initrd_len,
+		  const char *cmdline);
 bool find_debug_console(const void *fdt);
 #endif /* CONFIG_KEXEC_FILE */
 
diff --git a/arch/powerpc/kernel/kexec_elf_64.c b/arch/powerpc/kernel/kexec_elf_64.c
index 3cc8ebce1a86..0c576e300384 100644
--- a/arch/powerpc/kernel/kexec_elf_64.c
+++ b/arch/powerpc/kernel/kexec_elf_64.c
@@ -208,7 +208,7 @@ void *elf64_load(struct kimage *image, char *kernel_buf,
 		goto out;
 	}
 
-	ret = setup_new_fdt(fdt, initrd_load_addr, initrd_len, cmdline);
+	ret = setup_new_fdt(image, fdt, initrd_load_addr, initrd_len, cmdline);
 	if (ret)
 		goto out;
 
diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c
index 3879b6d91c0b..d6077898200a 100644
--- a/arch/powerpc/kernel/machine_kexec_64.c
+++ b/arch/powerpc/kernel/machine_kexec_64.c
@@ -489,6 +489,77 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
 	return image->fops->cleanup(image->image_loader_data);
 }
 
+bool kexec_can_hand_over_buffer(void)
+{
+	return true;
+}
+
+int arch_kexec_add_handover_buffer(struct kimage *image,
+				   unsigned long load_addr, unsigned long size)
+{
+	image->arch.handover_buffer_addr = load_addr;
+	image->arch.handover_buffer_size = size;
+
+	return 0;
+}
+
+int kexec_get_handover_buffer(void **addr, unsigned long *size)
+{
+	int ret;
+	u64 start_addr, end_addr;
+
+	ret = of_property_read_u64(of_chosen,
+				   "linux,kexec-handover-buffer-start",
+				   &start_addr);
+	if (ret == -EINVAL)
+		return -ENOENT;
+	else if (ret)
+		return -EINVAL;
+
+	ret = of_property_read_u64(of_chosen, "linux,kexec-handover-buffer-end",
+				   &end_addr);
+	if (ret == -EINVAL)
+		return -ENOENT;
+	else if (ret)
+		return -EINVAL;
+
+	*addr =  __va(start_addr);
+	/* -end is the first address after the buffer. */
+	*size = end_addr - start_addr;
+
+	return 0;
+}
+
+int kexec_free_handover_buffer(void)
+{
+	int ret;
+	void *addr;
+	unsigned long size;
+	struct property *prop;
+
+	ret = kexec_get_handover_buffer(&addr, &size);
+	if (ret)
+		return ret;
+
+	ret = memblock_free(__pa(addr), size);
+	if (ret)
+		return ret;
+
+	prop = of_find_property(of_chosen, "linux,kexec-handover-buffer-start",
+				NULL);
+	ret = of_remove_property(of_chosen, prop);
+	if (ret)
+		return ret;
+
+	prop = of_find_property(of_chosen, "linux,kexec-handover-buffer-end",
+				NULL);
+	ret = of_remove_property(of_chosen, prop);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 /**
  * arch_kexec_walk_mem() - call func(data) for each unreserved memory block
  * @kbuf:	Context info for the search. Also passed to @func.
@@ -686,26 +757,16 @@ int setup_purgatory(struct kimage *image, const void *slave_code,
 	return 0;
 }
 
-/*
- * setup_new_fdt() - modify /chosen and memory reservation for the next kernel
- * @fdt:
- * @initrd_load_addr:	Address where the next initrd will be loaded.
- * @initrd_len:		Size of the next initrd, or 0 if there will be none.
- * @cmdline:		Command line for the next kernel, or NULL if there will
- *			be none.
+/**
+ * delete_fdt_mem_rsv() - delete memory reservation with given address and size
  *
  * Return: 0 on success, or negative errno on error.
  */
-int setup_new_fdt(void *fdt, unsigned long initrd_load_addr,
-		  unsigned long initrd_len, const char *cmdline)
+static int delete_fdt_mem_rsv(void *fdt, uint64_t start, uint64_t size)
 {
-	uint64_t oldfdt_addr;
-	int i, ret, chosen_node;
-	const void *prop;
+	int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
 
-	/* Remove memory reservation for the current device tree. */
-	oldfdt_addr = __pa(initial_boot_params);
-	for (i = 0; i < fdt_num_mem_rsv(fdt); i++) {
+	for (i = 0; i < num_rsvs; i++) {
 		uint64_t rsv_start, rsv_size;
 
 		ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
@@ -714,19 +775,152 @@ int setup_new_fdt(void *fdt, unsigned long initrd_load_addr,
 			return -EINVAL;
 		}
 
-		if (rsv_start == oldfdt_addr &&
-		    rsv_size == fdt_totalsize(initial_boot_params)) {
+		if (rsv_start == start && rsv_size == size) {
 			ret = fdt_del_mem_rsv(fdt, i);
 			if (ret) {
-				pr_err("Error deleting fdt reservation.\n");
+				pr_err("Error deleting device tree reservation.\n");
 				return -EINVAL;
 			}
 
-			pr_debug("Removed old device tree reservation.\n");
-			break;
+			return 0;
 		}
 	}
 
+	return -ENOENT;
+}
+
+/**
+ * setup_handover_buffer() - add handover buffer information to the fdt
+ * @image:		kexec image being loaded.
+ * @fdt:		Flattened device tree for the next kernel.
+ * @chosen_node:	Offset to the chosen node.
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+static int setup_handover_buffer(const struct kimage *image, void *fdt,
+				 int chosen_node)
+{
+	int ret;
+	const void *prop;
+
+	/* Did we receive a buffer from the previous kernel? */
+	prop = fdt_getprop(fdt, chosen_node,
+			   "linux,kexec-handover-buffer-start", NULL);
+	if (prop) {
+		u64 orig_start, orig_end;
+		unsigned long size;
+		void *addr;
+
+		orig_start = fdt64_to_cpu(*((const fdt64_t *) prop));
+
+		prop = fdt_getprop(fdt, chosen_node,
+				   "linux,kexec-handover-buffer-end", NULL);
+		if (!prop) {
+			pr_err("Malformed device tree.\n");
+			return -EINVAL;
+		}
+		orig_end = fdt64_to_cpu(*((const fdt64_t *) prop));
+
+		/* Did we free the hand-over buffer from the previous kernel? */
+		ret = kexec_get_handover_buffer(&addr, &size);
+		if (ret == -ENOENT)
+			addr = NULL;
+		else if (ret)
+			return ret;
+
+		/*
+		 * If we received a buffer from the previous kernel but deleted
+		 * it from the live DT, or if we have a new buffer for the next
+		 * kernel then we should remove the memory reservation from
+		 * the FDT.
+		 */
+		if (addr == NULL || image->arch.handover_buffer_addr != 0) {
+			ret = delete_fdt_mem_rsv(fdt, orig_start,
+						 orig_end - orig_start);
+			if (ret == 0)
+				pr_debug("Removed old hand-over buffer reservation.\n");
+			else if (ret != -ENOENT)
+				return ret;
+		}
+
+		/*
+		 * If we received a buffer from the previous kernel but deleted
+		 * it from the live DT and we have no buffer for the next kernel
+		 * then we should remove the hand-over buffer properties from
+		 * the FDT.
+		 */
+		if (addr == NULL && image->arch.handover_buffer_addr == 0) {
+			ret = fdt_delprop(fdt, chosen_node,
+					  "linux,kexec-handover-buffer-start");
+			if (ret) {
+				pr_err("Error setting up the new device tree.\n");
+				return -EINVAL;
+			}
+
+			ret = fdt_delprop(fdt, chosen_node,
+					  "linux,kexec-handover-buffer-end");
+			if (ret) {
+				pr_err("Error setting up the new device tree.\n");
+				return -EINVAL;
+			}
+		}
+	}
+
+	if (image->arch.handover_buffer_addr == 0)
+		return 0;
+
+	ret = fdt_setprop_u64(fdt, chosen_node,
+			      "linux,kexec-handover-buffer-start",
+			      image->arch.handover_buffer_addr);
+	if (ret < 0)
+		return -EINVAL;
+
+	/* -end is the first address after the buffer. */
+	ret = fdt_setprop_u64(fdt, chosen_node,
+			      "linux,kexec-handover-buffer-end",
+			      image->arch.handover_buffer_addr +
+			      image->arch.handover_buffer_size);
+	if (ret < 0)
+		return -EINVAL;
+
+	ret = fdt_add_mem_rsv(fdt, image->arch.handover_buffer_addr,
+			      image->arch.handover_buffer_size);
+	if (ret)
+		return -EINVAL;
+
+	pr_debug("kexec handover buffer at 0x%llx, size = 0x%lx\n",
+		 image->arch.handover_buffer_addr,
+		 image->arch.handover_buffer_size);
+
+	return 0;
+}
+
+/**
+ * setup_new_fdt() - modify /chosen and memory reservations for the next kernel
+ * @image:		kexec image being loaded.
+ * @fdt:		Flattened device tree for the next kernel.
+ * @initrd_load_addr:	Address where the next initrd will be loaded.
+ * @initrd_len:		Size of the next initrd, or 0 if there will be none.
+ * @cmdline:		Command line for the next kernel, or NULL if there will
+ *			be none.
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int setup_new_fdt(const struct kimage *image, void *fdt,
+		  unsigned long initrd_load_addr, unsigned long initrd_len,
+		  const char *cmdline)
+{
+	int ret, chosen_node;
+	const void *prop;
+
+	/* Remove memory reservation for the current device tree. */
+	ret = delete_fdt_mem_rsv(fdt, __pa(initial_boot_params),
+				 fdt_totalsize(initial_boot_params));
+	if (ret == 0)
+		pr_debug("Removed old device tree reservation.\n");
+	else if (ret != -ENOENT)
+		return ret;
+
 	chosen_node = fdt_path_offset(fdt, "/chosen");
 	if (chosen_node == -FDT_ERR_NOTFOUND) {
 		chosen_node = fdt_add_subnode(fdt, fdt_path_offset(fdt, "/"),
@@ -743,7 +937,7 @@ int setup_new_fdt(void *fdt, unsigned long initrd_load_addr,
 	/* Did we boot using an initrd? */
 	prop = fdt_getprop(fdt, chosen_node, "linux,initrd-start", NULL);
 	if (prop) {
-		uint64_t tmp_start, tmp_end, tmp_size, tmp_sizepg;
+		uint64_t tmp_start, tmp_end, tmp_size;
 
 		tmp_start = fdt64_to_cpu(*((const fdt64_t *) prop));
 
@@ -759,30 +953,14 @@ int setup_new_fdt(void *fdt, unsigned long initrd_load_addr,
 		 * reserve a multiple of PAGE_SIZE, so check for both.
 		 */
 		tmp_size = tmp_end - tmp_start;
-		tmp_sizepg = round_up(tmp_size, PAGE_SIZE);
-
-		/* Remove memory reservation for the current initrd. */
-		for (i = 0; i < fdt_num_mem_rsv(fdt); 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 == tmp_start &&
-			    (rsv_size == tmp_size || rsv_size == tmp_sizepg)) {
-				ret = fdt_del_mem_rsv(fdt, i);
-				if (ret) {
-					pr_err("Error deleting fdt reservation.\n");
-					return -EINVAL;
-				}
-				pr_debug("Removed old initrd reservation.\n");
-
-				break;
-			}
-		}
+		ret = delete_fdt_mem_rsv(fdt, tmp_start, tmp_size);
+		if (ret == -ENOENT)
+			ret = delete_fdt_mem_rsv(fdt, tmp_start,
+						 round_up(tmp_size, PAGE_SIZE));
+		if (ret == 0)
+			pr_debug("Removed old initrd reservation.\n");
+		else if (ret != -ENOENT)
+			return ret;
 
 		/* If there's no new initrd, delete the old initrd's info. */
 		if (initrd_len == 0) {
@@ -840,6 +1018,12 @@ int setup_new_fdt(void *fdt, unsigned long initrd_load_addr,
 		}
 	}
 
+	ret = setup_handover_buffer(image, fdt, chosen_node);
+	if (ret) {
+		pr_err("Error setting up the new device tree.\n");
+		return ret;
+	}
+
 	ret = fdt_setprop(fdt, chosen_node, "linux,booted-from-kexec", NULL, 0);
 	if (ret) {
 		pr_err("Error setting up the new device tree.\n");
-- 
1.9.1

[PATCH v4 3/5] kexec_file: Allow skipping checksum calculation for some segments.

[Thiago Jung Bauermann]

Add skip_checksum member to struct kexec_buf to specify whether the
corresponding segment should be part of the checksum calculation.

The next patch will add a way to update segments after a kimage is loaded.
Segments that will be updated in this way should not be checksummed,
otherwise they will cause the purgatory checksum verification to fail
when the machine is rebooted.

As a bonus, we don't need to special-case the purgatory segment anymore
to avoid checksumming it.

Places using struct kexec_buf get false as the default value for
skip_checksum since they all use designated initializers.  Therefore,
there is no behavior change with this patch and all segments except the
purgatory are checksummed.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/kexec.h | 23 ++++++++++++++---------
 kernel/kexec_file.c   | 15 +++++++--------
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 16561e96a6d7..edadff6c86ff 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -100,6 +100,9 @@ struct kexec_segment {
 	size_t bufsz;
 	unsigned long mem;
 	size_t memsz;
+
+	/* Whether this segment is ignored in the checksum calculation. */
+	bool skip_checksum;
 };
 
 #ifdef CONFIG_COMPAT
@@ -151,15 +154,16 @@ struct kexec_file_ops {
 
 /**
  * struct kexec_buf - parameters for finding a place for a buffer in memory
- * @image:	kexec image in which memory to search.
- * @buffer:	Contents which will be copied to the allocated memory.
- * @bufsz:	Size of @buffer.
- * @mem:	On return will have address of the buffer in memory.
- * @memsz:	Size for the buffer in memory.
- * @buf_align:	Minimum alignment needed.
- * @buf_min:	The buffer can't be placed below this address.
- * @buf_max:	The buffer can't be placed above this address.
- * @top_down:	Allocate from top of memory.
+ * @image:		kexec image in which memory to search.
+ * @buffer:		Contents which will be copied to the allocated memory.
+ * @bufsz:		Size of @buffer.
+ * @mem:		On return will have address of the buffer in memory.
+ * @memsz:		Size for the buffer in memory.
+ * @buf_align:		Minimum alignment needed.
+ * @buf_min:		The buffer can't be placed below this address.
+ * @buf_max:		The buffer can't be placed above this address.
+ * @top_down:		Allocate from top of memory.
+ * @skip_checksum:	Don't verify checksum for this buffer in purgatory.
  */
 struct kexec_buf {
 	struct kimage *image;
@@ -171,6 +175,7 @@ struct kexec_buf {
 	unsigned long buf_min;
 	unsigned long buf_max;
 	bool top_down;
+	bool skip_checksum;
 };
 
 int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f5684adfad07..0e90d1446cb0 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -584,6 +584,7 @@ int kexec_add_buffer(struct kexec_buf *kbuf)
 	ksegment->bufsz = kbuf->bufsz;
 	ksegment->mem = kbuf->mem;
 	ksegment->memsz = kbuf->memsz;
+	ksegment->skip_checksum = kbuf->skip_checksum;
 	kbuf->image->nr_segments++;
 	return 0;
 }
@@ -598,7 +599,6 @@ static int kexec_calculate_store_digests(struct kimage *image)
 	char *digest;
 	void *zero_buf;
 	struct kexec_sha_region *sha_regions;
-	struct purgatory_info *pi = &image->purgatory_info;
 
 	zero_buf = __va(page_to_pfn(ZERO_PAGE(0)) << PAGE_SHIFT);
 	zero_buf_sz = PAGE_SIZE;
@@ -638,11 +638,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
 		struct kexec_segment *ksegment;
 
 		ksegment = &image->segment[i];
-		/*
-		 * Skip purgatory as it will be modified once we put digest
-		 * info in purgatory.
-		 */
-		if (ksegment->kbuf == pi->purgatory_buf)
+		if (ksegment->skip_checksum)
 			continue;
 
 		ret = crypto_shash_update(desc, ksegment->kbuf,
@@ -714,7 +710,7 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 	Elf_Shdr *sechdrs = NULL;
 	struct kexec_buf kbuf = { .image = image, .bufsz = 0, .buf_align = 1,
 				  .buf_min = min, .buf_max = max,
-				  .top_down = top_down };
+				  .top_down = top_down, .skip_checksum = true };
 
 	/*
 	 * sechdrs_c points to section headers in purgatory and are read
@@ -819,7 +815,10 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 	if (kbuf.buf_align < bss_align)
 		kbuf.buf_align = bss_align;
 
-	/* Add buffer to segment list */
+	/*
+	 * Add buffer to segment list. Don't checksum the segment as
+	 * it will be modified once we put digest info in purgatory.
+	 */
 	ret = kexec_add_buffer(&kbuf);
 	if (ret)
 		goto out;
-- 
1.9.1

[PATCH v4 4/5] kexec_file: Add mechanism to update kexec segments.

[Thiago Jung Bauermann]

kexec_update_segment allows a given segment in kexec_image to have
its contents updated. This is useful if the current kernel wants to
send information to the next kernel that is up-to-date at the time of
reboot.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 include/linux/kexec.h |  2 ++
 kernel/kexec_core.c   | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index edadff6c86ff..ff3aa93649e2 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -256,6 +256,8 @@ extern int kexec_purgatory_get_set_symbol(struct kimage *image,
 					  unsigned int size, bool get_value);
 extern void *kexec_purgatory_get_symbol_addr(struct kimage *image,
 					     const char *name);
+int kexec_update_segment(const char *buffer, size_t bufsz,
+			 unsigned long load_addr, size_t memsz);
 extern void __crash_kexec(struct pt_regs *);
 extern void crash_kexec(struct pt_regs *);
 int kexec_should_crash(struct task_struct *);
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 561675589511..d3f1ebf66222 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -721,6 +721,104 @@ static struct page *kimage_alloc_page(struct kimage *image,
 	return page;
 }
 
+/**
+ * kexec_update_segment - update the contents of a kimage segment
+ * @buffer:	New contents of the segment.
+ * @bufsz:	@buffer size.
+ * @load_addr:	Segment's physical address in the next kernel.
+ * @memsz:	Segment size.
+ *
+ * This function assumes kexec_mutex is held.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int kexec_update_segment(const char *buffer, size_t bufsz,
+			 unsigned long load_addr, size_t memsz)
+{
+	int i;
+	unsigned long entry;
+	unsigned long *ptr = NULL;
+	void *dest = NULL;
+
+	if (kexec_image == NULL) {
+		pr_err("Can't update segment: no kexec image loaded.\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * kexec_add_buffer rounds up segment sizes to PAGE_SIZE, so
+	 * we have to do it here as well.
+	 */
+	memsz = ALIGN(memsz, PAGE_SIZE);
+
+	for (i = 0; i < kexec_image->nr_segments; i++)
+		/* We only support updating whole segments. */
+		if (load_addr == kexec_image->segment[i].mem &&
+		    memsz == kexec_image->segment[i].memsz) {
+			if (!kexec_image->segment[i].skip_checksum) {
+				pr_err("Trying to update non-modifiable segment.\n");
+				return -EINVAL;
+			}
+
+			break;
+		}
+	if (i == kexec_image->nr_segments) {
+		pr_err("Couldn't find segment to update: 0x%lx, size 0x%zx\n",
+		       load_addr, memsz);
+		return -EINVAL;
+	}
+
+	for (entry = kexec_image->head; !(entry & IND_DONE) && memsz;
+	     entry = *ptr++) {
+		void *addr = (void *) (entry & PAGE_MASK);
+
+		switch (entry & IND_FLAGS) {
+		case IND_DESTINATION:
+			dest = addr;
+			break;
+		case IND_INDIRECTION:
+			ptr = __va(entry & PAGE_MASK);
+			break;
+		case IND_SOURCE:
+			/* Shouldn't happen, but verify just to be safe. */
+			if (dest == NULL) {
+				pr_err("Invalid kexec entries list.");
+				return -EINVAL;
+			}
+
+			if (dest == (void *) load_addr) {
+				void *page_addr;
+				unsigned long offset;
+				size_t uchunk, mchunk;
+
+				page_addr = kmap_atomic(kmap_to_page(addr));
+
+				offset = load_addr & ~PAGE_MASK;
+				mchunk = min_t(size_t, memsz,
+					       PAGE_SIZE - offset);
+				uchunk = min(bufsz, mchunk);
+				memcpy(page_addr + offset, buffer, uchunk);
+
+				kunmap_atomic(page_addr);
+
+				bufsz -= uchunk;
+				load_addr += mchunk;
+				buffer += mchunk;
+				memsz -= mchunk;
+			}
+			dest += PAGE_SIZE;
+		}
+
+		/* Shouldn't happen, but verify just to be safe. */
+		if (ptr == NULL) {
+			pr_err("Invalid kexec entries list.");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static int kimage_load_normal_segment(struct kimage *image,
 					 struct kexec_segment *segment)
 {
-- 
1.9.1

[PATCH v4 5/5] IMA: Demonstration code for kexec buffer passing.

[Thiago Jung Bauermann]

This shows how kernel code can use the kexec buffer passing mechanism
to pass information to the next kernel.

This patch is not intended to be committed.

[akpm@linux-foundation.org: coding-style fixes]
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/ima.h                   | 11 +++++
 kernel/kexec_file.c                   |  4 ++
 security/integrity/ima/ima.h          |  5 +++
 security/integrity/ima/ima_init.c     | 26 +++++++++++
 security/integrity/ima/ima_template.c | 85 +++++++++++++++++++++++++++++++++++
 5 files changed, 131 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0eb7c2e7f0d6..96528d007139 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,6 +11,7 @@
 #define _LINUX_IMA_H
 
 #include <linux/fs.h>
+#include <linux/kexec.h>
 struct linux_binprm;
 
 #ifdef CONFIG_IMA
@@ -23,6 +24,10 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
 
+#ifdef CONFIG_KEXEC_FILE
+extern void ima_add_kexec_buffer(struct kimage *image);
+#endif
+
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
 {
@@ -60,6 +65,12 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 	return;
 }
 
+#ifdef CONFIG_KEXEC_FILE
+static inline void ima_add_kexec_buffer(struct kimage *image)
+{
+}
+#endif
+
 #endif /* CONFIG_IMA */
 
 #ifdef CONFIG_IMA_APPRAISE
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 0e90d1446cb0..75c1b8d67a72 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -19,6 +19,7 @@
 #include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/fs.h>
+#include <linux/ima.h>
 #include <crypto/hash.h>
 #include <crypto/sha.h>
 #include <linux/syscalls.h>
@@ -248,6 +249,9 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 		}
 	}
 
+	/* IMA needs to pass the measurement list to the next kernel. */
+	ima_add_kexec_buffer(image);
+
 	/* Call arch image load handlers */
 	ldata = arch_kexec_kernel_image_load(image);
 
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index db25f54a04fe..0334001055d7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -102,6 +102,11 @@ struct ima_queue_entry {
 };
 extern struct list_head ima_measurements;	/* list of all measurements */
 
+#ifdef CONFIG_KEXEC_FILE
+extern void *kexec_buffer;
+extern size_t kexec_buffer_size;
+#endif
+
 /* Internal IMA function definitions */
 int ima_init(void);
 int ima_fs_init(void);
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 32912bd54ead..a1924d0f3b2b 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -21,6 +21,7 @@
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/err.h>
+#include <linux/kexec.h>
 
 #include "ima.h"
 
@@ -104,6 +105,29 @@ void __init ima_load_x509(void)
 }
 #endif
 
+#ifdef CONFIG_KEXEC_FILE
+static void ima_load_kexec_buffer(void)
+{
+	int rc;
+
+	/* Fetch the buffer from the previous kernel, if any. */
+	rc = kexec_get_handover_buffer(&kexec_buffer, &kexec_buffer_size);
+	if (rc == 0) {
+		/* Demonstrate that buffer handover works. */
+		pr_err("kexec buffer contents: %s\n", (char *) kexec_buffer);
+		pr_err("kexec buffer contents after update: %s\n",
+		       (char *) kexec_buffer + 4 * PAGE_SIZE + 10);
+
+		kexec_free_handover_buffer();
+	} else if (rc == -ENOENT)
+		pr_debug("No kexec buffer from the previous kernel.\n");
+	else
+		pr_debug("Error restoring kexec buffer: %d\n", rc);
+}
+#else
+static void ima_load_kexec_buffer(void) { }
+#endif
+
 int __init ima_init(void)
 {
 	u8 pcr_i[TPM_DIGEST_SIZE];
@@ -134,5 +158,7 @@ int __init ima_init(void)
 
 	ima_init_policy();
 
+	ima_load_kexec_buffer();
+
 	return ima_fs_init();
 }
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index febd12ed9b55..e9ac260534b6 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -15,6 +15,8 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/kexec.h>
+#include <linux/reboot.h>
 #include "ima.h"
 #include "ima_template_lib.h"
 
@@ -182,6 +184,89 @@ static int template_desc_init_fields(const char *template_fmt,
 	return 0;
 }
 
+#ifdef CONFIG_KEXEC_FILE
+void *kexec_buffer;
+size_t kexec_buffer_size;
+
+/* Physical address of the measurement buffer in the next kernel. */
+unsigned long kexec_buffer_load_addr;
+
+/*
+ * Called during reboot. IMA can add here new events that were generated after
+ * the kexec image was loaded.
+ */
+static int ima_update_kexec_buffer(struct notifier_block *self,
+				   unsigned long action, void *data)
+{
+	int ret;
+
+	if (!kexec_in_progress)
+		return NOTIFY_OK;
+
+	/*
+	 * Add content deep in the buffer to show that we can update
+	 * all of it.
+	 */
+	strcpy(kexec_buffer + 4 * PAGE_SIZE + 10,
+	       "Updated kexec buffer contents.");
+
+	ret = kexec_update_segment(kexec_buffer, kexec_buffer_size,
+				   kexec_buffer_load_addr, kexec_buffer_size);
+	if (ret)
+		pr_err("Error updating kexec buffer: %d\n", ret);
+
+	return NOTIFY_OK;
+}
+
+struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+};
+
+/*
+ * Called during kexec_file_load so that IMA can add a segment to the kexec
+ * image with the measurement event log for the next kernel.
+ */
+void ima_add_kexec_buffer(struct kimage *image)
+{
+	/* Ask not to checksum the segment, we may have to update it later. */
+	struct kexec_buf kbuf = { .image = image, .buf_align = PAGE_SIZE,
+				  .buf_min = 0, .buf_max = ULONG_MAX,
+				  .top_down = true, .skip_checksum = true };
+	bool first_kexec_load = kexec_buffer_load_addr == 0;
+	int ret;
+
+	if (!kexec_can_hand_over_buffer())
+		return;
+
+	if (!first_kexec_load)
+		kfree(kexec_buffer);
+
+	/* Create a relatively big buffer, for testing. */
+	kexec_buffer_size = kbuf.bufsz = kbuf.memsz = 5 * PAGE_SIZE;
+	kexec_buffer = kbuf.buffer = kzalloc(kexec_buffer_size, GFP_KERNEL);
+	if (!kexec_buffer) {
+		pr_err("Not enough memory for the kexec measurement buffer.\n");
+		return;
+	}
+
+	/* Add some content for demonstration purposes. */
+	strcpy(kexec_buffer, "Buffer contents at kexec load time.");
+
+	ret = kexec_add_handover_buffer(&kbuf);
+	if (ret) {
+		pr_err("Error passing over kexec measurement buffer.\n");
+		return;
+	}
+	kexec_buffer_load_addr = kbuf.mem;
+
+	if (first_kexec_load)
+		register_reboot_notifier(&update_buffer_nb);
+
+	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
+		 kexec_buffer_load_addr);
+}
+#endif /* CONFIG_KEXEC_FILE */
+
 struct ima_template_desc *ima_template_desc_current(void)
 {
 	if (!ima_template)
-- 
1.9.1

Re: [PATCH v4 3/5] kexec_file: Allow skipping checksum calculation for some segments.

[Eric W. Biederman]

Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:

2> Add skip_checksum member to struct kexec_buf to specify whether the
> corresponding segment should be part of the checksum calculation.
>
> The next patch will add a way to update segments after a kimage is loaded.
> Segments that will be updated in this way should not be checksummed,
> otherwise they will cause the purgatory checksum verification to fail
> when the machine is rebooted.
>
> As a bonus, we don't need to special-case the purgatory segment anymore
> to avoid checksumming it.
>
> Places using struct kexec_buf get false as the default value for
> skip_checksum since they all use designated initializers.  Therefore,
> there is no behavior change with this patch and all segments except the
> purgatory are checksummed.

What is the world.  This fundamentally makes kexec unsafe to use.  If
any updates need to be made they should be made before they are part of
the entire image checksum.

No way should this be merged anywhere ever.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  include/linux/kexec.h | 23 ++++++++++++++---------
>  kernel/kexec_file.c   | 15 +++++++--------
>  2 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 16561e96a6d7..edadff6c86ff 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -100,6 +100,9 @@ struct kexec_segment {
>  	size_t bufsz;
>  	unsigned long mem;
>  	size_t memsz;
> +
> +	/* Whether this segment is ignored in the checksum calculation. */
> +	bool skip_checksum;
>  };
>  
>  #ifdef CONFIG_COMPAT
> @@ -151,15 +154,16 @@ struct kexec_file_ops {
>  
>  /**
>   * struct kexec_buf - parameters for finding a place for a buffer in memory
> - * @image:	kexec image in which memory to search.
> - * @buffer:	Contents which will be copied to the allocated memory.
> - * @bufsz:	Size of @buffer.
> - * @mem:	On return will have address of the buffer in memory.
> - * @memsz:	Size for the buffer in memory.
> - * @buf_align:	Minimum alignment needed.
> - * @buf_min:	The buffer can't be placed below this address.
> - * @buf_max:	The buffer can't be placed above this address.
> - * @top_down:	Allocate from top of memory.
> + * @image:		kexec image in which memory to search.
> + * @buffer:		Contents which will be copied to the allocated memory.
> + * @bufsz:		Size of @buffer.
> + * @mem:		On return will have address of the buffer in memory.
> + * @memsz:		Size for the buffer in memory.
> + * @buf_align:		Minimum alignment needed.
> + * @buf_min:		The buffer can't be placed below this address.
> + * @buf_max:		The buffer can't be placed above this address.
> + * @top_down:		Allocate from top of memory.
> + * @skip_checksum:	Don't verify checksum for this buffer in purgatory.
>   */
>  struct kexec_buf {
>  	struct kimage *image;
> @@ -171,6 +175,7 @@ struct kexec_buf {
>  	unsigned long buf_min;
>  	unsigned long buf_max;
>  	bool top_down;
> +	bool skip_checksum;
>  };
>  
>  int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index f5684adfad07..0e90d1446cb0 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -584,6 +584,7 @@ int kexec_add_buffer(struct kexec_buf *kbuf)
>  	ksegment->bufsz = kbuf->bufsz;
>  	ksegment->mem = kbuf->mem;
>  	ksegment->memsz = kbuf->memsz;
> +	ksegment->skip_checksum = kbuf->skip_checksum;
>  	kbuf->image->nr_segments++;
>  	return 0;
>  }
> @@ -598,7 +599,6 @@ static int kexec_calculate_store_digests(struct kimage *image)
>  	char *digest;
>  	void *zero_buf;
>  	struct kexec_sha_region *sha_regions;
> -	struct purgatory_info *pi = &image->purgatory_info;
>  
>  	zero_buf = __va(page_to_pfn(ZERO_PAGE(0)) << PAGE_SHIFT);
>  	zero_buf_sz = PAGE_SIZE;
> @@ -638,11 +638,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
>  		struct kexec_segment *ksegment;
>  
>  		ksegment = &image->segment[i];
> -		/*
> -		 * Skip purgatory as it will be modified once we put digest
> -		 * info in purgatory.
> -		 */
> -		if (ksegment->kbuf == pi->purgatory_buf)
> +		if (ksegment->skip_checksum)
>  			continue;
>  
>  		ret = crypto_shash_update(desc, ksegment->kbuf,
> @@ -714,7 +710,7 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  	Elf_Shdr *sechdrs = NULL;
>  	struct kexec_buf kbuf = { .image = image, .bufsz = 0, .buf_align = 1,
>  				  .buf_min = min, .buf_max = max,
> -				  .top_down = top_down };
> +				  .top_down = top_down, .skip_checksum = true };
>  
>  	/*
>  	 * sechdrs_c points to section headers in purgatory and are read
> @@ -819,7 +815,10 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
>  	if (kbuf.buf_align < bss_align)
>  		kbuf.buf_align = bss_align;
>  
> -	/* Add buffer to segment list */
> +	/*
> +	 * Add buffer to segment list. Don't checksum the segment as
> +	 * it will be modified once we put digest info in purgatory.
> +	 */
>  	ret = kexec_add_buffer(&kbuf);
>  	if (ret)
>  		goto out;

Re: [PATCH v4 0/5] kexec_file: Add buffer hand-over for the next kernel

[Eric W. Biederman]

Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:

> Hello,
>
> The purpose of this new version of the series is to fix a small issue that
> I found, which is that the kernel doesn't remove the memory reservation
> for the hand-over buffer it received from the previous kernel in the
> device tree it sets up for the next kernel. The result is that for each
> successive kexec, a stale hand-over buffer is left behind, wasting memory.
>
> This is fixed by changes to kexec_free_handover_buffer and
> setup_handover_buffer in patch 2. The other change is to fix checkpatch
> warnings in the last patch.

This is fundamentally broken.  You do not increase the integrity of a
system by dropping integrity checks.

No. No. No. No.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Eric

Re: [PATCH v4 0/5] kexec_file: Add buffer hand-over for the next kernel

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
>
>> Hello,
>>
>> The purpose of this new version of the series is to fix a small issue that
>> I found, which is that the kernel doesn't remove the memory reservation
>> for the hand-over buffer it received from the previous kernel in the
>> device tree it sets up for the next kernel. The result is that for each
>> successive kexec, a stale hand-over buffer is left behind, wasting memory.
>>
>> This is fixed by changes to kexec_free_handover_buffer and
>> setup_handover_buffer in patch 2. The other change is to fix checkpatch
>> warnings in the last patch.
>
> This is fundamentally broken.  You do not increase the integrity of a
> system by dropping integrity checks.
>
> No. No. No. No.
>
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

To be constructive the way we have handled similiar situations in the
past (hotplu memory) is to call kexec_load again.

Eric

Re: [PATCH v4 0/5] kexec_file: Add buffer hand-over for the next kernel

[Thiago Jung Bauermann]

Am Mittwoch, 07 September 2016, 09:19:40 schrieb Eric W. Biederman:
> ebiederm@xmission.com (Eric W. Biederman) writes:
> > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> >> Hello,
> >> 
> >> The purpose of this new version of the series is to fix a small issue
> >> that I found, which is that the kernel doesn't remove the memory
> >> reservation for the hand-over buffer it received from the previous
> >> kernel in the device tree it sets up for the next kernel. The result
> >> is that for each successive kexec, a stale hand-over buffer is left
> >> behind, wasting memory.
> >> 
> >> This is fixed by changes to kexec_free_handover_buffer and
> >> setup_handover_buffer in patch 2. The other change is to fix checkpatch
> >> warnings in the last patch.
> > 
> > This is fundamentally broken.  You do not increase the integrity of a
> > system by dropping integrity checks.
> > 
> > No. No. No. No.
> > 
> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

The IMA measurement list can be verified without the need of a checksum over 
its contents by replaying the PCR extend operations and checking that the 
result matches the registers in the TPM device. So the fact that it is not 
part of the kexec segments checksum verification doesn't actually reduce the 
integrity of the system.

Currently, IMA doesn't perform that verification when it restores the 
measurement list from the kexec handover buffer, so if you believe it's 
necessary to do that check at boot time, we could do one of the following:

1. Have IMA replay the PCR extend operations when it restores the 
measurement list from the handover buffer and validate it against the TPM 
PCRs, or

2. Have a buffer hash in the ima_kexec_hdr that IMA includes in the handover 
buffer, and verify the buffer checksum before restoring the measurement 
list.

What do you think?

> To be constructive the way we have handled similiar situations in the
> past (hotplu memory) is to call kexec_load again.

Thanks for your suggestion. Unfortunately it's always possible for new 
measurements to be added to the measurement list between the kexec_file_load 
and the reboot. We see that happen in practice with system scripts and 
configuration files that are only read or executed during the reboot 
process. They are only measured by IMA as a result of the kexec execute.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v4 0/5] kexec_file: Add buffer hand-over for the next kernel

[Eric W. Biederman]

Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:

> Am Mittwoch, 07 September 2016, 09:19:40 schrieb Eric W. Biederman:
>> ebiederm@xmission.com (Eric W. Biederman) writes:
>> > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
>> >> Hello,
>> >> 
>> >> The purpose of this new version of the series is to fix a small issue
>> >> that I found, which is that the kernel doesn't remove the memory
>> >> reservation for the hand-over buffer it received from the previous
>> >> kernel in the device tree it sets up for the next kernel. The result
>> >> is that for each successive kexec, a stale hand-over buffer is left
>> >> behind, wasting memory.
>> >> 
>> >> This is fixed by changes to kexec_free_handover_buffer and
>> >> setup_handover_buffer in patch 2. The other change is to fix checkpatch
>> >> warnings in the last patch.
>> > 
>> > This is fundamentally broken.  You do not increase the integrity of a
>> > system by dropping integrity checks.
>> > 
>> > No. No. No. No.
>> > 
>> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> The IMA measurement list can be verified without the need of a checksum over 
> its contents by replaying the PCR extend operations and checking that the 
> result matches the registers in the TPM device. So the fact that it is not 
> part of the kexec segments checksum verification doesn't actually reduce the 
> integrity of the system.

Bit flips and errant DMA transfers are the concern here.  That happens
routinely and can easily result in a corrupt data structure which may be
non-trivial to verify.

> Currently, IMA doesn't perform that verification when it restores the 
> measurement list from the kexec handover buffer, so if you believe it's 
> necessary to do that check at boot time, we could do one of the following:
>
> 1. Have IMA replay the PCR extend operations when it restores the 
> measurement list from the handover buffer and validate it against the TPM 
> PCRs, or
>
> 2. Have a buffer hash in the ima_kexec_hdr that IMA includes in the handover 
> buffer, and verify the buffer checksum before restoring the measurement 
> list.
>
> What do you think?

I think you are playing very much with fire and I am extremely
uncomfortable with the entire concept.  I think you are making things
more complicated in a way that will allow system to try and start
booting when their memory is correct.  Which may wind up corrupting
someones non-volatile storage.

It makes me doubly nervous that this adds a general purpose facility
that is generally not at all reusable.

I have seen malicious actors cause entirely too much damage to be at all
comfortable using a data structure before we validate that it was valid
before we started booting.  This isn't the same case but it is close
enough I don't trust someone just splatting data structures.
We can't guarantee integrity but we should not bypass best practices
either.

>> To be constructive the way we have handled similiar situations in the
>> past (hotplu memory) is to call kexec_load again.
>
> Thanks for your suggestion. Unfortunately it's always possible for new 
> measurements to be added to the measurement list between the kexec_file_load 
> and the reboot. We see that happen in practice with system scripts and 
> configuration files that are only read or executed during the reboot 
> process. They are only measured by IMA as a result of the kexec execute.

If I understand what you are saying correctly I expect things could be
setup so that those measurements are forced to happen before kexec load.

Especially in a boot loader context which you described earlier I
believe you should have quite a lot of control of the system.  Having a
facility that fundamentally undermines the design of kexec for a case
where someone might do something you have not predicted does not make me
comfortable.

Which is to say I don't see why you can't measure things before the
kexec_load system call in a tightly controlled setup like a boot loader.
Which should make it that in practice no measurements change.  I believe
that should increase the reliability of the system overall.

Having code in the kernel that updates a buffer that kexec will use
after that buffer is loaded in memory honestely gives me the heebie
jeebies.

Eric

Re: [PATCH v4 0/5] kexec_file: Add buffer hand-over for the next kernel

[Mimi Zohar]

On Thu, 2016-09-08 at 23:07 -0500, Eric W. Biederman wrote:
> Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> 
> > Am Mittwoch, 07 September 2016, 09:19:40 schrieb Eric W. Biederman:
> >> ebiederm@xmission.com (Eric W. Biederman) writes:
> >> > Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> writes:
> >> >> Hello,
> >> >> 
> >> >> The purpose of this new version of the series is to fix a small issue
> >> >> that I found, which is that the kernel doesn't remove the memory
> >> >> reservation for the hand-over buffer it received from the previous
> >> >> kernel in the device tree it sets up for the next kernel. The result
> >> >> is that for each successive kexec, a stale hand-over buffer is left
> >> >> behind, wasting memory.
> >> >> 
> >> >> This is fixed by changes to kexec_free_handover_buffer and
> >> >> setup_handover_buffer in patch 2. The other change is to fix checkpatch
> >> >> warnings in the last patch.
> >> > 
> >> > This is fundamentally broken.  You do not increase the integrity of a
> >> > system by dropping integrity checks.
> >> > 
> >> > No. No. No. No.
> >> > 
> >> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >
> > The IMA measurement list can be verified without the need of a checksum over 
> > its contents by replaying the PCR extend operations and checking that the 
> > result matches the registers in the TPM device. So the fact that it is not 
> > part of the kexec segments checksum verification doesn't actually reduce the 
> > integrity of the system.
> 
> Bit flips and errant DMA transfers are the concern here.  That happens
> routinely and can easily result in a corrupt data structure which may be
> non-trivial to verify.

Point taken.  Before using the measurement list, the hash needs to be
verified.  So then the question is when and where that hash should be
taken and included.

New measurements can take place between the kexec load and execute.
Those measurements would be included in an additional execute of kexec
load, as you suggested. There are additional new measurements that are
the result of the kexec execute itself, as Thiago pointed out.  Having
to know a priori what those measurements are, or should be, makes
carrying the measurement list across kexec brittle.

One solution, as suggested by Thiago, would be to calculate the
measurement list hash, include the hash in the ima_kexec_hdr and verify
the hash, before restoring the measurement list.  Another option would
be to calculate the image hash on kexec load either with  a zero filled
measurement list segment or without the segment, verify the image hash
on kexec execute, before re-calculating the image hash.

> > Currently, IMA doesn't perform that verification when it restores the 
> > measurement list from the kexec handover buffer, so if you believe it's 
> > necessary to do that check at boot time, we could do one of the following:
> >
> > 1. Have IMA replay the PCR extend operations when it restores the 
> > measurement list from the handover buffer and validate it against the TPM 
> > PCRs, or
> >
> > 2. Have a buffer hash in the ima_kexec_hdr that IMA includes in the handover 
> > buffer, and verify the buffer checksum before restoring the measurement 
> > list.
> >
> > What do you think?
> 
> I think you are playing very much with fire and I am extremely
> uncomfortable with the entire concept.  I think you are making things
> more complicated in a way that will allow system to try and start
> booting when their memory is correct.  Which may wind up corrupting
> someones non-volatile storage.

Everything that was previously included in the image hash, before this
proposed patch set, is still included in the image hash.

Lets assume  for a moment that the measurement list hash is stored in
the ima_kexec_hdr.  In addition to preventing the measurement list from
being restored, we could either allow the reboot without the measurement
list to occur or halt the system.

Rebooting without the measurement list would result in the attestation
of the system failing.

> It makes me doubly nervous that this adds a general purpose facility
> that is generally not at all reusable.

Thiago has already agreed to reverting the skip hash bit. 

> I have seen malicious actors cause entirely too much damage to be at all
> comfortable using a data structure before we validate that it was valid
> before we started booting.  This isn't the same case but it is close
> enough I don't trust someone just splatting data structures.
> We can't guarantee integrity but we should not bypass best practices
> either.
> 
> >> To be constructive the way we have handled similiar situations in the
> >> past (hotplu memory) is to call kexec_load again.
> >
> > Thanks for your suggestion. Unfortunately it's always possible for new 
> > measurements to be added to the measurement list between the kexec_file_load 
> > and the reboot. We see that happen in practice with system scripts and 
> > configuration files that are only read or executed during the reboot 
> > process. They are only measured by IMA as a result of the kexec execute.
> 
> If I understand what you are saying correctly I expect things could be
> setup so that those measurements are forced to happen before kexec load.

> Especially in a boot loader context which you described earlier I
> believe you should have quite a lot of control of the system.  Having a
> facility that fundamentally undermines the design of kexec for a case
> where someone might do something you have not predicted does not make me
> comfortable.
> 
> Which is to say I don't see why you can't measure things before the
> kexec_load system call in a tightly controlled setup like a boot loader.
> Which should make it that in practice no measurements change.  I believe
> that should increase the reliability of the system overall.

Either we measure everything possible, making the measurement list
unnecessarily large, or have a brittle system that could break the
attestation of the target system, if any packages add new files/scripts
that are accessed on a soft reboot.

> Having code in the kernel that updates a buffer that kexec will use
> after that buffer is loaded in memory honestely gives me the heebie
> jeebies.

To address your understandable concerns, we suggested two possible
solutions for including and verifying the measurement list hash.
- calculate the image hash on load, verify it on kexec reload, before
recalculating the image hash with the measurement list segment.
- defer hashing and verifying the measurement list hash to IMA.

Mimi