need help: patches to capture events between kexec load and execute

need help: patches to capture events between kexec load and execute

[Tushar Sugandhi]

[-- Attachment #1: Type: text/plain, Size: 3916 bytes --]

Hello all,
We believe we have found one gap in the IMA/kexec interaction.
And we need your inputs as Linux Kernel maintainers/experts to fix that
gap.

-------------
Problem:
-------------
The current Kernel behavior is IMA measurements are snapshotted at
'kexec load' and not at 'kexec execute'.  And IMA log is then carried
over to the new Kernel after 'kexec execute'.

Some systems can be configured to call 'kexec load' first, and followed
by 'kexec execute' after some time.  (as opposed to calling 'load' and
'execute' in one single kexec command).  In this scenario, if new IMA
measurements are added between 'kexec load' and 'kexec execute' - the
TPM PCRs are extended with the IMA events between load and execute, but
those IMA events are not carried over to the new kernel after kexec soft
reboot.  This results in mismatch between TPM PCR quotes and the actual
IMA measurements log post kexec.

===========================================================================
-------------
Scenario
-------------
Here is the order of operations I followed to confirm the issue.

(a) Call 'kexec load'

     #kexec -s -l /etc/ima/Image.kexec --reuse-cmdline


(b) Touch one of the files that would be measured by IMA

     #cat /run/systemd/journal/streams/8:16351


(c) Verify that this measurement event is part of the IMA log.

     #cat /sys/kernel/security/ima/ascii_runtime_measurements | grep 16351

     <returns the file entry in IMA log>

(d) Call 'kexec execute'

     #kexec -s -e


(e) After kexec soft reboot, the file measurement event is not present
     in the IMA log anymore.  Because this measurement in the previous
     kernel had happened after the IMA log was snapshotted in the
     previous kernel.

     #cat /sys/kernel/security/ima/ascii_runtime_measurements | grep 16351

     <does not return the file entry in IMA log>

===========================================================================
-------------
Solution
-------------
Tyler pointed me to the past work in this area.
(Please see references section below)

I used it to create the patches for capturing IMA events in between
"kexec load" and "kexec execute". (please find attached)

- 0001-kexec_file-Add-mechanism-to-update-kexec-segments.patch
- 0002-ima-update-kexec-buffer-before-executing-soft-reboot.patch
- 0003-ima-on-soft-reboot-save-the-measurement-list.patch

My patches are based on [1] and [2] in the References section below.
I also looked at [3].  It has a few kexec_*_handover_buffer().
I was not sure if they were needed.  As per my limited understanding in
kexec space [1] and [2] together seemed sufficient for the solution.
===========================================================================
------------------------------------------
Problems in the solution above
------------------------------------------
Earlier my solution patches were crashing the Kernel. After a few fixes,
the patches are not crashing the Kernel anymore, but they don't seem to 
be working to capture the events between 'kexec load' and 'kexec 
execute' either.

When I was debugging it using printks and gdb, I found one potential
location where it was failing.

test_0001-move-ima_add_kexec_buffer-from-kexec-load-to-execute.patch
has that location.


I would really appreciate if someone of you could help me provide
further guidance to make progress on this work.

==================================================================
----------------
References
----------------

[1] [PATHC v2 5/9] ima: on soft reboot, save the measurement list
https://lore.kernel.org/lkml/1472596811-9596-6-git-send-email-zohar@linux.vnet.ibm.com/

[2] PATCH v2 4/6] kexec_file: Add mechanism to update kexec segments.
https://lkml.org/lkml/2016/8/16/577

[3] [PATCH 1/6] kexec_file: Add buffer hand-over support
https://lore.kernel.org/linuxppc-dev/1466473476-10104-6-git-send-email-bauerman@linux.vnet.ibm.com/T/

~Tushar

[-- Attachment #2: test_0001-move-ima_add_kexec_buffer-from-kexec-load-to-execute.patch --]
[-- Type: text/x-patch, Size: 2278 bytes --]

From 87fd7b7ef7b3936cd454999a26f546d2d1c2f5c0 Mon Sep 17 00:00:00 2001
From: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Date: Mon, 3 Jan 2022 13:59:01 -0800
Subject: [PATCH 1/1] move ima_add_kexec_buffer() from kexec load to execute

Experiment moving ima_add_kexec_buffer() from kexec load to execute.
This logic fails in kexec_add_buffer() - since the control pages are 
already added in kimage_alloc_normal_control_pages(), which get called
during kexec LOAD. 
 E.g. #kexec -s -l /etc/ima/Image.kexec --reuse-cmdline

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 kernel/kexec_core.c | 5 +++++
 kernel/kexec_file.c | 7 +++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 5a5d192a89ac..7c04203dedef 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -39,6 +39,7 @@
 #include <linux/hugetlb.h>
 #include <linux/objtool.h>
 #include <linux/kmsg_dump.h>
+#include <linux/ima.h>
 
 #include <asm/page.h>
 #include <asm/sections.h>
@@ -1166,6 +1167,10 @@ int kernel_kexec(void)
 #endif
 	{
 		kexec_in_progress = true;
+
+		/* IMA needs to pass the measurement list to the next kernel. */
+		ima_add_kexec_buffer(kexec_image);
+
 		kernel_restart_prepare("kexec reboot");
 		migrate_to_reboot_cpu();
 
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 8347fc158d2b..d6fc28be825c 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -275,8 +275,6 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 				  image->cmdline_buf_len - 1);
 	}
 
-	/* 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);
@@ -689,6 +687,11 @@ int kexec_add_buffer(struct kexec_buf *kbuf)
 	 * no destination overlaps.
 	 */
 	if (!list_empty(&kbuf->image->control_pages)) {
+		/* Tushar: This patch fails here,
+		 * since the control pages get added in kimage_alloc_normal_control_pages()
+		 * which get called during kexec LOAD. E.g.
+		 *     kexec -s -l /etc/ima/Image.kexec --reuse-cmdline
+		 */
 		WARN_ON(1);
 		return -EINVAL;
 	}
-- 
2.25.1


[-- Attachment #3: 0003-ima-on-soft-reboot-save-the-measurement-list.patch --]
[-- Type: text/x-patch, Size: 2721 bytes --]

From 75cffd7103a8018ded020b5d2a7a9f95cbf8428f Mon Sep 17 00:00:00 2001
From: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Date: Mon, 3 Jan 2022 14:09:55 -0800
Subject: [PATCH 3/3] ima: on soft reboot, save the measurement list

This patch uses the kexec buffer passing mechanism to pass the
serialized IMA binary_runtime_measurements to the next kernel.

Co-developed-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima_kexec.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 577462e9f9cc..84ddcf0d2673 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -12,9 +12,14 @@
 #include <linux/kexec.h>
 #include <linux/of.h>
 #include <linux/ima.h>
+#include <linux/reboot.h>
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+/* Physical address of the measurement buffer in the next kernel. */
+static unsigned long kexec_buffer_load_addr;
+static size_t kexec_segment_size;
+
 static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 				     unsigned long segment_size)
 {
@@ -127,19 +132,20 @@ void ima_add_kexec_buffer(struct kimage *image)
 	/* use more understandable variable names than defined in kbuf */
 	void *kexec_buffer = NULL;
 	size_t kexec_buffer_size;
-	size_t kexec_segment_size;
 	int ret;
 
+	bool first_kexec_load = kexec_buffer_load_addr == 0;
+
 	/*
-	 * Reserve an extra half page of memory for additional measurements
-	 * added during the kexec load.
+	 * Reserve an extra page of memory for additional measurements
+	 * added during the kexec load + execute.
 	 */
 	binary_runtime_size = ima_get_binary_runtime_size();
 	if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE)
 		kexec_segment_size = ULONG_MAX;
 	else
 		kexec_segment_size = ALIGN(ima_get_binary_runtime_size() +
-					   PAGE_SIZE / 2, PAGE_SIZE);
+					   PAGE_SIZE, PAGE_SIZE);
 	if ((kexec_segment_size == ULONG_MAX) ||
 	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
 		pr_err("Binary measurement list too large.\n");
@@ -167,6 +173,11 @@ void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_buffer_size = kexec_segment_size;
 	image->ima_buffer = kexec_buffer;
 
+	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",
 		 kbuf.mem);
 }
-- 
2.25.1


[-- Attachment #4: 0002-ima-update-kexec-buffer-before-executing-soft-reboot.patch --]
[-- Type: text/x-patch, Size: 2429 bytes --]

From 9282ead63eb7dccdce9430275889376d19dea59d Mon Sep 17 00:00:00 2001
From: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Date: Mon, 3 Jan 2022 14:21:18 -0800
Subject: [PATCH 2/3] ima: update kexec buffer before executing soft reboot

ima_update_kexec_buffer updates IMA buffer in kexec_image to have
its contents updated. This function should be called during kexec execute
so that IMA can save the measurement list. 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.

Co-developed-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima_kexec.c | 38 ++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index f799cc278a9a..577462e9f9cc 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -73,6 +73,44 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 	return ret;
 }
 
+/*
+ * Called during kexec execute so that IMA can save the measurement list.
+ */
+static int ima_update_kexec_buffer(struct notifier_block *self,
+				   unsigned long action, void *data)
+{
+	void *kexec_buffer = NULL;
+	size_t kexec_buffer_size;
+	int ret;
+
+	if (!kexec_in_progress)
+		return NOTIFY_OK;
+
+	kexec_buffer_size = ima_get_binary_runtime_size();
+	if (kexec_buffer_size >
+	    (kexec_segment_size - sizeof(struct ima_kexec_hdr))) {
+		pr_err("Binary measurement list grew too large.\n");
+		goto out;
+	}
+
+	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
+				  kexec_segment_size);
+	if (!kexec_buffer) {
+		pr_err("Not enough memory for the kexec measurement buffer.\n");
+		goto out;
+	}
+	ret = kexec_update_segment(kexec_buffer, kexec_buffer_size,
+				   kexec_buffer_load_addr, kexec_segment_size);
+	if (ret)
+		pr_err("Error updating kexec buffer: %d\n", ret);
+out:
+	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 for the measurement list for the next kernel.
-- 
2.25.1


[-- Attachment #5: 0001-kexec_file-Add-mechanism-to-update-kexec-segments.patch --]
[-- Type: text/x-patch, Size: 4492 bytes --]

From 0133b19ced41a10134973c374adb3429323b20e4 Mon Sep 17 00:00:00 2001
From: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Date: Mon, 3 Jan 2022 14:08:56 -0800
Subject: [PATCH 1/3] kexec_file: Add mechanism to update kexec segments

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.

Co-developed-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 include/linux/kexec.h |   2 +
 kernel/kexec_core.c   | 102 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 0c994ae37729..d14c9be7faa3 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -182,6 +182,8 @@ int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name,
 				   void *buf, unsigned int size,
 				   bool get_value);
 void *kexec_purgatory_get_symbol_addr(struct kimage *image, const char *name);
+int kexec_update_segment(const char *buffer, unsigned long bufsz,
+			 unsigned long load_addr, unsigned long memsz);
 
 /* Architectures may override the below functions */
 int arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 5a5d192a89ac..af8ea7bf0251 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -779,6 +779,108 @@ 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, unsigned long bufsz,
+			 unsigned long load_addr, unsigned long 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) {
+			/*
+			 * TusharSu: Not doing the checksum test for the time being.
+			 * if (kexec_image->segment[i].do_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%lx\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(addr);
+			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) {
+				struct page *page;
+				char *ptr;
+				size_t uchunk, mchunk;
+
+				page = kmap_to_page(addr);
+
+				ptr = kmap_atomic(page);
+				ptr += load_addr & ~PAGE_MASK;
+				mchunk = min_t(size_t, memsz,
+					       PAGE_SIZE - (load_addr & ~PAGE_MASK));
+				uchunk = min(bufsz, mchunk);
+				memcpy(ptr, buffer, uchunk);
+
+				kunmap_atomic(ptr);
+
+				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)
 {
-- 
2.25.1


[-- Attachment #6: Type: text/plain, Size: 143 bytes --]

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: need help: patches to capture events between kexec load and execute

[Jonathan McDowell]

On Fri, Jun 03, 2022 at 11:16:04PM -0700, Tushar Sugandhi wrote:
> We believe we have found one gap in the IMA/kexec interaction.
> And we need your inputs as Linux Kernel maintainers/experts to fix that
> gap.
> 
> -------------
> Problem:
> -------------
> The current Kernel behavior is IMA measurements are snapshotted at
> 'kexec load' and not at 'kexec execute'.  And IMA log is then carried
> over to the new Kernel after 'kexec execute'.
> 
> Some systems can be configured to call 'kexec load' first, and followed
> by 'kexec execute' after some time.  (as opposed to calling 'load' and
> 'execute' in one single kexec command).  In this scenario, if new IMA
> measurements are added between 'kexec load' and 'kexec execute' - the
> TPM PCRs are extended with the IMA events between load and execute, but
> those IMA events are not carried over to the new kernel after kexec soft
> reboot.  This results in mismatch between TPM PCR quotes and the actual
> IMA measurements log post kexec.
> 
> ===========================================================================
> -------------
> Scenario
> -------------
> Here is the order of operations I followed to confirm the issue.
> 
> (a) Call 'kexec load'
> 
>     #kexec -s -l /etc/ima/Image.kexec --reuse-cmdline
> 
> 
> (b) Touch one of the files that would be measured by IMA
> 
>     #cat /run/systemd/journal/streams/8:16351
> 
> 
> (c) Verify that this measurement event is part of the IMA log.
> 
>     #cat /sys/kernel/security/ima/ascii_runtime_measurements | grep 16351
> 
>     <returns the file entry in IMA log>
> 
> (d) Call 'kexec execute'
> 
>     #kexec -s -e
> 
> 
> (e) After kexec soft reboot, the file measurement event is not present
>     in the IMA log anymore.  Because this measurement in the previous
>     kernel had happened after the IMA log was snapshotted in the
>     previous kernel.
> 
>     #cat /sys/kernel/security/ima/ascii_runtime_measurements | grep 16351
> 
>     <does not return the file entry in IMA log>
> 
> ===========================================================================
> -------------
> Solution
> -------------
> Tyler pointed me to the past work in this area.
> (Please see references section below)
> 
> I used it to create the patches for capturing IMA events in between
> "kexec load" and "kexec execute". (please find attached)
> 
> - 0001-kexec_file-Add-mechanism-to-update-kexec-segments.patch
> - 0002-ima-update-kexec-buffer-before-executing-soft-reboot.patch
> - 0003-ima-on-soft-reboot-save-the-measurement-list.patch
> 
> My patches are based on [1] and [2] in the References section below.
> I also looked at [3].  It has a few kexec_*_handover_buffer().
> I was not sure if they were needed.  As per my limited understanding in
> kexec space [1] and [2] together seemed sufficient for the solution.
> ===========================================================================
> ------------------------------------------
> Problems in the solution above
> ------------------------------------------
> Earlier my solution patches were crashing the Kernel. After a few fixes,
> the patches are not crashing the Kernel anymore, but they don't seem to be
> working to capture the events between 'kexec load' and 'kexec execute'
> either.
> 
> When I was debugging it using printks and gdb, I found one potential
> location where it was failing.
> 
> test_0001-move-ima_add_kexec_buffer-from-kexec-load-to-execute.patch
> has that location.
> 
> 
> I would really appreciate if someone of you could help me provide
> further guidance to make progress on this work.

The main piece that seems to be missing is updating the size of the IMA
buffer in the mechanism used to pass its details to the new kernel (so
device tree in current mainline [setup_ima_buffer], the setup_data piece
in my x86_64 patches). Without that you'll only load the original set of
measurements even if the extra data is in the buffer.

I think there's also a potential problem in that you assume an extra
page is sufficient for any additional measurements which may or may not
be the case - I don't see any check in ima_update_kexec_buffer that the
buffer is big enough for the new IMA data.

J.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: need help: patches to capture events between kexec load and execute

[Tushar Sugandhi]

Hi Jonathan,
Thank you for taking a look at my patches.

On 6/6/22 01:19, Jonathan McDowell wrote:
> The main piece that seems to be missing is updating the size of the IMA
> buffer in the mechanism used to pass its details to the new kernel (so
> device tree in current mainline [setup_ima_buffer], the setup_data piece
> in my x86_64 patches). Without that you'll only load the original set of
> measurements even if the extra data is in the buffer.

Ok. Thanks for the pointer.
I will take a look at your patches and get back to you with updates.
> 
> I think there's also a potential problem in that you assume an extra
> page is sufficient for any additional measurements which may or may not
> be the case - I don't see any check in ima_update_kexec_buffer that the
> buffer is big enough for the new IMA data.
> 
Agreed.  I wanted to solve that problem next.
First I wanted to verify at least a few events are getting copied after
the kexec soft reboot.
But thanks for pointing this out.
> J.

Again, appreciate your pointers.

Thanks,
Tushar

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: need help: patches to capture events between kexec load and execute

[Mimi Zohar]

Hi Tushar,

On Thu, 2023-05-25 at 10:21 -0700, Tushar Sugandhi wrote:

> The issue of IMA measurements getting lost between kexec 'load' and 'execute' still exists. 
> I verified it on the mainline kernel 6.4.rc3. See *Appendix A* for details.
> 
> I went through Thiago's patches he wrote several years ago, and tried to develop a solution.
> 
> I was facing some issues with physical to virtual address translation. 
> One of my co-worker at Microsoft helped my re-write the logic which seems to be working.
> See the attached patch and *Appendix B* for details.
> 
> The basic functionality is working. I need to polish the code, and handle error paths in a better way.
> But before doing that, I need your feedback on the fundamental approach.
> Since I am not a kexec expert, it’d be great if I could get help with the code review
> and also suggestions on scenarios to test to validate the patch thoroughly. 
> Let me know if I should first post the patch as RFC on public forums for that.

Thanks, Tushar.   Measurements can and are currently being added to the
IMA measurement list between kexec load and execute, but are not being
carried across kexec.  These measurements also extend the TPM.  After
the soft reboot, without these additional measurements the IMA
measurement list cannot be verified against the TPM PCRs.

Your proposed patch, like Thiago's, saved the entire IMA measurement
list again.  Assuming the buffer size can't change between kexec load
and execute, as per the comment, why not just allocate the buffer on
kexec load and fill it on kexec exec?

To simulate the existing behavior, fill the buffer with as many
complete measurement records as the buffer will hold.

-- 
thanks,

Mimi


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: need help: patches to capture events between kexec load and execute

[Mimi Zohar]

On Wed, 2023-05-31 at 15:02 -0700, Tushar Sugandhi wrote:
> Hi Mimi,
> 
> On 5/31/23 04:39, Mimi Zohar wrote:
> > Hi Tushar,
> >
> > On Thu, 2023-05-25 at 10:21 -0700, Tushar Sugandhi wrote:
> >
> >> The issue of IMA measurements getting lost between kexec 'load' and 'execute' still exists.
> >> I verified it on the mainline kernel 6.4.rc3. See *Appendix A* for details.
> >>
> >> I went through Thiago's patches he wrote several years ago, and tried to develop a solution.
> >>
> >> I was facing some issues with physical to virtual address translation.
> >> One of my co-worker at Microsoft helped my re-write the logic which seems to be working.
> >> See the attached patch and *Appendix B* for details.
> >>
> >> The basic functionality is working. I need to polish the code, and handle error paths in a better way.
> >> But before doing that, I need your feedback on the fundamental approach.
> >> Since I am not a kexec expert, it’d be great if I could get help with the code review
> >> and also suggestions on scenarios to test to validate the patch thoroughly.
> >> Let me know if I should first post the patch as RFC on public forums for that.
> > Thanks, Tushar.   Measurements can and are currently being added to the
> > IMA measurement list between kexec load and execute, but are not being
> > carried across kexec.  These measurements also extend the TPM.  After
> > the soft reboot, without these additional measurements the IMA
> > measurement list cannot be verified against the TPM PCRs.
> Yup. IMA measurement list goes out of sync with TPM PCRs after the soft 
> reboot.
> So the measurement list cannot be verified against PCRs. That's the issue.
> Thanks for acknowledging it.
> > Your proposed patch, like Thiago's, saved the entire IMA measurement
> > list again.  Assuming the buffer size can't change between kexec load
> > and execute, as per the comment, why not just allocate the buffer on
> > kexec load and fill it on kexec exec?
> >
> > To simulate the existing behavior, fill the buffer with as many
> > complete measurement records as the buffer will hold.


> I was under the impression that I was doing the same. i.e. allocate at 
> 'load' and fill on 'execute'.
> But I realized that in my patch ima_dump_measurement_list() gets called 
> twice - once at 'load' and once at 'execute'.
> Is that what you meant by "my patch saved the entire IMA measurement 
> list again" ?

Exactly, there's no need for copying the measurement list twice.  The
first N number of measurements haven't changed.

> ima_dump_measurement_list() calls vmalloc, memset, and memcpy.
> So there is definitely some redundancy. Let me see how can I optimize it.

Please look at my suggestion.

> Attaching my original patch again for reference.

Patches should be posted inline.  Please don't do this.

-- 
thanks,

Mimi



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: need help: patches to capture events between kexec load and execute

[Stefan Berger]

On 5/31/23 18:43, Mimi Zohar wrote:
> On Wed, 2023-05-31 at 15:02 -0700, Tushar Sugandhi wrote:
>> Hi Mimi,
>>
>> On 5/31/23 04:39, Mimi Zohar wrote:
>>> Hi Tushar,
>>>
>>> On Thu, 2023-05-25 at 10:21 -0700, Tushar Sugandhi wrote:
>>>
>>>> The issue of IMA measurements getting lost between kexec 'load' and 'execute' still exists.
>>>> I verified it on the mainline kernel 6.4.rc3. See *Appendix A* for details.

I think there's a 2nd problem.  Once the IMA measurement list is frozen (at kexec 'exec' stage)
IMA must stop extending PCRs. It can log (into the void) if it wanted to but the PCR extensions
have to stop otherwise the TPM's PCR state won't match the log in the kexec'ed-to kernel. I have
seen that on PPC64 some processes are being kicked off by kexec 'exec' that end up causing TPM
driver error message due to what seems to be a shutdown of the driver subsystem at this point.
I am not sure what an elegant method would be to stop PCR extensions. Maybe a flag on the level
of IMA would do? Or notifying the TPM driver to reject PCR extensions or just any command?

    Stefan


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: need help: patches to capture events between kexec load and execute

[Mimi Zohar]

On Tue, 2023-06-06 at 11:37 -0400, Stefan Berger wrote:
> 
> On 5/31/23 18:43, Mimi Zohar wrote:
> > On Wed, 2023-05-31 at 15:02 -0700, Tushar Sugandhi wrote:
> >> Hi Mimi,
> >>
> >> On 5/31/23 04:39, Mimi Zohar wrote:
> >>> Hi Tushar,
> >>>
> >>> On Thu, 2023-05-25 at 10:21 -0700, Tushar Sugandhi wrote:
> >>>
> >>>> The issue of IMA measurements getting lost between kexec 'load' and 'execute' still exists.
> >>>> I verified it on the mainline kernel 6.4.rc3. See *Appendix A* for details.
> 
> I think there's a 2nd problem.  Once the IMA measurement list is frozen (at kexec 'exec' stage)
> IMA must stop extending PCRs. It can log (into the void) if it wanted to but the PCR extensions
> have to stop otherwise the TPM's PCR state won't match the log in the kexec'ed-to kernel. I have
> seen that on PPC64 some processes are being kicked off by kexec 'exec' that end up causing TPM
> driver error message due to what seems to be a shutdown of the driver subsystem at this point.
> I am not sure what an elegant method would be to stop PCR extensions. Maybe a flag on the level
> of IMA would do? Or notifying the TPM driver to reject PCR extensions or just any command?

Thank you for raising this concern.

Agreed "kexec exec" might trigger additional measurements.  As long as
these are known, consistent measurements, they could be pre-measured
before calling "kexec exec".

-- 
thanks,

Mimi


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: need help: patches to capture events between kexec load and execute

[Stefan Berger]

On 6/6/23 11:59, Mimi Zohar wrote:
> On Tue, 2023-06-06 at 11:37 -0400, Stefan Berger wrote:
>>
>> On 5/31/23 18:43, Mimi Zohar wrote:
>>> On Wed, 2023-05-31 at 15:02 -0700, Tushar Sugandhi wrote:
>>>> Hi Mimi,
>>>>
>>>> On 5/31/23 04:39, Mimi Zohar wrote:
>>>>> Hi Tushar,
>>>>>
>>>>> On Thu, 2023-05-25 at 10:21 -0700, Tushar Sugandhi wrote:
>>>>>
>>>>>> The issue of IMA measurements getting lost between kexec 'load' and 'execute' still exists.
>>>>>> I verified it on the mainline kernel 6.4.rc3. See *Appendix A* for details.
>>
>> I think there's a 2nd problem.  Once the IMA measurement list is frozen (at kexec 'exec' stage)
>> IMA must stop extending PCRs. It can log (into the void) if it wanted to but the PCR extensions
>> have to stop otherwise the TPM's PCR state won't match the log in the kexec'ed-to kernel. I have
>> seen that on PPC64 some processes are being kicked off by kexec 'exec' that end up causing TPM
>> driver error message due to what seems to be a shutdown of the driver subsystem at this point.
>> I am not sure what an elegant method would be to stop PCR extensions. Maybe a flag on the level
>> of IMA would do? Or notifying the TPM driver to reject PCR extensions or just any command?
> 
> Thank you for raising this concern.
> 
> Agreed "kexec exec" might trigger additional measurements.  As long as
> these are known, consistent measurements, they could be pre-measured
> before calling "kexec exec".
> 

Tushar, Mimi,

   I tried this now on ppc64 and with the following patch applied on top of yours it seems to work.
Also the problem with the TPM PCR extensions seems to be gone with your changes to
ima_add_template_entry()  -- it's probably worth mentioning this in the patch description.

phys_to_page doesn't seem to be generally available, so hopefully this will be a solution for more architectures.
[I haven't tested this on x86_64 since I couldn't get kexec to work just to begin with in any of my VMs.]

Regards,
    Stefan

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index f0bbdd403b6c..30ab7f81b508 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -966,7 +966,7 @@ void *kimage_map_segment(struct kimage *image,
                 else if (entry & IND_SOURCE) {
                         if (dest_page_addr >= addr && dest_page_addr < eaddr) {
                                 src_page_addr = entry & PAGE_MASK;
-                               src_pages[i++] = phys_to_page(src_page_addr);
+                               src_pages[i++] = virt_to_page(__va(src_page_addr));
                                 if (i == npages)
                                         break;
                                 dest_page_addr += PAGE_SIZE;

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec