[PATCH v5 0/8] ima: kexec: measure events between kexec load and execute

[PATCH v5 0/8] ima: kexec: measure events between kexec load and execute

[Tushar Sugandhi]

The current Kernel behavior is IMA measurements snapshot is taken at
kexec 'load' and not at kexec 'execute'.  IMA log is then carried
over to the new Kernel after kexec 'execute'.

New events can be measured during/after the IMA log snapshot at kexec 
'load' and before the system boots to the new Kernel.  In this scenario,
the TPM PCRs are extended with these events, but they are not carried
over to the new Kernel after kexec soft reboot since the snapshot is
already taken.  This results in mismatch between TPM PCR quotes and the
actual IMA measurements list after kexec soft reboot, which in turn
results in remote attestation failure.

To solve this problem - 
 - allocate the necessary buffer at kexec 'load' time,
 - populate the buffer with the IMA measurements at kexec 'execute' time, 
 - and measure two new IMA events 'kexec_load' and 'kexec_execute' as
   critical data to help detect missing events after kexec soft reboot.

The solution details include:
 - refactoring the existing code to allocate a buffer to hold IMA
   measurements at kexec 'load', and dump the measurements at kexec
   'execute'

 - IMA functionality to suspend and resume measurements as needed during
   buffer copy at kexec 'execute',

 - kexec functionality for mapping the segments from the current Kernel
   to the subsequent one, 

 - necessary changes to the kexec_file_load syscall, enabling it to call
   the ima functions,

 - registering a reboot notifier which gets called during kexec 
   'execute',

 - introducing a new Kconfig option to configure the extra memory to be
   allocated for passing IMA log from the current Kernel to the next,
   
 - introducing two new events to be measured by IMA during kexec, to
   help diagnose if the IMA log was copied fully or partially, from the
   current Kernel to the next,

 - excluding IMA segment while calculating and storing digest in function
   kexec_calculate_store_digests(), since IMA segment can be modified
   after the digest is computed during kexec 'load'.  This will ensure
   that the segment is not added to the 'purgatory_sha_regions', and thus
   not verified by verify_sha256_digest().

The changes proposed in this series ensure the integrity of the IMA
measurements is preserved across kexec soft reboots, thus significantly
improving the security of the Kernel post kexec soft reboots.

There were previous attempts to fix this issue [1], [2], [3].  But they
were not merged into the mainline Kernel.

We took inspiration from the past work [1] and [2] while working on this
patch series.

V4 of this series is available here[6] for reference.

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/

[4] [PATCH v2 0/7] ima: kexec: measure events between kexec load and execute
https://lore.kernel.org/all/20231005182602.634615-1-tusharsu@linux.microsoft.com/

[5] [PATCH v3 0/7] ima: kexec: measure events between kexec load and execute
https://lore.kernel.org/all/20231216010729.2904751-1-tusharsu@linux.microsoft.com/

[6] [PATCH v4 0/7] ima: kexec: measure events between kexec load and execute
https://lore.kernel.org/all/20240122183804.3293904-1-tusharsu@linux.microsoft.com/

Change Log v5:
 - Incorporated feedback from the community (Stefan Berger and
   Mimi Zohar) on v4 of this series[6].
 - Rebased the patch series to mainline 6.8.0-rc1.
 - Verified all the patches are bisect-safe by booting into each
   patch and verifying multiple kexec 'load' operations work,
   and also verifying kexec soft reboot works, and IMA log gets
   carried over for each patch.
 - Divided the patch #4 in the v4 of the series[6] into two separate
   patches. One (patch #4) to setup the infrastructure/stub functions
   to prepare the IMA log copy from Kexec 'load' to 'execute', and 
   another one (patch #5) to actually copy the log.
 - Updated the config description for IMA_KEXEC_EXTRA_MEMORY_KB
   to remove unnecessary references related to backwards compatibility.
 - Fixed a typo in log message/removed an extra line etc.
 - Updated patch descriptions as per the feedback.

Change Log v4:
 - Incorporated feedback from the community (Stefan Berger and
   Mimi Zohar) on v3 of this series[5].
 - Rearranged patches so that they remain bisect-safe i.e. the
   system can go through kexec soft reboot, and IMA log is carried
   over after each patch.
 - Verified all the patches are bisect-safe by booting into each
   patch and verifying kexec soft reboot works, and IMA log gets
   carried over.
 - Suspend-resume measurements is now a separate patch (patch #5)
   and all the relevant code is part of the same patch.
 - Excluding IMA segment from segment digest verification is now a
   separate patch. (patch #3).
 - Registering reboot notifier and functions related to move ima 
   log copy from kexec load to execute are now part of the same
   patch (patch #4) to protect bisect-safeness of the series.
 - Updated the title of patch #6 as per the feedback.
 - The default value of kexec extra memory for IMA measurements
   is set to half the PAGESIZE to maintain backwards compatibility.
 - Added number of IMA measurement records as part of 'kexec_load' 
   and 'kexec_execute' IMA critical data events.
 - Updated patch descriptions as necessary.

Change Log v3:
 - Incorporated feedback from the community (Stefan Berger and
   Mimi Zohar) on v2 of this series[4].
 - Renamed functions and removed extraneous checks and code comments.
 - Updated patch descriptions and titles as necessary.
 - Updated kexec_calculate_store_digests() in patch 2/7 to exclude ima
   segment from calculating and storing digest.
 - Updated patch 3/7 to use kmalloc_array instead of kmalloc and freed
   memory early to avoid potential memory leak.
 - Updated patch 6/7 to change Kconfig option IMA_KEXEC_EXTRA_PAGES to
   IMA_KEXEC_EXTRA_MEMORY_KB to allocate the memory in kb rather than
   in number of pages.
 - Optimized patch 7/7 not to free and alloc memory if the buffer size
   hasn't changed during multiple kexec 'load' operations.
 - Fixed a bug in patch 7/7 to measure multiple 'kexec_load' events even
   if buffer size hasn't changed.
 - Verified the patches are bisect-safe by compiling and booting into
   each patch individually.


Change Log v2:
 - Incorporated feedback from the community on v1 series.
 - Refactored the existing ima_dump_measurement_list to move buffer
   allocation functionality to ima_alloc_kexec_buf() function.
 - Introduced a new Kconfig option to configure the memory.
 - Updated the logic to copy the IMA log only in case of kexec soft 
   reboot, and not on kexec crash.
 - Updated the logic to copy as many IMA events as possible in case of
   memory constraint, rather than just bailing out.
 - Introduced two new events to be measured by IMA during kexec, to
   help diagnose if the IMA log was copied fully or partially from the
   current Kernel to the next.
 - Refactored patches to ensure no warnings during individual patch
   compilation.
 - Used virt_to_page instead of phys_to_page.
 - Updated patch descriptions as necessary.



Tushar Sugandhi (8):
  ima: define and call ima_alloc_kexec_file_buf
  kexec: define functions to map and unmap segments
  ima: kexec: skip IMA segment validation after kexec soft reboot
  ima: kexec: define functions to copy IMA log at soft boot
  ima: kexec: move IMA log copy from kexec load to execute
  ima: suspend measurements during buffer copy at kexec execute
  ima: make the kexec extra memory configurable
  ima: measure kexec load and exec events as critical data

 include/linux/ima.h                |   3 +
 include/linux/kexec.h              |  16 ++
 kernel/kexec_core.c                |  59 +++++++-
 kernel/kexec_file.c                |  16 ++
 security/integrity/ima/Kconfig     |   9 ++
 security/integrity/ima/ima.h       |   2 +
 security/integrity/ima/ima_kexec.c | 225 ++++++++++++++++++++++++-----
 security/integrity/ima/ima_queue.c |  32 ++++
 8 files changed, 322 insertions(+), 40 deletions(-)

-- 
2.25.1

[PATCH v5 1/8] ima: define and call ima_alloc_kexec_file_buf

[Tushar Sugandhi]

Carrying the IMA measurement list across kexec requires allocating a
buffer and copying the measurement records.  Separate allocating the
buffer and copying the measurement records into separate functions in
order to allocate the buffer at kexec 'load' and copy the measurements
at kexec 'execute'.

This patch includes the following changes:
 - Refactor ima_dump_measurement_list() to move the memory allocation
   to a separate function ima_alloc_kexec_file_buf() which allocates
   buffer of size 'kexec_segment_size' at kexec 'load'.
 - Make the local variable ima_kexec_file in ima_dump_measurement_list()
   as local static to the file, so that it can be accessed from 
   ima_alloc_kexec_file_buf().
 - Make necessary changes to the function ima_add_kexec_buffer() to call
   the above two functions.

Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima_kexec.c | 107 +++++++++++++++++++++--------
 1 file changed, 78 insertions(+), 29 deletions(-)

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index dadc1d138118..a9cb5e882e2e 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -15,62 +15,98 @@
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+static struct seq_file ima_kexec_file;
+
+static void ima_reset_kexec_file(struct seq_file *sf)
+{
+	sf->buf = NULL;
+	sf->size = 0;
+	sf->read_pos = 0;
+	sf->count = 0;
+}
+
+static void ima_free_kexec_file_buf(struct seq_file *sf)
+{
+	vfree(sf->buf);
+	ima_reset_kexec_file(sf);
+}
+
+static int ima_alloc_kexec_file_buf(size_t segment_size)
+{
+	/*
+	 * kexec 'load' may be called multiple times.
+	 * Free and realloc the buffer only if the segment_size is
+	 * changed from the previous kexec 'load' call.
+	 */
+	if (ima_kexec_file.buf &&
+	    ima_kexec_file.size == segment_size &&
+	    ima_kexec_file.read_pos == 0 &&
+	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
+		return 0;
+
+	ima_free_kexec_file_buf(&ima_kexec_file);
+
+	/* segment size can't change between kexec load and execute */
+	ima_kexec_file.buf = vmalloc(segment_size);
+	if (!ima_kexec_file.buf)
+		return -ENOMEM;
+
+	ima_kexec_file.size = segment_size;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
+
+	return 0;
+}
+
 static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 				     unsigned long segment_size)
 {
 	struct ima_queue_entry *qe;
-	struct seq_file file;
 	struct ima_kexec_hdr khdr;
-	int ret = 0;
 
-	/* segment size can't change between kexec load and execute */
-	file.buf = vmalloc(segment_size);
-	if (!file.buf) {
-		ret = -ENOMEM;
-		goto out;
+	if (!ima_kexec_file.buf) {
+		*buffer_size = 0;
+		*buffer = NULL;
+		pr_err("%s: Kexec file buf not allocated\n", __func__);
+		return -EINVAL;
 	}
 
-	file.size = segment_size;
-	file.read_pos = 0;
-	file.count = sizeof(khdr);	/* reserved space */
-
 	memset(&khdr, 0, sizeof(khdr));
 	khdr.version = 1;
+
+	/* Copy as many IMA measurements list records as possible */
 	list_for_each_entry_rcu(qe, &ima_measurements, later) {
-		if (file.count < file.size) {
+		if (ima_kexec_file.count < ima_kexec_file.size) {
 			khdr.count++;
-			ima_measurements_show(&file, qe);
+			ima_measurements_show(&ima_kexec_file, qe);
 		} else {
-			ret = -EINVAL;
+			pr_err("%s: IMA log file is too big for Kexec buf\n",
+			       __func__);
 			break;
 		}
 	}
 
-	if (ret < 0)
-		goto out;
-
 	/*
 	 * fill in reserved space with some buffer details
 	 * (eg. version, buffer size, number of measurements)
 	 */
-	khdr.buffer_size = file.count;
+	khdr.buffer_size = ima_kexec_file.count;
 	if (ima_canonical_fmt) {
 		khdr.version = cpu_to_le16(khdr.version);
 		khdr.count = cpu_to_le64(khdr.count);
 		khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
 	}
-	memcpy(file.buf, &khdr, sizeof(khdr));
+	memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr));
 
 	print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
-			     file.buf, file.count < 100 ? file.count : 100,
+			     ima_kexec_file.buf, ima_kexec_file.count < 100 ?
+			     ima_kexec_file.count : 100,
 			     true);
 
-	*buffer_size = file.count;
-	*buffer = file.buf;
-out:
-	if (ret == -EINVAL)
-		vfree(file.buf);
-	return ret;
+	*buffer_size = ima_kexec_file.count;
+	*buffer = ima_kexec_file.buf;
+
+	return 0;
 }
 
 /*
@@ -108,13 +144,20 @@ void ima_add_kexec_buffer(struct kimage *image)
 		return;
 	}
 
-	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
-				  kexec_segment_size);
-	if (!kexec_buffer) {
+	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
+	if (ret < 0) {
 		pr_err("Not enough memory for the kexec measurement buffer.\n");
 		return;
 	}
 
+	ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
+					kexec_segment_size);
+	if (ret < 0) {
+		pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
+		       __func__, ret);
+		return;
+	}
+
 	kbuf.buffer = kexec_buffer;
 	kbuf.bufsz = kexec_buffer_size;
 	kbuf.memsz = kexec_segment_size;
@@ -129,6 +172,12 @@ void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_buffer_size = kexec_segment_size;
 	image->ima_buffer = kexec_buffer;
 
+	/*
+	 * kexec owns kexec_buffer after kexec_add_buffer() is called
+	 * and it will vfree() that buffer.
+	 */
+	ima_reset_kexec_file(&ima_kexec_file);
+
 	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		      kbuf.mem);
 }
-- 
2.25.1

[PATCH v5 2/8] kexec: define functions to map and unmap segments

[Tushar Sugandhi]

Currently, the mechanism to map and unmap segments to the kimage
structure is not available to the subsystems outside of kexec.  This
functionality is needed when IMA is allocating the memory segments
during kexec 'load' operation.  Implement functions to map and unmap
segments to kimage.

Implement kimage_map_segment() to enable mapping of IMA buffer source
pages to the kimage structure post kexec 'load'.  This function,
accepting a kimage pointer, an address, and a size, will gather the
source pages within the specified address range, create an array of page
pointers, and map these to a contiguous virtual address range.  The
function returns the start of this range if successful, or NULL if
unsuccessful.

Implement kimage_unmap_segment() for unmapping segments
using vunmap().  Relocate 'for_each_kimage_entry()' macro from
kexec_core.c to kexec.h for broader accessibility.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 include/linux/kexec.h | 13 ++++++++++
 kernel/kexec_core.c   | 59 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 400cb6c02176..3145447eb77a 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -486,6 +486,11 @@ static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) {
 static inline void arch_crash_handle_hotplug_event(struct kimage *image) { }
 #endif
 
+#define for_each_kimage_entry(image, ptr, entry) \
+	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
+		ptr = (entry & IND_INDIRECTION) ? \
+			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
+
 int crash_check_update_elfcorehdr(void);
 
 #ifndef crash_hotplug_cpu_support
@@ -507,6 +512,10 @@ extern bool kexec_file_dbg_print;
 	       kexec_file_dbg_print ? KERN_INFO : KERN_DEBUG,	\
 	       ##__VA_ARGS__)
 
+extern void *kimage_map_segment(struct kimage *image,
+				unsigned long addr, unsigned long size);
+extern void kimage_unmap_segment(void *buffer);
+
 #else /* !CONFIG_KEXEC_CORE */
 struct pt_regs;
 struct task_struct;
@@ -514,6 +523,10 @@ static inline void __crash_kexec(struct pt_regs *regs) { }
 static inline void crash_kexec(struct pt_regs *regs) { }
 static inline int kexec_should_crash(struct task_struct *p) { return 0; }
 static inline int kexec_crash_loaded(void) { return 0; }
+static inline void *kimage_map_segment(struct kimage *image,
+				       unsigned long addr, unsigned long size)
+{ return NULL; }
+static inline void kimage_unmap_segment(void *buffer) { }
 #define kexec_in_progress false
 #endif /* CONFIG_KEXEC_CORE */
 
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index d08fc7b5db97..612ad8783bab 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -582,11 +582,6 @@ void kimage_terminate(struct kimage *image)
 	*image->entry = IND_DONE;
 }
 
-#define for_each_kimage_entry(image, ptr, entry) \
-	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
-		ptr = (entry & IND_INDIRECTION) ? \
-			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
-
 static void kimage_free_entry(kimage_entry_t entry)
 {
 	struct page *page;
@@ -909,6 +904,60 @@ int kimage_load_segment(struct kimage *image,
 	return result;
 }
 
+void *kimage_map_segment(struct kimage *image,
+			 unsigned long addr, unsigned long size)
+{
+	unsigned long eaddr = addr + size;
+	unsigned long src_page_addr, dest_page_addr;
+	unsigned int npages;
+	struct page **src_pages;
+	int i;
+	kimage_entry_t *ptr, entry;
+	void *vaddr = NULL;
+
+	/*
+	 * Collect the source pages and map them in a contiguous VA range.
+	 */
+	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
+	src_pages = kmalloc_array(npages, sizeof(*src_pages), GFP_KERNEL);
+	if (!src_pages) {
+		pr_err("%s: Could not allocate ima pages array.\n", __func__);
+		return NULL;
+	}
+
+	i = 0;
+	for_each_kimage_entry(image, ptr, entry) {
+		if (entry & IND_DESTINATION)
+			dest_page_addr = entry & PAGE_MASK;
+		else if (entry & IND_SOURCE) {
+			if (dest_page_addr >= addr && dest_page_addr < eaddr) {
+				src_page_addr = entry & PAGE_MASK;
+				src_pages[i++] =
+					virt_to_page(__va(src_page_addr));
+				if (i == npages)
+					break;
+				dest_page_addr += PAGE_SIZE;
+			}
+		}
+	}
+
+	/* Sanity check. */
+	WARN_ON(i < npages);
+
+	vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
+	kfree(src_pages);
+
+	if (!vaddr)
+		pr_err("%s: Could not map ima buffer.\n", __func__);
+
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
 struct kexec_load_limit {
 	/* Mutex protects the limit count. */
 	struct mutex mutex;
-- 
2.25.1

[PATCH v5 3/8] ima: kexec: skip IMA segment validation after kexec soft reboot

[Tushar Sugandhi]

kexec_calculate_store_digests() calculates and stores the digest of the
segment at kexec_file_load syscall where the IMA segment is also
allocated.  With this series, the IMA segment will be updated with the
measurement log at kexec soft reboot.  Therefore, it may fail digest
verification in verify_sha256_digest() after kexec soft reboot into the
new Kernel.  Therefore, the digest calculation, storage, and
verification of the IMA segment needs to be skipped.

Skip IMA segment from calculating and storing digest in function
kexec_calculate_store_digests() so that it is not added to the
'purgatory_sha_regions'.

Since verify_sha256_digest() only verifies 'purgatory_sha_regions',
no change is needed in verify_sha256_digest() in this context.

With this change, the IMA segment is not included in the digest
calculation, storage, and verification.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 include/linux/kexec.h              | 3 +++
 kernel/kexec_file.c                | 8 ++++++++
 security/integrity/ima/ima_kexec.c | 3 +++
 3 files changed, 14 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 3145447eb77a..73f0dd0e1787 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -358,6 +358,9 @@ struct kimage {
 
 	phys_addr_t ima_buffer_addr;
 	size_t ima_buffer_size;
+
+	unsigned long ima_segment_index;
+	bool is_ima_segment_index_set;
 #endif
 
 	/* Core ELF header buffer */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index bef2f6f2571b..0e3689bfb0bb 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -750,6 +750,14 @@ static int kexec_calculate_store_digests(struct kimage *image)
 		if (ksegment->kbuf == pi->purgatory_buf)
 			continue;
 
+		/*
+		 * Skip the segment if ima_segment_index is set and matches
+		 * the current index
+		 */
+		if (image->is_ima_segment_index_set &&
+		    i == image->ima_segment_index)
+			continue;
+
 		ret = crypto_shash_update(desc, ksegment->kbuf,
 					  ksegment->bufsz);
 		if (ret)
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index a9cb5e882e2e..ccb072617c2d 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -161,6 +161,7 @@ void ima_add_kexec_buffer(struct kimage *image)
 	kbuf.buffer = kexec_buffer;
 	kbuf.bufsz = kexec_buffer_size;
 	kbuf.memsz = kexec_segment_size;
+	image->is_ima_segment_index_set = false;
 	ret = kexec_add_buffer(&kbuf);
 	if (ret) {
 		pr_err("Error passing over kexec measurement buffer.\n");
@@ -171,6 +172,8 @@ void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_buffer_addr = kbuf.mem;
 	image->ima_buffer_size = kexec_segment_size;
 	image->ima_buffer = kexec_buffer;
+	image->ima_segment_index = image->nr_segments - 1;
+	image->is_ima_segment_index_set = true;
 
 	/*
 	 * kexec owns kexec_buffer after kexec_add_buffer() is called
-- 
2.25.1

[PATCH v5 4/8] ima: kexec: define functions to copy IMA log at soft boot

[Tushar Sugandhi]

IMA log is copied to the new Kernel during kexec 'load' using 
ima_dump_measurement_list().  The log copy at kexec 'load' may result in
loss of IMA measurements during kexec soft reboot.  It needs to be copied
over during kexec 'execute'.  Setup the needed infrastructure to move the
IMA log copy from kexec 'load' to 'execute'. 

Define a new IMA hook ima_update_kexec_buffer() as a stub function.
It will be used to call ima_dump_measurement_list() during kexec 
'execute'.   

Implement kimage_file_post_load() and ima_kexec_post_load() functions
to be invoked after the new Kernel image has been loaded for kexec.
ima_kexec_post_load() maps the IMA buffer to a segment in the newly
loaded Kernel.  It also registers the reboot notifier_block to trigger
ima_update_kexec_buffer() at exec 'execute'.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 include/linux/ima.h                |  3 ++
 kernel/kexec_file.c                |  5 ++++
 security/integrity/ima/ima_kexec.c | 46 ++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 86b57757c7b1..006db20f852d 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -49,6 +49,9 @@ static inline void ima_appraise_parse_cmdline(void) {}
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
+extern void ima_kexec_post_load(struct kimage *image);
+#else
+static inline void ima_kexec_post_load(struct kimage *image) {}
 #endif
 
 #else
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 0e3689bfb0bb..fe59cb7c179d 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -186,6 +186,11 @@ kimage_validate_signature(struct kimage *image)
 }
 #endif
 
+void kimage_file_post_load(struct kimage *image)
+{
+	ima_kexec_post_load(image);
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index ccb072617c2d..1d4d6c122d82 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -12,10 +12,14 @@
 #include <linux/kexec.h>
 #include <linux/of.h>
 #include <linux/ima.h>
+#include <linux/reboot.h>
+#include <asm/page.h>
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
 static struct seq_file ima_kexec_file;
+static void *ima_kexec_buffer;
+static bool ima_kexec_update_registered;
 
 static void ima_reset_kexec_file(struct seq_file *sf)
 {
@@ -184,6 +188,48 @@ void ima_add_kexec_buffer(struct kimage *image)
 	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		      kbuf.mem);
 }
+
+/*
+ * Called during kexec execute so that IMA can update the measurement list.
+ */
+static int ima_update_kexec_buffer(struct notifier_block *self,
+				   unsigned long action, void *data)
+{
+	return NOTIFY_OK;
+}
+
+struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+};
+
+/*
+ * Create a mapping for the source pages that contain the IMA buffer
+ * so we can update it later.
+ */
+void ima_kexec_post_load(struct kimage *image)
+{
+	if (ima_kexec_buffer) {
+		kimage_unmap_segment(ima_kexec_buffer);
+		ima_kexec_buffer = NULL;
+	}
+
+	if (!image->ima_buffer_addr)
+		return;
+
+	ima_kexec_buffer = kimage_map_segment(image,
+					      image->ima_buffer_addr,
+					      image->ima_buffer_size);
+	if (!ima_kexec_buffer) {
+		pr_err("%s: Could not map measurements buffer.\n", __func__);
+		return;
+	}
+
+	if (!ima_kexec_update_registered) {
+		register_reboot_notifier(&update_buffer_nb);
+		ima_kexec_update_registered = true;
+	}
+}
+
 #endif /* IMA_KEXEC */
 
 /*
-- 
2.25.1

[PATCH v5 5/8] ima: kexec: move IMA log copy from kexec load to execute

[Tushar Sugandhi]

ima_dump_measurement_list() is called during kexec 'load', which may
result in loss of IMA measurements during kexec soft reboot.  It needs
to be called during kexec 'execute'.

This patch includes the following changes:
 - Call kimage_file_post_load() from kexec_file_load() syscall only for
   kexec soft reboot scenarios and not for KEXEC_FILE_ON_CRASH.  It will
   map the IMA segment, and register reboot notifier for the function
   ima_update_kexec_buffer() which would copy the IMA log at kexec soft
   reboot.
 - Make kexec_segment_size variable local static to the file, for it to be
   accessible both during kexec 'load' and 'execute'.
 - Move ima_dump_measurement_list() call from ima_add_kexec_buffer()
   to ima_update_kexec_buffer().
 - Remove ima_reset_kexec_file() call from ima_add_kexec_buffer(), now
   that the buffer is being copied at kexec 'execute', and resetting the
   file at kexec 'load' will corrupt the buffer.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 kernel/kexec_file.c                |  3 ++
 security/integrity/ima/ima_kexec.c | 45 +++++++++++++++++++-----------
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index fe59cb7c179d..2d5df320c34f 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -410,6 +410,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 
 	kimage_terminate(image);
 
+	if (!(flags & KEXEC_FILE_ON_CRASH))
+		kimage_file_post_load(image);
+
 	ret = machine_kexec_post_load(image);
 	if (ret)
 		goto out;
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 1d4d6c122d82..98fc9b9782a2 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -19,6 +19,7 @@
 #ifdef CONFIG_IMA_KEXEC
 static struct seq_file ima_kexec_file;
 static void *ima_kexec_buffer;
+static size_t kexec_segment_size;
 static bool ima_kexec_update_registered;
 
 static void ima_reset_kexec_file(struct seq_file *sf)
@@ -129,7 +130,6 @@ 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;
 
 	/*
@@ -154,14 +154,6 @@ void ima_add_kexec_buffer(struct kimage *image)
 		return;
 	}
 
-	ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
-					kexec_segment_size);
-	if (ret < 0) {
-		pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
-		       __func__, ret);
-		return;
-	}
-
 	kbuf.buffer = kexec_buffer;
 	kbuf.bufsz = kexec_buffer_size;
 	kbuf.memsz = kexec_segment_size;
@@ -179,12 +171,6 @@ void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_segment_index = image->nr_segments - 1;
 	image->is_ima_segment_index_set = true;
 
-	/*
-	 * kexec owns kexec_buffer after kexec_add_buffer() is called
-	 * and it will vfree() that buffer.
-	 */
-	ima_reset_kexec_file(&ima_kexec_file);
-
 	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		      kbuf.mem);
 }
@@ -195,7 +181,34 @@ void ima_add_kexec_buffer(struct kimage *image)
 static int ima_update_kexec_buffer(struct notifier_block *self,
 				   unsigned long action, void *data)
 {
-	return NOTIFY_OK;
+	void *buf = NULL;
+	size_t buf_size;
+	int ret = NOTIFY_OK;
+
+	if (!kexec_in_progress) {
+		pr_info("%s: No kexec in progress.\n", __func__);
+		return ret;
+	}
+
+	if (!ima_kexec_buffer) {
+		pr_err("%s: Kexec buffer not set.\n", __func__);
+		return ret;
+	}
+
+	ret = ima_dump_measurement_list(&buf_size, &buf,
+					kexec_segment_size);
+
+	if (!buf) {
+		pr_err("%s: Dump measurements failed. Error:%d\n",
+		       __func__, ret);
+		goto out;
+	}
+	memcpy(ima_kexec_buffer, buf, buf_size);
+out:
+	kimage_unmap_segment(ima_kexec_buffer);
+	ima_kexec_buffer = NULL;
+
+	return ret;
 }
 
 struct notifier_block update_buffer_nb = {
-- 
2.25.1

[PATCH v5 6/8] ima: suspend measurements during buffer copy at kexec execute

[Tushar Sugandhi]

New measurements added to the IMA log while the log is being copied
during the kexec 'execute' may not get copied over.  This can cause the
measurement log to be out of sync with the TPM PCRs that IMA extends,
which could result in breaking the integrity of the measurements after
kexec soft reboot.

Implement and call the functions ima_measurements_suspend() and 
ima_measurements_resume() from ima_update_kexec_buffer().

Add a check in the ima_add_template_entry() function not to measure
events when 'suspend_ima_measurements' flag is set.

This ensures the integrity of the IMA log while it is being copied over
to the new Kernel during kexec 'execute'.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima.h       |  2 ++
 security/integrity/ima/ima_kexec.c |  7 +++++++
 security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c29db699c996..49a6047dd8eb 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
 int ima_restore_measurement_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_list(loff_t bufsize, void *buf);
 int ima_measurements_show(struct seq_file *m, void *v);
+void ima_measurements_suspend(void);
+void ima_measurements_resume(void);
 unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(void);
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 98fc9b9782a2..dbeeb7f1355e 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -184,6 +184,7 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 	void *buf = NULL;
 	size_t buf_size;
 	int ret = NOTIFY_OK;
+	bool resume = false;
 
 	if (!kexec_in_progress) {
 		pr_info("%s: No kexec in progress.\n", __func__);
@@ -195,12 +196,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 		return ret;
 	}
 
+	ima_measurements_suspend();
+
 	ret = ima_dump_measurement_list(&buf_size, &buf,
 					kexec_segment_size);
 
 	if (!buf) {
 		pr_err("%s: Dump measurements failed. Error:%d\n",
 		       __func__, ret);
+		resume = true;
 		goto out;
 	}
 	memcpy(ima_kexec_buffer, buf, buf_size);
@@ -208,6 +212,9 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 	kimage_unmap_segment(ima_kexec_buffer);
 	ima_kexec_buffer = NULL;
 
+	if (resume)
+		ima_measurements_resume();
+
 	return ret;
 }
 
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 532da87ce519..5946a26a2849 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
  */
 static DEFINE_MUTEX(ima_extend_list_mutex);
 
+/*
+ * Used internally by the kernel to suspend-resume ima measurements.
+ */
+static atomic_t suspend_ima_measurements;
+
 /* lookup up the digest value in the hash table, and return the entry */
 static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 						       int pcr)
@@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 	return result;
 }
 
+void ima_measurements_suspend(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 1);
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
+void ima_measurements_resume(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 0);
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
 /*
  * Add template entry to the measurement list and hash table, and
  * extend the pcr.
@@ -176,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 		}
 	}
 
+	/*
+	 * suspend_ima_measurements will be set if the system is
+	 * undergoing kexec soft boot to a new kernel.
+	 * suspending measurements in this short window ensures the
+	 * consistency of the IMA measurement list during copying
+	 * of the kexec buffer.
+	 */
+	if (atomic_read(&suspend_ima_measurements)) {
+		audit_cause = "measurements_suspended";
+		audit_info = 0;
+		goto out;
+	}
+
 	result = ima_add_digest_entry(entry,
 				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
 	if (result < 0) {
-- 
2.25.1

[PATCH v5 7/8] ima: make the kexec extra memory configurable

[Tushar Sugandhi]

The extra memory allocated for carrying the IMA measurement list across
kexec is hard-coded as half a PAGE.  Make it configurable.

Define a Kconfig option, IMA_KEXEC_EXTRA_MEMORY_KB, to configure the
extra memory (in kb) to be allocated for IMA measurements added during
kexec soft reboot.  Ensure the default value of the option is set such
that extra half a page of memory for additional measurements is allocated
for the additional measurements.

Update ima_add_kexec_buffer() function to allocate memory based on the
Kconfig option value, rather than the currently hard-coded one.

Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/Kconfig     |  9 +++++++++
 security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index b98bfe9efd0c..4c0fc53d6aa3 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -320,4 +320,13 @@ config IMA_DISABLE_HTABLE
 	help
 	   This option disables htable to allow measurement of duplicate records.
 
+config IMA_KEXEC_EXTRA_MEMORY_KB
+	int
+	depends on IMA_KEXEC
+	default 0
+	help
+	  IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be
+	  allocated (in kb) for IMA measurements added during kexec soft reboot.
+	  If set to the default value, an extra half a page of memory for those
+	  additional measurements will be allocated.
 endif
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index dbeeb7f1355e..50903d4ce880 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -126,6 +126,7 @@ void ima_add_kexec_buffer(struct kimage *image)
 				  .buf_min = 0, .buf_max = ULONG_MAX,
 				  .top_down = true };
 	unsigned long binary_runtime_size;
+	unsigned long extra_size;
 
 	/* use more understandable variable names than defined in kbuf */
 	void *kexec_buffer = NULL;
@@ -133,15 +134,19 @@ void ima_add_kexec_buffer(struct kimage *image)
 	int ret;
 
 	/*
-	 * Reserve an extra half page of memory for additional measurements
-	 * added during the kexec load.
+	 * Reserve extra memory for measurements added during kexec.
 	 */
-	binary_runtime_size = ima_get_binary_runtime_size();
+	if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0)
+		extra_size = PAGE_SIZE / 2;
+	else
+		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
+	binary_runtime_size = ima_get_binary_runtime_size() + extra_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);
+		kexec_segment_size = ALIGN(binary_runtime_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");
-- 
2.25.1

[PATCH v5 8/8] ima: measure kexec load and exec events as critical data

[Tushar Sugandhi]

The amount of memory allocated at kexec load, even with the extra memory
allocated, might not be large enough for the entire measurement list.  The
indeterminate interval between kexec 'load' and 'execute' could exacerbate
this problem.

Define two new IMA events, 'kexec_load' and 'kexec_execute', to be 
measured as critical data at kexec 'load' and 'execute' respectively.
Report the allocated kexec segment size, IMA binary log size and the
runtime measurements count as part of those events.

These events, and the values reported through them, serve as markers in
the IMA log to verify the IMA events are captured during kexec soft
reboot.  The presence of a 'kexec_load' event in between the last two
'boot_aggregate' events in the IMA log implies this is a kexec soft
reboot, and not a cold-boot.  And the absence of 'kexec_execute' event
after kexec soft reboot implies missing events in that window which
results in inconsistency with TPM PCR quotes, necessitating a cold boot
for a successful remote attestation.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima_kexec.c | 34 +++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 50903d4ce880..31495a043959 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -17,6 +17,8 @@
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+#define IMA_KEXEC_EVENT_LEN 256
+
 static struct seq_file ima_kexec_file;
 static void *ima_kexec_buffer;
 static size_t kexec_segment_size;
@@ -38,6 +40,10 @@ static void ima_free_kexec_file_buf(struct seq_file *sf)
 
 static int ima_alloc_kexec_file_buf(size_t segment_size)
 {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
+	size_t buf_size;
+	long len;
+
 	/*
 	 * kexec 'load' may be called multiple times.
 	 * Free and realloc the buffer only if the segment_size is
@@ -47,7 +53,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
 	    ima_kexec_file.size == segment_size &&
 	    ima_kexec_file.read_pos == 0 &&
 	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
-		return 0;
+		goto out;
 
 	ima_free_kexec_file_buf(&ima_kexec_file);
 
@@ -60,6 +66,18 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
 	ima_kexec_file.read_pos = 0;
 	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
 
+out:
+	buf_size = ima_get_binary_runtime_size();
+	len = atomic_long_read(&ima_htable.len);
+
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;"
+		  "ima_runtime_measurements_count=%ld;",
+		  segment_size, buf_size, len);
+
+	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
+				  strlen(ima_kexec_event), false, NULL, 0);
+
 	return 0;
 }
 
@@ -186,10 +204,12 @@ void ima_add_kexec_buffer(struct kimage *image)
 static int ima_update_kexec_buffer(struct notifier_block *self,
 				   unsigned long action, void *data)
 {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
 	void *buf = NULL;
 	size_t buf_size;
 	int ret = NOTIFY_OK;
 	bool resume = false;
+	long len;
 
 	if (!kexec_in_progress) {
 		pr_info("%s: No kexec in progress.\n", __func__);
@@ -201,6 +221,18 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 		return ret;
 	}
 
+	buf_size = ima_get_binary_runtime_size();
+	len = atomic_long_read(&ima_htable.len);
+
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;"
+		  "ima_runtime_measurements_count=%ld;",
+		  kexec_segment_size, buf_size, len);
+
+	ima_measure_critical_data("ima_kexec", "kexec_execute",
+				  ima_kexec_event, strlen(ima_kexec_event),
+				  false, NULL, 0);
+
 	ima_measurements_suspend();
 
 	ret = ima_dump_measurement_list(&buf_size, &buf,
-- 
2.25.1

Re: [PATCH v5 2/8] kexec: define functions to map and unmap segments

[Stefan Berger]

On 2/14/24 10:38, Tushar Sugandhi wrote:
> Currently, the mechanism to map and unmap segments to the kimage
> structure is not available to the subsystems outside of kexec.  This
> functionality is needed when IMA is allocating the memory segments
> during kexec 'load' operation.  Implement functions to map and unmap
> segments to kimage.
> 
> Implement kimage_map_segment() to enable mapping of IMA buffer source
> pages to the kimage structure post kexec 'load'.  This function,
> accepting a kimage pointer, an address, and a size, will gather the
> source pages within the specified address range, create an array of page
> pointers, and map these to a contiguous virtual address range.  The
> function returns the start of this range if successful, or NULL if
> unsuccessful.
> 
> Implement kimage_unmap_segment() for unmapping segments
> using vunmap().  Relocate 'for_each_kimage_entry()' macro from
> kexec_core.c to kexec.h for broader accessibility.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>   include/linux/kexec.h | 13 ++++++++++
>   kernel/kexec_core.c   | 59 +++++++++++++++++++++++++++++++++++++++----
>   2 files changed, 67 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 400cb6c02176..3145447eb77a 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -486,6 +486,11 @@ static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) {
>   static inline void arch_crash_handle_hotplug_event(struct kimage *image) { }
>   #endif
>   
> +#define for_each_kimage_entry(image, ptr, entry) \
> +	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
> +		ptr = (entry & IND_INDIRECTION) ? \
> +			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
> +
>   int crash_check_update_elfcorehdr(void);
>   
>   #ifndef crash_hotplug_cpu_support
> @@ -507,6 +512,10 @@ extern bool kexec_file_dbg_print;
>   	       kexec_file_dbg_print ? KERN_INFO : KERN_DEBUG,	\
>   	       ##__VA_ARGS__)
>   
> +extern void *kimage_map_segment(struct kimage *image,
> +				unsigned long addr, unsigned long size);
> +extern void kimage_unmap_segment(void *buffer);
> +
>   #else /* !CONFIG_KEXEC_CORE */
>   struct pt_regs;
>   struct task_struct;
> @@ -514,6 +523,10 @@ static inline void __crash_kexec(struct pt_regs *regs) { }
>   static inline void crash_kexec(struct pt_regs *regs) { }
>   static inline int kexec_should_crash(struct task_struct *p) { return 0; }
>   static inline int kexec_crash_loaded(void) { return 0; }
> +static inline void *kimage_map_segment(struct kimage *image,
> +				       unsigned long addr, unsigned long size)
> +{ return NULL; }
> +static inline void kimage_unmap_segment(void *buffer) { }
>   #define kexec_in_progress false
>   #endif /* CONFIG_KEXEC_CORE */
>   
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index d08fc7b5db97..612ad8783bab 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -582,11 +582,6 @@ void kimage_terminate(struct kimage *image)
>   	*image->entry = IND_DONE;
>   }
>   
> -#define for_each_kimage_entry(image, ptr, entry) \
> -	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
> -		ptr = (entry & IND_INDIRECTION) ? \
> -			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
> -
>   static void kimage_free_entry(kimage_entry_t entry)
>   {
>   	struct page *page;
> @@ -909,6 +904,60 @@ int kimage_load_segment(struct kimage *image,
>   	return result;
>   }
>   
> +void *kimage_map_segment(struct kimage *image,
> +			 unsigned long addr, unsigned long size)
> +{
> +	unsigned long eaddr = addr + size;
> +	unsigned long src_page_addr, dest_page_addr;
> +	unsigned int npages;
> +	struct page **src_pages;
> +	int i;
> +	kimage_entry_t *ptr, entry;
> +	void *vaddr = NULL;
> +
> +	/*
> +	 * Collect the source pages and map them in a contiguous VA range.
> +	 */
> +	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
> +	src_pages = kmalloc_array(npages, sizeof(*src_pages), GFP_KERNEL);
> +	if (!src_pages) {
> +		pr_err("%s: Could not allocate ima pages array.\n", __func__);
> +		return NULL;
> +	}
> +
> +	i = 0;
> +	for_each_kimage_entry(image, ptr, entry) {
> +		if (entry & IND_DESTINATION)
> +			dest_page_addr = entry & PAGE_MASK;
> +		else if (entry & IND_SOURCE) {
> +			if (dest_page_addr >= addr && dest_page_addr < eaddr) {
> +				src_page_addr = entry & PAGE_MASK;
> +				src_pages[i++] =
> +					virt_to_page(__va(src_page_addr));
> +				if (i == npages)
> +					break;
> +				dest_page_addr += PAGE_SIZE;
> +			}
> +		}
> +	}
> +
> +	/* Sanity check. */
> +	WARN_ON(i < npages);
> +
> +	vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
> +	kfree(src_pages);
> +
> +	if (!vaddr)
> +		pr_err("%s: Could not map ima buffer.\n", __func__);
> +
> +	return vaddr;
> +}
> +
> +void kimage_unmap_segment(void *segment_buffer)
> +{
> +	vunmap(segment_buffer);
> +}
> +
>   struct kexec_load_limit {
>   	/* Mutex protects the limit count. */
>   	struct mutex mutex;

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [PATCH v5 4/8] ima: kexec: define functions to copy IMA log at soft boot

[Stefan Berger]

On 2/14/24 10:38, Tushar Sugandhi wrote:
> IMA log is copied to the new Kernel during kexec 'load' using
> ima_dump_measurement_list().  The log copy at kexec 'load' may result in
> loss of IMA measurements during kexec soft reboot.  It needs to be copied
> over during kexec 'execute'.  Setup the needed infrastructure to move the
> IMA log copy from kexec 'load' to 'execute'.
> 
> Define a new IMA hook ima_update_kexec_buffer() as a stub function.
> It will be used to call ima_dump_measurement_list() during kexec
> 'execute'.
> 
> Implement kimage_file_post_load() and ima_kexec_post_load() functions
> to be invoked after the new Kernel image has been loaded for kexec.
> ima_kexec_post_load() maps the IMA buffer to a segment in the newly
> loaded Kernel.  It also registers the reboot notifier_block to trigger
> ima_update_kexec_buffer() at exec 'execute'.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>   include/linux/ima.h                |  3 ++
>   kernel/kexec_file.c                |  5 ++++
>   security/integrity/ima/ima_kexec.c | 46 ++++++++++++++++++++++++++++++
>   3 files changed, 54 insertions(+)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 86b57757c7b1..006db20f852d 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -49,6 +49,9 @@ static inline void ima_appraise_parse_cmdline(void) {}
>   
>   #ifdef CONFIG_IMA_KEXEC
>   extern void ima_add_kexec_buffer(struct kimage *image);
> +extern void ima_kexec_post_load(struct kimage *image);
> +#else
> +static inline void ima_kexec_post_load(struct kimage *image) {}
>   #endif
>   
>   #else
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 0e3689bfb0bb..fe59cb7c179d 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -186,6 +186,11 @@ kimage_validate_signature(struct kimage *image)
>   }
>   #endif
>   
> +void kimage_file_post_load(struct kimage *image)
> +{
> +	ima_kexec_post_load(image);
> +}
> +

We get this here at this point but it disappears later -- missing header?

kernel/kexec_file.c:189:6: warning: no previous prototype for 
‘kimage_file_post_load’ [-Wmissing-prototypes]
   189 | void kimage_file_post_load(struct kimage *image)
       |      ^~~~~~~~~~~~~~~~~~~~~


>   /*
>    * In file mode list of segments is prepared by kernel. Copy relevant
>    * data from user space, do error checking, prepare segment list
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index ccb072617c2d..1d4d6c122d82 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -12,10 +12,14 @@
>   #include <linux/kexec.h>
>   #include <linux/of.h>
>   #include <linux/ima.h>
> +#include <linux/reboot.h>
> +#include <asm/page.h>
>   #include "ima.h"
>   
>   #ifdef CONFIG_IMA_KEXEC
>   static struct seq_file ima_kexec_file;
> +static void *ima_kexec_buffer;
> +static bool ima_kexec_update_registered;
>   
>   static void ima_reset_kexec_file(struct seq_file *sf)
>   {
> @@ -184,6 +188,48 @@ void ima_add_kexec_buffer(struct kimage *image)
>   	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
>   		      kbuf.mem);
>   }
> +
> +/*
> + * Called during kexec execute so that IMA can update the measurement list.
> + */
> +static int ima_update_kexec_buffer(struct notifier_block *self,
> +				   unsigned long action, void *data)
> +{
> +	return NOTIFY_OK;
> +}
> +
> +struct notifier_block update_buffer_nb = {
> +	.notifier_call = ima_update_kexec_buffer,
> +};
> +
> +/*
> + * Create a mapping for the source pages that contain the IMA buffer
> + * so we can update it later.
> + */
> +void ima_kexec_post_load(struct kimage *image)
> +{
> +	if (ima_kexec_buffer) {
> +		kimage_unmap_segment(ima_kexec_buffer);
> +		ima_kexec_buffer = NULL;
> +	}
> +
> +	if (!image->ima_buffer_addr)
> +		return;
> +
> +	ima_kexec_buffer = kimage_map_segment(image,
> +					      image->ima_buffer_addr,
> +					      image->ima_buffer_size);
> +	if (!ima_kexec_buffer) {
> +		pr_err("%s: Could not map measurements buffer.\n", __func__);
> +		return;
> +	}
> +
> +	if (!ima_kexec_update_registered) {
> +		register_reboot_notifier(&update_buffer_nb);
> +		ima_kexec_update_registered = true;
> +	}
> +}
> +
>   #endif /* IMA_KEXEC */
>   
>   /*

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [PATCH v5 5/8] ima: kexec: move IMA log copy from kexec load to execute

[Stefan Berger]

On 2/14/24 10:38, Tushar Sugandhi wrote:
> ima_dump_measurement_list() is called during kexec 'load', which may
> result in loss of IMA measurements during kexec soft reboot.  It needs
> to be called during kexec 'execute'.
> 
> This patch includes the following changes:
>   - Call kimage_file_post_load() from kexec_file_load() syscall only for
>     kexec soft reboot scenarios and not for KEXEC_FILE_ON_CRASH.  It will
>     map the IMA segment, and register reboot notifier for the function
>     ima_update_kexec_buffer() which would copy the IMA log at kexec soft
>     reboot.
>   - Make kexec_segment_size variable local static to the file, for it to be
>     accessible both during kexec 'load' and 'execute'.
>   - Move ima_dump_measurement_list() call from ima_add_kexec_buffer()
>     to ima_update_kexec_buffer().
>   - Remove ima_reset_kexec_file() call from ima_add_kexec_buffer(), now
>     that the buffer is being copied at kexec 'execute', and resetting the
>     file at kexec 'load' will corrupt the buffer.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>   kernel/kexec_file.c                |  3 ++
>   security/integrity/ima/ima_kexec.c | 45 +++++++++++++++++++-----------
>   2 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index fe59cb7c179d..2d5df320c34f 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -410,6 +410,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>   
>   	kimage_terminate(image);
>   
> +	if (!(flags & KEXEC_FILE_ON_CRASH))
> +		kimage_file_post_load(image);
> +
>   	ret = machine_kexec_post_load(image);
>   	if (ret)
>   		goto out;
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 1d4d6c122d82..98fc9b9782a2 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -19,6 +19,7 @@
>   #ifdef CONFIG_IMA_KEXEC
>   static struct seq_file ima_kexec_file;
>   static void *ima_kexec_buffer;
> +static size_t kexec_segment_size;
>   static bool ima_kexec_update_registered;
>   
>   static void ima_reset_kexec_file(struct seq_file *sf)
> @@ -129,7 +130,6 @@ 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;
>   
>   	/*
> @@ -154,14 +154,6 @@ void ima_add_kexec_buffer(struct kimage *image)
>   		return;
>   	}
>   
> -	ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
> -					kexec_segment_size);
> -	if (ret < 0) {
> -		pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
> -		       __func__, ret);
> -		return;
> -	}
> -
>   	kbuf.buffer = kexec_buffer;
>   	kbuf.bufsz = kexec_buffer_size;
>   	kbuf.memsz = kexec_segment_size;
> @@ -179,12 +171,6 @@ void ima_add_kexec_buffer(struct kimage *image)
>   	image->ima_segment_index = image->nr_segments - 1;
>   	image->is_ima_segment_index_set = true;
>   
> -	/*
> -	 * kexec owns kexec_buffer after kexec_add_buffer() is called
> -	 * and it will vfree() that buffer.
> -	 */
> -	ima_reset_kexec_file(&ima_kexec_file);
> -
>   	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
>   		      kbuf.mem);
>   }
> @@ -195,7 +181,34 @@ void ima_add_kexec_buffer(struct kimage *image)
>   static int ima_update_kexec_buffer(struct notifier_block *self,
>   				   unsigned long action, void *data)
>   {
> -	return NOTIFY_OK;
> +	void *buf = NULL;
> +	size_t buf_size;
> +	int ret = NOTIFY_OK;
> +
> +	if (!kexec_in_progress) {
> +		pr_info("%s: No kexec in progress.\n", __func__);
> +		return ret;
> +	}
> +
> +	if (!ima_kexec_buffer) {
> +		pr_err("%s: Kexec buffer not set.\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = ima_dump_measurement_list(&buf_size, &buf,
> +					kexec_segment_size);
> +
> +	if (!buf) {

Use 'if (ret)' ? You're setting *buffer = NULL in case of error but ret 
should be used to test for failure.

> +		pr_err("%s: Dump measurements failed. Error:%d\n",
> +		       __func__, ret);
> +		goto out;
> +	}
> +	memcpy(ima_kexec_buffer, buf, buf_size);
> +out:
> +	kimage_unmap_segment(ima_kexec_buffer);
> +	ima_kexec_buffer = NULL;
> +
> +	return ret;
>   }
>   
>   struct notifier_block update_buffer_nb = {

Re: [PATCH v5 8/8] ima: measure kexec load and exec events as critical data

[Stefan Berger]

On 2/14/24 10:38, Tushar Sugandhi wrote:
> The amount of memory allocated at kexec load, even with the extra memory
> allocated, might not be large enough for the entire measurement list.  The
> indeterminate interval between kexec 'load' and 'execute' could exacerbate
> this problem.
> 
> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
> measured as critical data at kexec 'load' and 'execute' respectively.
> Report the allocated kexec segment size, IMA binary log size and the
> runtime measurements count as part of those events.
> 
> These events, and the values reported through them, serve as markers in
> the IMA log to verify the IMA events are captured during kexec soft
> reboot.  The presence of a 'kexec_load' event in between the last two
> 'boot_aggregate' events in the IMA log implies this is a kexec soft
> reboot, and not a cold-boot.  And the absence of 'kexec_execute' event
> after kexec soft reboot implies missing events in that window which
> results in inconsistency with TPM PCR quotes, necessitating a cold boot
> for a successful remote attestation.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>   security/integrity/ima/ima_kexec.c | 34 +++++++++++++++++++++++++++++-
>   1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 50903d4ce880..31495a043959 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -17,6 +17,8 @@
>   #include "ima.h"
>   
>   #ifdef CONFIG_IMA_KEXEC
> +#define IMA_KEXEC_EVENT_LEN 256
> +
>   static struct seq_file ima_kexec_file;
>   static void *ima_kexec_buffer;
>   static size_t kexec_segment_size;
> @@ -38,6 +40,10 @@ static void ima_free_kexec_file_buf(struct seq_file *sf)
>   
>   static int ima_alloc_kexec_file_buf(size_t segment_size)
>   {
> +	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
> +	size_t buf_size;
> +	long len;
> +
>   	/*
>   	 * kexec 'load' may be called multiple times.
>   	 * Free and realloc the buffer only if the segment_size is
> @@ -47,7 +53,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
>   	    ima_kexec_file.size == segment_size &&
>   	    ima_kexec_file.read_pos == 0 &&
>   	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
> -		return 0;
> +		goto out;
>   
>   	ima_free_kexec_file_buf(&ima_kexec_file);
>   
> @@ -60,6 +66,18 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
>   	ima_kexec_file.read_pos = 0;
>   	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
>   
> +out:
> +	buf_size = ima_get_binary_runtime_size();
> +	len = atomic_long_read(&ima_htable.len);
> +
> +	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
> +		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;"
> +		  "ima_runtime_measurements_count=%ld;",
> +		  segment_size, buf_size, len);
> +
> +	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
> +				  strlen(ima_kexec_event), false, NULL, 0);
> +
>   	return 0;
>   }
>   
> @@ -186,10 +204,12 @@ void ima_add_kexec_buffer(struct kimage *image)
>   static int ima_update_kexec_buffer(struct notifier_block *self,
>   				   unsigned long action, void *data)
>   {
> +	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
>   	void *buf = NULL;
>   	size_t buf_size;
>   	int ret = NOTIFY_OK;
>   	bool resume = false;
> +	long len;
>   
>   	if (!kexec_in_progress) {
>   		pr_info("%s: No kexec in progress.\n", __func__);
> @@ -201,6 +221,18 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>   		return ret;
>   	}
>   
> +	buf_size = ima_get_binary_runtime_size();
> +	len = atomic_long_read(&ima_htable.len);
> +
> +	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
> +		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;"
> +		  "ima_runtime_measurements_count=%ld;",
> +		  kexec_segment_size, buf_size, len);
> +
> +	ima_measure_critical_data("ima_kexec", "kexec_execute",
> +				  ima_kexec_event, strlen(ima_kexec_event),
> +				  false, NULL, 0);
> +
>   	ima_measurements_suspend();
>   
>   	ret = ima_dump_measurement_list(&buf_size, &buf,

It's twice the same code almost in the same file. You could move it into 
a function.

Re: [PATCH v5 8/8] ima: measure kexec load and exec events as critical data

[Stefan Berger]

On 2/14/24 10:38, Tushar Sugandhi wrote:
> The amount of memory allocated at kexec load, even with the extra memory
> allocated, might not be large enough for the entire measurement list.  The
> indeterminate interval between kexec 'load' and 'execute' could exacerbate
> this problem.
> 
> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
> measured as critical data at kexec 'load' and 'execute' respectively.
> Report the allocated kexec segment size, IMA binary log size and the
> runtime measurements count as part of those events.
> 
> These events, and the values reported through them, serve as markers in
> the IMA log to verify the IMA events are captured during kexec soft
> reboot.  The presence of a 'kexec_load' event in between the last two
> 'boot_aggregate' events in the IMA log implies this is a kexec soft
> reboot, and not a cold-boot.  And the absence of 'kexec_execute' event
> after kexec soft reboot implies missing events in that window which
> results in inconsistency with TPM PCR quotes, necessitating a cold boot
> for a successful remote attestation.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

I successfully tested this series on ppc64. I will give you a Tested-by 
tag in the next round.

    Stefan

Re: [PATCH v5 2/8] kexec: define functions to map and unmap segments

[Tushar Sugandhi]

On 2/14/24 11:43, Stefan Berger wrote:
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Thanks for the tag Stefan.

Re: [PATCH v5 4/8] ima: kexec: define functions to copy IMA log at soft boot

[Tushar Sugandhi]

On 2/14/24 12:47, Stefan Berger wrote:
> 
> 
> On 2/14/24 10:38, Tushar Sugandhi wrote:
...
<snip/>
...
>> +void kimage_file_post_load(struct kimage *image)
>> +{
>> +    ima_kexec_post_load(image);
>> +}
>> +
> 
> We get this here at this point but it disappears later -- missing header?
> 
> kernel/kexec_file.c:189:6: warning: no previous prototype for 
> ‘kimage_file_post_load’ [-Wmissing-prototypes]
>    189 | void kimage_file_post_load(struct kimage *image)
>        |      ^~~~~~~~~~~~~~~~~~~~~
> 
> 
Thanks Stefan.
I was also getting it.
But couldn't figure out why. And I was puzzled why it was going away.

Since kimage_file_post_load() is called from the same file in patch 5/8,
I don't see a need of declaring it in a header file like 
include/linux/kexec.h.

Making kimage_file_post_load() local static resolves the warning.
But then it throws "defined but not used" warning. I will have to call 
it from kexec_file_load syscall in this patch (4/8) instead 5/8 to 
resolve that warning.

I will make the function a stub function in this patch and
make it call ima_kexec_post_load(image) in the next patch to avoid any 
potential bisect safe issues.

It aligns with the goals of patch 4/8 and 5/8 anyways.

+static void kimage_file_post_load(struct kimage *image)
+{
+	/*
+	 * this will call ima_kexec_post_load(image) to map the segment
+	 * and register the reboot notifier for moving the IMA log at
+	 * kexec execute
+	 */
+}
+


@@ -410,6 +410,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, 
int, initrd_fd,
         kimage_terminate(image);
+    if (!(flags & KEXEC_FILE_ON_CRASH))
+        kimage_file_post_load(image);
+

...
...<snip/>
...

> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Thanks for the tag. I will apply it on the next version.

~Tushar

Re: [PATCH v5 8/8] ima: measure kexec load and exec events as critical data

[Tushar Sugandhi]

On 2/14/24 13:00, Stefan Berger wrote:
> 
> 
> On 2/14/24 10:38, Tushar Sugandhi wrote:
>> The amount of memory allocated at kexec load, even with the extra memory
>> allocated, might not be large enough for the entire measurement list.  
>> The
>> indeterminate interval between kexec 'load' and 'execute' could 
>> exacerbate
>> this problem.
>>
>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
>> measured as critical data at kexec 'load' and 'execute' respectively.
>> Report the allocated kexec segment size, IMA binary log size and the
>> runtime measurements count as part of those events.
>>
>> These events, and the values reported through them, serve as markers in
>> the IMA log to verify the IMA events are captured during kexec soft
>> reboot.  The presence of a 'kexec_load' event in between the last two
>> 'boot_aggregate' events in the IMA log implies this is a kexec soft
>> reboot, and not a cold-boot.  And the absence of 'kexec_execute' event
>> after kexec soft reboot implies missing events in that window which
>> results in inconsistency with TPM PCR quotes, necessitating a cold boot
>> for a successful remote attestation.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima_kexec.c | 34 +++++++++++++++++++++++++++++-
>>   1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 50903d4ce880..31495a043959 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -17,6 +17,8 @@
>>   #include "ima.h"
>>   #ifdef CONFIG_IMA_KEXEC
>> +#define IMA_KEXEC_EVENT_LEN 256
>> +
>>   static struct seq_file ima_kexec_file;
>>   static void *ima_kexec_buffer;
>>   static size_t kexec_segment_size;
>> @@ -38,6 +40,10 @@ static void ima_free_kexec_file_buf(struct seq_file 
>> *sf)
>>   static int ima_alloc_kexec_file_buf(size_t segment_size)
>>   {
>> +    char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
>> +    size_t buf_size;
>> +    long len;
>> +
>>       /*
>>        * kexec 'load' may be called multiple times.
>>        * Free and realloc the buffer only if the segment_size is
>> @@ -47,7 +53,7 @@ static int ima_alloc_kexec_file_buf(size_t 
>> segment_size)
>>           ima_kexec_file.size == segment_size &&
>>           ima_kexec_file.read_pos == 0 &&
>>           ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
>> -        return 0;
>> +        goto out;
>>       ima_free_kexec_file_buf(&ima_kexec_file);
>> @@ -60,6 +66,18 @@ static int ima_alloc_kexec_file_buf(size_t 
>> segment_size)
>>       ima_kexec_file.read_pos = 0;
>>       ima_kexec_file.count = sizeof(struct ima_kexec_hdr);    /* 
>> reserved space */
>> +out:
>> +    buf_size = ima_get_binary_runtime_size();
>> +    len = atomic_long_read(&ima_htable.len);
>> +
>> +    scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
>> +          "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;"
>> +          "ima_runtime_measurements_count=%ld;",
>> +          segment_size, buf_size, len);
>> +
>> +    ima_measure_critical_data("ima_kexec", "kexec_load", 
>> ima_kexec_event,
>> +                  strlen(ima_kexec_event), false, NULL, 0);
>> +
>>       return 0;
>>   }
>> @@ -186,10 +204,12 @@ void ima_add_kexec_buffer(struct kimage *image)
>>   static int ima_update_kexec_buffer(struct notifier_block *self,
>>                      unsigned long action, void *data)
>>   {
>> +    char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
>>       void *buf = NULL;
>>       size_t buf_size;
>>       int ret = NOTIFY_OK;
>>       bool resume = false;
>> +    long len;
>>       if (!kexec_in_progress) {
>>           pr_info("%s: No kexec in progress.\n", __func__);
>> @@ -201,6 +221,18 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>           return ret;
>>       }
>> +    buf_size = ima_get_binary_runtime_size();
>> +    len = atomic_long_read(&ima_htable.len);
>> +
>> +    scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
>> +          "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;"
>> +          "ima_runtime_measurements_count=%ld;",
>> +          kexec_segment_size, buf_size, len);
>> +
>> +    ima_measure_critical_data("ima_kexec", "kexec_execute",
>> +                  ima_kexec_event, strlen(ima_kexec_event),
>> +                  false, NULL, 0);
>> +
>>       ima_measurements_suspend();
>>       ret = ima_dump_measurement_list(&buf_size, &buf,
> 
> It's twice the same code almost in the same file. You could move it into 
> a function.
Fair enough. Will do.

~Tushar

Re: [PATCH v5 8/8] ima: measure kexec load and exec events as critical data

[Tushar Sugandhi]

On 2/14/24 13:03, Stefan Berger wrote:
> 
> 
> On 2/14/24 10:38, Tushar Sugandhi wrote:
>> The amount of memory allocated at kexec load, even with the extra memory
>> allocated, might not be large enough for the entire measurement list.  
>> The
>> indeterminate interval between kexec 'load' and 'execute' could 
>> exacerbate
>> this problem.
>>
>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
>> measured as critical data at kexec 'load' and 'execute' respectively.
>> Report the allocated kexec segment size, IMA binary log size and the
>> runtime measurements count as part of those events.
>>
>> These events, and the values reported through them, serve as markers in
>> the IMA log to verify the IMA events are captured during kexec soft
>> reboot.  The presence of a 'kexec_load' event in between the last two
>> 'boot_aggregate' events in the IMA log implies this is a kexec soft
>> reboot, and not a cold-boot.  And the absence of 'kexec_execute' event
>> after kexec soft reboot implies missing events in that window which
>> results in inconsistency with TPM PCR quotes, necessitating a cold boot
>> for a successful remote attestation.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> I successfully tested this series on ppc64. I will give you a Tested-by 
> tag in the next round.
> 
>     Stefan
Awesome! Appreciate it.

~Tushar

Re: [PATCH v5 1/8] ima: define and call ima_alloc_kexec_file_buf

[Stefan Berger]

On 2/14/24 10:38, Tushar Sugandhi wrote:
> Carrying the IMA measurement list across kexec requires allocating a
> buffer and copying the measurement records.  Separate allocating the
> buffer and copying the measurement records into separate functions in
> order to allocate the buffer at kexec 'load' and copy the measurements
> at kexec 'execute'.
> 
> This patch includes the following changes:
>   - Refactor ima_dump_measurement_list() to move the memory allocation
>     to a separate function ima_alloc_kexec_file_buf() which allocates
>     buffer of size 'kexec_segment_size' at kexec 'load'.
>   - Make the local variable ima_kexec_file in ima_dump_measurement_list()
>     as local static to the file, so that it can be accessed from

as -> a

>     ima_alloc_kexec_file_buf().
>   - Make necessary changes to the function ima_add_kexec_buffer() to call
>     the above two functions.
> 
> Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>   security/integrity/ima/ima_kexec.c | 107 +++++++++++++++++++++--------
>   1 file changed, 78 insertions(+), 29 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index dadc1d138118..a9cb5e882e2e 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -15,62 +15,98 @@
>   #include "ima.h"
>   
>   #ifdef CONFIG_IMA_KEXEC
> +static struct seq_file ima_kexec_file;
> +
> +static void ima_reset_kexec_file(struct seq_file *sf)
> +{
> +	sf->buf = NULL;
> +	sf->size = 0;
> +	sf->read_pos = 0;
> +	sf->count = 0;
> +}
> +
> +static void ima_free_kexec_file_buf(struct seq_file *sf)
> +{
> +	vfree(sf->buf);
> +	ima_reset_kexec_file(sf);
> +}
> +
> +static int ima_alloc_kexec_file_buf(size_t segment_size)
> +{
> +	/*
> +	 * kexec 'load' may be called multiple times.
> +	 * Free and realloc the buffer only if the segment_size is
> +	 * changed from the previous kexec 'load' call.
> +	 */
> +	if (ima_kexec_file.buf &&
> +	    ima_kexec_file.size == segment_size &&
> +	    ima_kexec_file.read_pos == 0 &&
> +	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
> +		return 0;
> +
> +	ima_free_kexec_file_buf(&ima_kexec_file);
> +
> +	/* segment size can't change between kexec load and execute */
> +	ima_kexec_file.buf = vmalloc(segment_size);
> +	if (!ima_kexec_file.buf)
> +		return -ENOMEM;
> +
> +	ima_kexec_file.size = segment_size;
> +	ima_kexec_file.read_pos = 0;
> +	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
> +
> +	return 0;
> +}
> +
>   static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
>   				     unsigned long segment_size)
>   {
>   	struct ima_queue_entry *qe;
> -	struct seq_file file;
>   	struct ima_kexec_hdr khdr;
> -	int ret = 0;
>   
> -	/* segment size can't change between kexec load and execute */
> -	file.buf = vmalloc(segment_size);
> -	if (!file.buf) {
> -		ret = -ENOMEM;
> -		goto out;
> +	if (!ima_kexec_file.buf) {
> +		*buffer_size = 0;
> +		*buffer = NULL;

If you return -EINVAL then the caller should not look at the buffer but 
at 'ret' (5/8). So I don't think it's necessary to initialize these 
variables since the caller shouldn't look at them.

> +		pr_err("%s: Kexec file buf not allocated\n", __func__);
> +		return -EINVAL;
>   	}
>   
> -	file.size = segment_size;
> -	file.read_pos = 0;
> -	file.count = sizeof(khdr);	/* reserved space */
> -
>   	memset(&khdr, 0, sizeof(khdr));
>   	khdr.version = 1;
> +
> +	/* Copy as many IMA measurements list records as possible */
>   	list_for_each_entry_rcu(qe, &ima_measurements, later) {
> -		if (file.count < file.size) {
> +		if (ima_kexec_file.count < ima_kexec_file.size) {
>   			khdr.count++;
> -			ima_measurements_show(&file, qe);
> +			ima_measurements_show(&ima_kexec_file, qe);
>   		} else {
> -			ret = -EINVAL;
> +			pr_err("%s: IMA log file is too big for Kexec buf\n",
> +			       __func__);
>   			break;
>   		}
>   	}
>   
> -	if (ret < 0)
> -		goto out;
> -
>   	/*
>   	 * fill in reserved space with some buffer details
>   	 * (eg. version, buffer size, number of measurements)
>   	 */
> -	khdr.buffer_size = file.count;
> +	khdr.buffer_size = ima_kexec_file.count;
>   	if (ima_canonical_fmt) {
>   		khdr.version = cpu_to_le16(khdr.version);
>   		khdr.count = cpu_to_le64(khdr.count);
>   		khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
>   	}
> -	memcpy(file.buf, &khdr, sizeof(khdr));
> +	memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr));
>   
>   	print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
> -			     file.buf, file.count < 100 ? file.count : 100,
> +			     ima_kexec_file.buf, ima_kexec_file.count < 100 ?
> +			     ima_kexec_file.count : 100,
>   			     true);
>   
> -	*buffer_size = file.count;
> -	*buffer = file.buf;
> -out:
> -	if (ret == -EINVAL)
> -		vfree(file.buf);
> -	return ret;
> +	*buffer_size = ima_kexec_file.count;
> +	*buffer = ima_kexec_file.buf;
> +
> +	return 0;
>   }
>   
>   /*
> @@ -108,13 +144,20 @@ void ima_add_kexec_buffer(struct kimage *image)
>   		return;
>   	}
>   
> -	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
> -				  kexec_segment_size);
> -	if (!kexec_buffer) {
> +	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
> +	if (ret < 0) {
>   		pr_err("Not enough memory for the kexec measurement buffer.\n");
>   		return;
>   	}
>   
> +	ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
> +					kexec_segment_size);
> +	if (ret < 0) {
> +		pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
> +		       __func__, ret);
> +		return;
> +	}
> +
>   	kbuf.buffer = kexec_buffer;
>   	kbuf.bufsz = kexec_buffer_size;
>   	kbuf.memsz = kexec_segment_size;
> @@ -129,6 +172,12 @@ void ima_add_kexec_buffer(struct kimage *image)
>   	image->ima_buffer_size = kexec_segment_size;
>   	image->ima_buffer = kexec_buffer;
>   
> +	/*
> +	 * kexec owns kexec_buffer after kexec_add_buffer() is called
> +	 * and it will vfree() that buffer.
> +	 */

Actually, that's not quite right what I told you.   "kexec owns 
kexec_buffer due to ima->ima_buffer and it will vfree() this buffer."


> +	ima_reset_kexec_file(&ima_kexec_file);
> +
>   	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
>   		      kbuf.mem);
>   }

Re: [PATCH v5 1/8] ima: define and call ima_alloc_kexec_file_buf

[Mimi Zohar]

On Wed, 2024-02-14 at 07:38 -0800, Tushar Sugandhi wrote:
> Carrying the IMA measurement list across kexec requires allocating a
> buffer and copying the measurement records.  Separate allocating the
> buffer and copying the measurement records into separate functions in
> order to allocate the buffer at kexec 'load' and copy the measurements
> at kexec 'execute'.
> 
> This patch includes the following changes:
>  - Refactor ima_dump_measurement_list() to move the memory allocation
>    to a separate function ima_alloc_kexec_file_buf() which allocates
>    buffer of size 'kexec_segment_size' at kexec 'load'.
>  - Make the local variable ima_kexec_file in ima_dump_measurement_list()
>    as local static to the file, so that it can be accessed from 
>    ima_alloc_kexec_file_buf().
>  - Make necessary changes to the function ima_add_kexec_buffer() to call
>    the above two functions.
> 
> Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  security/integrity/ima/ima_kexec.c | 107 +++++++++++++++++++++--------
>  1 file changed, 78 insertions(+), 29 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_kexec.c
> b/security/integrity/ima/ima_kexec.c
> index dadc1d138118..a9cb5e882e2e 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -15,62 +15,98 @@
>  #include "ima.h"
>  
>  #ifdef CONFIG_IMA_KEXEC
> +static struct seq_file ima_kexec_file;
> +
> +static void ima_reset_kexec_file(struct seq_file *sf)
> +{
> +	sf->buf = NULL;
> +	sf->size = 0;
> +	sf->read_pos = 0;
> +	sf->count = 0;
> +}
> +
> +static void ima_free_kexec_file_buf(struct seq_file *sf)
> +{
> +	vfree(sf->buf);
> +	ima_reset_kexec_file(sf);
> +}
> +
> +static int ima_alloc_kexec_file_buf(size_t segment_size)
> +{
> +	/*
> +	 * kexec 'load' may be called multiple times.
> +	 * Free and realloc the buffer only if the segment_size is
> +	 * changed from the previous kexec 'load' call.
> +	 */
> +	if (ima_kexec_file.buf &&
> +	    ima_kexec_file.size == segment_size &&
> +	    ima_kexec_file.read_pos == 0 &&
> +	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
> +		return 0;

ima_kexec_file.size will be initialized to zero.  No need to test anything other
than the segment size.

> +
> +	ima_free_kexec_file_buf(&ima_kexec_file);

The first time ima_free_kexec_file_buf() is called the memory has not been
allocated.  Test that the memory has been allocated before freeing it.  Since
there's no other ima_free_kexec_file_buf() caller, there's no need to clear the
other fields as they will be set below.

> +
> +	/* segment size can't change between kexec load and execute */
> +	ima_kexec_file.buf = vmalloc(segment_size);
> +	if (!ima_kexec_file.buf)
> +		return -ENOMEM;
> +
> +	ima_kexec_file.size = segment_size;
> +	ima_kexec_file.read_pos = 0;

static variables are initialized to zero.  No need to set it here.

> +	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved
> space */
> +
> +	return 0;
> +}
> +
>  static int ima_dump_measurement_list(unsigned long *buffer_size, void
> **buffer,
>  				     unsigned long segment_size)
>  {
>  	struct ima_queue_entry *qe;
> -	struct seq_file file;
>  	struct ima_kexec_hdr khdr;
> -	int ret = 0;
>  
> -	/* segment size can't change between kexec load and execute */
> -	file.buf = vmalloc(segment_size);
> -	if (!file.buf) {
> -		ret = -ENOMEM;
> -		goto out;
> +	if (!ima_kexec_file.buf) {
> +		*buffer_size = 0;
> +		*buffer = NULL;
> +		pr_err("%s: Kexec file buf not allocated\n", __func__);
> +		return -EINVAL;
>  	}
>  
> -	file.size = segment_size;
> -	file.read_pos = 0;
> -	file.count = sizeof(khdr);	/* reserved space */
> -
>  	memset(&khdr, 0, sizeof(khdr));
>  	khdr.version = 1;
> +
> +	/* Copy as many IMA measurements list records as possible */
>  	list_for_each_entry_rcu(qe, &ima_measurements, later) {
> -		if (file.count < file.size) {
> +		if (ima_kexec_file.count < ima_kexec_file.size) {

With an extra half page memory allocated, there should be enough memory for a
few extra records as a result of the kexec load.  Subsequent patches are going
to change this assumption.  This test doesn't ensure there is enough memory for
the entire measurement record.  Call get_binary_runtime_size() to get the record
size to ensure there is enough memory for the entire record.

>  			khdr.count++;
> -			ima_measurements_show(&file, qe);
> +			ima_measurements_show(&ima_kexec_file, qe);
>  		} else {
> -			ret = -EINVAL;
> +			pr_err("%s: IMA log file is too big for Kexec buf\n",
> +			       __func__);

This message and the one above should probably be pr_debug().  There's a new
error message after the call to ima_dump_measurement_list().

>  			break;
>  		}
>  	}
>  
> -	if (ret < 0)
> -		goto out;
> -

Removing these lines is in for "copying as many measurements as possible". That
change isn't mentioned in the patch description.

>  	/*
>  	 * fill in reserved space with some buffer details
>  	 * (eg. version, buffer size, number of measurements)
>  	 */
> -	khdr.buffer_size = file.count;
> +	khdr.buffer_size = ima_kexec_file.count;
>  	if (ima_canonical_fmt) {
>  		khdr.version = cpu_to_le16(khdr.version);
>  		khdr.count = cpu_to_le64(khdr.count);
>  		khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
>  	}
> -	memcpy(file.buf, &khdr, sizeof(khdr));
> +	memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr));
>  
>  	print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
> -			     file.buf, file.count < 100 ? file.count : 100,
> +			     ima_kexec_file.buf, ima_kexec_file.count < 100 ?
> +			     ima_kexec_file.count : 100,
>  			     true);
>  
> -	*buffer_size = file.count;
> -	*buffer = file.buf;
> -out:
> -	if (ret == -EINVAL)
> -		vfree(file.buf);
> -	return ret;
> +	*buffer_size = ima_kexec_file.count;
> +	*buffer = ima_kexec_file.buf;
> +
> +	return 0;
>  }
>  
>  /*
> @@ -108,13 +144,20 @@ void ima_add_kexec_buffer(struct kimage *image)
>  		return;
>  	}
>  
> -	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
> -				  kexec_segment_size);
> -	if (!kexec_buffer) {
> +	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
> +	if (ret < 0) {
>  		pr_err("Not enough memory for the kexec measurement buffer.\n");
>  		return;
>  	}
>  
> +	ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
> +					kexec_segment_size);
> +	if (ret < 0) {
> +		pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
> +		       __func__, ret);

Please remove "__func__" from the error message.

> +		return;
> +	}
> +
>  	kbuf.buffer = kexec_buffer;
>  	kbuf.bufsz = kexec_buffer_size;
>  	kbuf.memsz = kexec_segment_size;
> @@ -129,6 +172,12 @@ void ima_add_kexec_buffer(struct kimage *image)
>  	image->ima_buffer_size = kexec_segment_size;
>  	image->ima_buffer = kexec_buffer;
>  
> +	/*
> +	 * kexec owns kexec_buffer after kexec_add_buffer() is called
> +	 * and it will vfree() that buffer.
> +	 */
> +	ima_reset_kexec_file(&ima_kexec_file);
> +
>  	kexec_dprintk("kexec measurement buffer for the loaded kernel at
> 0x%lx.\n",
>  		      kbuf.mem);
>  }

thanks,

Mimi

Re: [PATCH v5 0/8] ima: kexec: measure events between kexec load and execute

[Mimi Zohar]

On Wed, 2024-02-14 at 07:38 -0800, Tushar Sugandhi wrote:
> The current Kernel behavior is IMA measurements snapshot is taken at
> kexec 'load' and not at kexec 'execute'.  IMA log is then carried
> over to the new Kernel after kexec 'execute'.

'Kernel' should not be capitalized since it isn't a proper name.  'Linux' would
be capitalized (e.g. The Linux kernel).

-> "The IMA measurement list is copied at kexec 'load', not kexec 'execute',
before being carried over to the new kexec'ed kernel.

Mimi

> 
> New events can be measured during/after the IMA log snapshot at kexec 
> 'load' and before the system boots to the new Kernel.  In this scenario,
> the TPM PCRs are extended with these events, but they are not carried
> over to the new Kernel after kexec soft reboot since the snapshot is
> already taken.  This results in mismatch between TPM PCR quotes and the
> actual IMA measurements list after kexec soft reboot, which in turn
> results in remote attestation failure.
> 
> To solve this problem - 
>  - allocate the necessary buffer at kexec 'load' time,
>  - populate the buffer with the IMA measurements at kexec 'execute' time, 
>  - and measure two new IMA events 'kexec_load' and 'kexec_execute' as
>    critical data to help detect missing events after kexec soft reboot.
> 
> The solution details include:
>  - refactoring the existing code to allocate a buffer to hold IMA
>    measurements at kexec 'load', and dump the measurements at kexec
>    'execute'
> 
>  - IMA functionality to suspend and resume measurements as needed during
>    buffer copy at kexec 'execute',
> 
>  - kexec functionality for mapping the segments from the current Kernel
>    to the subsequent one, 
> 
>  - necessary changes to the kexec_file_load syscall, enabling it to call
>    the ima functions,
> 
>  - registering a reboot notifier which gets called during kexec 
>    'execute',
> 
>  - introducing a new Kconfig option to configure the extra memory to be
>    allocated for passing IMA log from the current Kernel to the next,
>    
>  - introducing two new events to be measured by IMA during kexec, to
>    help diagnose if the IMA log was copied fully or partially, from the
>    current Kernel to the next,
> 
>  - excluding IMA segment while calculating and storing digest in function
>    kexec_calculate_store_digests(), since IMA segment can be modified
>    after the digest is computed during kexec 'load'.  This will ensure
>    that the segment is not added to the 'purgatory_sha_regions', and thus
>    not verified by verify_sha256_digest().
> 
> The changes proposed in this series ensure the integrity of the IMA
> measurements is preserved across kexec soft reboots, thus significantly
> improving the security of the Kernel post kexec soft reboots.
> 
> There were previous attempts to fix this issue [1], [2], [3].  But they
> were not merged into the mainline Kernel.
> 
> We took inspiration from the past work [1] and [2] while working on this
> patch series.
> 
> V4 of this series is available here[6] for reference.
> 

Re: [PATCH v5 2/8] kexec: define functions to map and unmap segments

[Mimi Zohar]

On Wed, 2024-02-14 at 07:38 -0800, Tushar Sugandhi wrote:
> Currently, the mechanism to map and unmap segments to the kimage
> structure is not available to the subsystems outside of kexec.  This
> functionality is needed when IMA is allocating the memory segments
> during kexec 'load' operation.  Implement functions to map and unmap
> segments to kimage.
> 
> Implement kimage_map_segment() to enable mapping of IMA buffer source
> pages to the kimage structure post kexec 'load'.  This function,
> accepting a kimage pointer, an address, and a size, will gather the
> source pages within the specified address range, create an array of page
> pointers, and map these to a contiguous virtual address range.  The
> function returns the start of this range if successful, or NULL if
> unsuccessful.
> 
> Implement kimage_unmap_segment() for unmapping segments
> using vunmap().  Relocate 'for_each_kimage_entry()' macro from
> kexec_core.c to kexec.h for broader accessibility.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  include/linux/kexec.h | 13 ++++++++++
>  kernel/kexec_core.c   | 59 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 67 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 400cb6c02176..3145447eb77a 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -486,6 +486,11 @@ static inline void arch_kexec_pre_free_pages(void *vaddr,
> unsigned int pages) {
>  static inline void arch_crash_handle_hotplug_event(struct kimage *image) { }
>  #endif
>  
> +#define for_each_kimage_entry(image, ptr, entry) \
> +	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
> +		ptr = (entry & IND_INDIRECTION) ? \
> +			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
> +

I don't see a reason for moving this macro.

Mimi

Re: [PATCH v5 4/8] ima: kexec: define functions to copy IMA log at soft boot

[Mimi Zohar]

On Wed, 2024-02-14 at 22:55 -0800, Tushar Sugandhi wrote:
> 
> On 2/14/24 12:47, Stefan Berger wrote:
> > 
> > On 2/14/24 10:38, Tushar Sugandhi wrote:
> ...
> <snip/>
> ...
> > > +void kimage_file_post_load(struct kimage *image)
> > > +{
> > > +    ima_kexec_post_load(image);
> > > +}
> > > +
> > 
> > We get this here at this point but it disappears later -- missing header?
> > 
> > kernel/kexec_file.c:189:6: warning: no previous prototype for 
> > ‘kimage_file_post_load’ [-Wmissing-prototypes]
> >    189 | void kimage_file_post_load(struct kimage *image)
> >        |      ^~~~~~~~~~~~~~~~~~~~~
> > 
> > 
> Thanks Stefan.
> I was also getting it. 
> But couldn't figure out why. And I was puzzled why it was going away.
> 
> Since kimage_file_post_load() is called from the same file in patch 5/8,
> I don't see a need of declaring it in a header file like 
> include/linux/kexec.h.

Before trying to "fix" it, is there a need for a wrapper around
ima_kexec_post_load().  ima_add_kexec_buffer() is called directly.

Mimi

> 
> Making kimage_file_post_load() local static resolves the warning.
> But then it throws "defined but not used" warning. I will have to call 
> it from kexec_file_load syscall in this patch (4/8) instead 5/8 to 
> resolve that warning.
> 
> I will make the function a stub function in this patch and
> make it call ima_kexec_post_load(image) in the next patch to avoid any 
> potential bisect safe issues.
> 
> It aligns with the goals of patch 4/8 and 5/8 anyways.
> 
> +static void kimage_file_post_load(struct kimage *image)
> +{
> +	/*
> +	 * this will call ima_kexec_post_load(image) to map the segment
> +	 * and register the reboot notifier for moving the IMA log at
> +	 * kexec execute
> +	 */
> +}
> +
> 
> 
> @@ -410,6 +410,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, 
> int, initrd_fd,
>          kimage_terminate(image);
> +    if (!(flags & KEXEC_FILE_ON_CRASH))
> +        kimage_file_post_load(image);
> +
> 
> ...
> ...<snip/>
> ...
> 
> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Thanks for the tag. I will apply it on the next version.
> 
> ~Tushar

Re: [PATCH v5 4/8] ima: kexec: define functions to copy IMA log at soft boot

[Mimi Zohar]

Additional comments ...

> diff --git a/security/integrity/ima/ima_kexec.c
> b/security/integrity/ima/ima_kexec.c
> index ccb072617c2d..1d4d6c122d82 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -12,10 +12,14 @@
>  #include <linux/kexec.h>
>  #include <linux/of.h>
>  #include <linux/ima.h>
> +#include <linux/reboot.h>
> +#include <asm/page.h>
>  #include "ima.h"
>  
>  #ifdef CONFIG_IMA_KEXEC
>  static struct seq_file ima_kexec_file;
> +static void *ima_kexec_buffer;
> +static bool ima_kexec_update_registered;
>  
>  static void ima_reset_kexec_file(struct seq_file *sf)
>  {
> @@ -184,6 +188,48 @@ void ima_add_kexec_buffer(struct kimage *image)
>  	kexec_dprintk("kexec measurement buffer for the loaded kernel at
> 0x%lx.\n",
>  		      kbuf.mem);
>  }
> +
> +/*
> + * Called during kexec execute so that IMA can update the measurement list.
> + */
> +static int ima_update_kexec_buffer(struct notifier_block *self,
> +				   unsigned long action, void *data)
> +{
> +	return NOTIFY_OK;
> +}
> +
> +struct notifier_block update_buffer_nb = {

This should be defined as static.  update_buffer_nb should be prefixed with
'ima_'.

> +	.notifier_call = ima_update_kexec_buffer,
> +};
> +
> +/*
> + * Create a mapping for the source pages that contain the IMA buffer
> + * so we can update it later.
> + */
> +void ima_kexec_post_load(struct kimage *image)
> +{

In ima_alloc_kexec_file_buf(), the buffer is only re-allocated if the size
changes.  Here there doesn't seem to be way of detecting a size change.  At
least, add a comment here indicating that kexec 'load' may be called multiple
times.

Mimi

> +	if (ima_kexec_buffer) {
> +		kimage_unmap_segment(ima_kexec_buffer);
> +		ima_kexec_buffer = NULL;
> +	}
> +
> +	if (!image->ima_buffer_addr)
> +		return;
> +
> +	ima_kexec_buffer = kimage_map_segment(image,
> +					      image->ima_buffer_addr,
> +					      image->ima_buffer_size);
> +	if (!ima_kexec_buffer) {
> +		pr_err("%s: Could not map measurements buffer.\n", __func__);
> +		return;
> +	}
> +
> +	if (!ima_kexec_update_registered) {
> +		register_reboot_notifier(&update_buffer_nb);
> +		ima_kexec_update_registered = true;
> +	}
> +}
> +
>  #endif /* IMA_KEXEC */
>  
>  /*

Re: [PATCH v5 5/8] ima: kexec: move IMA log copy from kexec load to execute

[Mimi Zohar]

> @@ -195,7 +181,34 @@ void ima_add_kexec_buffer(struct kimage *image)
>  static int ima_update_kexec_buffer(struct notifier_block *self,
>  				   unsigned long action, void *data)
>  {
> -	return NOTIFY_OK;
> +	void *buf = NULL;
> +	size_t buf_size;
> +	int ret = NOTIFY_OK;
> +
> +	if (!kexec_in_progress) {
> +		pr_info("%s: No kexec in progress.\n", __func__);
> +		return ret;
> +	}
> +
> +	if (!ima_kexec_buffer) {
> +		pr_err("%s: Kexec buffer not set.\n", __func__);
> +		return ret;
> +	}

pr_ messages should already be prefixed with the module name.  There shouldn't
be a need to include the function name as well.

> +	ret = ima_dump_measurement_list(&buf_size, &buf,
> +					kexec_segment_size);
> +
> +	if (!buf) {
> +		pr_err("%s: Dump measurements failed. Error:%d\n",
> +		       __func__, ret);
> +		goto out;
> +	}
> +	memcpy(ima_kexec_buffer, buf, buf_size);
> +out:
> +	kimage_unmap_segment(ima_kexec_buffer);
> +	ima_kexec_buffer = NULL;
> +
> +	return ret;
>  }
>  
>  struct notifier_block update_buffer_nb = {

Re: [PATCH v5 7/8] ima: make the kexec extra memory configurable

[Mimi Zohar]

Hi Tushar,

On Wed, 2024-02-14 at 07:38 -0800, Tushar Sugandhi wrote:
> The extra memory allocated for carrying the IMA measurement list across
> kexec is hard-coded as half a PAGE.  Make it configurable.
> 
> Define a Kconfig option, IMA_KEXEC_EXTRA_MEMORY_KB, to configure the
> extra memory (in kb) to be allocated for IMA measurements added during
> kexec soft reboot.  Ensure the default value of the option is set such
> that extra half a page of memory for additional measurements is allocated
> for the additional measurements.
> 
> Update ima_add_kexec_buffer() function to allocate memory based on the
> Kconfig option value, rather than the currently hard-coded one.
> 
> Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  security/integrity/ima/Kconfig     |  9 +++++++++
>  security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index b98bfe9efd0c..4c0fc53d6aa3 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -320,4 +320,13 @@ config IMA_DISABLE_HTABLE
>  	help
>  	   This option disables htable to allow measurement of duplicate
> records.
>  
> +config IMA_KEXEC_EXTRA_MEMORY_KB
> +	int
> +	depends on IMA_KEXEC
> +	default 0
> +	help
> +	  IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be
> +	  allocated (in kb) for IMA measurements added during kexec soft reboot.
> +	  If set to the default value, an extra half a page of memory for those
> +	  additional measurements will be allocated.
>  endif
> diff --git a/security/integrity/ima/ima_kexec.c
> b/security/integrity/ima/ima_kexec.c
> index dbeeb7f1355e..50903d4ce880 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -126,6 +126,7 @@ void ima_add_kexec_buffer(struct kimage *image)
>  				  .buf_min = 0, .buf_max = ULONG_MAX,
>  				  .top_down = true };
>  	unsigned long binary_runtime_size;
> +	unsigned long extra_size;
>  
>  	/* use more understandable variable names than defined in kbuf */
>  	void *kexec_buffer = NULL;
> @@ -133,15 +134,19 @@ void ima_add_kexec_buffer(struct kimage *image)
>  	int ret;
>  
>  	/*
> -	 * Reserve an extra half page of memory for additional measurements
> -	 * added during the kexec load.
> +	 * Reserve extra memory for measurements added during kexec.
>  	 */
> -	binary_runtime_size = ima_get_binary_runtime_size();
> +	if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0)
> +		extra_size = PAGE_SIZE / 2;
> +	else
> +		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
> +	binary_runtime_size = ima_get_binary_runtime_size() + extra_size;

It's clearer if the 'extra_size' is added to the segment_size, not the
binary_runtime_size.  Please revert this change.

> +
>  	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);
> +		kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE);

Please replace binary_runtime_size with "ima_get_binary_runtime_size() +
extra_size".

Mimi

>  	if ((kexec_segment_size == ULONG_MAX) ||
>  	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
>  		pr_err("Binary measurement list too large.\n");

Re: [PATCH v5 6/8] ima: suspend measurements during buffer copy at kexec execute

[Mimi Zohar]

Hi Tushar,

On Wed, 2024-02-14 at 07:38 -0800, Tushar Sugandhi wrote:
> New measurements added to the IMA log while the log is being copied
> during the kexec 'execute' may not get copied over.

As long as there is enough memory for the additional records, isn't the problem
"after" copying the mesaurement list records, not during?

> This can cause the
> measurement log to be out of sync with the TPM PCRs that IMA extends,
> which could result in breaking the integrity of the measurements after
> kexec soft reboot.
> 
> Implement and call the functions ima_measurements_suspend() and 
> ima_measurements_resume() from ima_update_kexec_buffer().

After copying the measurement list records, would not be the time to resume
taking additional measurements.

> Add a check in the ima_add_template_entry() function not to measure
> events when 'suspend_ima_measurements' flag is set.
> 
> This ensures the integrity of the IMA log while it is being copied over
> to the new Kernel during kexec 'execute'.
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  security/integrity/ima/ima.h       |  2 ++
>  security/integrity/ima/ima_kexec.c |  7 +++++++
>  security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c29db699c996..49a6047dd8eb 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct
> ima_template_desc *ima_template);
>  int ima_restore_measurement_entry(struct ima_template_entry *entry);
>  int ima_restore_measurement_list(loff_t bufsize, void *buf);
>  int ima_measurements_show(struct seq_file *m, void *v);
> +void ima_measurements_suspend(void);
> +void ima_measurements_resume(void);
>  unsigned long ima_get_binary_runtime_size(void);
>  int ima_init_template(void);
>  void ima_init_template_list(void);
> diff --git a/security/integrity/ima/ima_kexec.c
> b/security/integrity/ima/ima_kexec.c
> index 98fc9b9782a2..dbeeb7f1355e 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -184,6 +184,7 @@ static int ima_update_kexec_buffer(struct notifier_block
> *self,
>  	void *buf = NULL;
>  	size_t buf_size;
>  	int ret = NOTIFY_OK;
> +	bool resume = false;
>  
>  	if (!kexec_in_progress) {
>  		pr_info("%s: No kexec in progress.\n", __func__);
> @@ -195,12 +196,15 @@ static int ima_update_kexec_buffer(struct notifier_block
> *self,
>  		return ret;
>  	}
>  
> +	ima_measurements_suspend();
> +
>  	ret = ima_dump_measurement_list(&buf_size, &buf,
>  					kexec_segment_size);
>  
>  	if (!buf) {
>  		pr_err("%s: Dump measurements failed. Error:%d\n",
>  		       __func__, ret);
> +		resume = true;

Only on failure to copy the measurement list records will measurements be
"resumed".  Measurements will be suspended from when the IMA reboot notifier is
called until reboot.

The patch suject line, description and comments don't match the code.  There is
no "during buffer copy" in "ima: suspend measurements during buffer copy at
kexec execute".  Measurements are suspended.

The question is whether you could suspend measurements a bit later, just after
copying the measurement records.

>  		goto out;
>  	}
>  	memcpy(ima_kexec_buffer, buf, buf_size);
> @@ -208,6 +212,9 @@ static int ima_update_kexec_buffer(struct notifier_block
> *self,
>  	kimage_unmap_segment(ima_kexec_buffer);
>  	ima_kexec_buffer = NULL;
>  
> +	if (resume)
> +		ima_measurements_resume();
> +

What is the point in resuming the measurement list on failure to copy them?

>  	return ret;
>  }
>  
> diff --git a/security/integrity/ima/ima_queue.c
> b/security/integrity/ima/ima_queue.c
> index 532da87ce519..5946a26a2849 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
>   */
>  static DEFINE_MUTEX(ima_extend_list_mutex);
>  
> +/*
> + * Used internally by the kernel to suspend-resume ima measurements.
> + */
> +static atomic_t suspend_ima_measurements;
> +
>  /* lookup up the digest value in the hash table, and return the entry */
>  static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
>  						       int pcr)
> @@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg,
> int pcr)
>  	return result;
>  }
>  
> +void ima_measurements_suspend(void)
> +{
> +	mutex_lock(&ima_extend_list_mutex);
> +	atomic_set(&suspend_ima_measurements, 1);
> +	mutex_unlock(&ima_extend_list_mutex);
> +}
> +
> +void ima_measurements_resume(void)
> +{
> +	mutex_lock(&ima_extend_list_mutex);
> +	atomic_set(&suspend_ima_measurements, 0);
> +	mutex_unlock(&ima_extend_list_mutex);
> +}
> +
>  /*
>   * Add template entry to the measurement list and hash table, and
>   * extend the pcr.
> @@ -176,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry
> *entry, int violation,
>  		}
>  	}
>  
> +	/*
> +	 * suspend_ima_measurements will be set if the system is
> +	 * undergoing kexec soft boot to a new kernel.
> +	 * suspending measurements in this short window ensures the
> +	 * consistency of the IMA measurement list during copying
> +	 * of the kexec buffer.
> +	 */

Either remove the 2nd sentence "suspending measurements in this short window
..." or explain what is meant by "short window".

Mimi

> +	if (atomic_read(&suspend_ima_measurements)) {
> +		audit_cause = "measurements_suspended";
> +		audit_info = 0;
> +		goto out;
> +	}
> +
>  	result = ima_add_digest_entry(entry,
>  				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
>  	if (result < 0) {

Re: [PATCH v5 6/8] ima: suspend measurements during buffer copy at kexec execute

[Mimi Zohar]

> > @@ -176,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry
> > *entry, int violation,
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * suspend_ima_measurements will be set if the system is
> > +	 * undergoing kexec soft boot to a new kernel.
> > +	 * suspending measurements in this short window ensures the
> > +	 * consistency of the IMA measurement list during copying
> > +	 * of the kexec buffer.
> > +	 */
> 
> Either remove the 2nd sentence "suspending measurements in this short window
> ..." or explain what is meant by "short window".
> 
> 
> > +	if (atomic_read(&suspend_ima_measurements)) {
> > +		audit_cause = "measurements_suspended";
> > +		audit_info = 0;
> > +		goto out;

After the suggested changes, understanding how many measurements are not being
added to the measurement list and not being extended into the TPM would be
really interesting.

Mimi

> > +	}
> > +
> >  	result = ima_add_digest_entry(entry,
> >  				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
> >  	if (result < 0) {
> 
> 

Re: [PATCH v5 6/8] ima: suspend measurements during buffer copy at kexec execute

[Petr Tesařík]

On Thu, 22 Feb 2024 11:38:23 -0500
Mimi Zohar <zohar@linux.ibm.com> wrote:

> > > @@ -176,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry
> > > *entry, int violation,
> > >  		}
> > >  	}
> > >  
> > > +	/*
> > > +	 * suspend_ima_measurements will be set if the system is
> > > +	 * undergoing kexec soft boot to a new kernel.
> > > +	 * suspending measurements in this short window ensures the
> > > +	 * consistency of the IMA measurement list during copying
> > > +	 * of the kexec buffer.
> > > +	 */  
> > 
> > Either remove the 2nd sentence "suspending measurements in this short window
> > ..." or explain what is meant by "short window".
> > 
> >   
> > > +	if (atomic_read(&suspend_ima_measurements)) {
> > > +		audit_cause = "measurements_suspended";
> > > +		audit_info = 0;
> > > +		goto out;  
> 
> After the suggested changes, understanding how many measurements are not being
> added to the measurement list and not being extended into the TPM would be
> really interesting.

First, I'm sorry for chiming in when v5 is already around, but I have
just found this patch series now.

It indeed sounds conceptually wrong to suspend and resume measurements.
At some point during the handover, other CPUs are taken offline (look
for migrate_to_reboot_cpu() in kernel/kexec_core.c) and even the reboot
CPU will be sufficiently shut down as not to be able to add any more
measurements.

IMO it would make more sense to copy the measurement list at this late
stage, even if it means adding a new notifier list (or a new action).

It may be a bit challenging if you want to make 100% sure that a new
measurement cannot be made from hard interrupt context, but is that even
a supported scenario?

Just my two (euro)cents,
Petr T

Re: [PATCH v5 4/8] ima: kexec: define functions to copy IMA log at soft boot

[Petr Tesařík]

On Wed, 14 Feb 2024 07:38:23 -0800
Tushar Sugandhi <tusharsu@linux.microsoft.com> wrote:

> IMA log is copied to the new Kernel during kexec 'load' using 
> ima_dump_measurement_list().  The log copy at kexec 'load' may result in
> loss of IMA measurements during kexec soft reboot.  It needs to be copied
> over during kexec 'execute'.  Setup the needed infrastructure to move the
> IMA log copy from kexec 'load' to 'execute'. 
> 
> Define a new IMA hook ima_update_kexec_buffer() as a stub function.
> It will be used to call ima_dump_measurement_list() during kexec 
> 'execute'.   
> 
> Implement kimage_file_post_load() and ima_kexec_post_load() functions
> to be invoked after the new Kernel image has been loaded for kexec.
> ima_kexec_post_load() maps the IMA buffer to a segment in the newly
> loaded Kernel.  It also registers the reboot notifier_block to trigger
> ima_update_kexec_buffer() at exec 'execute'.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  include/linux/ima.h                |  3 ++
>  kernel/kexec_file.c                |  5 ++++
>  security/integrity/ima/ima_kexec.c | 46 ++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 86b57757c7b1..006db20f852d 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -49,6 +49,9 @@ static inline void ima_appraise_parse_cmdline(void) {}
>  
>  #ifdef CONFIG_IMA_KEXEC
>  extern void ima_add_kexec_buffer(struct kimage *image);
> +extern void ima_kexec_post_load(struct kimage *image);
> +#else
> +static inline void ima_kexec_post_load(struct kimage *image) {}
>  #endif
>  
>  #else
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 0e3689bfb0bb..fe59cb7c179d 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -186,6 +186,11 @@ kimage_validate_signature(struct kimage *image)
>  }
>  #endif
>  
> +void kimage_file_post_load(struct kimage *image)
> +{
> +	ima_kexec_post_load(image);
> +}
> +
>  /*
>   * In file mode list of segments is prepared by kernel. Copy relevant
>   * data from user space, do error checking, prepare segment list
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index ccb072617c2d..1d4d6c122d82 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -12,10 +12,14 @@
>  #include <linux/kexec.h>
>  #include <linux/of.h>
>  #include <linux/ima.h>
> +#include <linux/reboot.h>
> +#include <asm/page.h>
>  #include "ima.h"
>  
>  #ifdef CONFIG_IMA_KEXEC
>  static struct seq_file ima_kexec_file;
> +static void *ima_kexec_buffer;
> +static bool ima_kexec_update_registered;
>  
>  static void ima_reset_kexec_file(struct seq_file *sf)
>  {
> @@ -184,6 +188,48 @@ void ima_add_kexec_buffer(struct kimage *image)
>  	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
>  		      kbuf.mem);
>  }
> +
> +/*
> + * Called during kexec execute so that IMA can update the measurement list.
> + */
> +static int ima_update_kexec_buffer(struct notifier_block *self,
> +				   unsigned long action, void *data)
> +{
> +	return NOTIFY_OK;
> +}
> +
> +struct notifier_block update_buffer_nb = {
> +	.notifier_call = ima_update_kexec_buffer,
> +};
> +
> +/*
> + * Create a mapping for the source pages that contain the IMA buffer
> + * so we can update it later.
> + */
> +void ima_kexec_post_load(struct kimage *image)
> +{
> +	if (ima_kexec_buffer) {
> +		kimage_unmap_segment(ima_kexec_buffer);
> +		ima_kexec_buffer = NULL;
> +	}
> +
> +	if (!image->ima_buffer_addr)
> +		return;
> +
> +	ima_kexec_buffer = kimage_map_segment(image,
> +					      image->ima_buffer_addr,
> +					      image->ima_buffer_size);
> +	if (!ima_kexec_buffer) {
> +		pr_err("%s: Could not map measurements buffer.\n", __func__);
> +		return;
> +	}
> +
> +	if (!ima_kexec_update_registered) {
> +		register_reboot_notifier(&update_buffer_nb);

Specifically this means that the notifier is run from kernel_kexec()
through kernel_restart_prepare(). At that point the kernel is still
running at full speed, i.e. new IMA measurements can be added to the
list.

I believe it would be better to add a "kexec late notifier list" which
would be notified just before kmsg_dump() in kernel_kexec(). Functions
on that list would be called in a more quiet context, i.e. after other
CPUs and most devices have been shut down.

I can see that the current proposal does not address passing the IMA
measurements to a panic kernel, but I'm adding Baoquan nevertheless.
Would it make sense to implement such a late notifier list for the
crash kexec case. I know that in general, it is no good idea to run
more code in a crashed kernel context, but what about self-contained
things like copying IMA measurements?

Petr T

Re: [PATCH v5 5/8] ima: kexec: move IMA log copy from kexec load to execute

[Petr Tesařík]

On Wed, 14 Feb 2024 07:38:24 -0800
Tushar Sugandhi <tusharsu@linux.microsoft.com> wrote:

> ima_dump_measurement_list() is called during kexec 'load', which may
> result in loss of IMA measurements during kexec soft reboot.  It needs
> to be called during kexec 'execute'.
> 
> This patch includes the following changes:
>  - Call kimage_file_post_load() from kexec_file_load() syscall only for
>    kexec soft reboot scenarios and not for KEXEC_FILE_ON_CRASH.  It will
>    map the IMA segment, and register reboot notifier for the function
>    ima_update_kexec_buffer() which would copy the IMA log at kexec soft
>    reboot.
>  - Make kexec_segment_size variable local static to the file, for it to be
>    accessible both during kexec 'load' and 'execute'.
>  - Move ima_dump_measurement_list() call from ima_add_kexec_buffer()
>    to ima_update_kexec_buffer().
>  - Remove ima_reset_kexec_file() call from ima_add_kexec_buffer(), now
>    that the buffer is being copied at kexec 'execute', and resetting the
>    file at kexec 'load' will corrupt the buffer.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  kernel/kexec_file.c                |  3 ++
>  security/integrity/ima/ima_kexec.c | 45 +++++++++++++++++++-----------
>  2 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index fe59cb7c179d..2d5df320c34f 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -410,6 +410,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  
>  	kimage_terminate(image);
>  
> +	if (!(flags & KEXEC_FILE_ON_CRASH))
> +		kimage_file_post_load(image);
> +
>  	ret = machine_kexec_post_load(image);
>  	if (ret)
>  		goto out;
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 1d4d6c122d82..98fc9b9782a2 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -19,6 +19,7 @@
>  #ifdef CONFIG_IMA_KEXEC
>  static struct seq_file ima_kexec_file;
>  static void *ima_kexec_buffer;
> +static size_t kexec_segment_size;
>  static bool ima_kexec_update_registered;
>  
>  static void ima_reset_kexec_file(struct seq_file *sf)
> @@ -129,7 +130,6 @@ 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;
>  
>  	/*
> @@ -154,14 +154,6 @@ void ima_add_kexec_buffer(struct kimage *image)
>  		return;
>  	}
>  
> -	ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
> -					kexec_segment_size);
> -	if (ret < 0) {
> -		pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
> -		       __func__, ret);
> -		return;
> -	}
> -

After removing these lines...

>  	kbuf.buffer = kexec_buffer;
>  	kbuf.bufsz = kexec_buffer_size;
                     ^^^^^^^^^^^^^^^^^

... kexec_buffer_size is uninitialized here.

Petr T