ima: kexec: measure events between kexec load and execute

- Compared the memory size allocated with memory size of the entire measurement record. Copy only complete measurement records if there is enough memory. If there is not enough memory, it will not copy any IMA measurement records, and this situation will result in a failure of remote attestation.

Quoted 6 lines · steven chen

- Compared the memory size allocated with memory size of the entire measurement record. Copy only complete measurement records if there is enough memory. If there is not enough memory, it will not copy any IMA measurement records, and this situation will result in a failure of remote attestation.

In discussions with Tushar, I was very clear that as many measurement records as possible should be carried over to the kexec'ed kernel. The main change between v8 and v9 was to make sure the last record copied was a complete record.

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

This patch includes the following changes:

- Refactor ima_dump_measurement_list() to move the memory allocation to a separate function ima_alloc_kexec_file_buf() which allocates buffer of size 'kexec_segment_size' at kexec 'load'.
- Make the local variable ima_kexec_file in ima_dump_measurement_list() a local static to the file, so that it can be accessed from ima_alloc_kexec_file_buf(). Compare actual memory required to ensure there is enough memory for the entire measurement record.
- Copy only complete measurement records.
- Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions.
- Compared the memory size allocated with memory size of the entire measurement record. Copy only complete measurement records if there is enough memory. If there is not enough memory, it will not copy any IMA measurement records, and this situation will result in a failure of remote attestation.

Suggested-by Mimi Zohar <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Signed-off-by steven chen <[email protected]>

---
 security/integrity/ima/ima.h       |   1 +
 security/integrity/ima/ima_kexec.c | 105 ++++++++++++++++++++++-------
 security/integrity/ima/ima_queue.c |   4 +-
 3 files changed, 83 insertions(+), 27 deletions(-)

security/integrity/ima/ima.h +1 -0

index 24d09ea91b87..4428fcf42167 100644
@@ -274,6 +274,7 @@ 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);
+int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry);
 unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(void);

security/integrity/ima/ima_kexec.c +80 -25

index 9d45f4d26f73..6195df128482 100644
@@ -15,63 +15,106 @@
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+static struct seq_file ima_kexec_file;
+
+static void ima_reset_kexec_file(struct seq_file *sf)
+{
+	sf->buf = NULL;
+	sf->size = 0;
+	sf->read_pos = 0;
+	sf->count = 0;
+}
+
+static void ima_free_kexec_file_buf(struct seq_file *sf)
+{
+	vfree(sf->buf);
+	ima_reset_kexec_file(sf);
+}
+
+static int ima_alloc_kexec_file_buf(size_t segment_size)
+{
+	/*
+	 * kexec 'load' may be called multiple times.
+	 * Free and realloc the buffer only if the segment_size is
+	 * changed from the previous kexec 'load' call.
+	 */
+	if (ima_kexec_file.buf && ima_kexec_file.size == segment_size)
+		goto out;
+
+	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;
+
+out:
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
+
+	return 0;
+}
+
+/*
+ * Copy the measurement list to the allocated memory
+ * compare the size of IMA measurement list with the size of the allocated memory
+ *    if the size of the allocated memory is not less than the size of IMA measurement list
+ *        copy the measurement list to the allocated memory.
+ *    else return error
+ */
 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;
+	size_t entry_size = 0;
 
-	/* segment size can't change between kexec load and execute */
-	file.buf = vmalloc(segment_size);
-	if (!file.buf) {
-		ret = -ENOMEM;
-		goto out;
+	if (!ima_kexec_file.buf) {
+		pr_err("Kexec file buf not allocated\n");
+		return -EINVAL;
 	}
 
-	file.file = NULL;
-	file.size = segment_size;
-	file.read_pos = 0;
-	file.count = sizeof(khdr);	/* reserved space */
-
 	memset(&khdr, 0, sizeof(khdr));
 	khdr.version = 1;
 	/* This is an append-only list, no need to hold the RCU read lock */
 	list_for_each_entry_rcu(qe, &ima_measurements, later, true) {
-		if (file.count < file.size) {
+		entry_size += ima_get_binary_runtime_entry_size(qe->entry);
+		if (entry_size <= segment_size) {
 			khdr.count++;
-			ima_measurements_show(&file, qe);
+			ima_measurements_show(&ima_kexec_file, qe);
 		} else {
 			ret = -EINVAL;
+			pr_err("IMA log file is too big for Kexec buf\n");
 			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;
+	*buffer_size = ima_kexec_file.count;
+	*buffer = ima_kexec_file.buf;
+
 out:
-	if (ret == -EINVAL)
-		vfree(file.buf);
 	return ret;
 }
 

@@ -90,7 +133,7 @@ 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_buffer_size = 0;
 	size_t kexec_segment_size;
 	int ret;
 

@@ -110,13 +153,19 @@ 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("Failed to dump IMA measurements. Error:%d.\n", ret);
+		return;
+	}
+
 	kbuf.buffer = kexec_buffer;
 	kbuf.bufsz = kexec_buffer_size;
 	kbuf.memsz = kexec_segment_size;

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

security/integrity/ima/ima_queue.c +2 -2

index 83d53824aa98..3dfd178d4292 100644
@@ -78,7 +78,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
  * binary_runtime_measurement list entry, which contains a
  * couple of variable length fields (e.g template name and data).
  */
-static int get_binary_runtime_size(struct ima_template_entry *entry)
+int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry)
 {
 	int size = 0;
 

@@ -122,7 +122,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry,
 	if (binary_runtime_size != ULONG_MAX) {
 		int size;
 
-		size = get_binary_runtime_size(entry);
+		size = ima_get_binary_runtime_entry_size(entry);
 		binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ?
 		     binary_runtime_size + size : ULONG_MAX;
 	}

Signature · 1 line

2.25.1

Quoted 6 lines · steven chen

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

Quoted 1 line

This patch includes the following changes:

I don't know why one patch need include so many changes. From below log, it should be split into separate patches. It may not need to make one patch to reflect one change, we should at least split and wrap several kind of changes to ease patch understanding and reviewing. My personal opinion.

Quoted 20 lines

- Refactor ima_dump_measurement_list() to move the memory allocation to a separate function ima_alloc_kexec_file_buf() which allocates buffer of size 'kexec_segment_size' at kexec 'load'.
- Make the local variable ima_kexec_file in ima_dump_measurement_list() a local static to the file, so that it can be accessed from ima_alloc_kexec_file_buf(). Compare actual memory required to ensure there is enough memory for the entire measurement record.
- Copy only complete measurement records.
- Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions.
- Compared the memory size allocated with memory size of the entire measurement record. Copy only complete measurement records if there is enough memory. If there is not enough memory, it will not copy any IMA measurement records, and this situation will result in a failure of remote attestation.

Suggested-by Mimi Zohar <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Signed-off-by steven chen <[email protected]>

Quoted 8 lines · Baoquan He

Quoted 7 lines · steven chen

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

This patch includes the following changes:

Quoted 5 lines

I don't know why one patch need include so many changes. From below log, it should be split into separate patches. It may not need to make one patch to reflect one change, we should at least split and wrap several kind of changes to ease patch understanding and reviewing. My personal opinion.

Agreed, well explained.

Mimi

Quoted 19 lines

Quoted 19 lines

- Refactor ima_dump_measurement_list() to move the memory allocation to a separate function ima_alloc_kexec_file_buf() which allocates buffer of size 'kexec_segment_size' at kexec 'load'.
- Make the local variable ima_kexec_file in ima_dump_measurement_list() a local static to the file, so that it can be accessed from ima_alloc_kexec_file_buf(). Compare actual memory required to ensure there is enough memory for the entire measurement record.
- Copy only complete measurement records.
- Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions.
- Compared the memory size allocated with memory size of the entire measurement record. Copy only complete measurement records if there is enough memory. If there is not enough memory, it will not copy any IMA measurement records, and this situation will result in a failure of remote attestation.

Suggested-by Mimi Zohar <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Signed-off-by steven chen <[email protected]>

Quoted 34 lines · Mimi Zohar

Quoted 7 lines · Baoquan He

Quoted 6 lines · steven chen

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

Quoted 6 lines

Quoted 1 line

This patch includes the following changes:

I don't know why one patch need include so many changes. From below log, it should be split into separate patches. It may not need to make one patch to reflect one change, we should at least split and wrap several kind of changes to ease patch understanding and reviewing. My personal opinion.

Agreed, well explained.

Mimi

Quoted 16 lines

Quoted 16 lines

- Refactor ima_dump_measurement_list() to move the memory allocation to a separate function ima_alloc_kexec_file_buf() which allocates buffer of size 'kexec_segment_size' at kexec 'load'.
- Make the local variable ima_kexec_file in ima_dump_measurement_list() a local static to the file, so that it can be accessed from ima_alloc_kexec_file_buf(). Compare actual memory required to ensure there is enough memory for the entire measurement record.
- Copy only complete measurement records.
- Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions.
- Compared the memory size allocated with memory size of the entire measurement record. Copy only complete measurement records if there is enough memory. If there is not enough memory, it will not copy any IMA measurement records, and this situation will result in a failure of remote attestation.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Suggested-by Mimi Zohar <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Signed-off-by steven chen <[email protected]>

I will split this patch into the following two patches:

    ima: define and call ima_alloc_kexec_file_buf
    ima: copy measurement records as much as possible across kexec

Quoted 35 lines · steven chen

Quoted 34 lines · Mimi Zohar

Quoted 7 lines · Baoquan He

Quoted 6 lines · steven chen

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

Quoted 6 lines

Quoted 1 line

This patch includes the following changes:

I don't know why one patch need include so many changes. From below log, it should be split into separate patches. It may not need to make one patch to reflect one change, we should at least split and wrap several kind of changes to ease patch understanding and reviewing. My personal opinion.

Agreed, well explained.

Mimi

Quoted 16 lines

Quoted 16 lines

- Refactor ima_dump_measurement_list() to move the memory allocation to a separate function ima_alloc_kexec_file_buf() which allocates buffer of size 'kexec_segment_size' at kexec 'load'.
- Make the local variable ima_kexec_file in ima_dump_measurement_list() a local static to the file, so that it can be accessed from ima_alloc_kexec_file_buf(). Compare actual memory required to ensure there is enough memory for the entire measurement record.
- Copy only complete measurement records.
- Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions.
- Compared the memory size allocated with memory size of the entire measurement record. Copy only complete measurement records if there is enough memory. If there is not enough memory, it will not copy any IMA measurement records, and this situation will result in a failure of remote attestation.

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Suggested-by Mimi Zohar <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Signed-off-by steven chen <[email protected]>

I will split this patch into the following two patches:

    ima: define and call ima_alloc_kexec_file_buf
    ima: copy measurement records as much as possible across kexec

Steven, breaking up code into patches is in order to simplify patch review. This is done by limiting each patch to a single "logical change" [1]. For example, the change below has nothing to do with "separate allocating the buffer and copying the measurement records into separate functions".

        /* This is an append-only list, no need to hold the RCU read lock */
        list_for_each_entry_rcu(qe, &ima_measurements, later, true) {

security/integrity/ima/ima_kexec.c +4 -2

-               if (file.count < file.size) {
+               entry_size += ima_get_binary_runtime_entry_size(qe->entry); 
+               if (entry_size <= segment_size) {
                        khdr.count++;
-                       ima_measurements_show(&file, qe);
+                       ima_measurements_show(&ima_kexec_file, qe);
                } else {
                        ret = -EINVAL;
+                       pr_err("IMA log file is too big for Kexec buf\n");
                        break;
                }
        }

The original code potentially copied a partial last measurement record, not a complete measurement record. For ease of review, the above change is fine, but it needs to be a separate patch.

Patches:
1. ima: copy only complete measurement records across kexec
2. ima: define and call ima_alloc_kexec_file_buf()

Quoted 63 lines · Mimi Zohar

Quoted 35 lines · steven chen

Quoted 34 lines · Mimi Zohar

Quoted 7 lines · Baoquan He

Quoted 6 lines · steven chen

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

Quoted 6 lines

Quoted 1 line

This patch includes the following changes:

I don't know why one patch need include so many changes. From below log, it should be split into separate patches. It may not need to make one patch to reflect one change, we should at least split and wrap several kind of changes to ease patch understanding and reviewing. My personal opinion.

Agreed, well explained.

Mimi

Quoted 16 lines

Quoted 16 lines

- Refactor ima_dump_measurement_list() to move the memory allocation to a separate function ima_alloc_kexec_file_buf() which allocates buffer of size 'kexec_segment_size' at kexec 'load'.
- Make the local variable ima_kexec_file in ima_dump_measurement_list() a local static to the file, so that it can be accessed from ima_alloc_kexec_file_buf(). Compare actual memory required to ensure there is enough memory for the entire measurement record.
- Copy only complete measurement records.
- Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions.
- Compared the memory size allocated with memory size of the entire measurement record. Copy only complete measurement records if there is enough memory. If there is not enough memory, it will not copy any IMA measurement records, and this situation will result in a failure of remote attestation.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Suggested-by Mimi Zohar <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Signed-off-by steven chen <[email protected]>

I will split this patch into the following two patches:

    ima: define and call ima_alloc_kexec_file_buf
    ima: copy measurement records as much as possible across kexec

Steven, breaking up code into patches is in order to simplify patch review. This is done by limiting each patch to a single "logical change" [1]. For example, the change below has nothing to do with "separate allocating the buffer and copying the measurement records into separate functions".

         /* This is an append-only list, no need to hold the RCU read lock */
         list_for_each_entry_rcu(qe, &ima_measurements, later, true) {

security/integrity/ima/ima_kexec.c +4 -2

-               if (file.count < file.size) {
+               entry_size += ima_get_binary_runtime_entry_size(qe->entry);
+               if (entry_size <= segment_size) {
                         khdr.count++;
-                       ima_measurements_show(&file, qe);
+                       ima_measurements_show(&ima_kexec_file, qe);
                 } else {
                         ret = -EINVAL;
+                       pr_err("IMA log file is too big for Kexec buf\n");
                         break;
                 }
         }

Quoted 7 lines

The original code potentially copied a partial last measurement record, not a complete measurement record. For ease of review, the above change is fine, but it needs to be a separate patch.

Patches:
1. ima: copy only complete measurement records across kexec
2. ima: define and call ima_alloc_kexec_file_buf()

Steven,

The alternative would be to revert using ima_get_binary_runtime_entry_size() and simply use "ima_kexec_file.count < ima_kexec_file.size". Only ima_kexec_file.size would be initialized in ima_alloc_kexec_buf(). The rest would remain in ima_dump_measurement_list(). get_binary_runtime_size() wouldn't need to be made global.

To further simplify the patch review, first define a separate patch to just rename the seq_file "file" to "ima_kexec_file".

Mimi

The content of memory segments carried over to the new kernel during the kexec systemcall can be changed at kexec 'execute' stage, but the size of the memory segments cannot be changed at kexec 'execute' stage.

To copy IMA measurement logs during the kexec operation, IMA needs to allocate memory at the kexec 'load' stage and map the segments to the kimage structure. The mapped address will then be used to copy IMA measurements during the kexec 'execute' stage.

Currently, the mechanism to map and unmap segments to the kimage structure is not available to subsystems outside of kexec.

Implement kimage_map_segment() to enable IMA to map measurement log list to the kimage structure during kexec 'load' stage. This function takes a kimage pointer, a memory address, and a size, then gathers the
source pages within the specified address range, creates an array of page pointers, and maps these to a contiguous virtual address range. The function returns the start virtual address of this range if successful, or NULL on failure.

Implement kimage_unmap_segment() for unmapping segments using vunmap().

From: Tushar Sugandhi <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Cc Eric Biederman <[email protected]>

Cc Baoquan He <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Dave Young <[email protected]>

Signed-off-by steven chen <[email protected]>

---
 include/linux/kexec.h |  6 +++++
 kernel/kexec_core.c   | 54 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

include/linux/kexec.h +6 -0

index f0e9f8eda7a3..7d6b12f8b8d0 100644
@@ -467,13 +467,19 @@ extern bool kexec_file_dbg_print;
 #define kexec_dprintk(fmt, arg...) \
         do { if (kexec_file_dbg_print) pr_info(fmt, ##arg); } while (0)
 
+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;
+struct kimage;
 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 */
 

kernel/kexec_core.c +54 -0

index c0bdc1686154..63e4d16b6023 100644
@@ -867,6 +867,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("Could not allocate ima pages array.\n");
+		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("Could not map ima buffer.\n");
+
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
 struct kexec_load_limit {
 	/* Mutex protects the limit count. */
 	struct mutex mutex;

Signature · 1 line

2.25.1

Quoted 9 lines · steven chen

The content of memory segments carried over to the new kernel during the kexec systemcall can be changed at kexec 'execute' stage, but the size of the memory segments cannot be changed at kexec 'execute' stage.

To copy IMA measurement logs during the kexec operation, IMA needs to allocate memory at the kexec 'load' stage and map the segments to the kimage structure. The mapped address will then be used to copy IMA measurements during the kexec 'execute' stage.

Quoted 2 lines

Currently, the mechanism to map and unmap segments to the kimage structure is not available to subsystems outside of kexec.

How does IMA work with kexec without having this? Just interested (and confused).

Quoted 116 lines

Implement kimage_map_segment() to enable IMA to map measurement log list to the kimage structure during kexec 'load' stage. This function takes a kimage pointer, a memory address, and a size, then gathers the
source pages within the specified address range, creates an array of page pointers, and maps these to a contiguous virtual address range. The function returns the start virtual address of this range if successful, or NULL on failure.

Implement kimage_unmap_segment() for unmapping segments using vunmap().

From: Tushar Sugandhi <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Cc Eric Biederman <[email protected]>

Cc Baoquan He <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Dave Young <[email protected]>

Signed-off-by steven chen <[email protected]>

---
  include/linux/kexec.h |  6 +++++
  kernel/kexec_core.c   | 54 +++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 60 insertions(+)

include/linux/kexec.h +6 -0

index f0e9f8eda7a3..7d6b12f8b8d0 100644
@@ -467,13 +467,19 @@ extern bool kexec_file_dbg_print;
  #define kexec_dprintk(fmt, arg...) \
          do { if (kexec_file_dbg_print) pr_info(fmt, ##arg); } while (0)
 
+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;
+struct kimage;
  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 */
 

kernel/kexec_core.c +54 -0

index c0bdc1686154..63e4d16b6023 100644
@@ -867,6 +867,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("Could not allocate ima pages array.\n");
+		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("Could not map ima buffer.\n");
+
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
  struct kexec_load_limit {
  	/* Mutex protects the limit count. */
  	struct mutex mutex;

Signature · 1 line

2.25.1

BR, Jarkko

Quoted 10 lines · Jarkko Sakkinen

Quoted 9 lines · steven chen

The content of memory segments carried over to the new kernel during the kexec systemcall can be changed at kexec 'execute' stage, but the size of the memory segments cannot be changed at kexec 'execute' stage.

To copy IMA measurement logs during the kexec operation, IMA needs to allocate memory at the kexec 'load' stage and map the segments to the kimage structure. The mapped address will then be used to copy IMA measurements during the kexec 'execute' stage.

Quoted 4 lines

Quoted 2 lines

Currently, the mechanism to map and unmap segments to the kimage structure is not available to subsystems outside of kexec.

How does IMA work with kexec without having this? Just interested (and confused).

Currently, all IMA-related operations during a soft reboot, such as memory allocation and IMA log list copy, are handled in the kexec 'load' stage, so the map/unmap mechanism is not required.

The new design separates these two operations into different stages: memory allocation remains in the kexec 'load' stage, while the IMA log list copy is moved to the kexec 'execute' stage. Therefore, the map/unmap mechanism is introduced.

Please refer to "[PATCH v9 0/7] ima: kexec: measure events between kexec load and execute" for the reason why to add this.

Steven

Quoted 54 lines

Quoted 54 lines

Implement kimage_map_segment() to enable IMA to map measurement log list to the kimage structure during kexec 'load' stage. This function takes a kimage pointer, a memory address, and a size, then gathers the
source pages within the specified address range, creates an array of page pointers, and maps these to a contiguous virtual address range. The function returns the start virtual address of this range if successful, or NULL on failure.

Implement kimage_unmap_segment() for unmapping segments using vunmap().

From: Tushar Sugandhi <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Cc Eric Biederman <[email protected]>

Cc Baoquan He <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Dave Young <[email protected]>

Signed-off-by steven chen <[email protected]>

---
  include/linux/kexec.h |  6 +++++
  kernel/kexec_core.c   | 54 +++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 60 insertions(+)

include/linux/kexec.h +6 -0

index f0e9f8eda7a3..7d6b12f8b8d0 100644
@@ -467,13 +467,19 @@ extern bool kexec_file_dbg_print;
  #define kexec_dprintk(fmt, arg...) \
          do { if (kexec_file_dbg_print) pr_info(fmt, ##arg); } while (0)
+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;
+struct kimage;
  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 */

kernel/kexec_core.c +2 -0

index c0bdc1686154..63e4d16b6023 100644
@@ -867,6 +867,60 @@ int kimage_load_segment(struct kimage *image,
  	return result;
  }
+void *kimage_map_segment(struct kimage *image,
+			 unsigned long addr, unsigned long size)

Quoted 8 lines

kernel/kexec_core.c int kimage_load_segment(struct kimage *image, · line 879

Quoted 8 lines

kernel/kexec_core.c +8 -0

+{
+	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;

Quoted 52 lines

Quoted 51 lines

kernel/kexec_core.c +44 -0

+
+	/*
+	 * 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("Could not allocate ima pages array.\n");
+		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("Could not map ima buffer.\n");
+
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
  struct kexec_load_limit {
  	/* Mutex protects the limit count. */
  	struct mutex mutex;

Signature · 1 line

2.25.1

BR, Jarkko

Quoted 15 lines · steven chen

Quoted 10 lines · Jarkko Sakkinen

Quoted 9 lines · steven chen

The content of memory segments carried over to the new kernel during the kexec systemcall can be changed at kexec 'execute' stage, but the size of the memory segments cannot be changed at kexec 'execute' stage.

To copy IMA measurement logs during the kexec operation, IMA needs to allocate memory at the kexec 'load' stage and map the segments to the kimage structure. The mapped address will then be used to copy IMA measurements during the kexec 'execute' stage.

Quoted 4 lines

Quoted 2 lines

Currently, the mechanism to map and unmap segments to the kimage structure is not available to subsystems outside of kexec.

How does IMA work with kexec without having this? Just interested (and confused).

Quoted 8 lines

Currently, all IMA-related operations during a soft reboot, such as memory allocation and IMA log list copy, are handled in the kexec 'load' stage, so the map/unmap mechanism is not required.

The new design separates these two operations into different stages: memory allocation remains in the kexec 'load' stage, while the IMA log list copy is moved to the kexec 'execute' stage. Therefore, the map/unmap mechanism is introduced.

I think the log can be improved. About the found problem and solution part, we possible can describe them like below:

===
Currently, the kernel behaviour of kexec load is the IMA measurements log is fetched from TPM PCRs and stored into buffer and hold. When kexec reboot is triggered, the stored log buffer is carried over to the 2nd kernel. However, the time gap between kexec load and kexec reboot could be very long. Then those new events extended into TPM PCRs during the time window misses the chance to be carried over to 2nd kernel. This results in mismatch between TPM PCR quotes and the actual IMA measurements list after kexec reboot, which in turn results in remote attestation failure.

To solve this problem, the new design is to defer the reading TPM PCRs content out into kexec buffer to kexec reboot phase. While still allocating the necessary buffer at kexec load time because it's not appropriate to allocate memory at kexec reboot moment.
===

It may still need be improved, just for your reference. You can change and add more detail needed and add them into your log.

Quoted 61 lines

Please refer to "[PATCH v9 0/7] ima: kexec: measure events between kexec load and execute" for the reason why to add this.

Steven

Quoted 56 lines

Quoted 56 lines

Implement kimage_map_segment() to enable IMA to map measurement log list to the kimage structure during kexec 'load' stage. This function takes a kimage pointer, a memory address, and a size, then gathers the
source pages within the specified address range, creates an array of page pointers, and maps these to a contiguous virtual address range. The function returns the start virtual address of this range if successful, or NULL on failure.

Implement kimage_unmap_segment() for unmapping segments using vunmap().

From: Tushar Sugandhi <[email protected]>

Signed-off-by Tushar Sugandhi <[email protected]>

Cc Eric Biederman <[email protected]>

Cc Baoquan He <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Dave Young <[email protected]>

Signed-off-by steven chen <[email protected]>

---
   include/linux/kexec.h |  6 +++++
   kernel/kexec_core.c   | 54 +++++++++++++++++++++++++++++++++++++++++++
   2 files changed, 60 insertions(+)

include/linux/kexec.h +6 -0

index f0e9f8eda7a3..7d6b12f8b8d0 100644
@@ -467,13 +467,19 @@ extern bool kexec_file_dbg_print;
   #define kexec_dprintk(fmt, arg...) \
           do { if (kexec_file_dbg_print) pr_info(fmt, ##arg); } while (0)
+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;
+struct kimage;
   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 */

kernel/kexec_core.c +4 -0

index c0bdc1686154..63e4d16b6023 100644
@@ -867,6 +867,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;

Quoted 6 lines

Quoted 6 lines

kernel/kexec_core.c int kimage_load_segment(struct kimage *image, · line 879

Quoted 6 lines

kernel/kexec_core.c +6 -0

+	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;

When adding a new function, it's suggested to take the reverse xmas tree style for local variable ordering usually.

Quoted 53 lines

Quoted 52 lines

Quoted 51 lines

kernel/kexec_core.c +44 -0

+
+	/*
+	 * 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("Could not allocate ima pages array.\n");
+		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("Could not map ima buffer.\n");
+
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
   struct kexec_load_limit {
   	/* Mutex protects the limit count. */
   	struct mutex mutex;

Signature · 1 line

2.25.1

BR, Jarkko

The kexec_calculate_store_digests() function calculates and stores the digest of the segment during the kexec_file_load syscall, where the IMA segment is also allocated.

With this series, the IMA segment will be updated with the measurement log at the kexec execute stage when a soft reboot is initiated. Therefore, the digests should be updated for the IMA segment in the normal case.

The content of memory segments carried over to the new kernel during the kexec systemcall can be changed at kexec 'execute' stage, but the size and the location of the memory segments cannot be changed at kexec 'execute' stage.

However, during the kexec execute stage, if kexec_calculate_store_digests() API is call to update the digest, it does not reuse the same memory segment

allocated during the kexec 'load' stage and the new memory segment required cannot be transferred/mapped to the new kernel.

As a result, digest verification will fail in verify_sha256_digest() after a kexec soft reboot into the new kernel. Therefore, the digest calculation/verification of the IMA segment needs to be skipped.

To address this, skip the calculation and storage of the digest for the IMA segment in kexec_calculate_store_digests() so that it is not added to the purgatory_sha_regions.

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

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

Signed-off-by Tushar Sugandhi <[email protected]>

Cc Eric Biederman <[email protected]>

Cc Baoquan He <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Dave Young <[email protected]>

Signed-off-by steven chen <[email protected]>

Reviewed-by Stefan Berger <[email protected]>

Reviewed-by Mimi Zohar <[email protected]>

---
 include/linux/kexec.h              |  3 +++
 kernel/kexec_file.c                | 22 ++++++++++++++++++++++
 security/integrity/ima/ima_kexec.c |  3 +++
 3 files changed, 28 insertions(+)

include/linux/kexec.h +3 -0

index 7d6b12f8b8d0..107e726f2ef3 100644
@@ -362,6 +362,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 */

kernel/kexec_file.c +22 -0

index 3eedb8c226ad..606132253c79 100644
@@ -38,6 +38,21 @@ void set_kexec_sig_enforced(void)
 }
 #endif
 
+#ifdef CONFIG_IMA_KEXEC
+static bool check_ima_segment_index(struct kimage *image, int i)
+{
+	if (image->is_ima_segment_index_set && i == image->ima_segment_index)
+		return true;
+	else
+		return false;
+}
+#else
+static bool check_ima_segment_index(struct kimage *image, int i)
+{
+	return false;
+}
+#endif
+
 static int kexec_calculate_store_digests(struct kimage *image);
 
 /* Maximum size in bytes for kernel/initrd files. */

@@ -764,6 +779,13 @@ 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 (check_ima_segment_index(image, i))
+			continue;
+
 		ret = crypto_shash_update(desc, ksegment->kbuf,
 					  ksegment->bufsz);
 		if (ret)

security/integrity/ima/ima_kexec.c +3 -0

index 6195df128482..0465beca8867 100644
@@ -169,6 +169,7 @@ void ima_add_kexec_buffer(struct kimage *image)
 	kbuf.buffer = kexec_buffer;
 	kbuf.bufsz = kexec_buffer_size;
 	kbuf.memsz = kexec_segment_size;
+	image->is_ima_segment_index_set = false;
 	ret = kexec_add_buffer(&kbuf);
 	if (ret) {
 		pr_err("Error passing over kexec measurement buffer.\n");

@@ -179,6 +180,8 @@ void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_buffer_addr = kbuf.mem;
 	image->ima_buffer_size = kexec_segment_size;
 	image->ima_buffer = kexec_buffer;
+	image->ima_segment_index = image->nr_segments - 1;
+	image->is_ima_segment_index_set = true;
 
 	/*
 	 * kexec owns kexec_buffer after kexec_add_buffer() is called

Signature · 1 line

2.25.1

security/integrity/ima/ima_kexec.c +4 -0

+
+	ima_measure_critical_data("ima_kexec", event_name, ima_kexec_event, n, false, NULL, 0);
+}
+

Hi Steven,

security/integrity/ima/ima_kexec.c static void ima_free_kexec_file_buf(struct seq_file *sf) · line 53

Quoted 13 lines · steven chen

security/integrity/ima/ima_kexec.c +13 -0

+void ima_measure_kexec_event(const char *event_name)
+{
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
+	size_t buf_size = 0;
+	long len;
+
+	buf_size = ima_get_binary_runtime_size();
+	len = atomic_long_read(&ima_htable.len);
+
+	int n = scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+					"kexec_segment_size=%lu;ima_binary_runtime_size=%lu;"
+					"ima_runtime_measurements_count=%ld;",
+					kexec_segment_size, buf_size, len);

Variables should not be defined inline, but at the beginning of the function. After doing that, scripts/checkpatch.pl complains about the formatting.

Mimi

Quoted 4 lines

security/integrity/ima/ima_kexec.c +4 -0

+
+	ima_measure_critical_data("ima_kexec", event_name, ima_kexec_event, n, false, NULL, 0);
+}
+