ima: kexec: measure events between kexec load and execute

From: Steven Chen <[email protected]>

Before making the function local seq_file "file" variable file static global, rename it to "ima_kexec_file".

Signed-off-by Steven Chen <[email protected]>

---
 security/integrity/ima/ima_kexec.c | 31 +++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

security/integrity/ima/ima_kexec.c +16 -15

index 9d45f4d26f73..650beb74346c 100644
@@ -18,30 +18,30 @@
 static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 				     unsigned long segment_size)
 {
+	struct seq_file ima_kexec_file;
 	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) {
+	ima_kexec_file.buf = vmalloc(segment_size);
+	if (!ima_kexec_file.buf) {
 		ret = -ENOMEM;
 		goto out;
 	}
 
-	file.file = NULL;
-	file.size = segment_size;
-	file.read_pos = 0;
-	file.count = sizeof(khdr);	/* reserved space */
+	ima_kexec_file.file = NULL;
+	ima_kexec_file.size = segment_size;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_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) {
+		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;
 			break;

@@ -55,23 +55,24 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 	 * 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);
+		vfree(ima_kexec_file.buf);
 	return ret;
 }
 

Signature · 1 line

2.43.0

From: Steven Chen <[email protected]>

In the current implementation, the ima_dump_measurement_list() API is called during the kexec "load" phase, where a buffer is allocated and the measurement records are copied. Due to this, new events added after kexec load but before kexec execute are not carried over to the new kernel during kexec operation

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'.

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

Signed-off-by Steven Chen <[email protected]>

---
 security/integrity/ima/ima_kexec.c | 46 +++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 11 deletions(-)

security/integrity/ima/ima_kexec.c +35 -11

index 650beb74346c..b12ac3619b8f 100644
@@ -15,26 +15,46 @@
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+static struct seq_file ima_kexec_file;
+
+static 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)
+{
+	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 seq_file ima_kexec_file;
 	struct ima_queue_entry *qe;
 	struct ima_kexec_hdr khdr;
 	int ret = 0;
 
 	/* segment size can't change between kexec load and execute */
-	ima_kexec_file.buf = vmalloc(segment_size);
 	if (!ima_kexec_file.buf) {
-		ret = -ENOMEM;
-		goto out;
+		pr_err("Kexec file buf not allocated\n");
+		return -EINVAL;
 	}
 
-	ima_kexec_file.file = NULL;
-	ima_kexec_file.size = segment_size;
-	ima_kexec_file.read_pos = 0;
-	ima_kexec_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 */

@@ -71,8 +91,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 	*buffer_size = ima_kexec_file.count;
 	*buffer = ima_kexec_file.buf;
 out:
-	if (ret == -EINVAL)
-		vfree(ima_kexec_file.buf);
 	return ret;
 }
 

@@ -111,6 +129,12 @@ void ima_add_kexec_buffer(struct kimage *image)
 		return;
 	}
 
+	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
+	if (ret < 0) {
+		pr_err("Not enough memory for the kexec measurement buffer.\n");
+		return;
+	}
+
 	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
 				  kexec_segment_size);
 	if (!kexec_buffer) {

Signature · 1 line

2.43.0

Hi Steven,

Quoted 8 lines · steven chen

From: Steven Chen <[email protected]>

In the current implementation, the ima_dump_measurement_list() API is called during the kexec "load" phase, where a buffer is allocated and the measurement records are copied. Due to this, new events added after kexec load but before kexec execute are not carried over to the new kernel during kexec operation

Quoted 5 lines

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'.

Seems you didn't add note in this patch log to mention that the IMA measurement list fails to verify when doing two consecuritv "kexec -s -l" with/without a "kexec -s -u" in between. When people do bisecting and come to this patch, she/he will see the failure even though it's not a fatal blocker.

====
After moving the vfree() here at this stage in the patch set, the IMA measurement list fails to verify when doing two consecutive "kexec -s -l" with/without a "kexec -s -u" in between. Only after "ima: kexec: move IMA log copy from kexec load to execute" the IMA measurement list verifies properly with the vfree() here.
====

Quoted 91 lines

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

Signed-off-by Steven Chen <[email protected]>

---
  security/integrity/ima/ima_kexec.c | 46 +++++++++++++++++++++++-------
  1 file changed, 35 insertions(+), 11 deletions(-)

security/integrity/ima/ima_kexec.c +35 -11

index 650beb74346c..b12ac3619b8f 100644
@@ -15,26 +15,46 @@
  #include "ima.h"
 
  #ifdef CONFIG_IMA_KEXEC
+static struct seq_file ima_kexec_file;
+
+static 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)
+{
+	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 seq_file ima_kexec_file;
  	struct ima_queue_entry *qe;
  	struct ima_kexec_hdr khdr;
  	int ret = 0;
 
  	/* segment size can't change between kexec load and execute */
-	ima_kexec_file.buf = vmalloc(segment_size);
  	if (!ima_kexec_file.buf) {
-		ret = -ENOMEM;
-		goto out;
+		pr_err("Kexec file buf not allocated\n");
+		return -EINVAL;
  	}
 
-	ima_kexec_file.file = NULL;
-	ima_kexec_file.size = segment_size;
-	ima_kexec_file.read_pos = 0;
-	ima_kexec_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 */

@@ -71,8 +91,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
  	*buffer_size = ima_kexec_file.count;
  	*buffer = ima_kexec_file.buf;
  out:
-	if (ret == -EINVAL)
-		vfree(ima_kexec_file.buf);
  	return ret;
  }
 

@@ -111,6 +129,12 @@ void ima_add_kexec_buffer(struct kimage *image)
  		return;
  	}
 
+	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
+	if (ret < 0) {
+		pr_err("Not enough memory for the kexec measurement buffer.\n");
+		return;
+	}
+
  	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
  				  kexec_segment_size);
  	if (!kexec_buffer) {

Signature · 1 line

2.43.0

From: Steven Chen <[email protected]>

Implement kimage_map_segment() to enable IMA to map the measurement log list to the kimage structure during the kexec 'load' stage. This function gathers the source pages within the specified address range, and maps them to a contiguous virtual address range.

This is a preparation for later usage.

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

Acked-by Baoquan He <[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..a5e378e1dc7f 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 src_page_addr, dest_page_addr = 0;
+	unsigned long eaddr = addr + size;
+	kimage_entry_t *ptr, entry;
+	struct page **src_pages;
+	unsigned int npages;
+	void *vaddr = NULL;
+	int i;
+
+	/*
+	 * 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.43.0

Quoted 1 line · steven chen

From: Steven Chen <[email protected]>

^^^^^^

Quoted 10 lines

Implement kimage_map_segment() to enable IMA to map the measurement log list to the kimage structure during the kexec 'load' stage. This function gathers the source pages within the specified address range, and maps them to a contiguous virtual address range.

This is a preparation for later usage.

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

From: Tushar Sugandhi <[email protected]>

^^^^^^

Quoted 1 line

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

^^^^^^^

Quoted 5 lines

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

^^^^^

The signing on this patch is a little confusing. I can't see who is the real author, who is the co-author, between you and Tushar. You may need to refer to Documentation/process/5.Posting.rst to make that clear.

Quoted 98 lines

Acked-by Baoquan He <[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..a5e378e1dc7f 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 src_page_addr, dest_page_addr = 0;
+	unsigned long eaddr = addr + size;
+	kimage_entry_t *ptr, entry;
+	struct page **src_pages;
+	unsigned int npages;
+	void *vaddr = NULL;
+	int i;
+
+	/*
+	 * 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.43.0

Quoted 21 lines · Baoquan He

Quoted 1 line · steven chen

From: Steven Chen <[email protected]>

^^^^^^

Quoted 10 lines

Implement kimage_map_segment() to enable IMA to map the measurement log list to the kimage structure during the kexec 'load' stage. This function gathers the source pages within the specified address range, and maps them to a contiguous virtual address range.

This is a preparation for later usage.

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

From: Tushar Sugandhi <[email protected]>

^^^^^^

Quoted 1 line

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

^^^^^^^

Quoted 5 lines

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

Quoted 5 lines

^^^^^

The signing on this patch is a little confusing. I can't see who is the real author, who is the co-author, between you and Tushar. You may need to refer to Documentation/process/5.Posting.rst to make that clear.

Hi Baoquan,

From my understanding, if there is no change from the original author patch, need to add
From tag and Signed-off-by tag; otherwise, if there are changes, Signed-off-by can be used.

Steven

Quoted 95 lines

Quoted 95 lines

Acked-by Baoquan He <[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..a5e378e1dc7f 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 src_page_addr, dest_page_addr = 0;
+	unsigned long eaddr = addr + size;
+	kimage_entry_t *ptr, entry;
+	struct page **src_pages;
+	unsigned int npages;
+	void *vaddr = NULL;
+	int i;
+
+	/*
+	 * 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.43.0

Quoted 1 line · steven chen

From: Steven Chen <[email protected]>

^^^^^^

Quoted 6 lines

Implement kimage_map_segment() to enable IMA to map the measurement log list to the kimage structure during the kexec 'load' stage. This function gathers the source pages within the specified address range, and maps them to a contiguous virtual address range.

Quoted 5 lines

This is a preparation for later usage.

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

From: Tushar Sugandhi <[email protected]>

^^^^^^

Quoted 1 line

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

^^^^^^^

Quoted 5 lines

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

^^^^^

The signing on this patch is a little confusing. I can't see who is the real author, who is the co-author, between you and Tushar. You may need to refer to Documentation/process/5.Posting.rst to make that clear.

Quoted 98 lines

Acked-by Baoquan He <[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..a5e378e1dc7f 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 src_page_addr, dest_page_addr = 0;
+	unsigned long eaddr = addr + size;
+	kimage_entry_t *ptr, entry;
+	struct page **src_pages;
+	unsigned int npages;
+	void *vaddr = NULL;
+	int i;
+
+	/*
+	 * 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.43.0

Quoted 3 lines · Baoquan He

Quoted 1 line · steven chen

From: Steven Chen <[email protected]>

^^^^^^

As James Bottomley previously explained[1], if you haven't made any changes to Tushar's patch, then the very first line of the patch description would be "From: Tushar Sugandhi <[email protected]>" followed by a blank line. If there is a minor change, you would add "<your email address>: explanation". For example:

Steven Chen <[email protected]>: modified patch description

[1]
https://lore.kernel.org/lkml/58e70121aaee33679ac295847197c1e5511b2a81.camel@HansenPartnership.com/

Quoted 6 lines

Quoted 6 lines

Implement kimage_map_segment() to enable IMA to map the measurement log list to the kimage structure during the kexec 'load' stage. This function gathers the source pages within the specified address range, and maps them to a contiguous virtual address range.

Quoted 6 lines

Quoted 5 lines

This is a preparation for later usage.

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

From: Tushar Sugandhi <[email protected]>

^^^^^^

Neither "Author:" nor "From:" belong here. Please remove.

Quoted 2 lines

Quoted 1 line

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

^^^^^^^

Having Tushar's "Signed-off-by" tag and yours below indicate that you modified the original author's patch.

Quoted 13 lines

Quoted 6 lines

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

^^^^^

The signing on this patch is a little confusing. I can't see who is the real author, who is the co-author, between you and Tushar. You may need to refer to Documentation/process/5.Posting.rst to make that clear.

Quoted 1 line

Acked-by Baoquan He <[email protected]>

Quoted 11 lines · Mimi Zohar

Quoted 3 lines · Baoquan He

Quoted 1 line · steven chen

From: Steven Chen <[email protected]>

^^^^^^

As James Bottomley previously explained[1], if you haven't made any changes to Tushar's patch, then the very first line of the patch description would be "From: Tushar Sugandhi <[email protected]>" followed by a blank line. If there is a minor change, you would add "<your email address>: explanation". For example:

Steven Chen <[email protected]>: modified patch description

To clarify: This line would be included below with your Signed-off-by tag.

Quoted 14 lines

[1]
https://lore.kernel.org/lkml/58e70121aaee33679ac295847197c1e5511b2a81.camel@HansenPartnership.com/

Quoted 11 lines

Quoted 10 lines

Implement kimage_map_segment() to enable IMA to map the measurement log list to the kimage structure during the kexec 'load' stage. This function gathers the source pages within the specified address range, and maps them to a contiguous virtual address range.

This is a preparation for later usage.

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

From: Tushar Sugandhi <[email protected]>

^^^^^^

Quoted 6 lines

Neither "Author:" nor "From:" belong here. Please remove.

Quoted 2 lines

Quoted 1 line

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

^^^^^^^

Having Tushar's "Signed-off-by" tag and yours below indicate that you modified the original author's patch.

To clarify: "Just" having Tushar's "Signed-off-by" tag and yours below indicate that you modified the original author's patch.

Quoted 12 lines

Quoted 12 lines

Quoted 5 lines

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

^^^^^

The signing on this patch is a little confusing. I can't see who is the real author, who is the co-author, between you and Tushar. You may need to refer to Documentation/process/5.Posting.rst to make that clear.

Quoted 1 line

Acked-by Baoquan He <[email protected]>

Quoted 32 lines · Mimi Zohar

Quoted 11 lines · Mimi Zohar

Quoted 3 lines · Baoquan He

Quoted 1 line · steven chen

From: Steven Chen <[email protected]>

^^^^^^

As James Bottomley previously explained[1], if you haven't made any changes to Tushar's patch, then the very first line of the patch description would be "From: Tushar Sugandhi <[email protected]>" followed by a blank line. If there is a minor change, you would add "<your email address>: explanation". For example:

Steven Chen <[email protected]>: modified patch description

To clarify: This line would be included below with your Signed-off-by tag.

Quoted 18 lines

[1]
https://lore.kernel.org/lkml/58e70121aaee33679ac295847197c1e5511b2a81.camel@HansenPartnership.com/

Quoted 11 lines

Quoted 10 lines

Implement kimage_map_segment() to enable IMA to map the measurement log list to the kimage structure during the kexec 'load' stage. This function gathers the source pages within the specified address range, and maps them to a contiguous virtual address range.

This is a preparation for later usage.

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

From: Tushar Sugandhi <[email protected]>

^^^^^^

Neither "Author:" nor "From:" belong here. Please remove.

Quoted 2 lines

Quoted 1 line

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

^^^^^^^

Quoted 4 lines

Quoted 2 lines

Having Tushar's "Signed-off-by" tag and yours below indicate that you modified the original author's patch.

To clarify: "Just" having Tushar's "Signed-off-by" tag and yours below indicate that you modified the original author's patch.

Hi Mimi,

I will update it in next version.

Just wandering are you done reviewing or still need more time?

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 6 lines

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

^^^^^

The signing on this patch is a little confusing. I can't see who is the real author, who is the co-author, between you and Tushar. You may need to refer to Documentation/process/5.Posting.rst to make that clear.

Quoted 1 line

Acked-by Baoquan He <[email protected]>

Quoted 32 lines · steven chen

Quoted 31 lines · Mimi Zohar

Quoted 11 lines · Mimi Zohar

Quoted 3 lines · Baoquan He

Quoted 1 line · steven chen

From: Steven Chen <[email protected]>

^^^^^^

As James Bottomley previously explained[1], if you haven't made any changes to Tushar's patch, then the very first line of the patch description would be "From: Tushar Sugandhi <[email protected]>" followed by a blank line. If there is a minor change, you would add "<your email address>: explanation". For example:

Steven Chen <[email protected]>: modified patch description

To clarify: This line would be included below with your Signed-off-by tag.

Quoted 17 lines

[1]
https://lore.kernel.org/lkml/58e70121aaee33679ac295847197c1e5511b2a81.camel@HansenPartnership.com/

Quoted 11 lines

Quoted 10 lines

Implement kimage_map_segment() to enable IMA to map the measurement log list to the kimage structure during the kexec 'load' stage. This function gathers the source pages within the specified address range, and maps them to a contiguous virtual address range.

This is a preparation for later usage.

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

From: Tushar Sugandhi <[email protected]>

    ^^^^^^

Neither "Author:" nor "From:" belong here. Please remove.

Quoted 1 line

Quoted 1 line

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

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

    ^^^^^^^

Having Tushar's "Signed-off-by" tag and yours below indicate that you modified the original author's patch.

To clarify: "Just" having Tushar's "Signed-off-by" tag and yours below indicate that you modified the original author's patch.

Hi Mimi,

I will update it in next version.

Sigh, after reviewing your discussion with Baoquan, I'm not sure whether my comment this morning added more confusion.

Option 1: Include a single "From:" line at the very top to change the author from yourself to someone else. The following from line would make Tushar the patch author: From: Tushar Sugandhi <[email protected]>

In addition, any minor changes you made should be added before your Signed-off- by tag. For example: Steven Chen <[email protected]>: modified patch description

Option 2: As mentioned previously and now discussed with Baoquan, adding "Co- developed-by:"

Quoted 1 line

Just wandering are you done reviewing or still need more time?

Yes, I just finished reviewing/testing. The patch descriptions are looking much better. As Baoquan reminded you, please remember to update the "ima: define and call ima_alloc_kexec_file_buf()" patch description.

thanks,

Mimi

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 5 lines

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

    ^^^^^

The signing on this patch is a little confusing. I can't see who is the real author, who is the co-author, between you and Tushar. You may need to refer to Documentation/process/5.Posting.rst to make that clear.

Quoted 1 line

Acked-by Baoquan He <[email protected]>

From: Steven Chen <[email protected]>

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

Later, the IMA segment will be updated with the measurement log at the kexec execute stage when a kexec reboot is initiated. Therefore, the digests should be updated for the IMA segment in the normal case. The problem is that 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.

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.

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

Cc Eric Biederman <[email protected]>

Cc Baoquan He <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Dave Young <[email protected]>

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

Signed-off-by Steven Chen <[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 b12ac3619b8f..7e0a19c3483f 100644
@@ -145,6 +145,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");

@@ -155,6 +156,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_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		      kbuf.mem);

Signature · 1 line

2.43.0

Quoted 21 lines · steven chen

From: Steven Chen <[email protected]>

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

Later, the IMA segment will be updated with the measurement log at the kexec execute stage when a kexec reboot is initiated. Therefore, the digests should be updated for the IMA segment in the normal case. The problem is that 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.

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.

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

Quoted 6 lines

Cc Eric Biederman <[email protected]>

Cc Baoquan He <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Dave Young <[email protected]>

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

Signed-off-by Steven Chen <[email protected]>

You may need to set tags as below for this patch:

Co-developed-by Tushar Sugandhi <[email protected]>

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

Signed-off-by Steven Chen <[email protected]>

=======Quoted from Documentation/process/5.Posting.rst=====
- Co-developed-by: states that the patch was co-created by several developers; it is a used to give attribution to co-authors (in addition to the author attributed by the From: tag) when multiple people work on a single patch. Every Co-developed-by: must be immediately followed by a Signed-off-by: of the associated co-author. Details and examples can be found in :ref:Documentation/process/submitting-patches.rst <submittingpatches>.
========

Other than this nit, this looks good to me:

Acked-by Baoquan He <[email protected]>

Quoted 84 lines

---
  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 b12ac3619b8f..7e0a19c3483f 100644
@@ -145,6 +145,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");

@@ -155,6 +156,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_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
  		      kbuf.mem);

Signature · 1 line

2.43.0

From: Steven Chen <[email protected]>

The IMA log is currently copied to the new kernel during kexec 'load' using ima_dump_measurement_list(). However, the log copied at kexec 'load' may result in loss of IMA measurements that only occurred after kexec "load'. Setup the needed infrastructure to move the IMA log copy from kexec 'load' to 'execute'.

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

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

Set the priority of register_reboot_notifier to INT_MIN to ensure that the IMA log copy operation will happen at the end of the operation chain, so that all the IMA measurement records extended into the TPM are copied

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 Mimi Zohar <[email protected]>

---
 include/linux/ima.h                |  3 ++
 security/integrity/ima/ima_kexec.c | 47 ++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

include/linux/ima.h +3 -0

index 0bae61a15b60..8e29cb4e6a01 100644
@@ -32,6 +32,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

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

index 7e0a19c3483f..e79f6caf895b 100644
@@ -12,10 +12,14 @@
 #include <linux/kexec.h>
 #include <linux/of.h>
 #include <linux/ima.h>
+#include <linux/reboot.h>
+#include <asm/page.h>
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+static bool ima_kexec_update_registered;
 static struct seq_file ima_kexec_file;
+static void *ima_kexec_buffer;
 
 static void ima_free_kexec_file_buf(struct seq_file *sf)
 {

@@ -162,6 +166,49 @@ void ima_add_kexec_buffer(struct kimage *image)
 	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		      kbuf.mem);
 }
+
+/*
+ * Called during kexec execute so that IMA can update the measurement list.
+ */
+static int ima_update_kexec_buffer(struct notifier_block *self,
+				   unsigned long action, void *data)
+{
+	return NOTIFY_OK;
+}
+
+static struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+	.priority = INT_MIN
+};
+
+/*
+ * 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("Could not map measurements buffer.\n");
+		return;
+	}
+
+	if (!ima_kexec_update_registered) {
+		register_reboot_notifier(&update_buffer_nb);
+		ima_kexec_update_registered = true;
+	}
+}
+
 #endif /* IMA_KEXEC */
 
 /*

Signature · 1 line

2.43.0

Quoted 17 lines · steven chen

From: Steven Chen <[email protected]>

The IMA log is currently copied to the new kernel during kexec 'load' using ima_dump_measurement_list(). However, the log copied at kexec 'load' may result in loss of IMA measurements that only occurred after kexec "load'. Setup the needed infrastructure to move the IMA log copy from kexec 'load' to 'execute'.

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

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

Quoted 5 lines

Set the priority of register_reboot_notifier to INT_MIN to ensure that the IMA log copy operation will happen at the end of the operation chain, so that all the IMA measurement records extended into the TPM are copied

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

^^^

The tag

Quoted 10 lines

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 Mimi Zohar <[email protected]>

---
  include/linux/ima.h                |  3 ++
  security/integrity/ima/ima_kexec.c | 47 ++++++++++++++++++++++++++++++
  2 files changed, 50 insertions(+)

Other than the tag, this looks good to me:

Acked-by Baoquan He <[email protected]>

Quoted 86 lines

include/linux/ima.h +3 -0

index 0bae61a15b60..8e29cb4e6a01 100644
@@ -32,6 +32,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

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

index 7e0a19c3483f..e79f6caf895b 100644
@@ -12,10 +12,14 @@
  #include <linux/kexec.h>
  #include <linux/of.h>
  #include <linux/ima.h>
+#include <linux/reboot.h>
+#include <asm/page.h>
  #include "ima.h"
 
  #ifdef CONFIG_IMA_KEXEC
+static bool ima_kexec_update_registered;
  static struct seq_file ima_kexec_file;
+static void *ima_kexec_buffer;
 
  static void ima_free_kexec_file_buf(struct seq_file *sf)
  {

@@ -162,6 +166,49 @@ void ima_add_kexec_buffer(struct kimage *image)
  	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
  		      kbuf.mem);
  }
+
+/*
+ * Called during kexec execute so that IMA can update the measurement list.
+ */
+static int ima_update_kexec_buffer(struct notifier_block *self,
+				   unsigned long action, void *data)
+{
+	return NOTIFY_OK;
+}
+
+static struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+	.priority = INT_MIN
+};
+
+/*
+ * 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("Could not map measurements buffer.\n");
+		return;
+	}
+
+	if (!ima_kexec_update_registered) {
+		register_reboot_notifier(&update_buffer_nb);
+		ima_kexec_update_registered = true;
+	}
+}
+
  #endif /* IMA_KEXEC */
 
  /*

Signature · 1 line

2.43.0

From: Steven Chen <[email protected]>

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

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

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

Suggested-by Stefan Berger <[email protected]>

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

Signed-off-by Steven Chen <[email protected]>

---
 security/integrity/ima/Kconfig     | 11 +++++++++++
 security/integrity/ima/ima_kexec.c | 16 +++++++++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)

security/integrity/ima/Kconfig +11 -0

index 475c32615006..976e75f9b9ba 100644
@@ -321,4 +321,15 @@ config IMA_DISABLE_HTABLE
 	help
 	   This option disables htable to allow measurement of duplicate records.
 
+config IMA_KEXEC_EXTRA_MEMORY_KB
+	int "Extra memory for IMA measurements added during kexec soft reboot"
+	range 0 40
+	depends on IMA_KEXEC
+	default 0
+	help
+	  IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be
+	  allocated (in kb) for IMA measurements added during kexec soft reboot.
+	  If set to the default value of 0, an extra half page of memory for those
+	  additional measurements will be allocated.
+
 endif

security/integrity/ima/ima_kexec.c +11 -5

index ed867734ee70..d1c9d369ba08 100644
@@ -118,6 +118,7 @@ void ima_add_kexec_buffer(struct kimage *image)
 				  .buf_min = 0, .buf_max = ULONG_MAX,
 				  .top_down = true };
 	unsigned long binary_runtime_size;
+	unsigned long extra_memory;
 
 	/* use more understandable variable names than defined in kbuf */
 	size_t kexec_buffer_size = 0;

@@ -125,15 +126,20 @@ void ima_add_kexec_buffer(struct kimage *image)
 	int ret;
 
 	/*
-	 * Reserve an extra half page of memory for additional measurements
-	 * added during the kexec load.
+	 * Reserve extra memory for measurements added during kexec.
 	 */
-	binary_runtime_size = ima_get_binary_runtime_size();
+	if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0)
+		extra_memory = PAGE_SIZE / 2;
+	else
+		extra_memory = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
+
+	binary_runtime_size = ima_get_binary_runtime_size() + extra_memory;
+
 	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");

Signature · 1 line

2.43.0

Quoted 11 lines · steven chen

From: Steven Chen <[email protected]>

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

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

Quoted 5 lines

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

Suggested-by Stefan Berger <[email protected]>

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

    ^^^^^^^^^^^^^^^^

Quoted 5 lines

Signed-off-by Steven Chen <[email protected]>

---
  security/integrity/ima/Kconfig     | 11 +++++++++++
  security/integrity/ima/ima_kexec.c | 16 +++++++++++-----
  2 files changed, 22 insertions(+), 5 deletions(-)

The contributor's tag need be updated too, otherwise,

Acked-by Baoquan He <[email protected]>

Quoted 61 lines

security/integrity/ima/Kconfig +11 -0

index 475c32615006..976e75f9b9ba 100644
@@ -321,4 +321,15 @@ config IMA_DISABLE_HTABLE
  	help
  	   This option disables htable to allow measurement of duplicate records.
 
+config IMA_KEXEC_EXTRA_MEMORY_KB
+	int "Extra memory for IMA measurements added during kexec soft reboot"
+	range 0 40
+	depends on IMA_KEXEC
+	default 0
+	help
+	  IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be
+	  allocated (in kb) for IMA measurements added during kexec soft reboot.
+	  If set to the default value of 0, an extra half page of memory for those
+	  additional measurements will be allocated.
+
  endif

security/integrity/ima/ima_kexec.c +11 -5

index ed867734ee70..d1c9d369ba08 100644
@@ -118,6 +118,7 @@ void ima_add_kexec_buffer(struct kimage *image)
  				  .buf_min = 0, .buf_max = ULONG_MAX,
  				  .top_down = true };
  	unsigned long binary_runtime_size;
+	unsigned long extra_memory;
 
  	/* use more understandable variable names than defined in kbuf */
  	size_t kexec_buffer_size = 0;

@@ -125,15 +126,20 @@ void ima_add_kexec_buffer(struct kimage *image)
  	int ret;
 
  	/*
-	 * Reserve an extra half page of memory for additional measurements
-	 * added during the kexec load.
+	 * Reserve extra memory for measurements added during kexec.
  	 */
-	binary_runtime_size = ima_get_binary_runtime_size();
+	if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0)
+		extra_memory = PAGE_SIZE / 2;
+	else
+		extra_memory = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
+
+	binary_runtime_size = ima_get_binary_runtime_size() + extra_memory;
+
  	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");

Signature · 1 line

2.43.0

From: Steven Chen <[email protected]>

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

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

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

These critical data events are displayed as hex encoded ascii in the ascii_runtime_measurement_list. Verifying the critical data hash requires calculating the hash of the decoded ascii string.

For example, to verify the 'kexec_load' data hash:

sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep kexec_load | cut -d' ' -f 6 | xxd -r -p | sha256sum

To verify the 'kexec_execute' data hash:

sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep kexec_execute | cut -d' ' -f 6 | xxd -r -p | sha256sum

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

Signed-off-by Steven Chen <[email protected]>

Reviewed-by Mimi Zohar <[email protected]>

---
 security/integrity/ima/ima.h       |  6 ++++++
 security/integrity/ima/ima_kexec.c | 21 +++++++++++++++++++++
 security/integrity/ima/ima_queue.c |  5 +++++
 3 files changed, 32 insertions(+)

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

index 24d09ea91b87..34815baf5e21 100644
@@ -240,6 +240,12 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 				   unsigned long flags, bool create);
 #endif
 
+#ifdef CONFIG_IMA_KEXEC
+void ima_measure_kexec_event(const char *event_name);
+#else
+static inline void ima_measure_kexec_event(const char *event_name) {}
+#endif
+
 /*
  * The default binary_runtime_measurements list format is defined as the
  * platform native format.  The canonical format is defined as little-endian.

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

index d1c9d369ba08..38cb2500f4c3 100644
@@ -17,6 +17,8 @@
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+#define IMA_KEXEC_EVENT_LEN 256
+
 static bool ima_kexec_update_registered;
 static struct seq_file ima_kexec_file;
 static size_t kexec_segment_size;

@@ -31,6 +33,24 @@ static void ima_free_kexec_file_buf(struct seq_file *sf)
 	sf->count = 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;
+	int n;
+
+	buf_size = ima_get_binary_runtime_size();
+	len = atomic_long_read(&ima_htable.len);
+
+	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);
+
+	ima_measure_critical_data("ima_kexec", event_name, ima_kexec_event, n, false, NULL, 0);
+}
+
 static int ima_alloc_kexec_file_buf(size_t segment_size)
 {
 	/*

@@ -53,6 +73,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
 out:
 	ima_kexec_file.read_pos = 0;
 	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
+	ima_measure_kexec_event("kexec_load");
 
 	return 0;
 }

security/integrity/ima/ima_queue.c +5 -0

index 83d53824aa98..590637e81ad1 100644
@@ -241,6 +241,11 @@ static int ima_reboot_notifier(struct notifier_block *nb,
 			       unsigned long action,
 			       void *data)
 {
+#ifdef CONFIG_IMA_KEXEC
+	if (action == SYS_RESTART && data && !strcmp(data, "kexec reboot"))
+		ima_measure_kexec_event("kexec_execute");
+#endif
+
 	ima_measurements_suspend();
 
 	return NOTIFY_DONE;

Signature · 1 line

2.43.0

Quoted 33 lines · steven chen

From: Steven Chen <[email protected]>

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

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

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

These critical data events are displayed as hex encoded ascii in the ascii_runtime_measurement_list. Verifying the critical data hash requires calculating the hash of the decoded ascii string.

For example, to verify the 'kexec_load' data hash:

sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep kexec_load | cut -d' ' -f 6 | xxd -r -p | sha256sum

To verify the 'kexec_execute' data hash:

Quoted 4 lines

sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep kexec_execute | cut -d' ' -f 6 | xxd -r -p | sha256sum

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

^^^^^

Quoted 1 line

Signed-off-by Steven Chen <[email protected]>

^^^^^

Quoted 6 lines

Reviewed-by Mimi Zohar <[email protected]>

---
  security/integrity/ima/ima.h       |  6 ++++++
  security/integrity/ima/ima_kexec.c | 21 +++++++++++++++++++++
  security/integrity/ima/ima_queue.c |  5 +++++
  3 files changed, 32 insertions(+)

Acked-by Baoquan He <[email protected]>

Quoted 82 lines

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

index 24d09ea91b87..34815baf5e21 100644
@@ -240,6 +240,12 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
  				   unsigned long flags, bool create);
  #endif
 
+#ifdef CONFIG_IMA_KEXEC
+void ima_measure_kexec_event(const char *event_name);
+#else
+static inline void ima_measure_kexec_event(const char *event_name) {}
+#endif
+
  /*
   * The default binary_runtime_measurements list format is defined as the
   * platform native format.  The canonical format is defined as little-endian.

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

index d1c9d369ba08..38cb2500f4c3 100644
@@ -17,6 +17,8 @@
  #include "ima.h"
 
  #ifdef CONFIG_IMA_KEXEC
+#define IMA_KEXEC_EVENT_LEN 256
+
  static bool ima_kexec_update_registered;
  static struct seq_file ima_kexec_file;
  static size_t kexec_segment_size;

@@ -31,6 +33,24 @@ static void ima_free_kexec_file_buf(struct seq_file *sf)
  	sf->count = 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;
+	int n;
+
+	buf_size = ima_get_binary_runtime_size();
+	len = atomic_long_read(&ima_htable.len);
+
+	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);
+
+	ima_measure_critical_data("ima_kexec", event_name, ima_kexec_event, n, false, NULL, 0);
+}
+
  static int ima_alloc_kexec_file_buf(size_t segment_size)
  {
  	/*

@@ -53,6 +73,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
  out:
  	ima_kexec_file.read_pos = 0;
  	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
+	ima_measure_kexec_event("kexec_load");
 
  	return 0;
  }

security/integrity/ima/ima_queue.c +5 -0

index 83d53824aa98..590637e81ad1 100644
@@ -241,6 +241,11 @@ static int ima_reboot_notifier(struct notifier_block *nb,
  			       unsigned long action,
  			       void *data)
  {
+#ifdef CONFIG_IMA_KEXEC
+	if (action == SYS_RESTART && data && !strcmp(data, "kexec reboot"))
+		ima_measure_kexec_event("kexec_execute");
+#endif
+
  	ima_measurements_suspend();
 
  	return NOTIFY_DONE;

Signature · 1 line

2.43.0

From: Steven Chen <[email protected]>

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'.

Currently, the kernel behavior during kexec load is to fetch the IMA measurements log from TPM PCRs and store it in a buffer. When a kexec reboot is triggered, this stored log buffer is carried over to the second kernel. However, the time gap between kexec load and kexec reboot can be very long. During this time window, new events extended into TPM PCRs miss the chance to be carried over to the second kernel. 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.

Quoted 6 lines · steven chen

From: Steven Chen <[email protected]>

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'.

Quoted 8 lines

Currently, the kernel behavior during kexec load is to fetch the IMA measurements log from TPM PCRs and store it in a buffer. When a kexec reboot is triggered, this stored log buffer is carried over to the second kernel. However, the time gap between kexec load and kexec reboot can be very long. During this time window, new events extended into TPM PCRs miss the chance to be carried over to the second kernel. 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.

Tested-by Stefan Berger <[email protected]> # ppc64/kvm