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

[PATCH v3 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'.

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

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


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

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

 - 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,

 - excluding ima segment while calculating and storing digest in function
   kexec_calculate_store_digests(), since ima segment can be modified
   after the digest is computed during kexec 'load',

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.

V2 of this series is available here[4] for reference.

References:
-----------

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

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

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

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

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


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

Tushar Sugandhi (7):
  ima: define and call ima_alloc_kexec_file_buf
  ima: kexec: move ima log copy from kexec load to execute
  ima: kexec: map IMA buffer source pages to image after kexec load
  kexec: update kexec_file_load syscall to alloc ima buffer after load
  ima: suspend measurements during buffer copy at kexec execute
  ima: configure memory to log events between kexec load and execute
  ima: measure kexec load and exec events as critical data

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

-- 
2.25.1

[PATCH v3 1/7] ima: define and call ima_alloc_kexec_file_buf

[Tushar Sugandhi]

Refactor ima_dump_measurement_list() to move the memory allocation part
to a separate function ima_alloc_kexec_file_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_file_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 | 96 +++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 29 deletions(-)

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

[PATCH v3 2/7] ima: kexec: move ima log copy from kexec load to execute

[Tushar Sugandhi]

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

Implement ima_update_kexec_buffer(), to be called during kexec 'execute'.
Move ima_dump_measurement_list() function call from ima_add_kexec_buffer()
to ima_update_kexec_buffer().  Make the needed variables global for
accessibility during kexec 'load' and 'execute'. Implement and call
ima_measurements_suspend() and ima_measurements_resume() to help ensure
the integrity of the IMA log during copy. Add a reboot notifier_block to
trigger ima_update_kexec_buffer() during kexec soft-reboot.  Exclude ima
segment from calculating and storing digest in function
kexec_calculate_store_digests(), since ima segment can be modified
after the digest is computed during kexec 'load'.

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

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 22b5cd24f581..fd94404acc66 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -366,6 +366,9 @@ struct kimage {
 
 	phys_addr_t ima_buffer_addr;
 	size_t ima_buffer_size;
+
+	unsigned long ima_segment_index;
+	bool is_ima_segment_index_set;
 #endif
 
 	/* Core ELF header buffer */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f989f5f1933b..bf758fd5062c 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -734,6 +734,14 @@ static int kexec_calculate_store_digests(struct kimage *image)
 		if (ksegment->kbuf == pi->purgatory_buf)
 			continue;
 
+		/*
+		 * Skip the segment if ima_segment_index is set and matches
+		 * the current index
+		 */
+		if (image->is_ima_segment_index_set &&
+		    i == image->ima_segment_index)
+			continue;
+
 		ret = crypto_shash_update(desc, ksegment->kbuf,
 					  ksegment->bufsz);
 		if (ret)
diff --git a/security/integrity/ima/ima.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 ae27101d0615..0063d5e7b634 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -16,6 +16,8 @@
 
 #ifdef CONFIG_IMA_KEXEC
 struct seq_file ima_kexec_file;
+static void *ima_kexec_buffer;
+static size_t kexec_segment_size;
 
 void ima_free_kexec_file_buf(struct seq_file *sf)
 {
@@ -120,7 +122,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;
 
 	/*
@@ -145,17 +146,10 @@ 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;
+	image->is_ima_segment_index_set = false;
 	ret = kexec_add_buffer(&kbuf);
 	if (ret) {
 		pr_err("Error passing over kexec measurement buffer.\n");
@@ -166,10 +160,59 @@ void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_buffer_addr = kbuf.mem;
 	image->ima_buffer_size = kexec_segment_size;
 	image->ima_buffer = kexec_buffer;
+	image->ima_segment_index = image->nr_segments - 1;
+	image->is_ima_segment_index_set = true;
 
 	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 = NOTIFY_OK;
+
+	if (!kexec_in_progress) {
+		pr_info("%s: No kexec in progress.\n", __func__);
+		return ret;
+	}
+
+	if (!ima_kexec_buffer) {
+		pr_err("%s: Kexec buffer not set.\n", __func__);
+		return ret;
+	}
+
+	ima_measurements_suspend();
+
+	ret = ima_dump_measurement_list(&buf_size, &buf,
+					kexec_segment_size);
+
+	if (!buf) {
+		pr_err("%s: Dump measurements failed. Error:%d\n",
+		       __func__, ret);
+		resume = true;
+		goto out;
+	}
+	memcpy(ima_kexec_buffer, buf, buf_size);
+out:
+	ima_kexec_buffer = NULL;
+
+	if (resume)
+		ima_measurements_resume();
+
+	return ret;
+}
+
+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..cb9abc02a304 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
  */
 static DEFINE_MUTEX(ima_extend_list_mutex);
 
+/*
+ * Used internally by the kernel to suspend-resume ima measurements.
+ */
+static atomic_t suspend_ima_measurements;
+
 /* lookup up the digest value in the hash table, and return the entry */
 static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 						       int pcr)
@@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 	return result;
 }
 
+void ima_measurements_suspend(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 1);
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
+void ima_measurements_resume(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 0);
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
 /*
  * Add template entry to the measurement list and hash table, and
  * extend the pcr.
-- 
2.25.1

[PATCH v3 3/7] ima: kexec: map IMA buffer source pages to image after kexec load

[Tushar Sugandhi]

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

Additionally, introduce kimage_unmap_segment() for unmapping segments
using vunmap().

Introduce ima_kexec_post_load(), to be invoked by IMA following the kexec
'load' of the new Kernel image. This function will map the IMA buffer,
allocated during kexec 'load', to a segment in the loaded image.

Lastly, relocate the for_each_kimage_entry() macro from kexec_core.c to
kexec.h for broader accessibility.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 include/linux/ima.h                |  3 ++
 include/linux/kexec.h              | 13 +++++++
 kernel/kexec_core.c                | 59 +++++++++++++++++++++++++++---
 security/integrity/ima/ima_kexec.c | 32 ++++++++++++++++
 4 files changed, 102 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 fd94404acc66..eb98aca7f4c7 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -493,6 +493,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;
@@ -500,6 +509,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..26978ad02676 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,60 @@ int kimage_load_segment(struct kimage *image,
 	return result;
 }
 
+void *kimage_map_segment(struct kimage *image,
+			 unsigned long addr, unsigned long size)
+{
+	unsigned long eaddr = addr + size;
+	unsigned long src_page_addr, dest_page_addr;
+	unsigned int npages;
+	struct page **src_pages;
+	int i;
+	kimage_entry_t *ptr, entry;
+	void *vaddr = NULL;
+
+	/*
+	 * Collect the source pages and map them in a contiguous VA range.
+	 */
+	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
+	src_pages = kmalloc_array(npages, sizeof(*src_pages), GFP_KERNEL);
+	if (!src_pages) {
+		pr_err("%s: Could not allocate ima pages array.\n", __func__);
+		return NULL;
+	}
+
+	i = 0;
+	for_each_kimage_entry(image, ptr, entry) {
+		if (entry & IND_DESTINATION)
+			dest_page_addr = entry & PAGE_MASK;
+		else if (entry & IND_SOURCE) {
+			if (dest_page_addr >= addr && dest_page_addr < eaddr) {
+				src_page_addr = entry & PAGE_MASK;
+				src_pages[i++] =
+					virt_to_page(__va(src_page_addr));
+				if (i == npages)
+					break;
+				dest_page_addr += PAGE_SIZE;
+			}
+		}
+	}
+
+	/* Sanity check. */
+	WARN_ON(i < npages);
+
+	vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
+	kfree(src_pages);
+
+	if (!vaddr)
+		pr_err("%s: Could not map imap buffer.\n", __func__);
+
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
 struct kexec_load_limit {
 	/* Mutex protects the limit count. */
 	struct mutex mutex;
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 0063d5e7b634..55bd5362262e 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -12,12 +12,15 @@
 #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
 struct seq_file ima_kexec_file;
 static void *ima_kexec_buffer;
 static size_t kexec_segment_size;
+static bool ima_kexec_update_registered;
 
 void ima_free_kexec_file_buf(struct seq_file *sf)
 {
@@ -201,6 +204,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)
@@ -213,6 +217,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 v3 4/7] kexec: update kexec_file_load syscall to alloc ima buffer after load

[Tushar Sugandhi]

Implement function kimage_file_post_load() to call ima_kexec_post_load()
This ensures the IMA buffer allocated at kexec 'load' is mapped to a
segment in the next loaded Kernel image.

Modify the kexec_file_load() syscall to call kimage_file_post_load() after
the image has been loaded and prepared for kexec.  Call the function
kimage_file_post_load() only for kexec soft reboot scenarios and not
for KEXEC_FILE_ON_CRASH scenarios.

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 bf758fd5062c..ee38799ff1a3 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 v3 5/7] ima: suspend measurements during buffer copy at kexec execute

[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 cb9abc02a304..5946a26a2849 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -195,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 		}
 	}
 
+	/*
+	 * suspend_ima_measurements will be set if the system is
+	 * undergoing kexec soft boot to a new kernel.
+	 * suspending measurements in this short window ensures the
+	 * consistency of the IMA measurement list during copying
+	 * of the kexec buffer.
+	 */
+	if (atomic_read(&suspend_ima_measurements)) {
+		audit_cause = "measurements_suspended";
+		audit_info = 0;
+		goto out;
+	}
+
 	result = ima_add_digest_entry(entry,
 				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
 	if (result < 0) {
-- 
2.25.1

[PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[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_MEMORY_KB, to configure the
extra memory (in kb) 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..8792b7aab768 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_MEMORY_KB
+	int
+	depends on IMA && IMA_KEXEC
+	default 64
+	help
+	  IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be
+	  allocated (in kb) 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 55bd5362262e..063da9c834a0 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -128,15 +128,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_MEMORY_KB * 1024);
+
 	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 v3 7/7] ima: measure kexec load and exec events as critical data

[Tushar Sugandhi]

There could be a potential mismatch between IMA measurements and TPM PCR
quotes caused by the indeterminate interval between kexec 'load' and
'execute'.  Memory allocated at kexec 'load' for IMA log buffer may run
out. It can lead to missing events in the IMA log after a soft reboot to
the new Kernel, resulting in TPM PCR quotes mismatch and remote
attestation failures.

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

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

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

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 063da9c834a0..47da41a66dcc 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;
 static void *ima_kexec_buffer;
 static size_t kexec_segment_size;
@@ -33,6 +35,8 @@ void ima_free_kexec_file_buf(struct seq_file *sf)
 
 static int ima_alloc_kexec_file_buf(size_t segment_size)
 {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
+
 	/*
 	 * kexec 'load' may be called multiple times.
 	 * Free and realloc the buffer only if the segment_size is
@@ -42,7 +46,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
 	    ima_kexec_file.size == segment_size &&
 	    ima_kexec_file.read_pos == 0 &&
 	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
-		return 0;
+		goto out;
 
 	ima_free_kexec_file_buf(&ima_kexec_file);
 
@@ -55,6 +59,13 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
 	ima_kexec_file.read_pos = 0;
 	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
 
+out:
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;", segment_size);
+
+	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
+				  strlen(ima_kexec_event), false, NULL, 0);
+
 	return 0;
 }
 
@@ -179,6 +190,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;
@@ -194,6 +206,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 		return ret;
 	}
 
+	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);
+
+	ima_measure_critical_data("ima_kexec", "kexec_execute",
+				  ima_kexec_event, strlen(ima_kexec_event),
+				  false, NULL, 0);
+
 	ima_measurements_suspend();
 
 	ret = ima_dump_measurement_list(&buf_size, &buf,
-- 
2.25.1

Re: [PATCH v3 1/7] ima: define and call ima_alloc_kexec_file_buf

[Mimi Zohar]

On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
> Refactor ima_dump_measurement_list() to move the memory allocation part
> to a separate function ima_alloc_kexec_file_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_file_buf().  

Only ima_kexec_file is being moved and does not need to be global, but
should be defined as local static.

> 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 | 96 +++++++++++++++++++++---------
>  1 file changed, 67 insertions(+), 29 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 419dc405c831..ae27101d0615 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -15,62 +15,93 @@
>  #include "ima.h"
>  
>  #ifdef CONFIG_IMA_KEXEC
> +struct seq_file ima_kexec_file;

^static.

> +void ima_free_kexec_file_buf(struct seq_file *sf)

^static

> +{
> +	vfree(sf->buf);
> +	sf->buf = NULL;
> +	sf->size = 0;
> +	sf->read_pos = 0;
> +	sf->count = 0;

--  
thanks,

Mimi

Re: [PATCH v3 2/7] ima: kexec: move ima log copy from kexec load to execute

[Mimi Zohar]

Hi Tushar,

On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
> ima_dump_measurement_list() is called from  ima_add_kexec_buffer() during
> kexec 'load', which may result in loss of IMA measurements between kexec
> 'load' and 'execute'.  It needs to be called during kexec 'execute'.
> 
> Implement ima_update_kexec_buffer(), to be called during kexec 'execute'.
> Move ima_dump_measurement_list() function call from ima_add_kexec_buffer()
> to ima_update_kexec_buffer().  Make the needed variables global for
> accessibility during kexec 'load' and 'execute'. Implement and call
> ima_measurements_suspend() and ima_measurements_resume() to help ensure
> the integrity of the IMA log during copy. Add a reboot notifier_block to
> trigger ima_update_kexec_buffer() during kexec soft-reboot.  Exclude ima
> segment from calculating and storing digest in function
> kexec_calculate_store_digests(), since ima segment can be modified
> after the digest is computed during kexec 'load'.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

Wow!   That's quite a bit for a single patch.

This patch moves the ima_dump_measurement_list() call from kexec load
to exec, but doesn't register the reboot notifier in this patch.  I
don't see how it is possible with just the previous and this patch
applied that the measurement list is carried across kexec. 

Please test after applying each patch in the patch set to make sure
that the measurement list is properly carried across kexec.

Additional inline comments below.

> ---
>  include/linux/kexec.h              |  3 ++
>  kernel/kexec_file.c                |  8 ++++
>  security/integrity/ima/ima.h       |  2 +
>  security/integrity/ima/ima_kexec.c | 61 +++++++++++++++++++++++++-----
>  security/integrity/ima/ima_queue.c | 19 ++++++++++
>  5 files changed, 84 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 22b5cd24f581..fd94404acc66 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -366,6 +366,9 @@ struct kimage {
>  
>  	phys_addr_t ima_buffer_addr;
>  	size_t ima_buffer_size;
> +
> +	unsigned long ima_segment_index;
> +	bool is_ima_segment_index_set;
>  #endif
>  
>  	/* Core ELF header buffer */
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index f989f5f1933b..bf758fd5062c 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -734,6 +734,14 @@ static int kexec_calculate_store_digests(struct kimage *image)
>  		if (ksegment->kbuf == pi->purgatory_buf)
>  			continue;
>  
> +		/*
> +		 * Skip the segment if ima_segment_index is set and matches
> +		 * the current index
> +		 */
> +		if (image->is_ima_segment_index_set &&
> +		    i == image->ima_segment_index)
> +			continue;

With this change, the IMA segment is not included in the digest
calculation, nor should it be included in the digest verification. 
However, I'm not seeing the matching code change in the digest
verification.

Please make ignoring the IMA segment a separate patch.

>  		ret = crypto_shash_update(desc, ksegment->kbuf,
>  					  ksegment->bufsz);
>  		if (ret)
> 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 ae27101d0615..0063d5e7b634 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -16,6 +16,8 @@
>  
>  #ifdef CONFIG_IMA_KEXEC
>  struct seq_file ima_kexec_file;
> +static void *ima_kexec_buffer;
> +static size_t kexec_segment_size;
>  
>  void ima_free_kexec_file_buf(struct seq_file *sf)
>  {
> @@ -120,7 +122,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;
>  
>  	/*
> @@ -145,17 +146,10 @@ 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;
> +	image->is_ima_segment_index_set = false;
>  	ret = kexec_add_buffer(&kbuf);
>  	if (ret) {
>  		pr_err("Error passing over kexec measurement buffer.\n");
> @@ -166,10 +160,59 @@ void ima_add_kexec_buffer(struct kimage *image)
>  	image->ima_buffer_addr = kbuf.mem;
>  	image->ima_buffer_size = kexec_segment_size;
>  	image->ima_buffer = kexec_buffer;
> +	image->ima_segment_index = image->nr_segments - 1;
> +	image->is_ima_segment_index_set = true;
>  
>  	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 = NOTIFY_OK;
> +
> +	if (!kexec_in_progress) {
> +		pr_info("%s: No kexec in progress.\n", __func__);
> +		return ret;
> +	}
> +
> +	if (!ima_kexec_buffer) {
> +		pr_err("%s: Kexec buffer not set.\n", __func__);
> +		return ret;
> +	}
> +
> +	ima_measurements_suspend();
> +
> +	ret = ima_dump_measurement_list(&buf_size, &buf,
> +					kexec_segment_size);
> +
> +	if (!buf) {
> +		pr_err("%s: Dump measurements failed. Error:%d\n",
> +		       __func__, ret);
> +		resume = true;
> +		goto out;
> +	}
> +	memcpy(ima_kexec_buffer, buf, buf_size);
> +out:
> +	ima_kexec_buffer = NULL;
> +
> +	if (resume)
> +		ima_measurements_resume();
> +
> +	return ret;
> +}
> +
> +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..cb9abc02a304 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
>   */
>  static DEFINE_MUTEX(ima_extend_list_mutex);
>  
> +/*
> + * Used internally by the kernel to suspend-resume ima measurements.
> + */
> +static atomic_t suspend_ima_measurements;
> +
>  /* lookup up the digest value in the hash table, and return the entry */
>  static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
>  						       int pcr)
> @@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
>  	return result;
>  }
>  
> +void ima_measurements_suspend(void)
> +{
> +	mutex_lock(&ima_extend_list_mutex);
> +	atomic_set(&suspend_ima_measurements, 1);
> +	mutex_unlock(&ima_extend_list_mutex);
> +}
> +
> +void ima_measurements_resume(void)
> +{
> +	mutex_lock(&ima_extend_list_mutex);
> +	atomic_set(&suspend_ima_measurements, 0);
> +	mutex_unlock(&ima_extend_list_mutex);
> +}
> +

Suspending and resuming extending the measurement list should be a
separate patch as well, with its own patch description.

>  /*
>   * Add template entry to the measurement list and hash table, and
>   * extend the pcr.

-- 
thanks,

Mimi

Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[Mimi Zohar]

Hi Tushar,

The Subject line should include the word "extra".   The use of the
extra memory isn't limited to the measurements between the kexec load
and exec.  Additional records could be added as a result of the kexec
load itself.  Let's simplify the title to "ima: make the kexec extra
memory configurable".

Please remove any references to measurements between kexec load and
execute.

On Fri, 2023-12-15 at 17:07 -0800, 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.

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

> Define a Kconfig option, IMA_KEXEC_EXTRA_MEMORY_KB, to configure the
> extra memory (in kb) to be allocated for IMA measurements added 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..8792b7aab768 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_MEMORY_KB
> +	int
> +	depends on IMA && IMA_KEXEC
> +	default 64

Since this isn't optional, the default should remain as a half page. 
Since a page is architecture specific, the default will need to be arch
 specific.

thanks,

Mimih

> +	help
> +	  IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be
> +	  allocated (in kb) 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 55bd5362262e..063da9c834a0 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -128,15 +128,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_MEMORY_KB * 1024);
> +
>  	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 v3 7/7] ima: measure kexec load and exec events as critical data

[Mimi Zohar]

On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
> There could be a potential mismatch between IMA measurements and TPM PCR
> quotes caused by the indeterminate interval between kexec 'load' and
> 'execute'.  Memory allocated at kexec 'load' for IMA log buffer may run
> out. It can lead to missing events in the IMA log after a soft reboot to
> the new Kernel, resulting in TPM PCR quotes mismatch and remote
> attestation failures.
> 
> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be 
> measured as critical data at kexec 'load' and 'execute' respectively.
> 
> These events serve as markers in the IMA log to verify the IMA events are
> captured between kexec 'load' and 'execute' window.  The presence of a
> 'kexec_load' event in between the last two 'boot_aggregate' events in the
> IMA log implies this is a kexec soft reboot, and not a cold-boot. And the
> absence of 'kexec_execute' event after kexec soft reboot implies missing
> events in that window which results in inconsistency with TPM PCR quotes,
> necessitating a cold boot for further successful remote attestation.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  security/integrity/ima/ima_kexec.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 063da9c834a0..47da41a66dcc 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;
>  static void *ima_kexec_buffer;
>  static size_t kexec_segment_size;
> @@ -33,6 +35,8 @@ void ima_free_kexec_file_buf(struct seq_file *sf)
>  
>  static int ima_alloc_kexec_file_buf(size_t segment_size)
>  {
> +	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
> +
>  	/*
>  	 * kexec 'load' may be called multiple times.
>  	 * Free and realloc the buffer only if the segment_size is
> @@ -42,7 +46,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
>  	    ima_kexec_file.size == segment_size &&
>  	    ima_kexec_file.read_pos == 0 &&
>  	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
> -		return 0;
> +		goto out;
>  
>  	ima_free_kexec_file_buf(&ima_kexec_file);
>  
> @@ -55,6 +59,13 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
>  	ima_kexec_file.read_pos = 0;
>  	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
>  
> +out:
> +	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
> +		  "kexec_segment_size=%lu;", segment_size);
> +
> +	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
> +				  strlen(ima_kexec_event), false, NULL, 0);
> +
>  	return 0;
>  }
>  
> @@ -179,6 +190,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;
> @@ -194,6 +206,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>  		return ret;
>  	}
>  
> +	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);
> +
> +	ima_measure_critical_data("ima_kexec", "kexec_execute",
> +				  ima_kexec_event, strlen(ima_kexec_event),
> +				  false, NULL, 0);
> +

Please consider including the number of measurement records as well.

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

-- 
thanks,

Mimi

Re: [PATCH v3 5/7] ima: suspend measurements during buffer copy at kexec execute

[Mimi Zohar]

On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
> 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 cb9abc02a304..5946a26a2849 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -195,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>  		}
>  	}
>  
> +	/*
> +	 * suspend_ima_measurements will be set if the system is
> +	 * undergoing kexec soft boot to a new kernel.
> +	 * suspending measurements in this short window ensures the
> +	 * consistency of the IMA measurement list during copying
> +	 * of the kexec buffer.
> +	 */
> +	if (atomic_read(&suspend_ima_measurements)) {
> +		audit_cause = "measurements_suspended";
> +		audit_info = 0;
> +		goto out;
> +	}
> +
>  	result = ima_add_digest_entry(entry,
>  				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
>  	if (result < 0) {

I assume you meant to include the suspend/resume code in "ima: kexec:
move ima log copy from kexec load to execute"  in this patch.

-- 
thanks,

Mimi

Re: [PATCH v3 1/7] ima: define and call ima_alloc_kexec_file_buf

[Tushar Sugandhi]

Thanks Mimi for the feedback on V3.

Sorry for the late response, I was on vacation during the holidays.

Responses inline.

Happy new year! :)

On 12/20/23 08:13, Mimi Zohar wrote:
> On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
>> Refactor ima_dump_measurement_list() to move the memory allocation part
>> to a separate function ima_alloc_kexec_file_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_file_buf().
> 
> Only ima_kexec_file is being moved and does not need to be global, but
> should be defined as local static.
> 

Will do.
I followed your comment on V2 of this series [1], and didn't make the
change last time. But making it local static scoped to this source file
(ima_kexec.c) makes sense.


[1] From v2 of this series
https://lore.kernel.org/all/2c9e3b71-5416-4336-82f1-cd78e26dd62e@linux.microsoft.com/
 >> 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
 >


>> 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 | 96 +++++++++++++++++++++---------
>>   1 file changed, 67 insertions(+), 29 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>> index 419dc405c831..ae27101d0615 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -15,62 +15,93 @@
>>   #include "ima.h"
>>   
>>   #ifdef CONFIG_IMA_KEXEC
>> +struct seq_file ima_kexec_file;
> 
> ^static.
> 
Will do.

>> +void ima_free_kexec_file_buf(struct seq_file *sf)
> 
> ^static
Will do.

~Tushar
> 
>> +{
>> +	vfree(sf->buf);
>> +	sf->buf = NULL;
>> +	sf->size = 0;
>> +	sf->read_pos = 0;
>> +	sf->count = 0;
> 
> --
> thanks,
> 
> Mimi

Re: [PATCH v3 5/7] ima: suspend measurements during buffer copy at kexec execute

[Tushar Sugandhi]

On 12/20/23 12:44, Mimi Zohar wrote:
> On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
>> 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 cb9abc02a304..5946a26a2849 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -195,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>   		}
>>   	}
>>   
>> +	/*
>> +	 * suspend_ima_measurements will be set if the system is
>> +	 * undergoing kexec soft boot to a new kernel.
>> +	 * suspending measurements in this short window ensures the
>> +	 * consistency of the IMA measurement list during copying
>> +	 * of the kexec buffer.
>> +	 */
>> +	if (atomic_read(&suspend_ima_measurements)) {
>> +		audit_cause = "measurements_suspended";
>> +		audit_info = 0;
>> +		goto out;
>> +	}
>> +
>>   	result = ima_add_digest_entry(entry,
>>   				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
>>   	if (result < 0) {
> 
> I assume you meant to include the suspend/resume code in "ima: kexec:
> move ima log copy from kexec load to execute"  in this patch.
> 

Sure, I can move the suspend/resume code from Patch 2/7 of this series
to this patch (5/7).

Earlier I introduced the suspend/resume functionality in patch 2 because
it was used in the functions in that patch.

But shifting it hear will make the patches cleaner.

~Tushar

Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[Tushar Sugandhi]

On 12/20/23 12:15, Mimi Zohar wrote:
> Hi Tushar,
> 
> The Subject line should include the word "extra".   The use of the
> extra memory isn't limited to the measurements between the kexec load
> and exec.  Additional records could be added as a result of the kexec
> load itself.  Let's simplify the title to "ima: make the kexec extra
> memory configurable".
> 
> Please remove any references to measurements between kexec load and
> execute.
> 
Thanks Mimi. I will make these changes.

> On Fri, 2023-12-15 at 17:07 -0800, 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.
> 
> The extra memory allocated for carrying the IMA measurement list across
> kexec is hardcoded as a half a PAGE.   Make it configurable.
> 
Will do.
>> Define a Kconfig option, IMA_KEXEC_EXTRA_MEMORY_KB, to configure the
>> extra memory (in kb) 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..8792b7aab768 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_MEMORY_KB
>> +	int
>> +	depends on IMA && IMA_KEXEC
>> +	default 64
> 
> Since this isn't optional, the default should remain as a half page.
> Since a page is architecture specific, the default will need to be arch
>   specific.
> 
> thanks,
> 
> Mimih
> 
It was a feedback from Stefan in the V2 of this series to convert it 
from number of PAGES to KB.[1]

But I can revert it to number of pages again.

Also, making the default value as a fraction (1/2 page) feels weird for 
a CONFIG variable.

Is it ok to make the default value as one page rather than half page?

[1]
https://lore.kernel.org/all/15a12e79-2e90-28f7-ffa3-ff470c673099@linux.ibm.com/
 >>> +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

>> +	help
>> +	  IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be
>> +	  allocated (in kb) 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 55bd5362262e..063da9c834a0 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -128,15 +128,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_MEMORY_KB * 1024);
>> +
>>   	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 v3 7/7] ima: measure kexec load and exec events as critical data

[Tushar Sugandhi]

On 12/20/23 12:41, Mimi Zohar wrote:
> On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
>> There could be a potential mismatch between IMA measurements and TPM PCR
>> quotes caused by the indeterminate interval between kexec 'load' and
>> 'execute'.  Memory allocated at kexec 'load' for IMA log buffer may run
>> out. It can lead to missing events in the IMA log after a soft reboot to
>> the new Kernel, resulting in TPM PCR quotes mismatch and remote
>> attestation failures.
>>
>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
>> measured as critical data at kexec 'load' and 'execute' respectively.
>>
>> These events serve as markers in the IMA log to verify the IMA events are
>> captured between kexec 'load' and 'execute' window.  The presence of a
>> 'kexec_load' event in between the last two 'boot_aggregate' events in the
>> IMA log implies this is a kexec soft reboot, and not a cold-boot. And the
>> absence of 'kexec_execute' event after kexec soft reboot implies missing
>> events in that window which results in inconsistency with TPM PCR quotes,
>> necessitating a cold boot for further successful remote attestation.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima_kexec.c | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
>> index 063da9c834a0..47da41a66dcc 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;
>>   static void *ima_kexec_buffer;
>>   static size_t kexec_segment_size;
>> @@ -33,6 +35,8 @@ void ima_free_kexec_file_buf(struct seq_file *sf)
>>   
>>   static int ima_alloc_kexec_file_buf(size_t segment_size)
>>   {
>> +	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
>> +
>>   	/*
>>   	 * kexec 'load' may be called multiple times.
>>   	 * Free and realloc the buffer only if the segment_size is
>> @@ -42,7 +46,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
>>   	    ima_kexec_file.size == segment_size &&
>>   	    ima_kexec_file.read_pos == 0 &&
>>   	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
>> -		return 0;
>> +		goto out;
>>   
>>   	ima_free_kexec_file_buf(&ima_kexec_file);
>>   
>> @@ -55,6 +59,13 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
>>   	ima_kexec_file.read_pos = 0;
>>   	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
>>   
>> +out:
>> +	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
>> +		  "kexec_segment_size=%lu;", segment_size);
>> +
>> +	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
>> +				  strlen(ima_kexec_event), false, NULL, 0);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -179,6 +190,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;
>> @@ -194,6 +206,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>>   		return ret;
>>   	}
>>   
>> +	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);
>> +
>> +	ima_measure_critical_data("ima_kexec", "kexec_execute",
>> +				  ima_kexec_event, strlen(ima_kexec_event),
>> +				  false, NULL, 0);
>> +
> 
> Please consider including the number of measurement records as well.
Will do. I think that would be valuable information.

Per my understanding, I will have to use the function
ima_show_measurements_count() or ima_show_htable_value() to get the
number of measurement records value[1]. So I will have to expose that
function from "ima_fs.c" to "ima_kexec.c". Hope that's ok.

[1] 
https://elixir.bootlin.com/linux/latest/sourcesecurity/integrity/ima/ima_fs.c 


static ssize_t ima_show_measurements_count(struct file *filp,
                                          char __user *buf,
                                          size_t count, loff_t *ppos)
{
         return ima_show_htable_value(buf, count, ppos, &ima_htable.len);

}


~Tushar
> 
>>   	ima_measurements_suspend();
>>   
>>   	ret = ima_dump_measurement_list(&buf_size, &buf,
> 

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

[Mimi Zohar]

On Fri, 2024-01-05 at 12:22 -0800, Tushar Sugandhi wrote:
> >> @@ -194,6 +206,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
> >>              return ret;
> >>      }
> >>   
> >> +    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);
> >> +
> >> +    ima_measure_critical_data("ima_kexec", "kexec_execute",
> >> +                              ima_kexec_event, strlen(ima_kexec_event),
> >> +                              false, NULL, 0);
> >> +
> > 
> > Please consider including the number of measurement records as well.
> Will do. I think that would be valuable information.
> 
> Per my understanding, I will have to use the function
> ima_show_measurements_count() or ima_show_htable_value() to get the
> number of measurement records value[1]. So I will have to expose that
> function from "ima_fs.c" to "ima_kexec.c". Hope that's ok.
> 
> [1] 
> https://elixir.bootlin.com/linux/latest/sourcesecurity/integrity/ima/ima_fs.c 
> 
> 
> static ssize_t ima_show_measurements_count(struct file *filp,
>                                           char __user *buf,
>                                           size_t count, loff_t *ppos)
> {
>          return ima_show_htable_value(buf, count, ppos, &ima_htable.len);

I don't see a need to expose this function.  ima_htable is defined in ima.h. 
You can read the ima_htable.len directly, as ima_show_htable_value does.

-- 
thanks,
 
Mimi
 

Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[Mimi Zohar]

On Fri, 2024-01-05 at 12:20 -0800, Tushar Sugandhi wrote:
> >> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> >> index 60a511c6b583..8792b7aab768 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_MEMORY_KB
> >> +    int
> >> +    depends on IMA && IMA_KEXEC
> >> +    default 64
> > 
> > Since this isn't optional, the default should remain as a half page.
> > Since a page is architecture specific, the default will need to be arch
> >   specific
> > 
> It was a feedback from Stefan in the V2 of this series to convert it 
> from number of PAGES to KB.[1]
> 
> But I can revert it to number of pages again.
> 
> Also, making the default value as a fraction (1/2 page) feels weird for 
> a CONFIG variable.
> 
> Is it ok to make the default value as one page rather than half page?

The point is not whether the extra memory is specified in terms of pages or KB. 
For backwards compatibility the existing default should be the same as
previously.  This means the default needs to be architecture specific.b
 
$ uname -m; getconf PAGESIZE
x86_64
4096
 
$ uname -m; getconf PAGESIZE 
ppc64le
65536

For example:

default 32 if PPC_64K_PAGES
default 2 

-- 
thanks,

Mimi

Re: [PATCH v3 5/7] ima: suspend measurements during buffer copy at kexec execute

[Mimi Zohar]

On Fri, 2024-01-05 at 11:50 -0800, Tushar Sugandhi wrote:
> 
> On 12/20/23 12:44, Mimi Zohar wrote:
> > On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
> >> 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 cb9abc02a304..5946a26a2849 100644
> >> --- a/security/integrity/ima/ima_queue.c
> >> +++ b/security/integrity/ima/ima_queue.c
> >> @@ -195,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
> >>   		}
> >>   	}
> >>   
> >> +	/*
> >> +	 * suspend_ima_measurements will be set if the system is
> >> +	 * undergoing kexec soft boot to a new kernel.
> >> +	 * suspending measurements in this short window ensures the
> >> +	 * consistency of the IMA measurement list during copying
> >> +	 * of the kexec buffer.
> >> +	 */
> >> +	if (atomic_read(&suspend_ima_measurements)) {
> >> +		audit_cause = "measurements_suspended";
> >> +		audit_info = 0;
> >> +		goto out;
> >> +	}
> >> +
> >>   	result = ima_add_digest_entry(entry,
> >>   				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
> >>   	if (result < 0) {
> > 
> > I assume you meant to include the suspend/resume code in "ima: kexec:
> > move ima log copy from kexec load to execute"  in this patch.
> > 
> 
> Sure, I can move the suspend/resume code from Patch 2/7 of this series
> to this patch (5/7).
> 
> Earlier I introduced the suspend/resume functionality in patch 2 because
> it was used in the functions in that patch.
> 
> But shifting it hear will make the patches cleaner.

Just a reminder this isn't the only issued mentioned in 2/7.  Please refer to it
for the other comments (e.g. make not including/verifying the IMA segment hash a
separate patch).

Before reposting, please remember to test after applying each patch in the patch
set to ensure that the measurement list is properly carried across kexec.
-- 
thanks,

Mimi

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

[Tushar Sugandhi]

On 1/7/24 06:24, Mimi Zohar wrote:
> On Fri, 2024-01-05 at 12:22 -0800, Tushar Sugandhi wrote:
>>>> @@ -194,6 +206,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>>>>               return ret;
>>>>       }
>>>>    
>>>> +    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);
>>>> +
>>>> +    ima_measure_critical_data("ima_kexec", "kexec_execute",
>>>> +                              ima_kexec_event, strlen(ima_kexec_event),
>>>> +                              false, NULL, 0);
>>>> +
>>>
>>> Please consider including the number of measurement records as well.
>> Will do. I think that would be valuable information.
>>
>> Per my understanding, I will have to use the function
>> ima_show_measurements_count() or ima_show_htable_value() to get the
>> number of measurement records value[1]. So I will have to expose that
>> function from "ima_fs.c" to "ima_kexec.c". Hope that's ok.
>>
>> [1]
>> https://elixir.bootlin.com/linux/latest/sourcesecurity/integrity/ima/ima_fs.c
>>
>>
>> static ssize_t ima_show_measurements_count(struct file *filp,
>>                                            char __user *buf,
>>                                            size_t count, loff_t *ppos)
>> {
>>           return ima_show_htable_value(buf, count, ppos, &ima_htable.len);
> 
> I don't see a need to expose this function.  ima_htable is defined in ima.h.
> You can read the ima_htable.len directly, as ima_show_htable_value does.
> 
Agreed. Thanks for the pointer.
That's what I concluded too when I was implementing this change.
I will use ima_htable.len directly.

~Tushar

Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[Tushar Sugandhi]

On 1/7/24 09:00, Mimi Zohar wrote:
> On Fri, 2024-01-05 at 12:20 -0800, Tushar Sugandhi wrote:
>>>> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
>>>> index 60a511c6b583..8792b7aab768 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_MEMORY_KB
>>>> +    int
>>>> +    depends on IMA && IMA_KEXEC
>>>> +    default 64
>>>
>>> Since this isn't optional, the default should remain as a half page.
>>> Since a page is architecture specific, the default will need to be arch
>>>    specific
>>>
>> It was a feedback from Stefan in the V2 of this series to convert it
>> from number of PAGES to KB.[1]
>>
>> But I can revert it to number of pages again.
>>
>> Also, making the default value as a fraction (1/2 page) feels weird for
>> a CONFIG variable.
>>
>> Is it ok to make the default value as one page rather than half page?
> 
> The point is not whether the extra memory is specified in terms of pages or KB.
> For backwards compatibility the existing default should be the same as
> previously.  This means the default needs to be architecture specific.b
>   
> $ uname -m; getconf PAGESIZE
> x86_64
> 4096
>   
> $ uname -m; getconf PAGESIZE
> ppc64le
> 65536
> 
> For example:
> 
> default 32 if PPC_64K_PAGES
> default 2
> 
Ok. Thanks for the clarification.


Do we want to support only 64K or 4K as possible PAGE_SIZE values?
I spot checked a few architectures, there are scenarios where PAGE_SIZE
could be 8K, 16K, 128K, 256K etc. And of course mega pages with
PAGE_SIZE IN MBs (details below).

About the unit of the config value (KB v/s num_pages), if we go with
num pages, I am having hard time figuring out how to set the config
value to a float - if we truly want to support 1/2 a page (0.5) as
a default. Majority are bools or ints. I couldn't find any
config value with float which I can refer to.

Being said that, I can think of two ways to handle it:
Option (A) fine tune it to half a page given arch specific page size
config.

Option (B) Keep it simple and make the default extra memory to be
a single page rather than half-a-page.

(A) seems fragile, and I am worried we will not cover all the scenarios.

(B) Even though we are technically breaking the backward compatibility
by changing the default extra memory from half-a-page to a full page,
I don't see it adversely affecting anything else in the IMA/kexec
functionality.

I am leaning towards (B), but please let me know your thoughts.

Sample code for both the options:

Option A:
---------
config IMA_KEXEC_EXTRA_MEMORY_KB
     int
     depends on IMA && IMA_KEXEC
     default 128 if PAGE_SIZE_256KB
     default 32 if PPC_64K_PAGES || PAGE_SIZE_64KB || PARISC_PAGE_SIZE_16KB
     default 16 if PAGE_SIZE_32KB
     default 8 if PAGE_SIZE_16KB || ARC_PAGE_SIZE_16K || 
PARISC_PAGE_SIZE_16KB
     default 4 if PAGE_SIZE_8KB || ARC_PAGE_SIZE_8K
     default 2
	  IMA_KEXEC_EXTRA_MEMORY_KB determines the number of extra
	  memory (in KB) to be allocated for IMA measurements added
	  during kexec soft-reboot.

Option B:
--------
config IMA_KEXEC_EXTRA_PAGES
	int
	depends on IMA && IMA_KEXEC
	default 1
	help
	  IMA_KEXEC_EXTRA_PAGES determines the number of extra
	  pages to be allocated for IMA measurements added during
	  kexec soft-reboot.


Below are a few PAGE_SIZE configs I found for a some architectures.


---------------------------------------------------------------------------------
https://elixir.bootlin.com/linux/v6.7-rc8/source/arch/arc/Kconfig
choice
prompt "MMU Page Size"
default ARC_PAGE_SIZE_8K

config ARC_PAGE_SIZE_8K
bool "8KB"
help
  Choose between 8k vs 16k

config ARC_PAGE_SIZE_16K
bool "16KB"

config ARC_PAGE_SIZE_4K
bool "4KB"
depends on ARC_MMU_V3 || ARC_MMU_V4

endchoice

choice
prompt "MMU Super Page Size"
depends on ISA_ARCV2 && TRANSPARENT_HUGEPAGE
default ARC_HUGEPAGE_2M

config ARC_HUGEPAGE_2M
bool "2MB"

config ARC_HUGEPAGE_16M
bool "16MB"

endchoice
---------------------------------------------------------------------------------------------------------
https://elixir.bootlin.com/linux/v6.7-rc8/source/arch/hexagon/Kconfig
choice
prompt "Kernel page size"
default PAGE_SIZE_4KB
help
  Changes the default page size; use with caution.

config PAGE_SIZE_4KB
bool "4KB"

config PAGE_SIZE_16KB
bool "16KB"

config PAGE_SIZE_64KB
bool "64KB"

config PAGE_SIZE_256KB
bool "256KB"

endchoice
---------------------------------------------------------------------------------------------------------
https://elixir.bootlin.com/linux/v6.7-rc8/source/arch/loongarch/Kconfig

config PAGE_SIZE_4KB
bool

config PAGE_SIZE_16KB
bool

config PAGE_SIZE_64KB
bool

---------------------------------------------------------------------------------------------------------

https://elixir.bootlin.com/linux/v6.7-rc8/source/arch/mips/Kconfig
choice
prompt "Kernel page size"
default PAGE_SIZE_4KB

config PAGE_SIZE_4KB
bool "4kB"
depends on !CPU_LOONGSON2EF && !CPU_LOONGSON64
help
  This option select the standard 4kB Linux page size.  On some
  R3000-family processors this is the only available page size.  Using
  4kB page size will minimize memory consumption and is therefore
  recommended for low memory systems.

config PAGE_SIZE_8KB
bool "8kB"
depends on CPU_CAVIUM_OCTEON
depends on !MIPS_VA_BITS_48
help
  Using 8kB page size will result in higher performance kernel at
  the price of higher memory consumption.  This option is available
  only on cnMIPS processors.  Note that you will need a suitable Linux
  distribution to support this.

config PAGE_SIZE_16KB
bool "16kB"
depends on !CPU_R3000
help
  Using 16kB page size will result in higher performance kernel at
  the price of higher memory consumption.  This option is available on
  all non-R3000 family processors.  Note that you will need a suitable
  Linux distribution to support this.

config PAGE_SIZE_32KB
bool "32kB"
depends on CPU_CAVIUM_OCTEON
depends on !MIPS_VA_BITS_48
help
  Using 32kB page size will result in higher performance kernel at
  the price of higher memory consumption.  This option is available
  only on cnMIPS cores.  Note that you will need a suitable Linux
  distribution to support this.

config PAGE_SIZE_64KB
bool "64kB"
depends on !CPU_R3000
help
  Using 64kB page size will result in higher performance kernel at
  the price of higher memory consumption.  This option is available on
  all non-R3000 family processor.  Not that at the time of this
  writing this option is still high experimental.

endchoice

----------------------------------------------------------------------------
https://elixir.bootlin.com/linux/v6.7-rc8/source/arch/parisc/Kconfig

choice
prompt "Kernel page size"
default PARISC_PAGE_SIZE_4KB

config PARISC_PAGE_SIZE_4KB
bool "4KB"
help
  This lets you select the page size of the kernel.  For best
  performance, a page size of 16KB is recommended.  For best
  compatibility with 32bit applications, a page size of 4KB should be
  selected (the vast majority of 32bit binaries work perfectly fine
  with a larger page size).

  4KB                For best 32bit compatibility
  16KB               For best performance
  64KB               For best performance, might give more overhead.

  If you don't know what to do, choose 4KB.

config PARISC_PAGE_SIZE_16KB
bool "16KB"
depends on PA8X00 && BROKEN && !KFENCE

config PARISC_PAGE_SIZE_64KB
bool "64KB"
depends on PA8X00 && BROKEN && !KFENCE

endchoice

----------------------------------------------------------------------------
https://elixir.bootlin.com/linux/v6.7-rc8/source/arch/sh/Kconfig

config ENTRY_OFFSET
hex
default "0x00001000" if PAGE_SIZE_4KB
default "0x00002000" if PAGE_SIZE_8KB
default "0x00004000" if PAGE_SIZE_16KB
default "0x00010000" if PAGE_SIZE_64KB
default "0x00000000"


~Tushar

Re: [PATCH v3 5/7] ima: suspend measurements during buffer copy at kexec execute

[Tushar Sugandhi]

On 1/11/24 09:30, Mimi Zohar wrote:
> On Fri, 2024-01-05 at 11:50 -0800, Tushar Sugandhi wrote:
>>
>> On 12/20/23 12:44, Mimi Zohar wrote:
>>> On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
>>>> 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 cb9abc02a304..5946a26a2849 100644
>>>> --- a/security/integrity/ima/ima_queue.c
>>>> +++ b/security/integrity/ima/ima_queue.c
>>>> @@ -195,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>>>    		}
>>>>    	}
>>>>    
>>>> +	/*
>>>> +	 * suspend_ima_measurements will be set if the system is
>>>> +	 * undergoing kexec soft boot to a new kernel.
>>>> +	 * suspending measurements in this short window ensures the
>>>> +	 * consistency of the IMA measurement list during copying
>>>> +	 * of the kexec buffer.
>>>> +	 */
>>>> +	if (atomic_read(&suspend_ima_measurements)) {
>>>> +		audit_cause = "measurements_suspended";
>>>> +		audit_info = 0;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>>    	result = ima_add_digest_entry(entry,
>>>>    				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
>>>>    	if (result < 0) {
>>>
>>> I assume you meant to include the suspend/resume code in "ima: kexec:
>>> move ima log copy from kexec load to execute"  in this patch.
>>>
>>
>> Sure, I can move the suspend/resume code from Patch 2/7 of this series
>> to this patch (5/7).
>>
>> Earlier I introduced the suspend/resume functionality in patch 2 because
>> it was used in the functions in that patch.
>>
>> But shifting it hear will make the patches cleaner.
> 
> Just a reminder this isn't the only issued mentioned in 2/7.  Please refer to it
> for the other comments (e.g. make not including/verifying the IMA segment hash a
> separate patch).
> 
> Before reposting, please remember to test after applying each patch in the patch
> set to ensure that the measurement list is properly carried across kexec.

Yes, I had read your responses on patch 2/7.
I have been meaning to respond to you on 2/7, but I kept getting 
distracted by some other work-items on my plate. Really sorry :(

I will respond to your comments on 2/7 by end of the day, and 
incorporate the feedback before reposting.

Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[Stefan Berger]

On 1/11/24 13:13, Tushar Sugandhi wrote:
> 
> 
> On 1/7/24 09:00, Mimi Zohar wrote:
>> On Fri, 2024-01-05 at 12:20 -0800, Tushar Sugandhi wrote:
>>>>> diff --git a/security/integrity/ima/Kconfig 
>>>>> b/security/integrity/ima/Kconfig
>>>>> index 60a511c6b583..8792b7aab768 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_MEMORY_KB
>>>>> +    int
>>>>> +    depends on IMA && IMA_KEXEC
>>>>> +    default 64
>>>>
>>>> Since this isn't optional, the default should remain as a half page.
>>>> Since a page is architecture specific, the default will need to be arch
>>>>    specific
>>>>
>>> It was a feedback from Stefan in the V2 of this series to convert it
>>> from number of PAGES to KB.[1]
>>>
>>> But I can revert it to number of pages again.
>>>
>>> Also, making the default value as a fraction (1/2 page) feels weird for
>>> a CONFIG variable.
>>>
>>> Is it ok to make the default value as one page rather than half page?
>>
>> The point is not whether the extra memory is specified in terms of 
>> pages or KB.
>> For backwards compatibility the existing default should be the same as
>> previously.  This means the default needs to be architecture specific.b
>> $ uname -m; getconf PAGESIZE
>> x86_64
>> 4096
>> $ uname -m; getconf PAGESIZE
>> ppc64le
>> 65536
>>
>> For example:
>>
>> default 32 if PPC_64K_PAGES
>> default 2
>>
> Ok. Thanks for the clarification.
> 
> 
> Do we want to support only 64K or 4K as possible PAGE_SIZE values?
> I spot checked a few architectures, there are scenarios where PAGE_SIZE
> could be 8K, 16K, 128K, 256K etc. And of course mega pages with
> PAGE_SIZE IN MBs (details below).

I would let the user specify the number of kilobytes to reserve and from 
this you can conclude the page numbers:

needed_pages = KBs_TO_RESERVE / PAGE_SIZE
if (KBs_TO_RESERVER % PAGE_SIZE)
     needed_pages++;

    Stefan

Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[Tushar Sugandhi]

On 1/11/24 11:20, Stefan Berger wrote:
> 
> 
> On 1/11/24 13:13, Tushar Sugandhi wrote:
>>
>>
>> On 1/7/24 09:00, Mimi Zohar wrote:
>>> On Fri, 2024-01-05 at 12:20 -0800, Tushar Sugandhi wrote:
>>>>>> diff --git a/security/integrity/ima/Kconfig 
>>>>>> b/security/integrity/ima/Kconfig
>>>>>> index 60a511c6b583..8792b7aab768 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_MEMORY_KB
>>>>>> +    int
>>>>>> +    depends on IMA && IMA_KEXEC
>>>>>> +    default 64
>>>>>
>>>>> Since this isn't optional, the default should remain as a half page.
>>>>> Since a page is architecture specific, the default will need to be 
>>>>> arch
>>>>>    specific
>>>>>
>>>> It was a feedback from Stefan in the V2 of this series to convert it
>>>> from number of PAGES to KB.[1]
>>>>
>>>> But I can revert it to number of pages again.
>>>>
>>>> Also, making the default value as a fraction (1/2 page) feels weird for
>>>> a CONFIG variable.
>>>>
>>>> Is it ok to make the default value as one page rather than half page?
>>>
>>> The point is not whether the extra memory is specified in terms of 
>>> pages or KB.
>>> For backwards compatibility the existing default should be the same as
>>> previously.  This means the default needs to be architecture specific.b
>>> $ uname -m; getconf PAGESIZE
>>> x86_64
>>> 4096
>>> $ uname -m; getconf PAGESIZE
>>> ppc64le
>>> 65536
>>>
>>> For example:
>>>
>>> default 32 if PPC_64K_PAGES
>>> default 2
>>>
>> Ok. Thanks for the clarification.
>>
>>
>> Do we want to support only 64K or 4K as possible PAGE_SIZE values?
>> I spot checked a few architectures, there are scenarios where PAGE_SIZE
>> could be 8K, 16K, 128K, 256K etc. And of course mega pages with
>> PAGE_SIZE IN MBs (details below).
> 
> I would let the user specify the number of kilobytes to reserve and from 
> this you can conclude the page numbers:
> 
> needed_pages = KBs_TO_RESERVE / PAGE_SIZE
> if (KBs_TO_RESERVER % PAGE_SIZE)
>      needed_pages++;
> 
>     Stefan
Thanks Stefan.

But the issue here is about the default value,
not the user specified value.

Mimi is suggesting to keep the default value half-a-page,
to maintain backwards compatibility.

If we go with the KBs approach -
half-a-page translates to different KBs on different architectures.
And setting the right default value in KBs which would translate to
the desired half-a-page, on a given arch, inside the Kconfig seems
fragile (as I mentioned in the context of Option A in my previous
response.

And if we go with num_pages approach -
putting a fractional value (0.5) as a default in Kconfig seems to be non
trivial too.

Translating num_pages to KBs is trivial in code, but I think its
orthogonal to this conversation, since its about setting the desired 
arch specific default value in Kconfig.


Option A:
---------
config IMA_KEXEC_EXTRA_MEMORY_KB
     int
     depends on IMA && IMA_KEXEC
     default 128 if PAGE_SIZE_256KB
     default 32 if PPC_64K_PAGES || PAGE_SIZE_64KB || PARISC_PAGE_SIZE_16KB
     default 16 if PAGE_SIZE_32KB
     default 8 if PAGE_SIZE_16KB || ARC_PAGE_SIZE_16K || 
PARISC_PAGE_SIZE_16KB
     default 4 if PAGE_SIZE_8KB || ARC_PAGE_SIZE_8K
     default 2
       IMA_KEXEC_EXTRA_MEMORY_KB determines the number of extra
       memory (in KB) to be allocated for IMA measurements added
       during kexec soft-reboot.

Option B:
--------
config IMA_KEXEC_EXTRA_PAGES
     int
     depends on IMA && IMA_KEXEC
     default 1
     help
       IMA_KEXEC_EXTRA_PAGES determines the number of extra
       pages to be allocated for IMA measurements added during
       kexec soft-reboot.

~Tushar

Re: [PATCH v3 2/7] ima: kexec: move ima log copy from kexec load to execute

[Tushar Sugandhi]

Apologies for the late response on this particular patch (v3 2/7) Mimi.
I was on vacation in December.
I was meaning to respond to this one when I came back, but I was caught 
in between other work items last few days. Sorry if it caused any 
confusion.

Responses below.

On 12/20/23 11:02, Mimi Zohar wrote:
> Hi Tushar,
> 
> On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
>> ima_dump_measurement_list() is called from  ima_add_kexec_buffer() during
>> kexec 'load', which may result in loss of IMA measurements between kexec
>> 'load' and 'execute'.  It needs to be called during kexec 'execute'.
>>
>> Implement ima_update_kexec_buffer(), to be called during kexec 'execute'.
>> Move ima_dump_measurement_list() function call from ima_add_kexec_buffer()
>> to ima_update_kexec_buffer().  Make the needed variables global for
>> accessibility during kexec 'load' and 'execute'. Implement and call
>> ima_measurements_suspend() and ima_measurements_resume() to help ensure
>> the integrity of the IMA log during copy. Add a reboot notifier_block to
>> trigger ima_update_kexec_buffer() during kexec soft-reboot.  Exclude ima
>> segment from calculating and storing digest in function
>> kexec_calculate_store_digests(), since ima segment can be modified
>> after the digest is computed during kexec 'load'.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> Wow!   That's quite a bit for a single patch.
> 
> This patch moves the ima_dump_measurement_list() call from kexec load
> to exec, but doesn't register the reboot notifier in this patch.  I
> don't see how it is possible with just the previous and this patch
> applied that the measurement list is carried across kexec.
Ah. That's a good catch.
I was only checking if I can boot into the Kernel for testing 
bisect-safe readiness for each patch.  I will ensure the move of 
ima_dump_measurement_list() and registering the reboot notifier at 
execute stays an atomic operation in a single patch.


> 
> Please test after applying each patch in the patch set to make sure
> that the measurement list is properly carried across kexec.
> 
Yup. I was only checking if I can boot into the Kernel after each patch.
My bad. :(

Going forward, I will check each patch for the measurement list carry 
over after kexec.

> Additional inline comments below.
> 
>> ---
>>   include/linux/kexec.h              |  3 ++
>>   kernel/kexec_file.c                |  8 ++++
>>   security/integrity/ima/ima.h       |  2 +
>>   security/integrity/ima/ima_kexec.c | 61 +++++++++++++++++++++++++-----
>>   security/integrity/ima/ima_queue.c | 19 ++++++++++
>>   5 files changed, 84 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 22b5cd24f581..fd94404acc66 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -366,6 +366,9 @@ struct kimage {
>>   
>>   	phys_addr_t ima_buffer_addr;
>>   	size_t ima_buffer_size;
>> +
>> +	unsigned long ima_segment_index;
>> +	bool is_ima_segment_index_set;
>>   #endif
>>   
>>   	/* Core ELF header buffer */
>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
>> index f989f5f1933b..bf758fd5062c 100644
>> --- a/kernel/kexec_file.c
>> +++ b/kernel/kexec_file.c
>> @@ -734,6 +734,14 @@ static int kexec_calculate_store_digests(struct kimage *image)
>>   		if (ksegment->kbuf == pi->purgatory_buf)
>>   			continue;
>>   
>> +		/*
>> +		 * Skip the segment if ima_segment_index is set and matches
>> +		 * the current index
>> +		 */
>> +		if (image->is_ima_segment_index_set &&
>> +		    i == image->ima_segment_index)
>> +			continue;
> 
> With this change, the IMA segment is not included in the digest
> calculation, nor should it be included in the digest verification.
> However, I'm not seeing the matching code change in the digest
> verification.
> 
Fair question.

But I don't think anything else needs to be done here.

The way kexec_calculate_store_digests() and verify_sha256_digest()
are implemented, it already skips verification of the segments if
the segment is not part of 'purgatory_sha_regions'.

In kexec_calculate_store_digests(), my change is to 'continue' when the
segment is the IMA segment when the function is going through all the
segments in a for loop [1].

Therefore in kexec_calculate_store_digests() -
  - crypto_shash_update() is not called for IMA segment [1].
  - sha_regions[j] is not updated with IMA segment  [1].
  - This 'sha_regions' variable later becomes 'purgatory_sha_regions'
    in kexec_calculate_store_digests  [1].
  - and verify_sha256_digest() only verifies 'purgatory_sha_regions'[2].

  Since IMA segment is not part of the 'purgatory_sha_regions', it is
  not included in the verification as part of verify_sha256_digest().

I have pasted the relevant code below for quick reference [1][2].

> Please make ignoring the IMA segment a separate patch.
> 
Sure. Will do.

>>   		ret = crypto_shash_update(desc, ksegment->kbuf,
>>   					  ksegment->bufsz);
>>   		if (ret)
...
...
...
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index c29db699c996..49a6047dd8eb 100644
> 
> Suspending and resuming extending the measurement list should be a
> separate patch as well, with its own patch description.
> 
Sure. Will do.
>>   /*
>>    * Add template entry to the measurement list and hash table, and
>>    * extend the pcr.
> 
----------------------------------------------------------------------------------
Reference code in the context of kexec_calculate_store_digests()
and verify_sha256_digest() conversation above.
----------------------------------------------------------------------------------
[1]
https://lore.kernel.org/all/20231216010729.2904751-3-tusharsu@linux.microsoft.com/
/* Calculate and store the digest of segments */
static int kexec_calculate_store_digests(struct kimage *image)
{
...
...

      for (j = i = 0; i < image->nr_segments; i++) {
            struct kexec_segment *ksegment;

            ksegment = &image->segment[i];
            /*
             * Skip purgatory as it will be modified once we put digest
             * info in purgatory.
             */
            if (ksegment->kbuf == pi->purgatory_buf)
                  continue;

+           /*
+            * Skip the segment if ima_segment_index is set and matches
+            * the current index
+            */
+           if (image->is_ima_segment_index_set &&
+               i == image->ima_segment_index)
+                 continue;
+
            ret = crypto_shash_update(desc, ksegment->kbuf,
                                ksegment->bufsz);
            if (ret)
                  break;

            /*
             * Assume rest of the buffer is filled with zero and
             * update digest accordingly.
             */
            nullsz = ksegment->memsz - ksegment->bufsz;
            while (nullsz) {
                  unsigned long bytes = nullsz;

                  if (bytes > zero_buf_sz)
                        bytes = zero_buf_sz;
                  ret = crypto_shash_update(desc, zero_buf, bytes);
                  if (ret)
                        break;
                  nullsz -= bytes;
            }

            if (ret)
                  break;

            sha_regions[j].start = ksegment->mem;
            sha_regions[j].len = ksegment->memsz;
            j++;
      }

      if (!ret) {
            ret = crypto_shash_final(desc, digest);
            if (ret)
                  goto out_free_digest;
            ret = kexec_purgatory_get_set_symbol(image, 
"purgatory_sha_regions",
                                         sha_regions, sha_region_sz, 0);
            if (ret)
                  goto out_free_digest;

            ret = kexec_purgatory_get_set_symbol(image, 
"purgatory_sha256_digest",
                                         digest, SHA256_DIGEST_SIZE, 0);
            if (ret)
                  goto out_free_digest;
      }

out_free_digest:
      kfree(digest);
out_free_sha_regions:
      vfree(sha_regions);
out_free_desc:
      kfree(desc);
out_free_tfm:
      kfree(tfm);
out:
      return ret;
}
---------------------------------------------------------------------------
[2] 
https://elixir.bootlin.com/linux/latest/source/arch/x86/purgatory/purgatory.c#L24

u8 purgatory_sha256_digest[SHA256_DIGEST_SIZE] 
__section(".kexec-purgatory");

struct kexec_sha_region purgatory_sha_regions[KEXEC_SEGMENT_MAX] 
__section(".kexec-purgatory");

static int verify_sha256_digest(void)
{
      struct kexec_sha_region *ptr, *end;
      u8 digest[SHA256_DIGEST_SIZE];
      struct sha256_state sctx;

      sha256_init(&sctx);
      end = purgatory_sha_regions + ARRAY_SIZE(purgatory_sha_regions);

      for (ptr = purgatory_sha_regions; ptr < end; ptr++)
            sha256_update(&sctx, (uint8_t *)(ptr->start), ptr->len);

      sha256_final(&sctx, digest);

      if (memcmp(digest, purgatory_sha256_digest, sizeof(digest)))
            return 1;

      return 0;
}

Thanks,
Tushar

Re: [PATCH v3 2/7] ima: kexec: move ima log copy from kexec load to execute

[Mimi Zohar]

Hi Tushar,

> > This patch moves the ima_dump_measurement_list() call from kexec load
> > to exec, but doesn't register the reboot notifier in this patch.  I
> > don't see how it is possible with just the previous and this patch
> > applied that the measurement list is carried across kexec.
> Ah. That's a good catch.
> I was only checking if I can boot into the Kernel for testing 
> bisect-safe readiness for each patch.  I will ensure the move of 
> ima_dump_measurement_list() and registering the reboot notifier at 
> execute stays an atomic operation in a single patch.

Thanks!

> >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> >> index f989f5f1933b..bf758fd5062c 100644
> >> --- a/kernel/kexec_file.c
> >> +++ b/kernel/kexec_file.c
> >> @@ -734,6 +734,14 @@ static int kexec_calculate_store_digests(struct kimage *image)
> >>   		if (ksegment->kbuf == pi->purgatory_buf)
> >>   			continue;
> >>   
> >> +		/*
> >> +		 * Skip the segment if ima_segment_index is set and matches
> >> +		 * the current index
> >> +		 */
> >> +		if (image->is_ima_segment_index_set &&
> >> +		    i == image->ima_segment_index)
> >> +			continue;
> > 
> > With this change, the IMA segment is not included in the digest
> > calculation, nor should it be included in the digest verification.
> > However, I'm not seeing the matching code change in the digest
> > verification.
> > 
> Fair question.
> 
> But I don't think anything else needs to be done here.
> 
> The way kexec_calculate_store_digests() and verify_sha256_digest()
> are implemented, it already skips verification of the segments if
> the segment is not part of 'purgatory_sha_regions'.
> 
> In kexec_calculate_store_digests(), my change is to 'continue' when the
> segment is the IMA segment when the function is going through all the
> segments in a for loop [1].
> 
> Therefore in kexec_calculate_store_digests() -
>   - crypto_shash_update() is not called for IMA segment [1].
>   - sha_regions[j] is not updated with IMA segment  [1].
>   - This 'sha_regions' variable later becomes 'purgatory_sha_regions'
>     in kexec_calculate_store_digests  [1].
>   - and verify_sha256_digest() only verifies 'purgatory_sha_regions'[2].
> 
>   Since IMA segment is not part of the 'purgatory_sha_regions', it is
>   not included in the verification as part of verify_sha256_digest().
> 
> > Please make ignoring the IMA segment a separate patch.
> > 
> Sure. Will do.

Thank you for the explanation.  Please include in the patch description a
statement about the "sha_regions" not including the IMA segment, so nothing is
needed on the verify side.

> 
> >>   		ret = crypto_shash_update(desc, ksegment->kbuf,
> >>   					  ksegment->bufsz);
> >>   		if (ret)
> ...
> ...
> ...
> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >> index c29db699c996..49a6047dd8eb 100644
> > 
> > Suspending and resuming extending the measurement list should be a
> > separate patch as well, with its own patch description.
> > 
> Sure. Will do.

Thanks!

Mimi

Re: [PATCH v3 2/7] ima: kexec: move ima log copy from kexec load to execute

[Tushar Sugandhi]

On 1/12/24 09:06, Mimi Zohar wrote:
> 
>>>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
>>>> index f989f5f1933b..bf758fd5062c 100644
>>>> --- a/kernel/kexec_file.c
>>>> +++ b/kernel/kexec_file.c
>>>> @@ -734,6 +734,14 @@ static int kexec_calculate_store_digests(struct kimage *image)
>>>>    		if (ksegment->kbuf == pi->purgatory_buf)
>>>>    			continue;
>>>>    
>>>> +		/*
>>>> +		 * Skip the segment if ima_segment_index is set and matches
>>>> +		 * the current index
>>>> +		 */
>>>> +		if (image->is_ima_segment_index_set &&
>>>> +		    i == image->ima_segment_index)
>>>> +			continue;
>>> With this change, the IMA segment is not included in the digest
>>> calculation, nor should it be included in the digest verification.
>>> However, I'm not seeing the matching code change in the digest
>>> verification.
>>>
>> Fair question.
>>
>> But I don't think anything else needs to be done here.
>>
>> The way kexec_calculate_store_digests() and verify_sha256_digest()
>> are implemented, it already skips verification of the segments if
>> the segment is not part of 'purgatory_sha_regions'.
>>
>> In kexec_calculate_store_digests(), my change is to 'continue' when the
>> segment is the IMA segment when the function is going through all the
>> segments in a for loop [1].
>>
>> Therefore in kexec_calculate_store_digests() -
>>    - crypto_shash_update() is not called for IMA segment [1].
>>    - sha_regions[j] is not updated with IMA segment  [1].
>>    - This 'sha_regions' variable later becomes 'purgatory_sha_regions'
>>      in kexec_calculate_store_digests  [1].
>>    - and verify_sha256_digest() only verifies 'purgatory_sha_regions'[2].
>>
>>    Since IMA segment is not part of the 'purgatory_sha_regions', it is
>>    not included in the verification as part of verify_sha256_digest().
>>
>>> Please make ignoring the IMA segment a separate patch.
>>>
>> Sure. Will do.
> Thank you for the explanation.  Please include in the patch description a
> statement about the "sha_regions" not including the IMA segment, so nothing is
> needed on the verify side.
Definitely.  Will do.

Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[Mimi Zohar]

On Thu, 2024-01-11 at 12:52 -0800, Tushar Sugandhi wrote:
[...]
> If we go with the KBs approach -
> 
> half-a-page translates to different KBs on different architectures.
> And setting the right default value in KBs which would translate to
> the desired half-a-page, on a given arch, inside the Kconfig seems
> fragile (as I mentioned in the context of Option A in my previous
> response.

How about setting the default value to 0, indicating not to change the current
half page default.  Any other value would be KBs, as Stefan suggested.

-- 
thanks,

Mimi

Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute

[Tushar Sugandhi]

On 1/12/24 09:44, Mimi Zohar wrote:
> On Thu, 2024-01-11 at 12:52 -0800, Tushar Sugandhi wrote:
> [...]
>> If we go with the KBs approach -
>>
>> half-a-page translates to different KBs on different architectures.
>> And setting the right default value in KBs which would translate to
>> the desired half-a-page, on a given arch, inside the Kconfig seems
>> fragile (as I mentioned in the context of Option A in my previous
>> response.
> 
> How about setting the default value to 0, indicating not to change the current
> half page default.  Any other value would be KBs, as Stefan suggested.
> 
Thanks.
That's way more elegant than the other alternatives.
It's definitely doable in KConfig and handle the default in code 
appropriately.

It may cause some confusion in the documentation of the config param.
But it would be a wording issue, and we can work on it.

Thanks for the suggestion.  I like it honestly.

~Tushar