[PATCH v2 0/7] ima: kexec: measure events between kexec load and execute

[PATCH v2 0/7] 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'.

Some systems can be configured to call kexec 'load' first, and followed
by kexec 'execute' after some time.  (as opposed to calling 'load' and
'execute' in one single kexec command).  In such scenario, if new IMA
measurements are added between kexec 'load' and kexec 'execute', the
TPM PCRs are extended with the IMA events between 'load' and 'execute'.
But those IMA events are not carried over to the new Kernel after kexec
soft reboot.  This results in mismatch between TPM PCR quotes, and the
actual IMA measurements list, after the system boots into the new kexec
image.  This mismatch results in the remote attestation failing for that
system.

This patch series proposes a solution to solve this problem by allocating
the necessary buffer at kexec 'load' time, and populating the buffer
with the IMA measurements at kexec 'execute' time. 

The solution includes:
 - 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',

 - ima functionality for mapping the measurement list 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 amount of 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,

The modifications 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.

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/

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 (7):
  ima: refactor ima_dump_measurement_list to move memory allocation to a
    separate function
  ima: move ima_dump_measurement_list call from kexec load to execute
  ima: kexec: map source pages containing IMA buffer to image post kexec
    load
  kexec: update kexec_file_load syscall to call ima_kexec_post_load
  ima: suspend measurements while the buffer is being copied during
    kexec reboot
  ima: make the memory for events between kexec load and exec
    configurable
  ima: record log size at kexec load and execute

 include/linux/ima.h                |   3 +
 include/linux/kexec.h              |  13 ++
 kernel/kexec_core.c                |  73 ++++++++-
 kernel/kexec_file.c                |   8 +
 security/integrity/ima/Kconfig     |   9 ++
 security/integrity/ima/ima.h       |   2 +
 security/integrity/ima/ima_kexec.c | 246 ++++++++++++++++++++++++-----
 security/integrity/ima/ima_queue.c |  31 ++++
 8 files changed, 341 insertions(+), 44 deletions(-)

-- 
2.25.1

[PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Tushar Sugandhi]

IMA allocates memory and dumps the measurement during kexec soft reboot
as a single function call ima_dump_measurement_list().  It gets called
during kexec 'load' operation.  It results in the IMA measurements
between the window of kexec 'load' and 'execute' getting dropped when the
system boots into the new Kernel.  One of the kexec requirements is the
segment size cannot change between the 'load' and the 'execute'.
Therefore, to address this problem, ima_dump_measurement_list() needs
to be refactored to allocate the memory at kexec 'load', and dump the
measurements at kexec 'execute'.  The function that allocates the memory
should handle the scenario where the kexec load is called multiple times.

Refactor ima_dump_measurement_list() to move the memory allocation part
to a separate function ima_alloc_kexec_buf() to allocate buffer of size
'kexec_segment_size' at kexec 'load'.  Make the local variables in
function ima_dump_measurement_list() global, so that they can be accessed
from ima_alloc_kexec_buf().  Make necessary changes to the function
ima_add_kexec_buffer() to call the above two functions.

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

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 419dc405c831..307e07991865 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -15,61 +15,114 @@
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+struct seq_file ima_kexec_file;
+struct ima_kexec_hdr ima_khdr;
+
+void ima_clear_kexec_file(void)
+{
+	vfree(ima_kexec_file.buf);
+	ima_kexec_file.buf = NULL;
+	ima_kexec_file.size = 0;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = 0;
+}
+
+static int ima_alloc_kexec_buf(size_t kexec_segment_size)
+{
+	if ((kexec_segment_size == 0) ||
+	    (kexec_segment_size == ULONG_MAX) ||
+	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
+		pr_err("%s: Invalid segment size for kexec: %zu\n",
+			__func__, kexec_segment_size);
+		return -EINVAL;
+	}
+
+	/*
+	 * If kexec load was called before, clear the existing buffer
+	 * before allocating a new one
+	 */
+	if (ima_kexec_file.buf)
+		ima_clear_kexec_file();
+
+	/* segment size can't change between kexec load and execute */
+	ima_kexec_file.buf = vmalloc(kexec_segment_size);
+	if (!ima_kexec_file.buf) {
+		pr_err("%s: No memory for ima kexec measurement buffer\n",
+			__func__);
+		return -ENOMEM;
+	}
+
+	ima_kexec_file.size = kexec_segment_size;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = sizeof(ima_khdr);	/* reserved space */
+
+	memset(&ima_khdr, 0, sizeof(ima_khdr));
+	ima_khdr.version = 1;
+
+	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) {
+		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 */
+	/*
+	 * Ensure the kexec buffer is large enough to hold ima_khdr
+	 */
+	if (ima_kexec_file.size < sizeof(ima_khdr)) {
+		pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
+			__func__);
+		ima_clear_kexec_file();
+		return -ENOMEM;
+	}
 
-	memset(&khdr, 0, sizeof(khdr));
-	khdr.version = 1;
+	/*
+	 * If we reach here, then there is enough memory
+	 * of size kexec_segment_size in ima_kexec_file.buf
+	 * to copy at least partial IMA log.
+	 * Make best effort to copy as many IMA measurements
+	 * as possible.
+	 */
 	list_for_each_entry_rcu(qe, &ima_measurements, later) {
-		if (file.count < file.size) {
-			khdr.count++;
-			ima_measurements_show(&file, qe);
+		if (ima_kexec_file.count < ima_kexec_file.size) {
+			ima_khdr.count++;
+			ima_measurements_show(&ima_kexec_file, qe);
 		} else {
-			ret = -EINVAL;
+			ret = EFBIG;
+			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;
+	ima_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);
+		ima_khdr.version = cpu_to_le16(ima_khdr.version);
+		ima_khdr.count = cpu_to_le64(ima_khdr.count);
+		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
 	}
-	memcpy(file.buf, &khdr, sizeof(khdr));
+	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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);
+	*buffer_size = ima_kexec_file.count;
+	*buffer = ima_kexec_file.buf;
+
 	return ret;
 }
 
@@ -108,13 +161,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_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;
-- 
2.25.1

[PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute

[Tushar Sugandhi]

In the current IMA implementation, ima_dump_measurement_list() is called
during the kexec 'load' operation.  This can result in loss of IMA
measurements taken between the 'load' and 'execute' phases when the
system goes through Kexec soft reboot to a new Kernel.  The call to the
function ima_dump_measurement_list() needs to be moved out of the
function ima_add_kexec_buffer() and needs to be called during the kexec
'execute' operation.

Implement a function ima_update_kexec_buffer() that is called during
kexec 'execute', allowing IMA to update the measurement list with the
events between kexec 'load' and 'execute'.  Move the 
ima_dump_measurement_list() call from ima_add_kexec_buffer() to
ima_update_kexec_buffer().  Make ima_kexec_buffer and kexec_segment_size
variables global, so that they can be accessed during both kexec 'load'
and 'execute'.  Add functions ima_measurements_suspend() and
ima_measurements_resume() to set and reset the 'suspend_ima_measurements'
variable respectively, to suspend/resume IMA measurements.  Use
the existing 'ima_extend_list_mutex' to ensure that the operations are
thread-safe.  These function calls will help maintaining the integrity
of the IMA log while it is being copied to the new Kernel's buffer.
Add a reboot notifier_block 'update_buffer_nb' to ensure
the function ima_update_kexec_buffer() gets called during kexec
soft-reboot.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima.h       |  2 ++
 security/integrity/ima/ima_kexec.c | 58 +++++++++++++++++++++++++-----
 security/integrity/ima/ima_queue.c | 18 ++++++++++
 3 files changed, 69 insertions(+), 9 deletions(-)

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 307e07991865..2c11bbe6efef 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -17,6 +17,8 @@
 #ifdef CONFIG_IMA_KEXEC
 struct seq_file ima_kexec_file;
 struct ima_kexec_hdr ima_khdr;
+static void *ima_kexec_buffer;
+static size_t kexec_segment_size;
 
 void ima_clear_kexec_file(void)
 {
@@ -142,7 +144,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;
 
 	/*
@@ -167,14 +168,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;
@@ -192,6 +185,53 @@ void ima_add_kexec_buffer(struct kimage *image)
 	pr_debug("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)
+{
+	void *buf = NULL;
+	size_t buf_size;
+	bool resume = false;
+	int ret;
+
+	if (!kexec_in_progress) {
+		pr_info("%s: No kexec in progress.\n", __func__);
+		return NOTIFY_OK;
+	}
+
+	if (!ima_kexec_buffer) {
+		pr_err("%s: Kexec buffer not set.\n", __func__);
+		return NOTIFY_OK;
+	}
+
+	ima_measurements_suspend();
+
+	buf_size = ima_get_binary_runtime_size();
+	ret = ima_dump_measurement_list(&buf_size, &buf,
+					kexec_segment_size);
+
+	if (!buf || ret < 0) {
+		pr_err("%s: Dump measurements failed. Error:%d\n",
+		       __func__, ret);
+		resume = true;
+		goto out;
+	}
+	memcpy(ima_kexec_buffer, buf, buf_size);
+out:
+	ima_kexec_buffer = NULL;
+
+	if (resume)
+		ima_measurements_resume();
+
+	return NOTIFY_OK;
+}
+struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+};
+
 #endif /* IMA_KEXEC */
 
 /*
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 532da87ce519..9e7d1196006e 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)
@@ -147,6 +152,19 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 		pr_err("Error Communicating to TPM chip, result: %d\n", result);
 	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
-- 
2.25.1

[PATCH v2 3/7] ima: kexec: map source pages containing IMA buffer to image post kexec load

[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 kimage_map_segment() which takes a kimage pointer, an address,
and a size.  Ensure that the entire segment is being mapped by comparing
the given address and size to each segment in the kimage's segment array.
Collect the source pages that correspond to the given address range,
allocate an array of pointers to these pages, and map them to a
contiguous range of virtual addresses.  If the mapping operation is
successful, the function returns the start of this range.  Otherwise, it
frees the page pointer array and returns NULL.

Implement kimage_unmap_segment() that takes a pointer to a segment buffer
and unmaps it using vunmap().

Implement function ima_kexec_post_load(), to be called by IMA after kexec
loads the new Kernel image.  ima_kexec_post_load() would map the IMA
buffer allocated during kexec 'load' to a segment in the loaded image.

Finally, move for_each_kimage_entry() macro from kexec_core.c to kexec.h.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 include/linux/ima.h                |  3 ++
 include/linux/kexec.h              | 13 ++++++
 kernel/kexec_core.c                | 73 ++++++++++++++++++++++++++++--
 security/integrity/ima/ima_kexec.c | 32 +++++++++++++
 4 files changed, 116 insertions(+), 5 deletions(-)

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/include/linux/kexec.h b/include/linux/kexec.h
index 22b5cd24f581..e00b8101b53b 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -490,6 +490,15 @@ static inline int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, g
 static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) { }
 #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)
+
+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;
@@ -497,6 +506,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 3d578c6fefee..e01156f3c404 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -594,11 +594,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;
@@ -921,6 +916,74 @@ 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;
+	struct page **src_pages;
+	int i, npages;
+	kimage_entry_t *ptr, entry;
+	void *vaddr = NULL;
+
+	/*
+	 * Make sure that we are mapping a whole segment.
+	 */
+	for (i = 0; i < image->nr_segments; i++) {
+		if (addr == image->segment[i].mem &&
+		    size == image->segment[i].memsz) {
+			break;
+		}
+	}
+
+	if (i == image->nr_segments) {
+		pr_err("%s: No segment matching [%lx, %lx)\n", __func__,
+		       addr, eaddr);
+		return NULL;
+	}
+
+	/*
+	 * Collect the source pages and map them in a contiguous VA range.
+	 */
+	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
+	src_pages = kmalloc(sizeof(*src_pages) * npages, 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);
+	if (!vaddr) {
+		pr_err("%s: Could not map imap buffer.\n", __func__);
+		kfree(src_pages);
+	}
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
 struct kexec_load_limit {
 	/* Mutex protects the limit count. */
 	struct mutex mutex;
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 2c11bbe6efef..13fbbb90319b 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -12,6 +12,8 @@
 #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
@@ -19,6 +21,7 @@ struct seq_file ima_kexec_file;
 struct ima_kexec_hdr ima_khdr;
 static void *ima_kexec_buffer;
 static size_t kexec_segment_size;
+static bool ima_kexec_update_registered;
 
 void ima_clear_kexec_file(void)
 {
@@ -221,6 +224,7 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 	}
 	memcpy(ima_kexec_buffer, buf, buf_size);
 out:
+	kimage_unmap_segment(ima_kexec_buffer);
 	ima_kexec_buffer = NULL;
 
 	if (resume)
@@ -232,6 +236,34 @@ 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 v2 4/7] kexec: update kexec_file_load syscall to call ima_kexec_post_load

[Tushar Sugandhi]

The kexec_file_load() syscall is used to load a new Kernel for kexec.
The syscall needs to be updated to call ima_kexec_post_load(),
which was implemented in a previous patch.  ima_kexec_post_load() should
take care of mapping the IMA log buffer segment into the next Kernel. It
should also register a reboot notifier which would call a function to
dump the IMA measurements into IMA log buffer segment during kexec soft
reboot.

Modify the kexec_file_load() syscall to call ima_kexec_post_load() after
the image has been loaded and prepared for kexec.  This ensures that the
IMA measurement list will be available to the next Kernel after a kexec
soft reboot.  This also ensures the measurements taken in the window
between kexec 'load' and 'execute' are captured and passed to the next
Kernel.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 kernel/kexec_file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f989f5f1933b..617dbbb6e46d 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -184,6 +184,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
@@ -399,6 +404,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;
-- 
2.25.1

[PATCH v2 5/7] ima: suspend measurements while the buffer is being copied during kexec reboot

[Tushar Sugandhi]

If the new measurements are added to the IMA log while it is being 
being copied to the kexec buffer during kexec 'execute', it can miss
copying those new measurements to the kexec buffer, and the buffer can go
out of sync with TPM PCRs.  This could result in breaking the integrity
of the measurements after the kexec soft reboot to the new Kernel.

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

This ensures the consistency of the IMA measurement list while copying 
them to the kexec buffer.  When the 'suspend_ima_measurements' flag is
set, any new measurements will be ignored until the flag is unset.  This
allows the buffer to be safely copied without worrying about concurrent
modifications to the measurement list.  This is crucial for maintaining
the integrity of the measurements during a kexec soft reboot.

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

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 9e7d1196006e..498b6b92f3f0 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -194,6 +194,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 v2 6/7] ima: make the memory for events between kexec load and exec configurable

[Tushar Sugandhi]

IMA currently allocates half a PAGE_SIZE for the extra events that would
be measured between kexec 'load' and 'execute'.  Depending on the IMA
policy and the system state, that memory may not be sufficient to hold
the extra IMA events measured after kexec 'load'.  The memory
requirements vary from system to system and they should be configurable.

Define a Kconfig option, IMA_KEXEC_EXTRA_PAGES, to configure the number
of extra pages to be allocated for IMA measurements added in the window
from kexec 'load' to kexec 'execute'.

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

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/Kconfig     |  9 +++++++++
 security/integrity/ima/ima_kexec.c | 13 ++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 60a511c6b583..1b55cd2bcb36 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -338,3 +338,12 @@ config IMA_DISABLE_HTABLE
 	default n
 	help
 	   This option disables htable to allow measurement of duplicate records.
+
+config IMA_KEXEC_EXTRA_PAGES
+	int
+	depends on IMA && IMA_KEXEC
+	default 16
+	help
+	  IMA_KEXEC_EXTRA_PAGES determines the number of extra
+	  pages to be allocated for IMA measurements added in the
+	  window from kexec 'load' to kexec 'execute'.
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 13fbbb90319b..6cd5f46a7208 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -150,15 +150,18 @@ 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 in the window from
+	 * kexec 'load' to kexec 'execute'.
 	 */
-	binary_runtime_size = ima_get_binary_runtime_size();
+	binary_runtime_size = ima_get_binary_runtime_size() +
+			      sizeof(struct ima_kexec_hdr) +
+			      (CONFIG_IMA_KEXEC_EXTRA_PAGES * PAGE_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 v2 7/7] ima: record log size at kexec load and execute

[Tushar Sugandhi]

The window between kexec 'load' and 'execute' could be arbitrarily long.
Even with the large chunk of memory allocated at kexec 'load', it may
run out which would result in missing events in IMA log after the system
soft reboots to the new Kernel.  This would result in IMA measurements
getting out of sync with the TPM PCR quotes which would result in remote
attestation failing for that system.  There is currently no way for the
new Kernel to know if the IMA log TPM PCR quote out of sync problem is
because of the missing measurements during kexec.

Define two new IMA events, 'kexec_load' and 'kexec_execute', to be 
measured at kexec 'load' and 'execute' respectively.

IMA measures 'boot_aggregate' as the first event when the system boots - 
either cold boot or kexec soft boot.  In case when the system goes
through multiple soft reboots, the number of 'boot_aggregate' events in
IMA log corresponds to total number of boots (cold boot plus multiple
kexec soft reboots).  With this change, there would be additional 
'kexec_load' and 'kexec_execute' events in between the two 
'boot_aggregate' events.  In rare cases, when the system runs out of
memory during kexec soft reboot, 'kexec_execute' won't be copied since
its one of the very last event measured just before kexec soft reboot.
The absence of the event 'kexec_execute' in between the two 
boot_aggregate' events would signal the attestation service that the IMA
log on the system is out of sync with TPM PCR quotes and the system needs
to be cold booted for the remote attestation to succeed again.

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

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 6cd5f46a7208..0f9c424fe808 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 128
+
 struct seq_file ima_kexec_file;
 struct ima_kexec_hdr ima_khdr;
 static void *ima_kexec_buffer;
@@ -34,6 +36,8 @@ void ima_clear_kexec_file(void)
 
 static int ima_alloc_kexec_buf(size_t kexec_segment_size)
 {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
+
 	if ((kexec_segment_size == 0) ||
 	    (kexec_segment_size == ULONG_MAX) ||
 	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
@@ -64,6 +68,12 @@ static int ima_alloc_kexec_buf(size_t kexec_segment_size)
 	memset(&ima_khdr, 0, sizeof(ima_khdr));
 	ima_khdr.version = 1;
 
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;", kexec_segment_size);
+
+	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
+				  strlen(ima_kexec_event), false, NULL, 0);
+
 	return 0;
 }
 
@@ -198,6 +208,7 @@ 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;
 	bool resume = false;
@@ -213,9 +224,31 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 		return NOTIFY_OK;
 	}
 
+	buf_size = ima_get_binary_runtime_size();
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;",
+		  kexec_segment_size, buf_size);
+
+	/*
+	 * This is one of the very last events measured by IMA before kexec
+	 * soft rebooting into the new Kernal.
+	 * This event can be used as a marker after the system soft reboots
+	 * to the new Kernel to check if there was sufficient memory allocated
+	 * at kexec 'load' to capture the events measured between the window
+	 * of kexec 'load' and 'execute'.
+	 * This event needs to be present in the IMA log, in between the two
+	 * 'boot_aggregate' events that are logged for the previous boot and
+	 * the current soft reboot. If it is not present after the system soft
+	 * reboots into the new Kernel, it would mean the IMA log is not
+	 * consistent with the TPM PCR quotes, and the system needs to be
+	 * cold-booted for the attestation to succeed again.
+	 */
+	ima_measure_critical_data("ima_kexec", "kexec_execute",
+				  ima_kexec_event, strlen(ima_kexec_event),
+				  false, NULL, 0);
+
 	ima_measurements_suspend();
 
-	buf_size = ima_get_binary_runtime_size();
 	ret = ima_dump_measurement_list(&buf_size, &buf,
 					kexec_segment_size);
 
-- 
2.25.1

Re: [PATCH v2 7/7] ima: record log size at kexec load and execute

[Stefan Berger]

On 10/5/23 14:26, Tushar Sugandhi wrote:
> The window between kexec 'load' and 'execute' could be arbitrarily long.
> Even with the large chunk of memory allocated at kexec 'load', it may
> run out which would result in missing events in IMA log after the system
> soft reboots to the new Kernel.  This would result in IMA measurements
> getting out of sync with the TPM PCR quotes which would result in remote
> attestation failing for that system.  There is currently no way for the
> new Kernel to know if the IMA log TPM PCR quote out of sync problem is
> because of the missing measurements during kexec.
>
> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
> measured at kexec 'load' and 'execute' respectively.
>
> IMA measures 'boot_aggregate' as the first event when the system boots -
> either cold boot or kexec soft boot.  In case when the system goes
> through multiple soft reboots, the number of 'boot_aggregate' events in
> IMA log corresponds to total number of boots (cold boot plus multiple
> kexec soft reboots).  With this change, there would be additional
> 'kexec_load' and 'kexec_execute' events in between the two
> 'boot_aggregate' events.  In rare cases, when the system runs out of
> memory during kexec soft reboot, 'kexec_execute' won't be copied since
> its one of the very last event measured just before kexec soft reboot.
> The absence of the event 'kexec_execute' in between the two
> boot_aggregate' events would signal the attestation service that the IMA
> log on the system is out of sync with TPM PCR quotes and the system needs
> to be cold booted for the remote attestation to succeed again.
>
>
> @@ -198,6 +208,7 @@ 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;
>   	bool resume = false;
> @@ -213,9 +224,31 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>   		return NOTIFY_OK;
>   	}
>   
> +	buf_size = ima_get_binary_runtime_size();
> +	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
> +		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;",
> +		  kexec_segment_size, buf_size);
> +
> +	/*
> +	 * This is one of the very last events measured by IMA before kexec
> +	 * soft rebooting into the new Kernal.
> +	 * This event can be used as a marker after the system soft reboots
> +	 * to the new Kernel to check if there was sufficient memory allocated
> +	 * at kexec 'load' to capture the events measured between the window
> +	 * of kexec 'load' and 'execute'.
> +	 * This event needs to be present in the IMA log, in between the two
> +	 * 'boot_aggregate' events that are logged for the previous boot and
> +	 * the current soft reboot. If it is not present after the system soft
> +	 * reboots into the new Kernel, it would mean the IMA log is not
> +	 * consistent with the TPM PCR quotes, and the system needs to be
> +	 * cold-booted for the attestation to succeed again.
> +	 */
> +	ima_measure_critical_data("ima_kexec", "kexec_execute",
> +				  ima_kexec_event, strlen(ima_kexec_event),
> +				  false, NULL, 0);
> +
>   	ima_measurements_suspend();
>   
> -	buf_size = ima_get_binary_runtime_size();

This should be removed earlier, in 2/7.



>   	ret = ima_dump_measurement_list(&buf_size, &buf,
>   					kexec_segment_size);
>   

Re: [PATCH v2 6/7] ima: make the memory for events between kexec load and exec configurable

[Stefan Berger]

On 10/5/23 14:26, Tushar Sugandhi wrote:
> IMA currently allocates half a PAGE_SIZE for the extra events that would
> be measured between kexec 'load' and 'execute'.  Depending on the IMA
> policy and the system state, that memory may not be sufficient to hold
> the extra IMA events measured after kexec 'load'.  The memory
> requirements vary from system to system and they should be configurable.
>
> Define a Kconfig option, IMA_KEXEC_EXTRA_PAGES, to configure the number
> of extra pages to be allocated for IMA measurements added in the window
> from kexec 'load' to kexec 'execute'.
>
> Update ima_add_kexec_buffer() function to allocate memory based on the
> Kconfig option value, rather than the currently hardcoded one.
>
> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
> ---
>   security/integrity/ima/Kconfig     |  9 +++++++++
>   security/integrity/ima/ima_kexec.c | 13 ++++++++-----
>   2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 60a511c6b583..1b55cd2bcb36 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -338,3 +338,12 @@ config IMA_DISABLE_HTABLE
>   	default n
>   	help
>   	   This option disables htable to allow measurement of duplicate records.
> +
> +config IMA_KEXEC_EXTRA_PAGES
> +	int
> +	depends on IMA && IMA_KEXEC
> +	default 16
> +	help
> +	  IMA_KEXEC_EXTRA_PAGES determines the number of extra
> +	  pages to be allocated for IMA measurements added in the
> +	  window from kexec 'load' to kexec 'execute'.


On ppc64 a page is 64kb. I would ask for additional kb here.


> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 13fbbb90319b..6cd5f46a7208 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -150,15 +150,18 @@ 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 in the window from
> +	 * kexec 'load' to kexec 'execute'.
>   	 */
> -	binary_runtime_size = ima_get_binary_runtime_size();
> +	binary_runtime_size = ima_get_binary_runtime_size() +
> +			      sizeof(struct ima_kexec_hdr) +
> +			      (CONFIG_IMA_KEXEC_EXTRA_PAGES * PAGE_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");

Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute

[Stefan Berger]

On 10/5/23 14:25, Tushar Sugandhi wrote:
> In the current IMA implementation, ima_dump_measurement_list() is called
> during the kexec 'load' operation.  This can result in loss of IMA
> measurements taken between the 'load' and 'execute' phases when the
> system goes through Kexec soft reboot to a new Kernel.  The call to the
> function ima_dump_measurement_list() needs to be moved out of the
> function ima_add_kexec_buffer() and needs to be called during the kexec
> 'execute' operation.
>
> Implement a function ima_update_kexec_buffer() that is called during
> kexec 'execute', allowing IMA to update the measurement list with the
> events between kexec 'load' and 'execute'.  Move the
> ima_dump_measurement_list() call from ima_add_kexec_buffer() to
> ima_update_kexec_buffer().  Make ima_kexec_buffer and kexec_segment_size
> variables global, so that they can be accessed during both kexec 'load'
> and 'execute'.  Add functions ima_measurements_suspend() and
> ima_measurements_resume() to set and reset the 'suspend_ima_measurements'
> variable respectively, to suspend/resume IMA measurements.  Use
> the existing 'ima_extend_list_mutex' to ensure that the operations are
> thread-safe.  These function calls will help maintaining the integrity
> of the IMA log while it is being copied to the new Kernel's buffer.
> Add a reboot notifier_block 'update_buffer_nb' to ensure
> the function ima_update_kexec_buffer() gets called during kexec
> soft-reboot.
>
> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
> ---
>   security/integrity/ima/ima.h       |  2 ++
>   security/integrity/ima/ima_kexec.c | 58 +++++++++++++++++++++++++-----
>   security/integrity/ima/ima_queue.c | 18 ++++++++++
>   3 files changed, 69 insertions(+), 9 deletions(-)
>
> 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 307e07991865..2c11bbe6efef 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -17,6 +17,8 @@
>   #ifdef CONFIG_IMA_KEXEC
>   struct seq_file ima_kexec_file;
>   struct ima_kexec_hdr ima_khdr;
> +static void *ima_kexec_buffer;
> +static size_t kexec_segment_size;
>   
>   void ima_clear_kexec_file(void)
>   {
> @@ -142,7 +144,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;
>   
>   	/*
> @@ -167,14 +168,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;
> @@ -192,6 +185,53 @@ void ima_add_kexec_buffer(struct kimage *image)
>   	pr_debug("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)
> +{
> +	void *buf = NULL;
> +	size_t buf_size;
> +	bool resume = false;
> +	int ret;
> +
> +	if (!kexec_in_progress) {
> +		pr_info("%s: No kexec in progress.\n", __func__);
> +		return NOTIFY_OK;
> +	}
> +
> +	if (!ima_kexec_buffer) {
> +		pr_err("%s: Kexec buffer not set.\n", __func__);
> +		return NOTIFY_OK;
> +	}
> +
> +	ima_measurements_suspend();
> +
> +	buf_size = ima_get_binary_runtime_size();
There doesn't seem to be a need to call this function and pass in the 
binary runtime size into the dump function. You should be able to remove it.
> +	ret = ima_dump_measurement_list(&buf_size, &buf,
> +					kexec_segment_size);
> +
> +	if (!buf || ret < 0) {
I don't think this function can (or should) ever return ret >= 0 with 
buf == NULL.
> +		pr_err("%s: Dump measurements failed. Error:%d\n",
> +		       __func__, ret);
> +		resume = true;
> +		goto out;
> +	}
> +	memcpy(ima_kexec_buffer, buf, buf_size);
> +out:
> +	ima_kexec_buffer = NULL;
> +
> +	if (resume)
> +		ima_measurements_resume();
> +
> +	return NOTIFY_OK;
> +}
> +struct notifier_block update_buffer_nb = {
> +	.notifier_call = ima_update_kexec_buffer,
> +};
> +
>   #endif /* IMA_KEXEC */
>   
>   /*
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index 532da87ce519..9e7d1196006e 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)
> @@ -147,6 +152,19 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
>   		pr_err("Error Communicating to TPM chip, result: %d\n", result);
>   	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

Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Stefan Berger]

On 10/5/23 14:25, Tushar Sugandhi wrote:
> IMA allocates memory and dumps the measurement during kexec soft reboot
> as a single function call ima_dump_measurement_list().  It gets called
> during kexec 'load' operation.  It results in the IMA measurements
> between the window of kexec 'load' and 'execute' getting dropped when the
> system boots into the new Kernel.  One of the kexec requirements is the
> segment size cannot change between the 'load' and the 'execute'.
> Therefore, to address this problem, ima_dump_measurement_list() needs
> to be refactored to allocate the memory at kexec 'load', and dump the
> measurements at kexec 'execute'.  The function that allocates the memory
> should handle the scenario where the kexec load is called multiple times.
>
> Refactor ima_dump_measurement_list() to move the memory allocation part
> to a separate function ima_alloc_kexec_buf() to allocate buffer of size
> 'kexec_segment_size' at kexec 'load'.  Make the local variables in
> function ima_dump_measurement_list() global, so that they can be accessed
> from ima_alloc_kexec_buf().  Make necessary changes to the function
> ima_add_kexec_buffer() to call the above two functions.
>
> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
> ---
>   security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
>   1 file changed, 93 insertions(+), 33 deletions(-)
>
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 419dc405c831..307e07991865 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -15,61 +15,114 @@
>   #include "ima.h"
>   
>   #ifdef CONFIG_IMA_KEXEC
> +struct seq_file ima_kexec_file;
> +struct ima_kexec_hdr ima_khdr;

Since you are only populating the buffer at kexec 'execute' time, you 
should be able to move this header into the function where it is needed.


> +
> +void ima_clear_kexec_file(void)
> +{
> +	vfree(ima_kexec_file.buf);
I would pass the ima_kexec_file onto this function here as a parameter 
rather than accessing the file-static variable. I find this better to 
read once you look at ima_clear_kexec_file() further below and wonder 
why it doesn't take a parameter.
> +	ima_kexec_file.buf = NULL;
> +	ima_kexec_file.size = 0;
> +	ima_kexec_file.read_pos = 0;
> +	ima_kexec_file.count = 0;
> +}
> +
> +static int ima_alloc_kexec_buf(size_t kexec_segment_size)

Call it segment size to avoid the later kexec_segment_size static 
variable in this file.


> +{
> +	if ((kexec_segment_size == 0) ||
> +	    (kexec_segment_size == ULONG_MAX) ||
> +	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {

These tests are already done before ima_alloca_kexec_buf() is called. 
Also, kexec_segment_size cannot be 0.


> +		pr_err("%s: Invalid segment size for kexec: %zu\n",
> +			__func__, kexec_segment_size);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * If kexec load was called before, clear the existing buffer
> +	 * before allocating a new one
> +	 */
> +	if (ima_kexec_file.buf)
> +		ima_clear_kexec_file();
> +

ima_clear_file(&ima_kexec_file);


> +	/* segment size can't change between kexec load and execute */
> +	ima_kexec_file.buf = vmalloc(kexec_segment_size);
> +	if (!ima_kexec_file.buf) {
> +		pr_err("%s: No memory for ima kexec measurement buffer\n",
> +			__func__);
> +		return -ENOMEM;
> +	}
> +
> +	ima_kexec_file.size = kexec_segment_size;
> +	ima_kexec_file.read_pos = 0;
> +	ima_kexec_file.count = sizeof(ima_khdr);	/* reserved space */
> +
> +	memset(&ima_khdr, 0, sizeof(ima_khdr));
> +	ima_khdr.version = 1;

Move this into ima_dump_measurement_list() since it's only used there 
once and getting rid of this file-static variable is a plus.


> +
> +	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;
Don't remove it from here.
>   	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) {
> +		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 */
> +	/*
> +	 * Ensure the kexec buffer is large enough to hold ima_khdr
> +	 */
> +	if (ima_kexec_file.size < sizeof(ima_khdr)) {
> +		pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
> +			__func__);
> +		ima_clear_kexec_file();
> +		return -ENOMEM;
> +	}
>   
> -	memset(&khdr, 0, sizeof(khdr));
> -	khdr.version = 1;
> +	/*
> +	 * If we reach here, then there is enough memory
> +	 * of size kexec_segment_size in ima_kexec_file.buf
> +	 * to copy at least partial IMA log.
> +	 * Make best effort to copy as many IMA measurements
> +	 * as possible.
You can reformat these comments to (at least) 80 columns.

> +	 */
>   	list_for_each_entry_rcu(qe, &ima_measurements, later) {
> -		if (file.count < file.size) {
> -			khdr.count++;
> -			ima_measurements_show(&file, qe);
> +		if (ima_kexec_file.count < ima_kexec_file.size) {
> +			ima_khdr.count++;
> +			ima_measurements_show(&ima_kexec_file, qe);
>   		} else {
> -			ret = -EINVAL;
> +			ret = EFBIG;

Hm, you are not looking for EFBIG after calling this function and the 
overrun could actually also happen in the ima_measurement_show() above 
and go unreported if this is the last element.

if (ima_kexec_file.count < ima_kexec_file.size) {
	ima_khdr.count++;
	ima_measurements_show(&ima_kexec_file, qe);
}

if (ima_kexec_file.count >= ima_kexec_file.size) {
	/* leave ret = 0; caller doesn't need to worry about undersized buffer */
	pr_err("%s: IMA log file is too big for Kexec buf\n",
		__func__);
	break;
}

> +			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;
> +	ima_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);
> +		ima_khdr.version = cpu_to_le16(ima_khdr.version);
> +		ima_khdr.count = cpu_to_le64(ima_khdr.count);
> +		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
>   	}
> -	memcpy(file.buf, &khdr, sizeof(khdr));
> +	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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);
> +	*buffer_size = ima_kexec_file.count;
> +	*buffer = ima_kexec_file.buf;
> +
>   	return ret;
>   }
>   
> @@ -108,13 +161,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_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;

Re: [PATCH v2 3/7] ima: kexec: map source pages containing IMA buffer to image post kexec load

[Stefan Berger]

On 10/5/23 14:25, 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 kimage_map_segment() which takes a kimage pointer, an address,
> and a size.  Ensure that the entire segment is being mapped by comparing
> the given address and size to each segment in the kimage's segment array.
> Collect the source pages that correspond to the given address range,
> allocate an array of pointers to these pages, and map them to a
> contiguous range of virtual addresses.  If the mapping operation is
> successful, the function returns the start of this range.  Otherwise, it
> frees the page pointer array and returns NULL.
>
> Implement kimage_unmap_segment() that takes a pointer to a segment buffer
> and unmaps it using vunmap().
>
> Implement function ima_kexec_post_load(), to be called by IMA after kexec
> loads the new Kernel image.  ima_kexec_post_load() would map the IMA
> buffer allocated during kexec 'load' to a segment in the loaded image.
>
> Finally, move for_each_kimage_entry() macro from kexec_core.c to kexec.h.
>
> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
> ---
>   include/linux/ima.h                |  3 ++
>   include/linux/kexec.h              | 13 ++++++
>   kernel/kexec_core.c                | 73 ++++++++++++++++++++++++++++--
>   security/integrity/ima/ima_kexec.c | 32 +++++++++++++
>   4 files changed, 116 insertions(+), 5 deletions(-)
>
> 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/include/linux/kexec.h b/include/linux/kexec.h
> index 22b5cd24f581..e00b8101b53b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -490,6 +490,15 @@ static inline int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, g
>   static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) { }
>   #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)
> +
> +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;
> @@ -497,6 +506,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 3d578c6fefee..e01156f3c404 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -594,11 +594,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;
> @@ -921,6 +916,74 @@ 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;
> +	struct page **src_pages;
> +	int i, npages;
> +	kimage_entry_t *ptr, entry;
> +	void *vaddr = NULL;
> +
> +	/*
> +	 * Make sure that we are mapping a whole segment.
> +	 */
> +	for (i = 0; i < image->nr_segments; i++) {
> +		if (addr == image->segment[i].mem &&
> +		    size == image->segment[i].memsz) {
> +			break;
> +		}
> +	}

I wonder whether this distrust in the segments or in the passed 
pointer&size are justifyable since you call this function like this:

         ima_kexec_buffer = kimage_map_segment(image,
image->ima_buffer_addr,
image->ima_buffer_size);

and previously kexec_add_buffer() was also called:

         kbuf.buffer = kexec_buffer;
         kbuf.bufsz = kexec_buffer_size;
         kbuf.memsz = kexec_segment_size;
         ret = kexec_add_buffer(&kbuf);
         if (ret) {
                 pr_err("Error passing over kexec measurement buffer.\n");
                 vfree(kexec_buffer);
                 return;
         }

         image->ima_buffer_addr = kbuf.mem;
         image->ima_buffer_size = kexec_segment_size;

So the one segment should be matching these addresses.


> +
> +	if (i == image->nr_segments) {
> +		pr_err("%s: No segment matching [%lx, %lx)\n", __func__,
> +		       addr, eaddr);
> +		return NULL;
> +	}
> +
> +	/*
> +	 * Collect the source pages and map them in a contiguous VA range.
> +	 */
> +	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
> +	src_pages = kmalloc(sizeof(*src_pages) * npages, 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);
On either success or failure kfree(sec_pages); should go right here to 
not have a memory leak.
> +	if (!vaddr) {
> +		pr_err("%s: Could not map imap buffer.\n", __func__);
> +		kfree(src_pages);
> +	}
> +	return vaddr;
> +}
> +
> +void kimage_unmap_segment(void *segment_buffer)
> +{
> +	vunmap(segment_buffer);
> +}
> +
>   struct kexec_load_limit {
>   	/* Mutex protects the limit count. */
>   	struct mutex mutex;
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 2c11bbe6efef..13fbbb90319b 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -12,6 +12,8 @@
>   #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
> @@ -19,6 +21,7 @@ struct seq_file ima_kexec_file;
>   struct ima_kexec_hdr ima_khdr;
>   static void *ima_kexec_buffer;
>   static size_t kexec_segment_size;
> +static bool ima_kexec_update_registered;
>   
>   void ima_clear_kexec_file(void)
>   {
> @@ -221,6 +224,7 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>   	}
>   	memcpy(ima_kexec_buffer, buf, buf_size);
>   out:
> +	kimage_unmap_segment(ima_kexec_buffer);
>   	ima_kexec_buffer = NULL;
>   
>   	if (resume)
> @@ -232,6 +236,34 @@ 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 */
>   
>   /*

Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Tushar Sugandhi]

Thanks a lot Stefan for reviewing this series.
Really appreciate it.

On 10/12/23 17:28, Stefan Berger wrote:
> 
> On 10/5/23 14:25, Tushar Sugandhi wrote:
>> IMA allocates memory and dumps the measurement during kexec soft reboot
>> as a single function call ima_dump_measurement_list().  It gets called
>> during kexec 'load' operation.  It results in the IMA measurements
>> between the window of kexec 'load' and 'execute' getting dropped when the
>> system boots into the new Kernel.  One of the kexec requirements is the
>> segment size cannot change between the 'load' and the 'execute'.
>> Therefore, to address this problem, ima_dump_measurement_list() needs
>> to be refactored to allocate the memory at kexec 'load', and dump the
>> measurements at kexec 'execute'.  The function that allocates the memory
>> should handle the scenario where the kexec load is called multiple times.
>>
>> Refactor ima_dump_measurement_list() to move the memory allocation part
>> to a separate function ima_alloc_kexec_buf() to allocate buffer of size
>> 'kexec_segment_size' at kexec 'load'.  Make the local variables in
>> function ima_dump_measurement_list() global, so that they can be accessed
>> from ima_alloc_kexec_buf().  Make necessary changes to the function
>> ima_add_kexec_buffer() to call the above two functions.
>>
>> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
>>   1 file changed, 93 insertions(+), 33 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 419dc405c831..307e07991865 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -15,61 +15,114 @@
>>   #include "ima.h"
>>   #ifdef CONFIG_IMA_KEXEC
>> +struct seq_file ima_kexec_file;
>> +struct ima_kexec_hdr ima_khdr;
> 
> Since you are only populating the buffer at kexec 'execute' time, you 
> should be able to move this header into the function where it is needed.
> 
Yup, ima_khdr doesn't need to be static. Will fix.
> 
>> +
>> +void ima_clear_kexec_file(void)
>> +{
>> +    vfree(ima_kexec_file.buf);
> I would pass the ima_kexec_file onto this function here as a parameter 
> rather than accessing the file-static variable. I find this better to 
> read once you look at ima_clear_kexec_file() further below and wonder 
> why it doesn't take a parameter.
Agreed. This will make the code more readable.

>> +    ima_kexec_file.buf = NULL;
>> +    ima_kexec_file.size = 0;
>> +    ima_kexec_file.read_pos = 0;
>> +    ima_kexec_file.count = 0;
>> +}
>> +
>> +static int ima_alloc_kexec_buf(size_t kexec_segment_size)
> 
> Call it segment size to avoid the later kexec_segment_size static 
> variable in this file.
> 
> 
Will do.
>> +{
>> +    if ((kexec_segment_size == 0) ||
>> +        (kexec_segment_size == ULONG_MAX) ||
>> +        ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
> 
> These tests are already done before ima_alloca_kexec_buf() is called. 
> Also, kexec_segment_size cannot be 0.
> 
> 
I was being extra cautious here. If ima_alloc_kexec_buf() gets called
from some other place, then this check would be handy. Being said that,
maybe this function is a better place to have this check. I will see
what I can do here to simplify things. Thanks for the feedback.

>> +        pr_err("%s: Invalid segment size for kexec: %zu\n",
>> +            __func__, kexec_segment_size);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /*
>> +     * If kexec load was called before, clear the existing buffer
>> +     * before allocating a new one
>> +     */
>> +    if (ima_kexec_file.buf)
>> +        ima_clear_kexec_file();
>> +
> 
> ima_clear_file(&ima_kexec_file);
> 
> 
Agreed. Will do.

>> +    /* segment size can't change between kexec load and execute */
>> +    ima_kexec_file.buf = vmalloc(kexec_segment_size);
>> +    if (!ima_kexec_file.buf) {
>> +        pr_err("%s: No memory for ima kexec measurement buffer\n",
>> +            __func__);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    ima_kexec_file.size = kexec_segment_size;
>> +    ima_kexec_file.read_pos = 0;
>> +    ima_kexec_file.count = sizeof(ima_khdr);    /* reserved space */
>> +
>> +    memset(&ima_khdr, 0, sizeof(ima_khdr));
>> +    ima_khdr.version = 1;
> 
> Move this into ima_dump_measurement_list() since it's only used there 
> once and getting rid of this file-static variable is a plus.
> 
> 
Yes. This will get rid of an extra static variable.

>> +
>> +    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;
> Don't remove it from here.
Agreed. khdr doesn't need to be static.

>>       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) {
>> +        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 */
>> +    /*
>> +     * Ensure the kexec buffer is large enough to hold ima_khdr
>> +     */
>> +    if (ima_kexec_file.size < sizeof(ima_khdr)) {
>> +        pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
>> +            __func__);
>> +        ima_clear_kexec_file();
>> +        return -ENOMEM;
>> +    }
>> -    memset(&khdr, 0, sizeof(khdr));
>> -    khdr.version = 1;
>> +    /*
>> +     * If we reach here, then there is enough memory
>> +     * of size kexec_segment_size in ima_kexec_file.buf
>> +     * to copy at least partial IMA log.
>> +     * Make best effort to copy as many IMA measurements
>> +     * as possible.
> You can reformat these comments to (at least) 80 columns.
> 
Sure thing.
>> +     */
>>       list_for_each_entry_rcu(qe, &ima_measurements, later) {
>> -        if (file.count < file.size) {
>> -            khdr.count++;
>> -            ima_measurements_show(&file, qe);
>> +        if (ima_kexec_file.count < ima_kexec_file.size) {
>> +            ima_khdr.count++;
>> +            ima_measurements_show(&ima_kexec_file, qe);
>>           } else {
>> -            ret = -EINVAL;
>> +            ret = EFBIG;
> 
> Hm, you are not looking for EFBIG after calling this function and the 
> overrun could actually also happen in the ima_measurement_show() above 
> and go unreported if this is the last element.
> 
> if (ima_kexec_file.count < ima_kexec_file.size) {
>      ima_khdr.count++;
>      ima_measurements_show(&ima_kexec_file, qe);
> }
> 
> if (ima_kexec_file.count >= ima_kexec_file.size) {
>      /* leave ret = 0; caller doesn't need to worry about undersized 
> buffer */
>      pr_err("%s: IMA log file is too big for Kexec buf\n",
>          __func__);
>      break;
> }
> 
This makes it more clean. Thanks. Will do.

~Tushar

>> +            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;
>> +    ima_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);
>> +        ima_khdr.version = cpu_to_le16(ima_khdr.version);
>> +        ima_khdr.count = cpu_to_le64(ima_khdr.count);
>> +        ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
>>       }
>> -    memcpy(file.buf, &khdr, sizeof(khdr));
>> +    memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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);
>> +    *buffer_size = ima_kexec_file.count;
>> +    *buffer = ima_kexec_file.buf;
>> +
>>       return ret;
>>   }
>> @@ -108,13 +161,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_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;

Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute

[Tushar Sugandhi]

On 10/12/23 17:28, Stefan Berger wrote:
> 
> On 10/5/23 14:25, Tushar Sugandhi wrote:
>> In the current IMA implementation, ima_dump_measurement_list() is called
>> during the kexec 'load' operation.  This can result in loss of IMA
>> measurements taken between the 'load' and 'execute' phases when the
>> system goes through Kexec soft reboot to a new Kernel.  The call to the
>> function ima_dump_measurement_list() needs to be moved out of the
>> function ima_add_kexec_buffer() and needs to be called during the kexec
>> 'execute' operation.
>>
>> Implement a function ima_update_kexec_buffer() that is called during
>> kexec 'execute', allowing IMA to update the measurement list with the
>> events between kexec 'load' and 'execute'.  Move the
>> ima_dump_measurement_list() call from ima_add_kexec_buffer() to
>> ima_update_kexec_buffer().  Make ima_kexec_buffer and kexec_segment_size
>> variables global, so that they can be accessed during both kexec 'load'
>> and 'execute'.  Add functions ima_measurements_suspend() and
>> ima_measurements_resume() to set and reset the 'suspend_ima_measurements'
>> variable respectively, to suspend/resume IMA measurements.  Use
>> the existing 'ima_extend_list_mutex' to ensure that the operations are
>> thread-safe.  These function calls will help maintaining the integrity
>> of the IMA log while it is being copied to the new Kernel's buffer.
>> Add a reboot notifier_block 'update_buffer_nb' to ensure
>> the function ima_update_kexec_buffer() gets called during kexec
>> soft-reboot.
>>
>> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima.h       |  2 ++
>>   security/integrity/ima/ima_kexec.c | 58 +++++++++++++++++++++++++-----
>>   security/integrity/ima/ima_queue.c | 18 ++++++++++
>>   3 files changed, 69 insertions(+), 9 deletions(-)
>>
>> 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 307e07991865..2c11bbe6efef 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -17,6 +17,8 @@
>>   #ifdef CONFIG_IMA_KEXEC
>>   struct seq_file ima_kexec_file;
>>   struct ima_kexec_hdr ima_khdr;
>> +static void *ima_kexec_buffer;
>> +static size_t kexec_segment_size;
>>   void ima_clear_kexec_file(void)
>>   {
>> @@ -142,7 +144,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;
>>       /*
>> @@ -167,14 +168,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;
>> @@ -192,6 +185,53 @@ void ima_add_kexec_buffer(struct kimage *image)
>>       pr_debug("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)
>> +{
>> +    void *buf = NULL;
>> +    size_t buf_size;
>> +    bool resume = false;
>> +    int ret;
>> +
>> +    if (!kexec_in_progress) {
>> +        pr_info("%s: No kexec in progress.\n", __func__);
>> +        return NOTIFY_OK;
>> +    }
>> +
>> +    if (!ima_kexec_buffer) {
>> +        pr_err("%s: Kexec buffer not set.\n", __func__);
>> +        return NOTIFY_OK;
>> +    }
>> +
>> +    ima_measurements_suspend();
>> +
>> +    buf_size = ima_get_binary_runtime_size();
> There doesn't seem to be a need to call this function and pass in the 
> binary runtime size into the dump function. You should be able to remove 
> it.
Oh. This was an oversight.
This line is removed in patch 7/7.  It should have been removed here
itself.
Thanks for catching it. Will fix.
>> +    ret = ima_dump_measurement_list(&buf_size, &buf,
>> +                    kexec_segment_size);
>> +
>> +    if (!buf || ret < 0) {
> I don't think this function can (or should) ever return ret >= 0 with 
> buf == NULL.
I will simplify this check. Thanks.

~Tushar
>> +        pr_err("%s: Dump measurements failed. Error:%d\n",
>> +               __func__, ret);
>> +        resume = true;
>> +        goto out;
>> +    }
>> +    memcpy(ima_kexec_buffer, buf, buf_size);
>> +out:
>> +    ima_kexec_buffer = NULL;
>> +
>> +    if (resume)
>> +        ima_measurements_resume();
>> +
>> +    return NOTIFY_OK;
>> +}
>> +struct notifier_block update_buffer_nb = {
>> +    .notifier_call = ima_update_kexec_buffer,
>> +};
>> +
>>   #endif /* IMA_KEXEC */
>>   /*
>> diff --git a/security/integrity/ima/ima_queue.c 
>> b/security/integrity/ima/ima_queue.c
>> index 532da87ce519..9e7d1196006e 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)
>> @@ -147,6 +152,19 @@ static int ima_pcr_extend(struct tpm_digest 
>> *digests_arg, int pcr)
>>           pr_err("Error Communicating to TPM chip, result: %d\n", 
>> result);
>>       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

Re: [PATCH v2 3/7] ima: kexec: map source pages containing IMA buffer to image post kexec load

[Tushar Sugandhi]

On 10/12/23 17:29, Stefan Berger wrote:
> 
> On 10/5/23 14:25, 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 kimage_map_segment() which takes a kimage pointer, an address,
>> and a size.  Ensure that the entire segment is being mapped by comparing
>> the given address and size to each segment in the kimage's segment array.
>> Collect the source pages that correspond to the given address range,
>> allocate an array of pointers to these pages, and map them to a
>> contiguous range of virtual addresses.  If the mapping operation is
>> successful, the function returns the start of this range.  Otherwise, it
>> frees the page pointer array and returns NULL.
>>
>> Implement kimage_unmap_segment() that takes a pointer to a segment buffer
>> and unmaps it using vunmap().
>>
>> Implement function ima_kexec_post_load(), to be called by IMA after kexec
>> loads the new Kernel image.  ima_kexec_post_load() would map the IMA
>> buffer allocated during kexec 'load' to a segment in the loaded image.
>>
>> Finally, move for_each_kimage_entry() macro from kexec_core.c to kexec.h.
>>
>> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
>> ---
>>   include/linux/ima.h                |  3 ++
>>   include/linux/kexec.h              | 13 ++++++
>>   kernel/kexec_core.c                | 73 ++++++++++++++++++++++++++++--
>>   security/integrity/ima/ima_kexec.c | 32 +++++++++++++
>>   4 files changed, 116 insertions(+), 5 deletions(-)
>>
>> 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/include/linux/kexec.h b/include/linux/kexec.h
>> index 22b5cd24f581..e00b8101b53b 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -490,6 +490,15 @@ static inline int 
>> arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, g
>>   static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned 
>> int pages) { }
>>   #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)
>> +
>> +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;
>> @@ -497,6 +506,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 3d578c6fefee..e01156f3c404 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -594,11 +594,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;
>> @@ -921,6 +916,74 @@ 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;
>> +    struct page **src_pages;
>> +    int i, npages;
>> +    kimage_entry_t *ptr, entry;
>> +    void *vaddr = NULL;
>> +
>> +    /*
>> +     * Make sure that we are mapping a whole segment.
>> +     */
>> +    for (i = 0; i < image->nr_segments; i++) {
>> +        if (addr == image->segment[i].mem &&
>> +            size == image->segment[i].memsz) {
>> +            break;
>> +        }
>> +    }
> 
> I wonder whether this distrust in the segments or in the passed 
> pointer&size are justifyable since you call this function like this:
> 
>          ima_kexec_buffer = kimage_map_segment(image,
> image->ima_buffer_addr,
> image->ima_buffer_size);
> 
> and previously kexec_add_buffer() was also called:
> 
>          kbuf.buffer = kexec_buffer;
>          kbuf.bufsz = kexec_buffer_size;
>          kbuf.memsz = kexec_segment_size;
>          ret = kexec_add_buffer(&kbuf);
>          if (ret) {
>                  pr_err("Error passing over kexec measurement buffer.\n");
>                  vfree(kexec_buffer);
>                  return;
>          }
> 
>          image->ima_buffer_addr = kbuf.mem;
>          image->ima_buffer_size = kexec_segment_size;
> 
> So the one segment should be matching these addresses.
> 
> 
Yeh. This check is an overkill at this point.
I agree. I will remove this extra check.
>> +
>> +    if (i == image->nr_segments) {
>> +        pr_err("%s: No segment matching [%lx, %lx)\n", __func__,
>> +               addr, eaddr);
>> +        return NULL;
>> +    }
>> +
>> +    /*
>> +     * Collect the source pages and map them in a contiguous VA range.
>> +     */
>> +    npages = PFN_UP(eaddr) - PFN_DOWN(addr);
>> +    src_pages = kmalloc(sizeof(*src_pages) * npages, 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);
> On either success or failure kfree(sec_pages); should go right here to 
> not have a memory leak.
Will do.

~Tushar
>> +    if (!vaddr) {
>> +        pr_err("%s: Could not map imap buffer.\n", __func__);
>> +        kfree(src_pages);
>> +    }
>> +    return vaddr;
>> +}
>> +
>> +void kimage_unmap_segment(void *segment_buffer)
>> +{
>> +    vunmap(segment_buffer);
>> +}
>> +
>>   struct kexec_load_limit {
>>       /* Mutex protects the limit count. */
>>       struct mutex mutex;
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 2c11bbe6efef..13fbbb90319b 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -12,6 +12,8 @@
>>   #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
>> @@ -19,6 +21,7 @@ struct seq_file ima_kexec_file;
>>   struct ima_kexec_hdr ima_khdr;
>>   static void *ima_kexec_buffer;
>>   static size_t kexec_segment_size;
>> +static bool ima_kexec_update_registered;
>>   void ima_clear_kexec_file(void)
>>   {
>> @@ -221,6 +224,7 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>       }
>>       memcpy(ima_kexec_buffer, buf, buf_size);
>>   out:
>> +    kimage_unmap_segment(ima_kexec_buffer);
>>       ima_kexec_buffer = NULL;
>>       if (resume)
>> @@ -232,6 +236,34 @@ 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 */
>>   /*

Re: [PATCH v2 6/7] ima: make the memory for events between kexec load and exec configurable

[Tushar Sugandhi]

On 10/12/23 17:27, Stefan Berger wrote:
> 
> On 10/5/23 14:26, Tushar Sugandhi wrote:
>> IMA currently allocates half a PAGE_SIZE for the extra events that would
>> be measured between kexec 'load' and 'execute'.  Depending on the IMA
>> policy and the system state, that memory may not be sufficient to hold
>> the extra IMA events measured after kexec 'load'.  The memory
>> requirements vary from system to system and they should be configurable.
>>
>> Define a Kconfig option, IMA_KEXEC_EXTRA_PAGES, to configure the number
>> of extra pages to be allocated for IMA measurements added in the window
>> from kexec 'load' to kexec 'execute'.
>>
>> Update ima_add_kexec_buffer() function to allocate memory based on the
>> Kconfig option value, rather than the currently hardcoded one.
>>
>> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/Kconfig     |  9 +++++++++
>>   security/integrity/ima/ima_kexec.c | 13 ++++++++-----
>>   2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/security/integrity/ima/Kconfig 
>> b/security/integrity/ima/Kconfig
>> index 60a511c6b583..1b55cd2bcb36 100644
>> --- a/security/integrity/ima/Kconfig
>> +++ b/security/integrity/ima/Kconfig
>> @@ -338,3 +338,12 @@ config IMA_DISABLE_HTABLE
>>       default n
>>       help
>>          This option disables htable to allow measurement of duplicate 
>> records.
>> +
>> +config IMA_KEXEC_EXTRA_PAGES
>> +    int
>> +    depends on IMA && IMA_KEXEC
>> +    default 16
>> +    help
>> +      IMA_KEXEC_EXTRA_PAGES determines the number of extra
>> +      pages to be allocated for IMA measurements added in the
>> +      window from kexec 'load' to kexec 'execute'.
> 
> 
> On ppc64 a page is 64kb. I would ask for additional kb here.
> 
> 
Not sure I understand.  Do you mean I should make the default value of
the config IMA_KEXEC_EXTRA_PAGES 64 or something?

In code, I multiply the config value with PAGE_SIZE.  So more memory
would be allocated on ppc64 for given default config value. Could you
please clarify what change you are suggesting here?


+        binary_runtime_size = ima_get_binary_runtime_size() +
+                               sizeof(struct ima_kexec_hdr) +
+                               (CONFIG_IMA_KEXEC_EXTRA_PAGES *
PAGE_SIZE);

~Tushar

>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 13fbbb90319b..6cd5f46a7208 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -150,15 +150,18 @@ 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 in the window from
>> +     * kexec 'load' to kexec 'execute'.
>>        */
>> -    binary_runtime_size = ima_get_binary_runtime_size();
>> +    binary_runtime_size = ima_get_binary_runtime_size() +
>> +                  sizeof(struct ima_kexec_hdr) +
>> +                  (CONFIG_IMA_KEXEC_EXTRA_PAGES * PAGE_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");

Re: [PATCH v2 7/7] ima: record log size at kexec load and execute

[Tushar Sugandhi]

On 10/12/23 17:27, Stefan Berger wrote:
> 
> On 10/5/23 14:26, Tushar Sugandhi wrote:
>> The window between kexec 'load' and 'execute' could be arbitrarily long.
>> Even with the large chunk of memory allocated at kexec 'load', it may
>> run out which would result in missing events in IMA log after the system
>> soft reboots to the new Kernel.  This would result in IMA measurements
>> getting out of sync with the TPM PCR quotes which would result in remote
>> attestation failing for that system.  There is currently no way for the
>> new Kernel to know if the IMA log TPM PCR quote out of sync problem is
>> because of the missing measurements during kexec.
>>
>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
>> measured at kexec 'load' and 'execute' respectively.
>>
>> IMA measures 'boot_aggregate' as the first event when the system boots -
>> either cold boot or kexec soft boot.  In case when the system goes
>> through multiple soft reboots, the number of 'boot_aggregate' events in
>> IMA log corresponds to total number of boots (cold boot plus multiple
>> kexec soft reboots).  With this change, there would be additional
>> 'kexec_load' and 'kexec_execute' events in between the two
>> 'boot_aggregate' events.  In rare cases, when the system runs out of
>> memory during kexec soft reboot, 'kexec_execute' won't be copied since
>> its one of the very last event measured just before kexec soft reboot.
>> The absence of the event 'kexec_execute' in between the two
>> boot_aggregate' events would signal the attestation service that the IMA
>> log on the system is out of sync with TPM PCR quotes and the system needs
>> to be cold booted for the remote attestation to succeed again.
>>
>>
>> @@ -198,6 +208,7 @@ 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;
>>       bool resume = false;
>> @@ -213,9 +224,31 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>           return NOTIFY_OK;
>>       }
>> +    buf_size = ima_get_binary_runtime_size();
>> +    scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
>> +          "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;",
>> +          kexec_segment_size, buf_size);
>> +
>> +    /*
>> +     * This is one of the very last events measured by IMA before kexec
>> +     * soft rebooting into the new Kernal.
>> +     * This event can be used as a marker after the system soft reboots
>> +     * to the new Kernel to check if there was sufficient memory 
>> allocated
>> +     * at kexec 'load' to capture the events measured between the window
>> +     * of kexec 'load' and 'execute'.
>> +     * This event needs to be present in the IMA log, in between the two
>> +     * 'boot_aggregate' events that are logged for the previous boot and
>> +     * the current soft reboot. If it is not present after the system 
>> soft
>> +     * reboots into the new Kernel, it would mean the IMA log is not
>> +     * consistent with the TPM PCR quotes, and the system needs to be
>> +     * cold-booted for the attestation to succeed again.
>> +     */
>> +    ima_measure_critical_data("ima_kexec", "kexec_execute",
>> +                  ima_kexec_event, strlen(ima_kexec_event),
>> +                  false, NULL, 0);
>> +
>>       ima_measurements_suspend();
>> -    buf_size = ima_get_binary_runtime_size();
> 
> This should be removed earlier, in 2/7.
> 
> 
> 
Agreed. Will do.

~Tushar

>>       ret = ima_dump_measurement_list(&buf_size, &buf,
>>                       kexec_segment_size);

Re: [PATCH v2 6/7] ima: make the memory for events between kexec load and exec configurable

[Stefan Berger]

On 10/20/23 16:39, Tushar Sugandhi wrote:

>
>
> On 10/12/23 17:27, Stefan Berger wrote:
>>
>> On 10/5/23 14:26, Tushar Sugandhi wrote:
>>> IMA currently allocates half a PAGE_SIZE for the extra events that 
>>> would
>>> be measured between kexec 'load' and 'execute'.  Depending on the IMA
>>> policy and the system state, that memory may not be sufficient to hold
>>> the extra IMA events measured after kexec 'load'.  The memory
>>> requirements vary from system to system and they should be 
>>> configurable.
>>>
>>> Define a Kconfig option, IMA_KEXEC_EXTRA_PAGES, to configure the number
>>> of extra pages to be allocated for IMA measurements added in the window
>>> from kexec 'load' to kexec 'execute'.
>>>
>>> Update ima_add_kexec_buffer() function to allocate memory based on the
>>> Kconfig option value, rather than the currently hardcoded one.
>>>
>>> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com>
>>> ---
>>>   security/integrity/ima/Kconfig     |  9 +++++++++
>>>   security/integrity/ima/ima_kexec.c | 13 ++++++++-----
>>>   2 files changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/security/integrity/ima/Kconfig 
>>> b/security/integrity/ima/Kconfig
>>> index 60a511c6b583..1b55cd2bcb36 100644
>>> --- a/security/integrity/ima/Kconfig
>>> +++ b/security/integrity/ima/Kconfig
>>> @@ -338,3 +338,12 @@ config IMA_DISABLE_HTABLE
>>>       default n
>>>       help
>>>          This option disables htable to allow measurement of 
>>> duplicate records.
>>> +
>>> +config IMA_KEXEC_EXTRA_PAGES
>>> +    int
>>> +    depends on IMA && IMA_KEXEC
>>> +    default 16
>>> +    help
>>> +      IMA_KEXEC_EXTRA_PAGES determines the number of extra
>>> +      pages to be allocated for IMA measurements added in the
>>> +      window from kexec 'load' to kexec 'execute'.
>>
>>
>> On ppc64 a page is 64kb. I would ask for additional kb here.
>>
>>
> Not sure I understand.  Do you mean I should make the default value of
> the config IMA_KEXEC_EXTRA_PAGES 64 or something?


No, what I mean is you should ask the user for how many extra kilobytes 
(kb) to allocate - not ask for pages.


    Stefan

>
> In code, I multiply the config value with PAGE_SIZE.  So more memory
> would be allocated on ppc64 for given default config value. Could you
> please clarify what change you are suggesting here?
>
>
> +        binary_runtime_size = ima_get_binary_runtime_size() +
> +                               sizeof(struct ima_kexec_hdr) +
> +                               (CONFIG_IMA_KEXEC_EXTRA_PAGES *
> PAGE_SIZE);
>
> ~Tushar
>
>>> diff --git a/security/integrity/ima/ima_kexec.c 
>>> b/security/integrity/ima/ima_kexec.c
>>> index 13fbbb90319b..6cd5f46a7208 100644
>>> --- a/security/integrity/ima/ima_kexec.c
>>> +++ b/security/integrity/ima/ima_kexec.c
>>> @@ -150,15 +150,18 @@ 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 in the window from
>>> +     * kexec 'load' to kexec 'execute'.
>>>        */
>>> -    binary_runtime_size = ima_get_binary_runtime_size();
>>> +    binary_runtime_size = ima_get_binary_runtime_size() +
>>> +                  sizeof(struct ima_kexec_hdr) +
>>> +                  (CONFIG_IMA_KEXEC_EXTRA_PAGES * PAGE_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");

Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Stefan Berger]

On 10/20/23 16:33, Tushar Sugandhi wrote:
> Thanks a lot Stefan for reviewing this series.
> Really appreciate it.


You are welcome.

What may be a bit problematic is the fact that between the time the 
buffer for the flattened IMA log is allocated (kexec 'load') and the 
time it is filled (kexec 'exec') that the log may grow quite a bit. I 
would say that the size of the growths may depend a lot on how people 
are using their system that the additional kilobytes  may or may not be 
enough. So a distro would probably have to specify additional bytes to 
allocate for like the worst case. But how many kilobytes is the worst case?

    Stefan

Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Tushar Sugandhi]

On 10/20/23 14:21, Stefan Berger wrote:
> 
> On 10/20/23 16:33, Tushar Sugandhi wrote:
>> Thanks a lot Stefan for reviewing this series.
>> Really appreciate it.
> 
> 
> You are welcome.
> 
> What may be a bit problematic is the fact that between the time the 
> buffer for the flattened IMA log is allocated (kexec 'load') and the 
> time it is filled (kexec 'exec') that the log may grow quite a bit. I 
> would say that the size of the growths may depend a lot on how people 
> are using their system that the additional kilobytes  may or may not be 
> enough. So a distro would probably have to specify additional bytes to 
> allocate for like the worst case. But how many kilobytes is the worst case?
> 
>     Stefan
Yes.  Its a genuine concern.

The window between kexec 'load' and 'execute' could be arbitrarily long.
(hours, days, months).  And the log can grow quite a bit.  And there is
always a possibility that it will run out of the extra allocated memory-
no matter how much we allocate at load.

We can never know with certainty - how many kilobytes is the worst case?
So I used another approach to address this issue.

I addressed this issue in patch 7/7 of this series[1] by measuring
a marker event ("kexec_execute") just before kexec 'execute'.
Also pasting the code from 7/7 below[1] for reference.

If IMA runs out of the extra allocated memory while copying the events,
this marker event ("kexec_execute") will not be present in the IMA log
when the system boots into the new Kernel.

So the event sequence in IMA log would be as follows:

IMA log
--------
'boot_aggregate' # clean boot
...
... # events before new kexec 'load'
...
'kexec_load'
...
...# arbitrary many more events
...
...
...
'kexec_execute'
#if this 'kexec_execute' event is missing after the
#system kexec soft boots into the new Kernel,
#i.e. between the two boot_aggregate events,
#it can be safely concluded that the IMA log
#ran out of memory in during kexec reboot,
#and now it is out of sync with PCR quotes
#and thus the system needs to be hard rebooted.

'boot_aggregate' # clean boot
...
... # events after kexec soft boot
...

This logic can effectively conclude if IMA log is out of
sync with the PCR quotes.  If it is, then the remote
attestation service/client can take appropriate action
on the system (clean boot) to recover.


Hope this approach makes sense.

~Tushar


[1] [v2,7/7] ima: record log size at kexec load and execute
https://patchwork.kernel.org/project/linux-integrity/patch/20231005182602.634615-8-tusharsu@linux.microsoft.com/ 


diff --git a/security/integrity/ima/ima_kexec.c 
b/security/integrity/ima/ima_kexec.c
index 6cd5f46a7208..0f9c424fe808 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 128
+
  struct seq_file ima_kexec_file;
  struct ima_kexec_hdr ima_khdr;
  static void *ima_kexec_buffer;
@@ -34,6 +36,8 @@  void ima_clear_kexec_file(void)

  static int ima_alloc_kexec_buf(size_t kexec_segment_size)
  {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
+
  	if ((kexec_segment_size == 0) ||
  	    (kexec_segment_size == ULONG_MAX) ||
  	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
@@ -64,6 +68,12 @@  static int ima_alloc_kexec_buf(size_t 
kexec_segment_size)
  	memset(&ima_khdr, 0, sizeof(ima_khdr));
  	ima_khdr.version = 1;

+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;", kexec_segment_size);
+
+	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
+				  strlen(ima_kexec_event), false, NULL, 0);
+
  	return 0;
  }

@@ -198,6 +208,7 @@  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;
  	bool resume = false;
@@ -213,9 +224,31 @@  static int ima_update_kexec_buffer(struct 
notifier_block *self,
  		return NOTIFY_OK;
  	}

+	buf_size = ima_get_binary_runtime_size();
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;",
+		  kexec_segment_size, buf_size);
+
+	/*
+	 * This is one of the very last events measured by IMA before kexec
+	 * soft rebooting into the new Kernal.
+	 * This event can be used as a marker after the system soft reboots
+	 * to the new Kernel to check if there was sufficient memory allocated
+	 * at kexec 'load' to capture the events measured between the window
+	 * of kexec 'load' and 'execute'.
+	 * This event needs to be present in the IMA log, in between the two
+	 * 'boot_aggregate' events that are logged for the previous boot and
+	 * the current soft reboot. If it is not present after the system soft
+	 * reboots into the new Kernel, it would mean the IMA log is not
+	 * consistent with the TPM PCR quotes, and the system needs to be
+	 * cold-booted for the attestation to succeed again.
+	 */
+	ima_measure_critical_data("ima_kexec", "kexec_execute",
+				  ima_kexec_event, strlen(ima_kexec_event),
+				  false, NULL, 0);
+
  	ima_measurements_suspend();

-	buf_size = ima_get_binary_runtime_size();
  	ret = ima_dump_measurement_list(&buf_size, &buf,
  					kexec_segment_size);

Re: [PATCH v2 6/7] ima: make the memory for events between kexec load and exec configurable

[Tushar Sugandhi]

On 10/20/23 14:16, Stefan Berger wrote:
> No, what I mean is you should ask the user for how many extra kilobytes 
> (kb) to allocate - not ask for pages.
> 
> 
>     Stefan
Ok. Will do.
I will align the input config value to the PAGE_SIZE as well.

Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Mimi Zohar]

Hi Tushar,

According to Documentation/process/submitting-patches.rst, the subject
line should be between 70-75 characters.

Perhaps something like "ima: define and call ima_alloc_kexec_buffer()".

On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote:
> IMA allocates memory and dumps the measurement during kexec soft reboot
> as a single function call ima_dump_measurement_list().  It gets called
> during kexec 'load' operation.  It results in the IMA measurements
> between the window of kexec 'load' and 'execute' getting dropped when the
> system boots into the new Kernel.  One of the kexec requirements is the
> segment size cannot change between the 'load' and the 'execute'.
> Therefore, to address this problem, ima_dump_measurement_list() needs
> to be refactored to allocate the memory at kexec 'load', and dump the
> measurements at kexec 'execute'.  The function that allocates the memory
> should handle the scenario where the kexec load is called multiple times

The above pragraph is unnecessary.

> Refactor ima_dump_measurement_list() to move the memory allocation part
> to a separate function ima_alloc_kexec_buf() to allocate buffer of size
> 'kexec_segment_size' at kexec 'load'.  Make the local variables in
> function ima_dump_measurement_list() global, so that they can be accessed
> from ima_alloc_kexec_buf().  Make necessary changes to the function
> ima_add_kexec_buffer() to call the above two functions.

Fix the wording based on the suggested changes below.

> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

Before re-posting this patch set, verify there aren't any
"checkpatch.pl --strict" issues.
After applying each patch, compile the kernel and verify it still
works.

> ---
>  security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
>  1 file changed, 93 insertions(+), 33 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 419dc405c831..307e07991865 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -15,61 +15,114 @@
>  #include "ima.h"
>  
>  #ifdef CONFIG_IMA_KEXEC
> +struct seq_file ima_kexec_file;

Define "ima_kexec_file" as static since it only used in this file.  
Since the variable does not need to be global, is there still a reason
for changing its name?   Minimize code change.

> +struct ima_kexec_hdr ima_khdr;
> +
> +void ima_clear_kexec_file(void)

The opposite of "set" is "clear" (e.g. set_git, clear_bit).  The
opposite of "alloc" would be "free" (e.g. ima_alloc_kexec_buf,
ima_free_kexec_buf)

> +{
> +	vfree(ima_kexec_file.buf);
> +	ima_kexec_file.buf = NULL;
> +	ima_kexec_file.size = 0;
> +	ima_kexec_file.read_pos = 0;
> +	ima_kexec_file.count = 0;
> +}
> +
> +static int ima_alloc_kexec_buf(size_t kexec_segment_size)
> +{
> +	if ((kexec_segment_size == 0) ||
> +	    (kexec_segment_size == ULONG_MAX) ||
> +	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
> +		pr_err("%s: Invalid segment size for kexec: %zu\n",
> +			__func__, kexec_segment_size);
> +		return -EINVAL;
> +	}

Please avoid code duplication.  If moving the test here, then remove it
from its original place.

> +
> +	/*
> +	 * If kexec load was called before, clear the existing buffer
> +	 * before allocating a new one
> +	 */

> +	if (ima_kexec_file.buf)
> +		ima_clear_kexec_file();

Perhaps instead of always freeing the buffer, check and see whether the
size has changed.  If it hasn't changed, then simply return.  If it has
changed, perhaps use realloc().  Possible wording:

"Kexec may be called multiple times.  Free and re-alloc the buffer if
the size changed."

> +
> +	/* segment size can't change between kexec load and execute */
> +	ima_kexec_file.buf = vmalloc(kexec_segment_size);
> +	if (!ima_kexec_file.buf) {
> +		pr_err("%s: No memory for ima kexec measurement buffer\n",
> +			__func__);
> +		return -ENOMEM;
> +	}
> +
> +	ima_kexec_file.size = kexec_segment_size;
> +	ima_kexec_file.read_pos = 0;
> +	ima_kexec_file.count = sizeof(ima_khdr);	/* reserved space */
> +
> +	memset(&ima_khdr, 0, sizeof(ima_khdr));
> +	ima_khdr.version = 1;
> +
> +	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) {
> +		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 */
> +	/*
> +	 * Ensure the kexec buffer is large enough to hold ima_khdr
> +	 */
> +	if (ima_kexec_file.size < sizeof(ima_khdr)) {
> +		pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
> +			__func__);
> +		ima_clear_kexec_file();
> +		return -ENOMEM;
> +	}

Is this necessary?
 
> -	memset(&khdr, 0, sizeof(khdr));
> -	khdr.version = 1;
> +	/*
> +	 * If we reach here, then there is enough memory
> +	 * of size kexec_segment_size in ima_kexec_file.buf
> +	 * to copy at least partial IMA log.
> +	 * Make best effort to copy as many IMA measurements
> +	 * as possible.
> +	 */

Suggestion:

/* Copy as many IMA measurements list records as possible */

>  	list_for_each_entry_rcu(qe, &ima_measurements, later) {
> -		if (file.count < file.size) {
> -			khdr.count++;
> -			ima_measurements_show(&file, qe);
> +		if (ima_kexec_file.count < ima_kexec_file.size) {
> +			ima_khdr.count++;
> +			ima_measurements_show(&ima_kexec_file, qe);
>  		} else {
> -			ret = -EINVAL;
> +			ret = EFBIG;
> +			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;
> +	ima_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);
> +		ima_khdr.version = cpu_to_le16(ima_khdr.version);
> +		ima_khdr.count = cpu_to_le64(ima_khdr.count);
> +		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
>  	}
> -	memcpy(file.buf, &khdr, sizeof(khdr));
> +	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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);
> +	*buffer_size = ima_kexec_file.count;
> +	*buffer = ima_kexec_file.buf;
> +
>  	return ret;
>  }

-- 
thanks,

Mimi

Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Mimi Zohar]

On Thu, 2023-10-26 at 16:16 -0400, Mimi Zohar wrote:
> Hi Tushar,
> 
> According to Documentation/process/submitting-patches.rst, the subject
> line should be between 70-75 characters.
> 
> Perhaps something like "ima: define and call ima_alloc_kexec_buffer()".
> 
> On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote:
> > IMA allocates memory and dumps the measurement during kexec soft reboot
> > as a single function call ima_dump_measurement_list().  It gets called
> > during kexec 'load' operation.  It results in the IMA measurements
> > between the window of kexec 'load' and 'execute' getting dropped when the
> > system boots into the new Kernel.  One of the kexec requirements is the
> > segment size cannot change between the 'load' and the 'execute'.
> > Therefore, to address this problem, ima_dump_measurement_list() needs
> > to be refactored to allocate the memory at kexec 'load', and dump the
> > measurements at kexec 'execute'.  The function that allocates the memory
> > should handle the scenario where the kexec load is called multiple times
> 
> The above pragraph is unnecessary.
> 
> > Refactor ima_dump_measurement_list() to move the memory allocation part
> > to a separate function ima_alloc_kexec_buf() to allocate buffer of size
> > 'kexec_segment_size' at kexec 'load'.  Make the local variables in
> > function ima_dump_measurement_list() global, so that they can be accessed
> > from ima_alloc_kexec_buf().  Make necessary changes to the function
> > ima_add_kexec_buffer() to call the above two functions.
> 
> Fix the wording based on the suggested changes below.
> 
> > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> - Before re-posting this patch set, verify there aren't any
> "checkpatch.pl --strict" issues.
> - After applying each patch, compile the kernel and verify it still
> works.

Doing this will detect whether or not the patch set is bisect safe.

> > ---
> >  security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
> >  1 file changed, 93 insertions(+), 33 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > index 419dc405c831..307e07991865 100644
> > --- a/security/integrity/ima/ima_kexec.c
> > +++ b/security/integrity/ima/ima_kexec.c
> > @@ -15,61 +15,114 @@
> >  #include "ima.h"
> >  
> >  #ifdef CONFIG_IMA_KEXEC
> > +struct seq_file ima_kexec_file;
> 
> Define "ima_kexec_file" as static since it only used in this file.  
> Since the variable does not need to be global, is there still a reason
> for changing its name?   Minimize code change.

Adding "static" would make ima_kexec_file a global static variable. 
Please ignore my comment about reverting the variable name change.

Mimi

> 
> > +struct ima_kexec_hdr ima_khdr;
> > +
> > +void ima_clear_kexec_file(void)
> 
> The opposite of "set" is "clear" (e.g. set_git, clear_bit).  The
> opposite of "alloc" would be "free" (e.g. ima_alloc_kexec_buf,
> ima_free_kexec_buf)
> 
> > +{
> > +	vfree(ima_kexec_file.buf);
> > +	ima_kexec_file.buf = NULL;
> > +	ima_kexec_file.size = 0;
> > +	ima_kexec_file.read_pos = 0;
> > +	ima_kexec_file.count = 0;
> > +}
> > +
> > +static int ima_alloc_kexec_buf(size_t kexec_segment_size)
> > +{
> > +	if ((kexec_segment_size == 0) ||
> > +	    (kexec_segment_size == ULONG_MAX) ||
> > +	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
> > +		pr_err("%s: Invalid segment size for kexec: %zu\n",
> > +			__func__, kexec_segment_size);
> > +		return -EINVAL;
> > +	}
> 
> Please avoid code duplication.  If moving the test here, then remove it
> from its original place.
> 
> > +
> > +	/*
> > +	 * If kexec load was called before, clear the existing buffer
> > +	 * before allocating a new one
> > +	 */
> 
> > +	if (ima_kexec_file.buf)
> > +		ima_clear_kexec_file();
> 
> Perhaps instead of always freeing the buffer, check and see whether the
> size has changed.  If it hasn't changed, then simply return.  If it has
> changed, perhaps use realloc().  Possible wording:
> 
> "Kexec may be called multiple times.  Free and re-alloc the buffer if
> the size changed."
> 
> > +
> > +	/* segment size can't change between kexec load and execute */
> > +	ima_kexec_file.buf = vmalloc(kexec_segment_size);
> > +	if (!ima_kexec_file.buf) {
> > +		pr_err("%s: No memory for ima kexec measurement buffer\n",
> > +			__func__);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ima_kexec_file.size = kexec_segment_size;
> > +	ima_kexec_file.read_pos = 0;
> > +	ima_kexec_file.count = sizeof(ima_khdr);	/* reserved space */
> > +
> > +	memset(&ima_khdr, 0, sizeof(ima_khdr));
> > +	ima_khdr.version = 1;
> > +
> > +	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) {
> > +		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 */
> > +	/*
> > +	 * Ensure the kexec buffer is large enough to hold ima_khdr
> > +	 */
> > +	if (ima_kexec_file.size < sizeof(ima_khdr)) {
> > +		pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
> > +			__func__);
> > +		ima_clear_kexec_file();
> > +		return -ENOMEM;
> > +	}
> 
> Is this necessary?
>  
> > -	memset(&khdr, 0, sizeof(khdr));
> > -	khdr.version = 1;
> > +	/*
> > +	 * If we reach here, then there is enough memory
> > +	 * of size kexec_segment_size in ima_kexec_file.buf
> > +	 * to copy at least partial IMA log.
> > +	 * Make best effort to copy as many IMA measurements
> > +	 * as possible.
> > +	 */
> 
> Suggestion:
> 
> /* Copy as many IMA measurements list records as possible */
> 
> >  	list_for_each_entry_rcu(qe, &ima_measurements, later) {
> > -		if (file.count < file.size) {
> > -			khdr.count++;
> > -			ima_measurements_show(&file, qe);
> > +		if (ima_kexec_file.count < ima_kexec_file.size) {
> > +			ima_khdr.count++;
> > +			ima_measurements_show(&ima_kexec_file, qe);
> >  		} else {
> > -			ret = -EINVAL;
> > +			ret = EFBIG;
> > +			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;
> > +	ima_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);
> > +		ima_khdr.version = cpu_to_le16(ima_khdr.version);
> > +		ima_khdr.count = cpu_to_le64(ima_khdr.count);
> > +		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
> >  	}
> > -	memcpy(file.buf, &khdr, sizeof(khdr));
> > +	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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);
> > +	*buffer_size = ima_kexec_file.count;
> > +	*buffer = ima_kexec_file.buf;
> > +
> >  	return ret;
> >  }
> 

Re: [PATCH v2 0/7] ima: kexec: measure events between kexec load and execute

[Mimi Zohar]

On Fri, 2023-10-27 at 11:18 -0400, Mimi Zohar wrote:
> On Thu, 2023-10-05 at 11:25 -0700, 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'.
> > 
> > Some systems can be configured to call kexec 'load' first, and followed
> > by kexec 'execute' after some time.  (as opposed to calling 'load' and
> > 'execute' in one single kexec command).
> 
> Additional measurements may be introduced by the kexec load itself. 
> Saving the measurement list as close as possible to the reboot is
> beneficial, whether or not the kexec load and kexec execute are
> executed separately.
> 
> > In such scenario, if new IMA
> > measurements are added between kexec 'load' and kexec 'execute', the
> > TPM PCRs are extended with the IMA events between 'load' and 'execute'.
> > But those IMA events are not carried over to the new Kernel after kexec
> > soft reboot.  This results in mismatch between TPM PCR quotes, and the
> > actual IMA measurements list, after the system boots into the new kexec
> > image.  This mismatch results in the remote attestation failing for that
> > system.
> > 
> > This patch series proposes a solution to solve this problem by allocating
> > the necessary buffer at kexec 'load' time, and populating the buffer
> > with the IMA measurements at kexec 'execute' time. 
> 
> How about beginning the paragraph with "To solve this problem allocate
> ... and populate ..."

Does this patch set take into account kexec_calculate_store_digests(),
which is called from kexec_load, and verify_sha256_digest()?
-- 
thanks,

Mimi

Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Tushar Sugandhi]

Thanks a lot for reviewing this patch set Mimi.

On 10/26/23 13:16, Mimi Zohar wrote:
> Hi Tushar,
> 
> According to Documentation/process/submitting-patches.rst, the subject
> line should be between 70-75 characters.
> 
> Perhaps something like "ima: define and call ima_alloc_kexec_buffer()".
> 
Sure thing. I will shorten the subject line. Here and elsewhere.
> On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote:
>> IMA allocates memory and dumps the measurement during kexec soft reboot
>> as a single function call ima_dump_measurement_list().  It gets called
>> during kexec 'load' operation.  It results in the IMA measurements
>> between the window of kexec 'load' and 'execute' getting dropped when the
>> system boots into the new Kernel.  One of the kexec requirements is the
>> segment size cannot change between the 'load' and the 'execute'.
>> Therefore, to address this problem, ima_dump_measurement_list() needs
>> to be refactored to allocate the memory at kexec 'load', and dump the
>> measurements at kexec 'execute'.  The function that allocates the memory
>> should handle the scenario where the kexec load is called multiple times
> 
> The above pragraph is unnecessary.
> 
Sounds good. I will remove the above paragraph.

>> Refactor ima_dump_measurement_list() to move the memory allocation part
>> to a separate function ima_alloc_kexec_buf() to allocate buffer of size
>> 'kexec_segment_size' at kexec 'load'.  Make the local variables in
>> function ima_dump_measurement_list() global, so that they can be accessed
>> from ima_alloc_kexec_buf().  Make necessary changes to the function
>> ima_add_kexec_buffer() to call the above two functions.
> 
> Fix the wording based on the suggested changes below.
> 
Will do.
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> Before re-posting this patch set, verify there aren't any
> "checkpatch.pl --strict" issues.
> After applying each patch, compile the kernel and verify it still
> works.
> >> ---
>>   security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
>>   1 file changed, 93 insertions(+), 33 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>> index 419dc405c831..307e07991865 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -15,61 +15,114 @@
>>   #include "ima.h"
>>   
>>   #ifdef CONFIG_IMA_KEXEC
>> +struct seq_file ima_kexec_file;
> 
> Define "ima_kexec_file" as static since it only used in this file.
> Since the variable does not need to be global, is there still a reason
> for changing its name?   Minimize code change.
> 
>> +struct ima_kexec_hdr ima_khdr;
>> +
>> +void ima_clear_kexec_file(void)
> 
> The opposite of "set" is "clear" (e.g. set_git, clear_bit).  The
> opposite of "alloc" would be "free" (e.g. ima_alloc_kexec_buf,
> ima_free_kexec_buf)
> 
Agreed. Will make it ima_free_kexec_buf.
>> +{
>> +	vfree(ima_kexec_file.buf);
>> +	ima_kexec_file.buf = NULL;
>> +	ima_kexec_file.size = 0;
>> +	ima_kexec_file.read_pos = 0;
>> +	ima_kexec_file.count = 0;
>> +}
>> +
>> +static int ima_alloc_kexec_buf(size_t kexec_segment_size)
>> +{
>> +	if ((kexec_segment_size == 0) ||
>> +	    (kexec_segment_size == ULONG_MAX) ||
>> +	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
>> +		pr_err("%s: Invalid segment size for kexec: %zu\n",
>> +			__func__, kexec_segment_size);
>> +		return -EINVAL;
>> +	}
> 
> Please avoid code duplication.  If moving the test here, then remove it
> from its original place.
> 
Sure. Will do.
>> +
>> +	/*
>> +	 * If kexec load was called before, clear the existing buffer
>> +	 * before allocating a new one
>> +	 */
> 
>> +	if (ima_kexec_file.buf)
>> +		ima_clear_kexec_file();
> 
> Perhaps instead of always freeing the buffer, check and see whether the
> size has changed.  If it hasn't changed, then simply return.  If it has
> changed, perhaps use realloc().  Possible wording:
> 
> "Kexec may be called multiple times.  Free and re-alloc the buffer if
> the size changed."
> 
That's a good optimization. Thanks for the suggestion. Will do.
>> +
>> +	/* segment size can't change between kexec load and execute */
>> +	ima_kexec_file.buf = vmalloc(kexec_segment_size);
>> +	if (!ima_kexec_file.buf) {
>> +		pr_err("%s: No memory for ima kexec measurement buffer\n",
>> +			__func__);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ima_kexec_file.size = kexec_segment_size;
>> +	ima_kexec_file.read_pos = 0;
>> +	ima_kexec_file.count = sizeof(ima_khdr);	/* reserved space */
>> +
>> +	memset(&ima_khdr, 0, sizeof(ima_khdr));
>> +	ima_khdr.version = 1;
>> +
>> +	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) {
>> +		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 */
>> +	/*
>> +	 * Ensure the kexec buffer is large enough to hold ima_khdr
>> +	 */
>> +	if (ima_kexec_file.size < sizeof(ima_khdr)) {
>> +		pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
>> +			__func__);
>> +		ima_clear_kexec_file();
>> +		return -ENOMEM;
>> +	}
> 
> Is this necessary?
>   
Yeh. It's an overkill. Will remove.

>> -	memset(&khdr, 0, sizeof(khdr));
>> -	khdr.version = 1;
>> +	/*
>> +	 * If we reach here, then there is enough memory
>> +	 * of size kexec_segment_size in ima_kexec_file.buf
>> +	 * to copy at least partial IMA log.
>> +	 * Make best effort to copy as many IMA measurements
>> +	 * as possible.
>> +	 */
> 
> Suggestion:
> 
> /* Copy as many IMA measurements list records as possible */
> 
That's a much cleaner comment. Will update.

~Tushar

>>   	list_for_each_entry_rcu(qe, &ima_measurements, later) {
>> -		if (file.count < file.size) {
>> -			khdr.count++;
>> -			ima_measurements_show(&file, qe);
>> +		if (ima_kexec_file.count < ima_kexec_file.size) {
>> +			ima_khdr.count++;
>> +			ima_measurements_show(&ima_kexec_file, qe);
>>   		} else {
>> -			ret = -EINVAL;
>> +			ret = EFBIG;
>> +			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;
>> +	ima_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);
>> +		ima_khdr.version = cpu_to_le16(ima_khdr.version);
>> +		ima_khdr.count = cpu_to_le64(ima_khdr.count);
>> +		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
>>   	}
>> -	memcpy(file.buf, &khdr, sizeof(khdr));
>> +	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_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);
>> +	*buffer_size = ima_kexec_file.count;
>> +	*buffer = ima_kexec_file.buf;
>> +
>>   	return ret;
>>   }
> 

Re: [PATCH v2 1/7] ima: refactor ima_dump_measurement_list to move memory allocation to a separate function

[Tushar Sugandhi]

On 10/26/23 20:25, Mimi Zohar wrote:
> On Thu, 2023-10-26 at 16:16 -0400, Mimi Zohar wrote:
>> Hi Tushar,
>>
>> According to Documentation/process/submitting-patches.rst, the subject
>> line should be between 70-75 characters.
>>
>> Perhaps something like "ima: define and call ima_alloc_kexec_buffer()".
>>
>> On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote:
>>> IMA allocates memory and dumps the measurement during kexec soft reboot
>>> as a single function call ima_dump_measurement_list().  It gets called
>>> during kexec 'load' operation.  It results in the IMA measurements
>>> between the window of kexec 'load' and 'execute' getting dropped when the
>>> system boots into the new Kernel.  One of the kexec requirements is the
>>> segment size cannot change between the 'load' and the 'execute'.
>>> Therefore, to address this problem, ima_dump_measurement_list() needs
>>> to be refactored to allocate the memory at kexec 'load', and dump the
>>> measurements at kexec 'execute'.  The function that allocates the memory
>>> should handle the scenario where the kexec load is called multiple times
>>
>> The above pragraph is unnecessary.
>>
>>> Refactor ima_dump_measurement_list() to move the memory allocation part
>>> to a separate function ima_alloc_kexec_buf() to allocate buffer of size
>>> 'kexec_segment_size' at kexec 'load'.  Make the local variables in
>>> function ima_dump_measurement_list() global, so that they can be accessed
>>> from ima_alloc_kexec_buf().  Make necessary changes to the function
>>> ima_add_kexec_buffer() to call the above two functions.
>>
>> Fix the wording based on the suggested changes below.
>>
>>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>>
>> - Before re-posting this patch set, verify there aren't any
>> "checkpatch.pl --strict" issues.
>> - After applying each patch, compile the kernel and verify it still
>> works.
> 
> Doing this will detect whether or not the patch set is bisect safe.
> 
I usually just do checkpatch.pl <.patch file>.
I didn't know about --strict and it's benefits.
Will do it going forward.


>>> ---
>>>   security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
>>>   1 file changed, 93 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>>> index 419dc405c831..307e07991865 100644
>>> --- a/security/integrity/ima/ima_kexec.c
>>> +++ b/security/integrity/ima/ima_kexec.c
>>> @@ -15,61 +15,114 @@
>>>   #include "ima.h"
>>>   
>>>   #ifdef CONFIG_IMA_KEXEC
>>> +struct seq_file ima_kexec_file;
>>
>> Define "ima_kexec_file" as static since it only used in this file.
>> Since the variable does not need to be global, is there still a reason
>> for changing its name?   Minimize code change.
> 
> Adding "static" would make ima_kexec_file a global static variable.
> Please ignore my comment about reverting the variable name change.
> 
> Mimi
> 
Sure :)

~Tushar
...

Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute

[Tushar Sugandhi]

On 10/27/23 06:08, Mimi Zohar wrote:
> Hi Tushar,
> 
> On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote:
>> In the current IMA implementation, ima_dump_measurement_list() is called
>> during the kexec 'load' operation.  This can result in loss of IMA
>> measurements taken between the 'load' and 'execute' phases when the
>> system goes through Kexec soft reboot to a new Kernel.  The call to the
>> function ima_dump_measurement_list() needs to be moved out of the
>> function ima_add_kexec_buffer() and needs to be called during the kexec
>> 'execute' operation.
>>
>> Implement a function ima_update_kexec_buffer() that is called during
>> kexec 'execute', allowing IMA to update the measurement list with the
>> events between kexec 'load' and 'execute'.  Move the
>> ima_dump_measurement_list() call from ima_add_kexec_buffer() to
>> ima_update_kexec_buffer().  Make ima_kexec_buffer and kexec_segment_size
>> variables global, so that they can be accessed during both kexec 'load'
>> and 'execute'.  Add functions ima_measurements_suspend() and
>> ima_measurements_resume() to set and reset the 'suspend_ima_measurements'
>> variable respectively, to suspend/resume IMA measurements.  Use
>> the existing 'ima_extend_list_mutex' to ensure that the operations are
>> thread-safe.  These function calls will help maintaining the integrity
>> of the IMA log while it is being copied to the new Kernel's buffer.
>> Add a reboot notifier_block 'update_buffer_nb' to ensure
>> the function ima_update_kexec_buffer() gets called during kexec
>> soft-reboot.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> The lengthiness and complexity of the patch description is an
> indication that the patch  needs to be broken up.  Please refer to
> Documentation/process/submitting-patches.rst  for further info.
> 
> In addition, this patch moves the function ima_dump_measurement_list()
> to a new function named ima_update_kexec_buffer(), which is never
> called.   The patch set is thus not bisect safe.
>  > [...]
>> +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);
>> +}
> 
> These function are being defined and called here, but are not enforced
> until a later patch.   It would make more sense to introduce and
> enforce these functions in a single patch with an explanation as to why
> suspend/resume is needed.
> 
> The cover letter describes the problem as "... new IMA measurements are
> added between kexec 'load' and kexec 'execute'".    Please include in
> the patch description the reason for needing suspend/resume, since
> saving the measurement records is done during kexec execute.
> 
Since I am introducing a few new functions in this patch set,
I am having hard time keeping the patches bisect safe and at the same 
time managing the length/complexity of the individual patches in the series.

I will go back and revisit the bisect-safeness of the patches in this 
series again.

Thanks for the feedback, appreciate it.

Re: [PATCH v2 7/7] ima: record log size at kexec load and execute

[Tushar Sugandhi]

On 10/27/23 07:56, Mimi Zohar wrote:
> Hi Tushar,
> 
> On Thu, 2023-10-05 at 11:26 -0700, Tushar Sugandhi wrote:
>> The window between kexec 'load' and 'execute' could be arbitrarily long.
>> Even with the large chunk of memory allocated at kexec 'load', it may
>> run out which would result in missing events in IMA log after the system
>> soft reboots to the new Kernel.  This would result in IMA measurements
>> getting out of sync with the TPM PCR quotes which would result in remote
>> attestation failing for that system.  There is currently no way for the
>> new Kernel to know if the IMA log TPM PCR quote out of sync problem is
>> because of the missing measurements during kexec.
>>
>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
>> measured at kexec 'load' and 'execute' respectively.
>>
>> IMA measures 'boot_aggregate' as the first event when the system boots -
>> either cold boot or kexec soft boot.  In case when the system goes
>> through multiple soft reboots, the number of 'boot_aggregate' events in
>> IMA log corresponds to total number of boots (cold boot plus multiple
>> kexec soft reboots).  With this change, there would be additional
>> 'kexec_load' and 'kexec_execute' events in between the two
>> 'boot_aggregate' events.  In rare cases, when the system runs out of
>> memory during kexec soft reboot, 'kexec_execute' won't be copied since
>> its one of the very last event measured just before kexec soft reboot.
>> The absence of the event 'kexec_execute' in between the two
>> boot_aggregate' events would signal the attestation service that the IMA
>> log on the system is out of sync with TPM PCR quotes and the system needs
>> to be cold booted for the remote attestation to succeed again.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> Adding a new type of critical data is fine.  The patch description
> should describe the reason for it.  Please update the Subject line and
> the patch description accordingly.
> 
Thanks.  I described the reasons in the code comment below, because I 
thought it is important enough to stay with the code to give context. 
But I can move it from the code comment to a patch description.
>> ---
>>   security/integrity/ima/ima_kexec.c | 35 +++++++++++++++++++++++++++++-
>>   1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>> index 6cd5f46a7208..0f9c424fe808 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 128
>> +
>>   struct seq_file ima_kexec_file;
>>   struct ima_kexec_hdr ima_khdr;
>>   static void *ima_kexec_buffer;
>> @@ -34,6 +36,8 @@ void ima_clear_kexec_file(void)
>>   
>>   static int ima_alloc_kexec_buf(size_t kexec_segment_size)
>>   {
>> +	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
>> +
>>   	if ((kexec_segment_size == 0) ||
>>   	    (kexec_segment_size == ULONG_MAX) ||
>>   	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
>> @@ -64,6 +68,12 @@ static int ima_alloc_kexec_buf(size_t kexec_segment_size)
>>   	memset(&ima_khdr, 0, sizeof(ima_khdr));
>>   	ima_khdr.version = 1;
>>   
>> +	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
>> +		  "kexec_segment_size=%lu;", kexec_segment_size);
>> +
>> +	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
>> +				  strlen(ima_kexec_event), false, NULL, 0);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -198,6 +208,7 @@ 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;
>>   	bool resume = false;
>> @@ -213,9 +224,31 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>>   		return NOTIFY_OK;
>>   	}
>>   
>> +	buf_size = ima_get_binary_runtime_size();
>> +	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
>> +		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;",
>> +		  kexec_segment_size, buf_size);
>> +
>> +	/*
>> +	 * This is one of the very last events measured by IMA before kexec
>> +	 * soft rebooting into the new Kernal.
>> +	 * This event can be used as a marker after the system soft reboots
>> +	 * to the new Kernel to check if there was sufficient memory allocated
>> +	 * at kexec 'load' to capture the events measured between the window
>> +	 * of kexec 'load' and 'execute'.
>> +	 * This event needs to be present in the IMA log, in between the two
>> +	 * 'boot_aggregate' events that are logged for the previous boot and
>> +	 * the current soft reboot. If it is not present after the system soft
>> +	 * reboots into the new Kernel, it would mean the IMA log is not
>> +	 * consistent with the TPM PCR quotes, and the system needs to be
>> +	 * cold-booted for the attestation to succeed again.
>> +	 */
> 
> Comments like this don't belong in the code.  Please refer to section
> "8) Commenting" of Documentation/process/coding-style.rst.
Will do.

~Tushar
> 
>> +	ima_measure_critical_data("ima_kexec", "kexec_execute",
>> +				  ima_kexec_event, strlen(ima_kexec_event),
>> +				  false, NULL, 0);
>> +
>>   	ima_measurements_suspend();
>>   
>> -	buf_size = ima_get_binary_runtime_size();
>>   	ret = ima_dump_measurement_list(&buf_size, &buf,
>>   					kexec_segment_size);
>>   

Re: [PATCH v2 0/7] ima: kexec: measure events between kexec load and execute

[Tushar Sugandhi]

On 10/27/23 08:18, Mimi Zohar wrote:
> On Thu, 2023-10-05 at 11:25 -0700, 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'.
>>
>> Some systems can be configured to call kexec 'load' first, and followed
>> by kexec 'execute' after some time.  (as opposed to calling 'load' and
>> 'execute' in one single kexec command).
> 
> Additional measurements may be introduced by the kexec load itself.
> Saving the measurement list as close as possible to the reboot is
> beneficial, whether or not the kexec load and kexec execute are
> executed separately.
> 
True. What I am trying to say here is the longer the window between
'load' and 'execute', greater are the chances of measurements getting
added.
But as long as a single measurement gets added between 'load' and
'execute', it will break the attestation after kexec soft-reboot.

So maybe the above line in the patch description is not necessary.
I will remove.

>> In such scenario, if new IMA
>> measurements are added between kexec 'load' and kexec 'execute', the
>> TPM PCRs are extended with the IMA events between 'load' and 'execute'.
>> But those IMA events are not carried over to the new Kernel after kexec
>> soft reboot.  This results in mismatch between TPM PCR quotes, and the
>> actual IMA measurements list, after the system boots into the new kexec
>> image.  This mismatch results in the remote attestation failing for that
>> system.
>>
>> This patch series proposes a solution to solve this problem by allocating
>> the necessary buffer at kexec 'load' time, and populating the buffer
>> with the IMA measurements at kexec 'execute' time.
> 
> How about beginning the paragraph with "To solve this problem allocate
> ... and populate ..."
> 
Sure. Will do.

~Tushar

Re: [PATCH v2 0/7] ima: kexec: measure events between kexec load and execute

[Tushar Sugandhi]

On 10/27/23 12:51, Mimi Zohar wrote:
> Does this patch set take into account kexec_calculate_store_digests(),
> which is called from kexec_load, and verify_sha256_digest()?
I am not yet sure if my patches will impact the
kexec_calculate_store_digests() and verify_sha256_digest()
functionality.

I will investigate further and get back to you as soon as possible.

Thanks for bringing this up Mimi.

~Tushar

Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute

[Tushar Sugandhi]

On 11/14/23 14:43, Tushar Sugandhi wrote:
> 
> In addition, this patch moves the function ima_dump_measurement_list()
> to a new function named ima_update_kexec_buffer(), which is never
> called.   The patch set is thus not bisect safe.

BTW, ima_update_kexec_buffer() is part of the notifier_block.
which is part of the same patch 2/7.

+struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+};
+

update_buffer_nb is being registered to the reboot notifiers in patch
3/7 of this series.

So ima_update_kexec_buffer() is being called.

+void ima_kexec_post_load(struct kimage *image)
+{
...
...
+
+	if (!ima_kexec_update_registered) {
+		register_reboot_notifier(&update_buffer_nb);
+		ima_kexec_update_registered = true;
+	}
+}

Maybe you meant 'update_buffer_nb' variable needs to be defined and used
in the same patch and not defined in 2/7 and used in 3/7.

Anyways, I think I took care of the majority of the bisect-safe issues 
from V1->V2 of this series.  But maybe I missed a few. I will look at
this with fresh perspective, to see if I missed anything, when I publish
V3 of the series.

Thanks,
Tushar