ima: kexec: measure events between kexec load and execute

The name of the local variable "file" of type seq_file defined in the ima_dump_measurement_list function is too generic. To better reflect the purpose of the variable, rename it to "ima_kexec_file". This change will help improve code readability and maintainability by making the variable's role more explicit.

The variable ima_kexec_file is indeed the memory allocated for copying IMA measurement records. The ima_dump_measurement_list function calculates the actual memory occupied by the IMA logs and compares it with the allocated memory. If there is enough memory, it copies all IMA measurement records; otherwise, it does not copy any records, which would result in a failure of remote attestation.

Suggested-by Mimi Zohar <[email protected]>

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

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

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

index 9d45f4d26f73..8567619889d1 100644

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

+ */
  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 +63,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;
  }
 

Quoted 26 lines · steven chen

The name of the local variable "file" of type seq_file defined in the ima_dump_measurement_list function is too generic. To better reflect the purpose of the variable, rename it to "ima_kexec_file". This change will help improve code readability and maintainability by making the variable's role more explicit.

The variable ima_kexec_file is indeed the memory allocated for copying IMA measurement records. The ima_dump_measurement_list function calculates the actual memory occupied by the IMA logs and compares it with the allocated memory. If there is enough memory, it copies all IMA measurement records; otherwise, it does not copy any records, which would result in a failure of remote attestation.

Suggested-by Mimi Zohar <[email protected]>

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

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

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

index 9d45f4d26f73..8567619889d1 100644

security/integrity/ima/ima_kexec.c line 19

Quoted 5 lines

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

@@ -15,33 +15,41 @@
  #include "ima.h"
  #ifdef CONFIG_IMA_KEXEC
+/*
+ * Copy the measurement list to the allocated memory
+ * compare the size of IMA measurement list with the size of the allocated memory

Compare the size of the IMA ... memory.

security/integrity/ima/ima_kexec.c line 24

Quoted 4 lines

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

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

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, return an error otherwise.

Quoted 35 lines · Stefan Berger

Quoted 25 lines · steven chen

The name of the local variable "file" of type seq_file defined in the ima_dump_measurement_list function is too generic. To better reflect the purpose of the variable, rename it to "ima_kexec_file". This change will help improve code readability and maintainability by making the variable's role more explicit.

The variable ima_kexec_file is indeed the memory allocated for copying IMA measurement records. The ima_dump_measurement_list function calculates the actual memory occupied by the IMA logs and compares it with the allocated memory. If there is enough memory, it copies all IMA measurement records; otherwise, it does not copy any records, which would result in a failure of remote attestation.

Suggested-by Mimi Zohar <[email protected]>

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

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

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

index 9d45f4d26f73..8567619889d1 100644

security/integrity/ima/ima_kexec.c line 19

Quoted 5 lines

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

@@ -15,33 +15,41 @@
   #include "ima.h"
   #ifdef CONFIG_IMA_KEXEC
+/*
+ * Copy the measurement list to the allocated memory
+ * compare the size of IMA measurement list with the size of the allocated memory

Compare the size of the IMA ... memory.

Quoted 7 lines

security/integrity/ima/ima_kexec.c line 24

Quoted 4 lines

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

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

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, return an error otherwise.

Ack the suggested change.

The name of the local variable "file" of type seq_file defined in the ima_dump_measurement_list function is too generic. To better reflect the purpose of the variable, rename it to "ima_kexec_file". This change will help improve code readability and maintainability by making the variable's role more explicit.

The variable ima_kexec_file is indeed the memory allocated for copying IMA measurement records. The ima_dump_measurement_list function calculates the actual memory occupied by the IMA logs and compares it with the allocated memory. If there is enough memory, it copies all IMA measurement records; otherwise, it does not copy any records, which would result in a failure of remote attestation.

Suggested-by Mimi Zohar <[email protected]>

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

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

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

index 9d45f4d26f73..8567619889d1 100644
@@ -15,33 +15,41 @@
 #include "ima.h"

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

 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 +63,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;
 }
 

Fix spelling: set_file

Quoted 5 lines · steven chen

The name of the local variable "file" of type seq_file defined in the ima_dump_measurement_list function is too generic. To better reflect the purpose of the variable, rename it to "ima_kexec_file". This change will help improve code readability and maintainability by making the variable's role more explicit.

The reason for making the variable name change is the variable scope.

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

Quoted 6 lines

The variable ima_kexec_file is indeed the memory allocated for copying IMA measurement records. The ima_dump_measurement_list function calculates the actual memory occupied by the IMA logs and compares it with the allocated memory. If there is enough memory, it copies all IMA measurement records; otherwise, it does not copy any records, which would result in a failure of remote attestation.

This paragraph is not applicable to the patch change.

Quoted 12 lines

Suggested-by Mimi Zohar <[email protected]>

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

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

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

index 9d45f4d26f73..8567619889d1 100644
@@ -15,33 +15,41 @@
  #include "ima.h"

security/integrity/ima/ima_kexec.c line 25

Quoted 10 lines

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

 
  #ifdef CONFIG_IMA_KEXEC
+/*
+ * 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
+ */

Please minimize patch changes. Before posting, please look at the patch(s) and remove anything not applicable to it. In this case, the comment is not applicable to the variable name change.

thanks,

Mimi

Quoted 59 lines

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

  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 +63,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);
 

security/integrity/ima/ima_kexec.c static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, · line 86

Quoted 11 lines

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

-	*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;
  }
 

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().
- Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions.

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

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

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

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

index 8567619889d1..45170e283272 100644
@@ -15,6 +15,48 @@
 #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

@@ -26,23 +68,16 @@
 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 */

@@ -79,8 +114,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;
 }
 

@@ -119,6 +152,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) {

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 9 lines

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().
- Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions.

We may not need above details about code change because it's not so difficult to get them from patch.

Quoted 33 lines

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

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

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

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

index 8567619889d1..45170e283272 100644
@@ -15,6 +15,48 @@
 #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)

security/integrity/ima/ima_kexec.c line 42

Quoted 8 lines

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

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

Quoted 72 lines

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

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

@@ -26,23 +68,16 @@
 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 */

@@ -79,8 +114,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;
 }
 

@@ -119,6 +152,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) {

@@ -140,6 +179,12 @@ void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_buffer_size = kexec_segment_size;
 	image->ima_buffer = kexec_buffer;
 

security/integrity/ima/ima_kexec.c void ima_add_kexec_buffer(struct kimage *image) · line 186

Quoted 5 lines

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

+	/*
+	 * kexec owns kexec_buffer after kexec_add_buffer() is called
+	 * and it will vfree() that buffer.
+	 */
+	ima_reset_kexec_file(&ima_kexec_file);

I can't see why we need call ima_reset_kexec_file() here. If we need reuse the buffer, we will reset the needed fields at the end of ima_alloc_kexec_file_buf(). Not sure if I miss anything.

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 */

        return 0;
}

Quoted 17 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 9 lines

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().
- Make necessary changes to the function ima_add_kexec_buffer() to call the above two functions.

Quoted 2 lines

We may not need above details about code change because it's not so difficult to get them from patch.

Agreed. The changes don't even reflect the current patch. Please remove the entire section.

Quoted 33 lines

Quoted 33 lines

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

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

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

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

index 8567619889d1..45170e283272 100644
@@ -15,6 +15,48 @@
 #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)

Quoted 8 lines

security/integrity/ima/ima_kexec.c line 42

Quoted 8 lines

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

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

The call to ima_reset_kexec_file() in ima_add_kexec_buffer() resets ima_kexec_file.buf() hiding the fact that the above test always fails and falls through. As a result, 'buf' is always being re-allocated.

Quoted 72 lines

Quoted 72 lines

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

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

@@ -26,23 +68,16 @@
 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 */

@@ -79,8 +114,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;
 }
 

@@ -119,6 +152,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) {

@@ -140,6 +179,12 @@ void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_buffer_size = kexec_segment_size;
 	image->ima_buffer = kexec_buffer;
 

Quoted 9 lines

security/integrity/ima/ima_kexec.c void ima_add_kexec_buffer(struct kimage *image) · line 186

Quoted 5 lines

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

+	/*
+	 * kexec owns kexec_buffer after kexec_add_buffer() is called
+	 * and it will vfree() that buffer.
+	 */
+	ima_reset_kexec_file(&ima_kexec_file);

I can't see why we need call ima_reset_kexec_file() here. If we need reuse the buffer, we will reset the needed fields at the end of ima_alloc_kexec_file_buf(). Not sure if I miss anything.

Without ima_reset_kexec_file(), calling 'kexec load' consecutively without "kexec -s -e" in between fails.

# kexec -s -l /boot/vmlinuz-`uname -r` --initrd=/boot/initramfs-`uname -r`.img --reuse-cmdline
# kexec -s -l /boot/vmlinuz-`uname -r` --initrd=/boot/initramfs-`uname -r`.img --reuse-cmdline
Killed

As mentioned above, the call to ima_reset_kexec_file() resets ima_kexec_file.buf, so the segment size test always fails and the memory is being allocated.

Quoted 29 lines

Quoted 29 lines

Quoted 29 lines

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

index 8567619889d1..45170e283272 100644
@@ -15,6 +15,48 @@
   #include "ima.h"

  #ifdef CONFIG_IMA_KEXEC

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

+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

Quoted 8 lines

Quoted 4 lines

security/integrity/ima/ima_kexec.c line 42

Quoted 4 lines

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

+	 * changed from the previous kexec 'load' call.
+	 */
+	if (ima_kexec_file.buf && ima_kexec_file.size == segment_size)
+		goto out;

The call to ima_reset_kexec_file() in ima_add_kexec_buffer() resets ima_kexec_file.buf() hiding the fact that the above test always fails and falls through.  As a result, 'buf' is always being re-allocated.

Quoted 28 lines · Baoquan He

Quoted 28 lines

Quoted 28 lines

Quoted 28 lines

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

index 8567619889d1..45170e283272 100644
@@ -15,6 +15,48 @@
   #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.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

security/integrity/ima/ima_kexec.c line 42

Quoted 5 lines

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

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

The call to ima_reset_kexec_file() in ima_add_kexec_buffer() resets ima_kexec_file.buf() hiding the fact that the above test always fails and falls through.  As a result, 'buf' is always being re-allocated.

Hi Steven,

[Reiterating the comment in the "ima: kexec: move IMA log copy from kexec load to execute" thread, here, for completeness.]

Instead of adding and then removing the ima_reset_kexec_file() call from ima_add_kexec_buffer(), defer adding the segment size test to when it is actually possible for the segment size to change. Please make the segment size test as a separate patch.

ima_reset_kexec_file() will then only be called by ima_free_kexec_file_buf(). Inline the ima_reset_kexec_file() code in ima_free_kexec_file_buf().

thanks,

Mimi

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 a mismatch between TPM PCR quotes and the actual IMA measurements list after kexec reboot, leading to remote attestation failure.

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

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

To copy IMA measurement logs during the kexec operation, IMA allocates 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 the measurement log list to the kimage structure during the 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..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.25.1

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 called 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 45170e283272..34020b1513b0 100644
@@ -168,6 +168,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");

@@ -178,6 +179,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

ima_dump_measurement_list() is called during kexec 'load', which may result in loss of IMA measurements during kexec soft reboot. Due to missed measurements that only occurred after kexec 'load', this function needs to be called during kexec 'execute'.

This patch includes the following changes:
- Implement kimage_file_post_load() function to be invoked after the new kernel image has been loaded for kexec.
- Call kimage_file_post_load() from kexec_file_load() syscall only for kexec soft reboot scenarios and not for KEXEC_FILE_ON_CRASH. It will map the IMA segment, and register reboot notifier for the function ima_update_kexec_buffer() which would copy the IMA log at kexec soft reboot.
- Make kexec_segment_size variable local static to the file so that it becomes accessible both during kexec 'load' and 'execute'.
- Move ima_dump_measurement_list() call from ima_add_kexec_buffer() to ima_update_kexec_buffer().
- Copy the measurement list as much as possible.

- Remove ima_reset_kexec_file() call from ima_add_kexec_buffer(), now that the buffer is being copied at kexec 'execute', and resetting the file at kexec 'load' would corrupt the buffer.

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

---
 kernel/kexec_file.c                | 10 ++++++
 security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++------------
 2 files changed, 40 insertions(+), 21 deletions(-)

kernel/kexec_file.c +10 -0

index 606132253c79..ab449b43aaee 100644
@@ -201,6 +201,13 @@ kimage_validate_signature(struct kimage *image)
 }
 #endif
 
+static void kimage_file_post_load(struct kimage *image)
+{
+#ifdef CONFIG_IMA_KEXEC
+	ima_kexec_post_load(image);
+#endif
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list

@@ -428,6 +435,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 
 	kimage_terminate(image);
 
+	if (!(flags & KEXEC_FILE_ON_CRASH))
+		kimage_file_post_load(image);
+
 	ret = machine_kexec_post_load(image);
 	if (ret)
 		goto out;

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

index 9336c4f60650..c390c745f882 100644
@@ -19,6 +19,7 @@
 #ifdef CONFIG_IMA_KEXEC
 static bool ima_kexec_update_registered;
 static struct seq_file ima_kexec_file;
+static size_t kexec_segment_size;
 static void *ima_kexec_buffer;
 
 static void ima_reset_kexec_file(struct seq_file *sf)

@@ -67,7 +68,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
  *    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
+ *        copy the measurement list as much as possible.
  */
 static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 				     unsigned long segment_size)

@@ -95,9 +96,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 		}
 	}
 
-	if (ret < 0)
-		goto out;
-
 	/*
 	 * fill in reserved space with some buffer details
 	 * (eg. version, buffer size, number of measurements)

@@ -117,7 +115,7 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 
 	*buffer_size = ima_kexec_file.count;
 	*buffer = ima_kexec_file.buf;
-out:
+
 	return ret;
 }
 

@@ -135,9 +133,8 @@ void ima_add_kexec_buffer(struct kimage *image)
 	unsigned long binary_runtime_size;
 
 	/* use more understandable variable names than defined in kbuf */
+	size_t kexec_buffer_size = 0;
 	void *kexec_buffer = NULL;
-	size_t kexec_buffer_size;
-	size_t kexec_segment_size;
 	int ret;
 
 	/*

@@ -162,13 +159,6 @@ void ima_add_kexec_buffer(struct kimage *image)
 		return;
 	}
 
-	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
-				  kexec_segment_size);
-	if (!kexec_buffer) {
-		pr_err("Not enough memory for the kexec measurement buffer.\n");
-		return;
-	}
-
 	kbuf.buffer = kexec_buffer;
 	kbuf.bufsz = kexec_buffer_size;
 	kbuf.memsz = kexec_segment_size;

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

@@ -202,7 +186,32 @@ void ima_add_kexec_buffer(struct kimage *image)
 static int ima_update_kexec_buffer(struct notifier_block *self,
 				   unsigned long action, void *data)
 {
-	return NOTIFY_OK;
+	size_t buf_size = 0;
+	int ret = NOTIFY_OK;
+	void *buf = NULL;
+
+	if (!kexec_in_progress) {
+		pr_info("No kexec in progress.\n");
+		return ret;
+	}
+
+	if (!ima_kexec_buffer) {
+		pr_err("Kexec buffer not set.\n");
+		return ret;
+	}
+
+	ret = ima_dump_measurement_list(&buf_size, &buf, kexec_segment_size);
+
+	if (ret)
+		pr_err("Dump measurements failed. Error:%d\n", ret);
+
+	if (buf_size != 0)
+		memcpy(ima_kexec_buffer, buf, buf_size);
+
+	kimage_unmap_segment(ima_kexec_buffer);
+	ima_kexec_buffer = NULL;
+
+	return ret;
 }
 
 static struct notifier_block update_buffer_nb = {

Quoted 4 lines · steven chen

ima_dump_measurement_list() is called during kexec 'load', which may result in loss of IMA measurements during kexec soft reboot. Due to missed measurements that only occurred after kexec 'load', this function needs to be called during kexec 'execute'.

-> , defer calling ima_dump_measurement_list() to kexec 'execute'.

Quoted 5 lines

This patch includes the following changes:
- Implement kimage_file_post_load() function to be invoked after the new kernel image has been loaded for kexec.
- Call kimage_file_post_load() from kexec_file_load() syscall only for kexec soft reboot scenarios and not for KEXEC_FILE_ON_CRASH. It will

Quoted 8 lines

map the IMA segment, and register reboot notifier for the function ima_update_kexec_buffer() which would copy the IMA log at kexec soft reboot.
- Make kexec_segment_size variable local static to the file so that it becomes accessible both during kexec 'load' and 'execute'.
- Move ima_dump_measurement_list() call from ima_add_kexec_buffer() to ima_update_kexec_buffer().
- Copy the measurement list as much as possible.

Let's summarize the above changes, as below, and then remove the list.

Define and call the new kimage_file_post_load() to call ima_kexec_post_load(), which registers the reboot notifier (ima_update_kexec_buffer).

Finally, move ima_dump_measurement_list() from ima_add_kexec_buffer() to ima_update_kexec_buffer() to defer copying the measurement list.

Quoted 3 lines

- Remove ima_reset_kexec_file() call from ima_add_kexec_buffer(), now that the buffer is being copied at kexec 'execute', and resetting the file at kexec 'load' would corrupt the buffer.

Instead of adding and then removing the ima_reset_kexec_file() call from ima_add_kexec_buffer(), defer adding the test in ima_alloc_kexec_file_buf() to see if the segment size has changed. That should be a subsequent patch, after this one.

ima_reset_kexec_file() is then ever called from only one place. Combine ima_reset_kexec_file() with ima_free_kexec_file_buf().

thanks,

Mimi

Quoted 153 lines

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

---
  kernel/kexec_file.c                | 10 ++++++
  security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++------------
  2 files changed, 40 insertions(+), 21 deletions(-)

kernel/kexec_file.c +10 -0

index 606132253c79..ab449b43aaee 100644
@@ -201,6 +201,13 @@ kimage_validate_signature(struct kimage *image)
  }
  #endif
 
+static void kimage_file_post_load(struct kimage *image)
+{
+#ifdef CONFIG_IMA_KEXEC
+	ima_kexec_post_load(image);
+#endif
+}
+
  /*
   * In file mode list of segments is prepared by kernel. Copy relevant
   * data from user space, do error checking, prepare segment list

@@ -428,6 +435,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 
  	kimage_terminate(image);
 
+	if (!(flags & KEXEC_FILE_ON_CRASH))
+		kimage_file_post_load(image);
+
  	ret = machine_kexec_post_load(image);
  	if (ret)
  		goto out;

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

index 9336c4f60650..c390c745f882 100644
@@ -19,6 +19,7 @@
  #ifdef CONFIG_IMA_KEXEC
  static bool ima_kexec_update_registered;
  static struct seq_file ima_kexec_file;
+static size_t kexec_segment_size;
  static void *ima_kexec_buffer;
 
  static void ima_reset_kexec_file(struct seq_file *sf)

@@ -67,7 +68,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
   *    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
+ *        copy the measurement list as much as possible.
   */
  static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
  				     unsigned long segment_size)

@@ -95,9 +96,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
  		}
  	}
 
-	if (ret < 0)
-		goto out;
-
  	/*
  	 * fill in reserved space with some buffer details
  	 * (eg. version, buffer size, number of measurements)

@@ -117,7 +115,7 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 
  	*buffer_size = ima_kexec_file.count;
  	*buffer = ima_kexec_file.buf;
-out:
+
  	return ret;
  }
 

@@ -135,9 +133,8 @@ void ima_add_kexec_buffer(struct kimage *image)
  	unsigned long binary_runtime_size;
 
  	/* use more understandable variable names than defined in kbuf */
+	size_t kexec_buffer_size = 0;
  	void *kexec_buffer = NULL;
-	size_t kexec_buffer_size;
-	size_t kexec_segment_size;
  	int ret;
 
  	/*

@@ -162,13 +159,6 @@ void ima_add_kexec_buffer(struct kimage *image)
  		return;
  	}
 
-	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
-				  kexec_segment_size);
-	if (!kexec_buffer) {
-		pr_err("Not enough memory for the kexec measurement buffer.\n");
-		return;
-	}
-
  	kbuf.buffer = kexec_buffer;
  	kbuf.bufsz = kexec_buffer_size;
  	kbuf.memsz = kexec_segment_size;

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

@@ -202,7 +186,32 @@ void ima_add_kexec_buffer(struct kimage *image)
  static int ima_update_kexec_buffer(struct notifier_block *self,
  				   unsigned long action, void *data)
  {
-	return NOTIFY_OK;
+	size_t buf_size = 0;
+	int ret = NOTIFY_OK;
+	void *buf = NULL;
+
+	if (!kexec_in_progress) {
+		pr_info("No kexec in progress.\n");
+		return ret;
+	}
+
+	if (!ima_kexec_buffer) {
+		pr_err("Kexec buffer not set.\n");
+		return ret;
+	}
+
+	ret = ima_dump_measurement_list(&buf_size, &buf, kexec_segment_size);
+
+	if (ret)
+		pr_err("Dump measurements failed. Error:%d\n", ret);
+
+	if (buf_size != 0)
+		memcpy(ima_kexec_buffer, buf, buf_size);
+
+	kimage_unmap_segment(ima_kexec_buffer);
+	ima_kexec_buffer = NULL;
+
+	return ret;
  }
 
  static struct notifier_block update_buffer_nb = {

---
 kernel/kexec_file.c                | 10 ++++++
 security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++------------
 2 files changed, 40 insertions(+), 21 deletions(-)

kernel/kexec_file.c +7 -0

index 606132253c79..ab449b43aaee 100644
@@ -201,6 +201,13 @@ kimage_validate_signature(struct kimage *image)
 }
 #endif
 
+static void kimage_file_post_load(struct kimage *image)
+{
+#ifdef CONFIG_IMA_KEXEC
+	ima_kexec_post_load(image);
+#endif
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list

kernel/kexec_file.c +1 -0

+
 	ret = machine_kexec_post_load(image);
 	if (ret)
 		goto out;

On 03/17/25 at 06:04pm, steven chen wrote:
...snip...

Quoted 25 lines

---
  kernel/kexec_file.c                | 10 ++++++
  security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++------------
  2 files changed, 40 insertions(+), 21 deletions(-)

kernel/kexec_file.c +7 -0

index 606132253c79..ab449b43aaee 100644
@@ -201,6 +201,13 @@ kimage_validate_signature(struct kimage *image)
  }
  #endif
 
+static void kimage_file_post_load(struct kimage *image)
+{
+#ifdef CONFIG_IMA_KEXEC
+	ima_kexec_post_load(image);
+#endif
+}
+
  /*
   * In file mode list of segments is prepared by kernel. Copy relevant
   * data from user space, do error checking, prepare segment list

@@ -428,6 +435,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 

kernel/kexec_file.c SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, · line 439

Quoted 4 lines

kernel/kexec_file.c +2 -0

  	kimage_terminate(image);
 
+	if (!(flags & KEXEC_FILE_ON_CRASH))
+		kimage_file_post_load(image);

machine_kexec_post_load() is called by both kexec_load and kexec_file_load, we should use it to do things post load, but not introducing another kimage_file_post_load().

Quoted 5 lines

kernel/kexec_file.c +1 -0

+
  	ret = machine_kexec_post_load(image);
  	if (ret)
  		goto out;

...snip...

Quoted 23 lines · Baoquan He

On 03/17/25 at 06:04pm, steven chen wrote:
...snip...

Quoted 21 lines

---
  kernel/kexec_file.c                | 10 ++++++
  security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++------------
  2 files changed, 40 insertions(+), 21 deletions(-)

kernel/kexec_file.c +7 -0

index 606132253c79..ab449b43aaee 100644
@@ -201,6 +201,13 @@ kimage_validate_signature(struct kimage *image)
  }
  #endif
+static void kimage_file_post_load(struct kimage *image)
+{
+#ifdef CONFIG_IMA_KEXEC
+	ima_kexec_post_load(image);
+#endif
+}
+
  /*
   * In file mode list of segments is prepared by kernel. Copy relevant

Quoted 8 lines

kernel/kexec_file.c SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, · line 437

Quoted 5 lines

kernel/kexec_file.c +2 -0

   * data from user space, do error checking, prepare segment list

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

machine_kexec_post_load() is called by both kexec_load and kexec_file_load, we should use it to do things post load, but not introducing another kimage_file_post_load().

Hi Baoquan,

Could you give me more detail about this?

Quoted 32 lines · steven chen

Quoted 31 lines · Baoquan He

On 03/17/25 at 06:04pm, steven chen wrote:
...snip...

Quoted 23 lines

---
   kernel/kexec_file.c                | 10 ++++++
   security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++------------
   2 files changed, 40 insertions(+), 21 deletions(-)

kernel/kexec_file.c +7 -0

index 606132253c79..ab449b43aaee 100644
@@ -201,6 +201,13 @@ kimage_validate_signature(struct kimage *image)
   }
   #endif
+static void kimage_file_post_load(struct kimage *image)
+{
+#ifdef CONFIG_IMA_KEXEC
+	ima_kexec_post_load(image);
+#endif
+}
+
   /*
    * In file mode list of segments is prepared by kernel. Copy relevant
    * data from user space, do error checking, prepare segment list

kernel/kexec_file.c SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, · line 437

Quoted 3 lines

kernel/kexec_file.c +2 -0

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

machine_kexec_post_load() is called by both kexec_load and kexec_file_load, we should use it to do things post load, but not introducing another kimage_file_post_load().

Quoted 3 lines

Hi Baoquan,

Could you give me more detail about this?

I mean machine_kexec_post_load() is the place where post load operations are done, including kexec_load and kexec_file_load. There's no need to specifically introduce a kimage_file_post_load() to do post load operaton for kexec_file_load.

Quoted 33 lines · Baoquan He

Quoted 32 lines · steven chen

Quoted 31 lines · Baoquan He

On 03/17/25 at 06:04pm, steven chen wrote:
...snip...

Quoted 23 lines

---
   kernel/kexec_file.c                | 10 ++++++
   security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++------------
   2 files changed, 40 insertions(+), 21 deletions(-)

kernel/kexec_file.c +7 -0

index 606132253c79..ab449b43aaee 100644
@@ -201,6 +201,13 @@ kimage_validate_signature(struct kimage *image)
   }
   #endif
+static void kimage_file_post_load(struct kimage *image)
+{
+#ifdef CONFIG_IMA_KEXEC
+	ima_kexec_post_load(image);
+#endif
+}
+
   /*
    * In file mode list of segments is prepared by kernel. Copy relevant
    * data from user space, do error checking, prepare segment list

kernel/kexec_file.c SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, · line 437

Quoted 3 lines

kernel/kexec_file.c +2 -0

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

machine_kexec_post_load() is called by both kexec_load and kexec_file_load, we should use it to do things post load, but not introducing another kimage_file_post_load().

Quoted 7 lines

Quoted 3 lines

Hi Baoquan,

Could you give me more detail about this?

I mean machine_kexec_post_load() is the place where post load operations are done, including kexec_load and kexec_file_load. There's no need to specifically introduce a kimage_file_post_load() to do post load operaton for kexec_file_load.

Hi Baoquan,

Updating the machine_kexec_post_load() API to carry flags would indeed require changes to multiple files. This approach involves the condition check if (!(flags & KEXEC_FILE_ON_CRASH)) and ensuring that the flags are properly passed and handled across the relevant file

if just adding a API kimage_file_post_load() here, it is much easy and clean, right?

Quoted 43 lines · steven chen

Quoted 33 lines · Baoquan He

Quoted 32 lines · steven chen

Quoted 31 lines · Baoquan He

On 03/17/25 at 06:04pm, steven chen wrote:
...snip...

Quoted 23 lines

---
    kernel/kexec_file.c                | 10 ++++++
    security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++------------
    2 files changed, 40 insertions(+), 21 deletions(-)

kernel/kexec_file.c +7 -0

index 606132253c79..ab449b43aaee 100644
@@ -201,6 +201,13 @@ kimage_validate_signature(struct kimage *image)
    }
    #endif
+static void kimage_file_post_load(struct kimage *image)
+{
+#ifdef CONFIG_IMA_KEXEC
+	ima_kexec_post_load(image);
+#endif
+}
+
    /*
     * In file mode list of segments is prepared by kernel. Copy relevant
     * data from user space, do error checking, prepare segment list

kernel/kexec_file.c SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, · line 437

Quoted 3 lines

kernel/kexec_file.c +2 -0

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

machine_kexec_post_load() is called by both kexec_load and kexec_file_load, we should use it to do things post load, but not introducing another kimage_file_post_load().

Quoted 7 lines

Quoted 3 lines

Hi Baoquan,

Could you give me more detail about this?

I mean machine_kexec_post_load() is the place where post load operations are done, including kexec_load and kexec_file_load. There's no need to specifically introduce a kimage_file_post_load() to do post load operaton for kexec_file_load.

Hi Baoquan,

Quoted 7 lines

Updating the machine_kexec_post_load() API to carry flags would indeed require changes to multiple files. This approach involves the condition check if (!(flags & KEXEC_FILE_ON_CRASH)) and ensuring that the flags are properly passed and handled across the relevant file

if just adding a API kimage_file_post_load() here, it is much easy and clean, right?

Hmm, it's easier, while maybe not good. We should not repeatedly introduce similar things into codes. Here, it's similar as what kexec_apply_relocations() and arch_kexec_apply_relocations() are doing.

int machine_kexec_post_load(struct kimage *image)
{

#ifdef CONFIG_IMA_KEXEC
         ima_kexec_post_load(image);
#endif
	return arch_machine_kexec_post_load();
}

Then a generic arch_machine_kexec_post_load(struct kimage *image) {return 0;} version, and a arm64 specific version.

Is it OK to you?

Quoted 7 lines

Hmm, it's easier, while maybe not good. We should not repeatedly introduce similar things into codes. Here, it's similar as what kexec_apply_relocations() and arch_kexec_apply_relocations() are doing.

int machine_kexec_post_load(struct kimage *image)

Quoted 6 lines

{

#ifdef CONFIG_IMA_KEXEC
         ima_kexec_post_load(image);
#endif
	return arch_machine_kexec_post_load();
}

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

Reviewed-by Stefan Berger <[email protected]>

Reviewed-by Mimi Zohar <[email protected]>

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

security/integrity/ima/Kconfig +0 -0

index 475c32615006..d73c96c3c1c9 100644

security/integrity/ima/Kconfig +6 -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 c390c745f882..0f214e41dd33 100644
@@ -131,6 +131,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;

@@ -138,15 +139,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.25.1

Quoted 30 lines · steven chen

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

Reviewed-by Stefan Berger <[email protected]>

Reviewed-by Mimi Zohar <[email protected]>

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

security/integrity/ima/Kconfig +0 -0

index 475c32615006..d73c96c3c1c9 100644
@@ -321,4 +321,14 @@ config IMA_DISABLE_HTABLE
  	help
  	   This option disables htable to allow measurement of duplicate records.
 

security/integrity/ima/Kconfig config IMA_DISABLE_HTABLE · line 327

Quoted 4 lines

security/integrity/ima/Kconfig +4 -0

+config IMA_KEXEC_EXTRA_MEMORY_KB
+	int "Extra memory for IMA measurements added during kexec soft reboot"
+	depends on IMA_KEXEC
+	default 0

Is there range for this memory, e.g [0, max] and max will be a value according to our current perception?

Quoted 49 lines

security/integrity/ima/Kconfig +6 -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 c390c745f882..0f214e41dd33 100644
@@ -131,6 +131,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;

@@ -138,15 +139,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.25.1

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

Quoted 1 line

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

Quoted 1 line

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 Stefan Berger <[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 0f214e41dd33..43223eb99046 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;

@@ -36,6 +38,24 @@ static void ima_free_kexec_file_buf(struct seq_file *sf)
 	ima_reset_kexec_file(sf);
 }
 
+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)
 {
 	/*

@@ -58,6 +78,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 +0 -0

index 83d53824aa98..590637e81ad1 100644

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

+
 	ima_measurements_suspend();
 
 	return NOTIFY_DONE;

Quoted 109 lines · steven chen

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

Quoted 1 line

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

Quoted 1 line

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 Stefan Berger <[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 0f214e41dd33..43223eb99046 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;

@@ -36,6 +38,24 @@ static void ima_free_kexec_file_buf(struct seq_file *sf)
  	ima_reset_kexec_file(sf);
  }
 
+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)
  {
  	/*

@@ -58,6 +78,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 +0 -0

index 83d53824aa98..590637e81ad1 100644

security/integrity/ima/ima_queue.c static int ima_reboot_notifier(struct notifier_block *nb, · line 247

Quoted 7 lines

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

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

After a kexec execute, the measurement list does not verify properly and the "kexec_execute" critical data does not appear in the measurement list. This makes me think the critical data IS being extended into the TPM, but the measurement list is being copied before the "kexec_execute" critical data is called.

This actually makes sense since the reboot notifier ima_update_kexec_buffer() priority is higher than ima_reboot_notifier().

INT_MIN: runs the callback late
INT_MAX: runs the callback early

Either reverse the callback priorities or revert moving the "kexec_execute" critical data to ima_reboot_notifier().

thanks,

Mimi

Quoted 4 lines

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

+
  	ima_measurements_suspend();
 
  	return NOTIFY_DONE;