ima: kexec: measure events between kexec load and execute

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

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

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

---
  security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
  1 file changed, 93 insertions(+), 33 deletions(-)

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

index 419dc405c831..307e07991865 100644

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

+	/* segment size can't change between kexec load and execute */
+	ima_kexec_file.buf = vmalloc(kexec_segment_size);
+	if (!ima_kexec_file.buf) {
+		pr_err("%s: No memory for ima kexec measurement buffer\n",

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

  	int ret = 0;
 
-	/* segment size can't change between kexec load and execute */
-	file.buf = vmalloc(segment_size);
-	if (!file.buf) {
-		ret = -ENOMEM;
-		goto out;
+	if (!ima_kexec_file.buf) {
+		pr_err("%s: Kexec file buf not allocated\n",
+			__func__);
+		return -EINVAL;
  	}
 
-	file.size = segment_size;
-	file.read_pos = 0;
-	file.count = sizeof(khdr);	/* reserved space */
+	/*
+	 * Ensure the kexec buffer is large enough to hold ima_khdr
+	 */
+	if (ima_kexec_file.size < sizeof(ima_khdr)) {
+		pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
+			__func__);
+		ima_clear_kexec_file();
+		return -ENOMEM;

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

+			pr_err("%s: IMA log file is too big for Kexec buf\n",
+				__func__);
  			break;
  		}
  	}
 
-	if (ret < 0)
-		goto out;
-
  	/*
  	 * fill in reserved space with some buffer details
  	 * (eg. version, buffer size, number of measurements)
  	 */
-	khdr.buffer_size = file.count;
+	ima_khdr.buffer_size = ima_kexec_file.count;
  	if (ima_canonical_fmt) {
-		khdr.version = cpu_to_le16(khdr.version);
-		khdr.count = cpu_to_le64(khdr.count);
-		khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
+		ima_khdr.version = cpu_to_le16(ima_khdr.version);
+		ima_khdr.count = cpu_to_le64(ima_khdr.count);
+		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
  	}
-	memcpy(file.buf, &khdr, sizeof(khdr));
+	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_khdr));
 
  	print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
-			     file.buf, file.count < 100 ? file.count : 100,
+			     ima_kexec_file.buf, ima_kexec_file.count < 100 ?
+			     ima_kexec_file.count : 100,
  			     true);
 
-	*buffer_size = file.count;
-	*buffer = file.buf;
-out:
-	if (ret == -EINVAL)
-		vfree(file.buf);
+	*buffer_size = ima_kexec_file.count;
+	*buffer = ima_kexec_file.buf;
+
  	return ret;
  }
 

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

Quoted 23 lines · Tushar Sugandhi

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

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

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

---
  security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------

  1 file changed, 93 insertions(+), 33 deletions(-)

security/integrity/ima/ima_kexec.c line 18

Quoted 9 lines

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

index 419dc405c831..307e07991865 100644
@@ -15,61 +15,114 @@
   #include "ima.h"
   #ifdef CONFIG_IMA_KEXEC
+struct seq_file ima_kexec_file;
+struct ima_kexec_hdr ima_khdr;

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

security/integrity/ima/ima_kexec.c line 23

Quoted 5 lines

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

+
+void ima_clear_kexec_file(void)
+{
+    vfree(ima_kexec_file.buf);

I would pass the ima_kexec_file onto this function here as a parameter rather than accessing the file-static variable. I find this better to read once you look at ima_clear_kexec_file() further below and wonder why it doesn't take a parameter.

security/integrity/ima/ima_kexec.c line 30

Quoted 7 lines

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

+    ima_kexec_file.buf = NULL;
+    ima_kexec_file.size = 0;
+    ima_kexec_file.read_pos = 0;
+    ima_kexec_file.count = 0;
+}
+
+static int ima_alloc_kexec_buf(size_t kexec_segment_size)

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

security/integrity/ima/ima_kexec.c line 34

Quoted 4 lines

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

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

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

Quoted 6 lines

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

+        pr_err("%s: Invalid segment size for kexec: %zu\n",
+            __func__, kexec_segment_size);
+        return -EINVAL;
+    }
+
+    /*

security/integrity/ima/ima_kexec.c line 46

Quoted 6 lines

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

+     * If kexec load was called before, clear the existing buffer
+     * before allocating a new one
+     */
+    if (ima_kexec_file.buf)
+        ima_clear_kexec_file();
+

ima_clear_file(&ima_kexec_file);

Quoted 8 lines

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

+    /* segment size can't change between kexec load and execute */
+    ima_kexec_file.buf = vmalloc(kexec_segment_size);
+    if (!ima_kexec_file.buf) {
+        pr_err("%s: No memory for ima kexec measurement buffer\n",
+            __func__);
+        return -ENOMEM;
+    }
+

security/integrity/ima/ima_kexec.c line 60

Quoted 6 lines

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

+    ima_kexec_file.size = kexec_segment_size;
+    ima_kexec_file.read_pos = 0;
+    ima_kexec_file.count = sizeof(ima_khdr);    /* reserved space */
+
+    memset(&ima_khdr, 0, sizeof(ima_khdr));
+    ima_khdr.version = 1;

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

security/integrity/ima/ima_kexec.c line 23

Quoted 10 lines

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

+
+    return 0;
+}
+
   static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
                        unsigned long segment_size)
   {
       struct ima_queue_entry *qe;
-    struct seq_file file;
-    struct ima_kexec_hdr khdr;

Don't remove it from here.

Quoted 21 lines

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

       int ret = 0;
-    /* segment size can't change between kexec load and execute */
-    file.buf = vmalloc(segment_size);
-    if (!file.buf) {
-        ret = -ENOMEM;
-        goto out;
+    if (!ima_kexec_file.buf) {
+        pr_err("%s: Kexec file buf not allocated\n",
+            __func__);
+        return -EINVAL;
       }
-    file.size = segment_size;
-    file.read_pos = 0;
-    file.count = sizeof(khdr);    /* reserved space */
+    /*
+     * Ensure the kexec buffer is large enough to hold ima_khdr
+     */
+    if (ima_kexec_file.size < sizeof(ima_khdr)) {
+        pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
+            __func__);
+        ima_clear_kexec_file();

security/integrity/ima/ima_kexec.c line 92

Quoted 10 lines

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

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

You can reformat these comments to (at least) 80 columns.

security/integrity/ima/ima_kexec.c line 99

Quoted 11 lines

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

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

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

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

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

Quoted 61 lines

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

+            pr_err("%s: IMA log file is too big for Kexec buf\n",
+                __func__);
               break;
           }
       }
-    if (ret < 0)
-        goto out;
-
       /*
        * fill in reserved space with some buffer details
        * (eg. version, buffer size, number of measurements)
        */
-    khdr.buffer_size = file.count;
+    ima_khdr.buffer_size = ima_kexec_file.count;
       if (ima_canonical_fmt) {
-        khdr.version = cpu_to_le16(khdr.version);
-        khdr.count = cpu_to_le64(khdr.count);
-        khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
+        ima_khdr.version = cpu_to_le16(ima_khdr.version);
+        ima_khdr.count = cpu_to_le64(ima_khdr.count);
+        ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
       }
-    memcpy(file.buf, &khdr, sizeof(khdr));
+    memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_khdr));
       print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
-                 file.buf, file.count < 100 ? file.count : 100,
+                 ima_kexec_file.buf, ima_kexec_file.count < 100 ?
+                 ima_kexec_file.count : 100,
                    true);
-    *buffer_size = file.count;
-    *buffer = file.buf;
-out:
-    if (ret == -EINVAL)
-        vfree(file.buf);
+    *buffer_size = ima_kexec_file.count;
+    *buffer = ima_kexec_file.buf;
+
       return ret;
   }

@@ -108,13 +161,20 @@ void ima_add_kexec_buffer(struct kimage *image)
           return;
       }
-    ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
-                  kexec_segment_size);
-    if (!kexec_buffer) {
+    ret = ima_alloc_kexec_buf(kexec_segment_size);
+    if (ret < 0) {
           pr_err("Not enough memory for the kexec measurement buffer.\n");
           return;
       }
+    ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
+                    kexec_segment_size);
+    if (ret < 0) {
+        pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
+               __func__, ret);
+        return;
+    }
+
       kbuf.buffer = kexec_buffer;
       kbuf.bufsz = kexec_buffer_size;
       kbuf.memsz = kexec_segment_size;

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

Quoted 2 lines · Tushar Sugandhi

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

You are welcome.

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

   Stefan

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

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

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

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

+{
+	vfree(ima_kexec_file.buf);
+	ima_kexec_file.buf = NULL;
+	ima_kexec_file.size = 0;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = 0;
+}

security/integrity/ima/ima_kexec.c +23 -9

+
+	/* segment size can't change between kexec load and execute */
+	ima_kexec_file.buf = vmalloc(kexec_segment_size);
+	if (!ima_kexec_file.buf) {
+		pr_err("%s: No memory for ima kexec measurement buffer\n",
+			__func__);
+		return -ENOMEM;
+	}
+
+	ima_kexec_file.size = kexec_segment_size;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = sizeof(ima_khdr);	/* reserved space */
+
+	memset(&ima_khdr, 0, sizeof(ima_khdr));
+	ima_khdr.version = 1;
+
+	return 0;
+}
+
 static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 				     unsigned long segment_size)
 {
 	struct ima_queue_entry *qe;
-	struct seq_file file;
-	struct ima_kexec_hdr khdr;
 	int ret = 0;
 
-	/* segment size can't change between kexec load and execute */
-	file.buf = vmalloc(segment_size);
-	if (!file.buf) {
-		ret = -ENOMEM;
-		goto out;
+	if (!ima_kexec_file.buf) {
+		pr_err("%s: Kexec file buf not allocated\n",
+			__func__);
+		return -EINVAL;
 	}
 
-	file.size = segment_size;
-	file.read_pos = 0;

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

 	list_for_each_entry_rcu(qe, &ima_measurements, later) {
-		if (file.count < file.size) {
-			khdr.count++;
-			ima_measurements_show(&file, qe);
+		if (ima_kexec_file.count < ima_kexec_file.size) {
+			ima_khdr.count++;
+			ima_measurements_show(&ima_kexec_file, qe);
 		} else {
-			ret = -EINVAL;
+			ret = EFBIG;
+			pr_err("%s: IMA log file is too big for Kexec buf\n",
+				__func__);
 			break;
 		}
 	}
 
-	if (ret < 0)
-		goto out;
-
 	/*
 	 * fill in reserved space with some buffer details
 	 * (eg. version, buffer size, number of measurements)
 	 */
-	khdr.buffer_size = file.count;
+	ima_khdr.buffer_size = ima_kexec_file.count;
 	if (ima_canonical_fmt) {
-		khdr.version = cpu_to_le16(khdr.version);
-		khdr.count = cpu_to_le64(khdr.count);
-		khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
+		ima_khdr.version = cpu_to_le16(ima_khdr.version);
+		ima_khdr.count = cpu_to_le64(ima_khdr.count);
+		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
 	}
-	memcpy(file.buf, &khdr, sizeof(khdr));
+	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_khdr));
 
 	print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
-			     file.buf, file.count < 100 ? file.count : 100,
+			     ima_kexec_file.buf, ima_kexec_file.count < 100 ?
+			     ima_kexec_file.count : 100,
 			     true);
 
-	*buffer_size = file.count;
-	*buffer = file.buf;
-out:
-	if (ret == -EINVAL)
-		vfree(file.buf);
+	*buffer_size = ima_kexec_file.count;
+	*buffer = ima_kexec_file.buf;
+
 	return ret;
 }

Hi Tushar,

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

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

Quoted 10 lines · Tushar Sugandhi

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

The above pragraph is unnecessary.

Quoted 6 lines

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

Fix the wording based on the suggested changes below.

Quoted 1 line

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

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

Quoted 11 lines

---
 security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
 1 file changed, 93 insertions(+), 33 deletions(-)

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

index 419dc405c831..307e07991865 100644
@@ -15,61 +15,114 @@
 #include "ima.h"
 

security/integrity/ima/ima_kexec.c line 18

Quoted 2 lines

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

 #ifdef CONFIG_IMA_KEXEC
+struct seq_file ima_kexec_file;

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

security/integrity/ima/ima_kexec.c line 21

Quoted 4 lines

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

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

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

Quoted 7 lines

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

+{
+	vfree(ima_kexec_file.buf);
+	ima_kexec_file.buf = NULL;
+	ima_kexec_file.size = 0;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = 0;
+}

security/integrity/ima/ima_kexec.c line 38

Quoted 10 lines

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

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

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

security/integrity/ima/ima_kexec.c line 83

Quoted 8 lines

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

+
+	/*
+	 * If kexec load was called before, clear the existing buffer
+	 * before allocating a new one
+	 */
 
+	if (ima_kexec_file.buf)
+		ima_clear_kexec_file();

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

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

Quoted 40 lines

security/integrity/ima/ima_kexec.c +23 -9

+
+	/* segment size can't change between kexec load and execute */
+	ima_kexec_file.buf = vmalloc(kexec_segment_size);
+	if (!ima_kexec_file.buf) {
+		pr_err("%s: No memory for ima kexec measurement buffer\n",
+			__func__);
+		return -ENOMEM;
+	}
+
+	ima_kexec_file.size = kexec_segment_size;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = sizeof(ima_khdr);	/* reserved space */
+
+	memset(&ima_khdr, 0, sizeof(ima_khdr));
+	ima_khdr.version = 1;
+
+	return 0;
+}
+
 static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 				     unsigned long segment_size)
 {
 	struct ima_queue_entry *qe;
-	struct seq_file file;
-	struct ima_kexec_hdr khdr;
 	int ret = 0;
 
-	/* segment size can't change between kexec load and execute */
-	file.buf = vmalloc(segment_size);
-	if (!file.buf) {
-		ret = -ENOMEM;
-		goto out;
+	if (!ima_kexec_file.buf) {
+		pr_err("%s: Kexec file buf not allocated\n",
+			__func__);
+		return -EINVAL;
 	}
 
-	file.size = segment_size;
-	file.read_pos = 0;

security/integrity/ima/ima_kexec.c line 85

Quoted 10 lines

security/integrity/ima/ima_kexec.c +9 -1

-	file.count = sizeof(khdr);	/* reserved space */
+	/*
+	 * Ensure the kexec buffer is large enough to hold ima_khdr
+	 */
+	if (ima_kexec_file.size < sizeof(ima_khdr)) {
+		pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
+			__func__);
+		ima_clear_kexec_file();
+		return -ENOMEM;
+	}

Is this necessary?

security/integrity/ima/ima_kexec.c line 93

Quoted 9 lines

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

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

Suggestion:

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

Quoted 42 lines

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

 	list_for_each_entry_rcu(qe, &ima_measurements, later) {
-		if (file.count < file.size) {
-			khdr.count++;
-			ima_measurements_show(&file, qe);
+		if (ima_kexec_file.count < ima_kexec_file.size) {
+			ima_khdr.count++;
+			ima_measurements_show(&ima_kexec_file, qe);
 		} else {
-			ret = -EINVAL;
+			ret = EFBIG;
+			pr_err("%s: IMA log file is too big for Kexec buf\n",
+				__func__);
 			break;
 		}
 	}
 
-	if (ret < 0)
-		goto out;
-
 	/*
 	 * fill in reserved space with some buffer details
 	 * (eg. version, buffer size, number of measurements)
 	 */
-	khdr.buffer_size = file.count;
+	ima_khdr.buffer_size = ima_kexec_file.count;
 	if (ima_canonical_fmt) {
-		khdr.version = cpu_to_le16(khdr.version);
-		khdr.count = cpu_to_le64(khdr.count);
-		khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
+		ima_khdr.version = cpu_to_le16(ima_khdr.version);
+		ima_khdr.count = cpu_to_le64(ima_khdr.count);
+		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
 	}
-	memcpy(file.buf, &khdr, sizeof(khdr));
+	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_khdr));
 
 	print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
-			     file.buf, file.count < 100 ? file.count : 100,
+			     ima_kexec_file.buf, ima_kexec_file.count < 100 ?
+			     ima_kexec_file.count : 100,
 			     true);
 

security/integrity/ima/ima_kexec.c line 127

Quoted 10 lines

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

-	*buffer_size = file.count;
-	*buffer = file.buf;
-out:
-	if (ret == -EINVAL)
-		vfree(file.buf);
+	*buffer_size = ima_kexec_file.count;
+	*buffer = ima_kexec_file.buf;
+
 	return ret;
 }

Quoted 30 lines · Mimi Zohar

Hi Tushar,

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

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

Quoted 10 lines · Tushar Sugandhi

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

The above pragraph is unnecessary.

Quoted 6 lines

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

Fix the wording based on the suggested changes below.

Quoted 6 lines

Quoted 1 line

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

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

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

Quoted 11 lines

Quoted 11 lines

---
  security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
  1 file changed, 93 insertions(+), 33 deletions(-)

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

index 419dc405c831..307e07991865 100644
@@ -15,61 +15,114 @@
  #include "ima.h"
 

Quoted 6 lines

security/integrity/ima/ima_kexec.c line 18

Quoted 2 lines

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

  #ifdef CONFIG_IMA_KEXEC
+struct seq_file ima_kexec_file;

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

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

Mimi

Hi Tushar,

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

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

Quoted 10 lines

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

The above pragraph is unnecessary.

Quoted 6 lines

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

Fix the wording based on the suggested changes below.

Quoted 1 line

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

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

security/integrity/ima/ima_kexec.c line 18

Quoted 13 lines

Quoted 1 line

---

  security/integrity/ima/ima_kexec.c | 126 +++++++++++++++++++++--------
  1 file changed, 93 insertions(+), 33 deletions(-)

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

index 419dc405c831..307e07991865 100644
@@ -15,61 +15,114 @@
  #include "ima.h"
 
  #ifdef CONFIG_IMA_KEXEC
+struct seq_file ima_kexec_file;

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

security/integrity/ima/ima_kexec.c line 21

Quoted 3 lines

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

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

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

Quoted 12 lines

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

+{
+	vfree(ima_kexec_file.buf);
+	ima_kexec_file.buf = NULL;
+	ima_kexec_file.size = 0;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = 0;
+}
+
+static int ima_alloc_kexec_buf(size_t kexec_segment_size)
+{
+	if ((kexec_segment_size == 0) ||
+	    (kexec_segment_size == ULONG_MAX) ||

security/integrity/ima/ima_kexec.c line 38

Quoted 5 lines

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

+	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
+		pr_err("%s: Invalid segment size for kexec: %zu\n",
+			__func__, kexec_segment_size);
+		return -EINVAL;
+	}

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

security/integrity/ima/ima_kexec.c line 83

Quoted 8 lines

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

+
+	/*
+	 * If kexec load was called before, clear the existing buffer
+	 * before allocating a new one
+	 */
 
+	if (ima_kexec_file.buf)
+		ima_clear_kexec_file();

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

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

Quoted 38 lines

security/integrity/ima/ima_kexec.c +23 -7

+
+	/* segment size can't change between kexec load and execute */
+	ima_kexec_file.buf = vmalloc(kexec_segment_size);
+	if (!ima_kexec_file.buf) {
+		pr_err("%s: No memory for ima kexec measurement buffer\n",
+			__func__);
+		return -ENOMEM;
+	}
+
+	ima_kexec_file.size = kexec_segment_size;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = sizeof(ima_khdr);	/* reserved space */
+
+	memset(&ima_khdr, 0, sizeof(ima_khdr));
+	ima_khdr.version = 1;
+
+	return 0;
+}
+
  static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
  				     unsigned long segment_size)
  {
  	struct ima_queue_entry *qe;
-	struct seq_file file;
-	struct ima_kexec_hdr khdr;
  	int ret = 0;
 
-	/* segment size can't change between kexec load and execute */
-	file.buf = vmalloc(segment_size);
-	if (!file.buf) {
-		ret = -ENOMEM;
-		goto out;
+	if (!ima_kexec_file.buf) {
+		pr_err("%s: Kexec file buf not allocated\n",
+			__func__);
+		return -EINVAL;
  	}
 

security/integrity/ima/ima_kexec.c line 85

Quoted 12 lines

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

-	file.size = segment_size;
-	file.read_pos = 0;
-	file.count = sizeof(khdr);	/* reserved space */
+	/*
+	 * Ensure the kexec buffer is large enough to hold ima_khdr
+	 */
+	if (ima_kexec_file.size < sizeof(ima_khdr)) {
+		pr_err("%s: Kexec buffer size too low to hold ima_khdr\n",
+			__func__);
+		ima_clear_kexec_file();
+		return -ENOMEM;
+	}

Is this necessary?

security/integrity/ima/ima_kexec.c line 93

Quoted 9 lines

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

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

Suggestion:

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

Quoted 52 lines

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

  	list_for_each_entry_rcu(qe, &ima_measurements, later) {
-		if (file.count < file.size) {
-			khdr.count++;
-			ima_measurements_show(&file, qe);
+		if (ima_kexec_file.count < ima_kexec_file.size) {
+			ima_khdr.count++;
+			ima_measurements_show(&ima_kexec_file, qe);
  		} else {
-			ret = -EINVAL;
+			ret = EFBIG;
+			pr_err("%s: IMA log file is too big for Kexec buf\n",
+				__func__);
  			break;
  		}
  	}
 
-	if (ret < 0)
-		goto out;
-
  	/*
  	 * fill in reserved space with some buffer details
  	 * (eg. version, buffer size, number of measurements)
  	 */
-	khdr.buffer_size = file.count;
+	ima_khdr.buffer_size = ima_kexec_file.count;
  	if (ima_canonical_fmt) {
-		khdr.version = cpu_to_le16(khdr.version);
-		khdr.count = cpu_to_le64(khdr.count);
-		khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
+		ima_khdr.version = cpu_to_le16(ima_khdr.version);
+		ima_khdr.count = cpu_to_le64(ima_khdr.count);
+		ima_khdr.buffer_size = cpu_to_le64(ima_khdr.buffer_size);
  	}
-	memcpy(file.buf, &khdr, sizeof(khdr));
+	memcpy(ima_kexec_file.buf, &ima_khdr, sizeof(ima_khdr));
 
  	print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
-			     file.buf, file.count < 100 ? file.count : 100,
+			     ima_kexec_file.buf, ima_kexec_file.count < 100 ?
+			     ima_kexec_file.count : 100,
  			     true);
 
-	*buffer_size = file.count;
-	*buffer = file.buf;
-out:
-	if (ret == -EINVAL)
-		vfree(file.buf);
+	*buffer_size = ima_kexec_file.count;
+	*buffer = ima_kexec_file.buf;
+
  	return ret;
  }

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

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

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

---
  security/integrity/ima/ima.h       |  2 ++
  security/integrity/ima/ima_kexec.c | 58 +++++++++++++++++++++++++-----
  security/integrity/ima/ima_queue.c | 18 ++++++++++
  3 files changed, 69 insertions(+), 9 deletions(-)

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

index c29db699c996..49a6047dd8eb 100644
@@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
  int ima_restore_measurement_entry(struct ima_template_entry *entry);
  int ima_restore_measurement_list(loff_t bufsize, void *buf);
  int ima_measurements_show(struct seq_file *m, void *v);
+void ima_measurements_suspend(void);
+void ima_measurements_resume(void);
  unsigned long ima_get_binary_runtime_size(void);
  int ima_init_template(void);
  void ima_init_template_list(void);

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

index 307e07991865..2c11bbe6efef 100644
@@ -17,6 +17,8 @@
  #ifdef CONFIG_IMA_KEXEC
  struct seq_file ima_kexec_file;
  struct ima_kexec_hdr ima_khdr;
+static void *ima_kexec_buffer;
+static size_t kexec_segment_size;
 
  void ima_clear_kexec_file(void)
  {

@@ -142,7 +144,6 @@ void ima_add_kexec_buffer(struct kimage *image)
  	/* use more understandable variable names than defined in kbuf */
  	void *kexec_buffer = NULL;
  	size_t kexec_buffer_size;
-	size_t kexec_segment_size;
  	int ret;
 
  	/*

@@ -167,14 +168,6 @@ void ima_add_kexec_buffer(struct kimage *image)
  		return;
  	}
 
-	ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
-					kexec_segment_size);
-	if (ret < 0) {
-		pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
-		       __func__, ret);
-		return;
-	}
-
  	kbuf.buffer = kexec_buffer;
  	kbuf.bufsz = kexec_buffer_size;
  	kbuf.memsz = kexec_segment_size;

@@ -192,6 +185,53 @@ void ima_add_kexec_buffer(struct kimage *image)
  	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
  		 kbuf.mem);
  }
+
+/*
+ * Called during kexec execute so that IMA can update the measurement list.
+ */
+static int ima_update_kexec_buffer(struct notifier_block *self,
+				   unsigned long action, void *data)
+{
+	void *buf = NULL;
+	size_t buf_size;
+	bool resume = false;
+	int ret;
+
+	if (!kexec_in_progress) {
+		pr_info("%s: No kexec in progress.\n", __func__);
+		return NOTIFY_OK;

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

+		pr_err("%s: Dump measurements failed. Error:%d\n",
+		       __func__, ret);
+		resume = true;
+		goto out;
+	}
+	memcpy(ima_kexec_buffer, buf, buf_size);
+out:
+	ima_kexec_buffer = NULL;
+
+	if (resume)
+		ima_measurements_resume();
+
+	return NOTIFY_OK;
+}
+struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+};
+
  #endif /* IMA_KEXEC */
 
  /*

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

index 532da87ce519..9e7d1196006e 100644
@@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
   */
  static DEFINE_MUTEX(ima_extend_list_mutex);
 
+/*
+ * Used internally by the kernel to suspend-resume ima measurements.
+ */
+static atomic_t suspend_ima_measurements;
+
  /* lookup up the digest value in the hash table, and return the entry */
  static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
  						       int pcr)

@@ -147,6 +152,19 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
  		pr_err("Error Communicating to TPM chip, result: %d\n", result);
  	return result;
  }
+void ima_measurements_suspend(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 1);
+	mutex_unlock(&ima_extend_list_mutex);
+}

Quoted 101 lines · Tushar Sugandhi

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

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

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

---
  security/integrity/ima/ima.h       |  2 ++ security/integrity/ima/ima_kexec.c | 58 +++++++++++++++++++++++++----- security/integrity/ima/ima_queue.c | 18 ++++++++++

  3 files changed, 69 insertions(+), 9 deletions(-)

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

index c29db699c996..49a6047dd8eb 100644
@@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct 
 ima_template_desc *ima_template);
   int ima_restore_measurement_entry(struct ima_template_entry *entry);
   int ima_restore_measurement_list(loff_t bufsize, void *buf);
   int ima_measurements_show(struct seq_file *m, void *v);
+void ima_measurements_suspend(void);
+void ima_measurements_resume(void);
   unsigned long ima_get_binary_runtime_size(void);
   int ima_init_template(void);

  void ima_init_template_list(void);

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

index 307e07991865..2c11bbe6efef 100644
@@ -17,6 +17,8 @@
   #ifdef CONFIG_IMA_KEXEC
   struct seq_file ima_kexec_file;
   struct ima_kexec_hdr ima_khdr;
+static void *ima_kexec_buffer;
+static size_t kexec_segment_size;
   void ima_clear_kexec_file(void)
   {

@@ -142,7 +144,6 @@ void ima_add_kexec_buffer(struct kimage *image)
       /* use more understandable variable names than defined in kbuf */
       void *kexec_buffer = NULL;
       size_t kexec_buffer_size;
-    size_t kexec_segment_size;
       int ret;
       /*

@@ -167,14 +168,6 @@ void ima_add_kexec_buffer(struct kimage *image)
           return;
       }
-    ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
-                    kexec_segment_size);
-    if (ret < 0) {
-        pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
-               __func__, ret);
-        return;
-    }
-
       kbuf.buffer = kexec_buffer;
       kbuf.bufsz = kexec_buffer_size;
       kbuf.memsz = kexec_segment_size;

@@ -192,6 +185,53 @@ void ima_add_kexec_buffer(struct kimage *image)
       pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
            kbuf.mem);
   }
+
+/*
+ * Called during kexec execute so that IMA can update the measurement 

list.
+ */

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

+static int ima_update_kexec_buffer(struct notifier_block *self,
+                   unsigned long action, void *data)
+{
+    void *buf = NULL;
+    size_t buf_size;
+    bool resume = false;
+    int ret;
+
+    if (!kexec_in_progress) {
+        pr_info("%s: No kexec in progress.\n", __func__);
+        return NOTIFY_OK;
+    }
+
+    if (!ima_kexec_buffer) {

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

Quoted 7 lines

+        pr_err("%s: Kexec buffer not set.\n", __func__); +        return NOTIFY_OK;
+    }

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

+
+    ima_measurements_suspend();
+
+    buf_size = ima_get_binary_runtime_size();

There doesn't seem to be a need to call this function and pass in the binary runtime size into the dump function. You should be able to remove it.

Quoted 4 lines

+    ret = ima_dump_measurement_list(&buf_size, &buf, +                    kexec_segment_size);
+
+    if (!buf || ret < 0) {

I don't think this function can (or should) ever return ret >= 0 with buf == NULL.

Quoted 47 lines

+        pr_err("%s: Dump measurements failed. Error:%d\n", +               __func__, ret);
+        resume = true;
+        goto out;
+    }
+    memcpy(ima_kexec_buffer, buf, buf_size);

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

+out:
+    ima_kexec_buffer = NULL;
+
+    if (resume)
+        ima_measurements_resume();
+
+    return NOTIFY_OK;
+}
+struct notifier_block update_buffer_nb = {
+    .notifier_call = ima_update_kexec_buffer,
+};
+

  #endif /* IMA_KEXEC */
  /*

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

index 532da87ce519..9e7d1196006e 100644
@@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
    */
   static DEFINE_MUTEX(ima_extend_list_mutex);
+/*
+ * Used internally by the kernel to suspend-resume ima measurements.
+ */
+static atomic_t suspend_ima_measurements;
+
   /* lookup up the digest value in the hash table, and return the entry */
   static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
                                  int pcr)

@@ -147,6 +152,19 @@ static int ima_pcr_extend(struct tpm_digest 
 *digests_arg, int pcr)
           pr_err("Error Communicating to TPM chip, result: %d\n", result);
       return result;
   }
+void ima_measurements_suspend(void)
+{
+    mutex_lock(&ima_extend_list_mutex);
+    atomic_set(&suspend_ima_measurements, 1);
+    mutex_unlock(&ima_extend_list_mutex);
+}
+

security/integrity/ima/ima_queue.c static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr) · line 170

Quoted 8 lines

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

+void ima_measurements_resume(void)
+{
+    mutex_lock(&ima_extend_list_mutex);
+    atomic_set(&suspend_ima_measurements, 0);
+    mutex_unlock(&ima_extend_list_mutex);
+}
   /*
    * Add template entry to the measurement list and hash table, and

Hi Tushar,

Quoted 24 lines · Tushar Sugandhi

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

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

Quoted 1 line

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

The lengthiness and complexity of the patch description is an indication that the patch needs to be broken up. Please refer to Documentation/process/submitting-patches.rst for further info.

In addition, this patch moves the function ima_dump_measurement_list() to a new function named ima_update_kexec_buffer(), which is never
called. The patch set is thus not bisect safe.

Quoted 4 lines

[...]

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

+void ima_measurements_suspend(void)
+{
+	mutex_lock(&ima_extend_list_mutex);

security/integrity/ima/ima_queue.c static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr) · line 167

Quoted 10 lines

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

+	atomic_set(&suspend_ima_measurements, 1);
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
+void ima_measurements_resume(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 0);
+	mutex_unlock(&ima_extend_list_mutex);
+}

These function are being defined and called here, but are not enforced
until a later patch. It would make more sense to introduce and
enforce these functions in a single patch with an explanation as to why suspend/resume is needed.

The cover letter describes the problem as "... new IMA measurements are
added between kexec 'load' and kexec 'execute'". Please include in
the patch description the reason for needing suspend/resume, since saving the measurement records is done during kexec execute.

In addition, this patch moves the function ima_dump_measurement_list() to a new function named ima_update_kexec_buffer(), which is never called.   The patch set is thus not bisect safe.

Currently, the mechanism to map and unmap segments to the kimage structure is not available to the subsystems outside of kexec. This functionality is needed when IMA is allocating the memory segments during kexec 'load' operation.

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

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

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

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

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

---
  include/linux/ima.h                |  3 ++
  include/linux/kexec.h              | 13 ++++++
  kernel/kexec_core.c                | 73 ++++++++++++++++++++++++++++--
  security/integrity/ima/ima_kexec.c | 32 +++++++++++++
  4 files changed, 116 insertions(+), 5 deletions(-)

include/linux/ima.h +3 -0

index 86b57757c7b1..006db20f852d 100644
@@ -49,6 +49,9 @@ static inline void ima_appraise_parse_cmdline(void) {}
 
  #ifdef CONFIG_IMA_KEXEC
  extern void ima_add_kexec_buffer(struct kimage *image);
+extern void ima_kexec_post_load(struct kimage *image);
+#else
+static inline void ima_kexec_post_load(struct kimage *image) {}
  #endif
 
  #else

include/linux/kexec.h +13 -0

index 22b5cd24f581..e00b8101b53b 100644
@@ -490,6 +490,15 @@ static inline int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, g
  static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) { }
  #endif
 
+#define for_each_kimage_entry(image, ptr, entry) \
+	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
+		ptr = (entry & IND_INDIRECTION) ? \
+			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
+
+extern void *kimage_map_segment(struct kimage *image,
+				unsigned long addr, unsigned long size);
+extern void kimage_unmap_segment(void *buffer);
+
  #else /* !CONFIG_KEXEC_CORE */
  struct pt_regs;
  struct task_struct;

@@ -497,6 +506,10 @@ static inline void __crash_kexec(struct pt_regs *regs) { }
  static inline void crash_kexec(struct pt_regs *regs) { }
  static inline int kexec_should_crash(struct task_struct *p) { return 0; }
  static inline int kexec_crash_loaded(void) { return 0; }
+static inline void *kimage_map_segment(struct kimage *image,
+				       unsigned long addr, unsigned long size)
+{ return NULL; }
+static inline void kimage_unmap_segment(void *buffer) { }
  #define kexec_in_progress false
  #endif /* CONFIG_KEXEC_CORE */
 

kernel/kexec_core.c +9 -5

index 3d578c6fefee..e01156f3c404 100644
@@ -594,11 +594,6 @@ void kimage_terminate(struct kimage *image)
  	*image->entry = IND_DONE;
  }
 
-#define for_each_kimage_entry(image, ptr, entry) \
-	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
-		ptr = (entry & IND_INDIRECTION) ? \
-			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
-
  static void kimage_free_entry(kimage_entry_t entry)
  {
  	struct page *page;

@@ -921,6 +916,74 @@ int kimage_load_segment(struct kimage *image,
  	return result;
  }
 
+void *kimage_map_segment(struct kimage *image,
+			 unsigned long addr, unsigned long size)
+{
+	unsigned long eaddr = addr + size;
+	unsigned long src_page_addr, dest_page_addr;
+	struct page **src_pages;
+	int i, npages;
+	kimage_entry_t *ptr, entry;
+	void *vaddr = NULL;

kernel/kexec_core.c +27 -0

+
+	if (i == image->nr_segments) {
+		pr_err("%s: No segment matching [%lx, %lx)\n", __func__,
+		       addr, eaddr);
+		return NULL;
+	}
+
+	/*
+	 * Collect the source pages and map them in a contiguous VA range.
+	 */
+	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
+	src_pages = kmalloc(sizeof(*src_pages) * npages, GFP_KERNEL);
+	if (!src_pages) {
+		pr_err("%s: Could not allocate ima pages array.\n", __func__);
+		return NULL;
+	}
+
+	i = 0;
+	for_each_kimage_entry(image, ptr, entry) {
+		if (entry & IND_DESTINATION)
+			dest_page_addr = entry & PAGE_MASK;
+		else if (entry & IND_SOURCE) {
+			if (dest_page_addr >= addr && dest_page_addr < eaddr) {
+				src_page_addr = entry & PAGE_MASK;
+				src_pages[i++] =
+					virt_to_page(__va(src_page_addr));
+				if (i == npages)

kernel/kexec_core.c +12 -0

+	if (!vaddr) {
+		pr_err("%s: Could not map imap buffer.\n", __func__);
+		kfree(src_pages);
+	}
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
  struct kexec_load_limit {
  	/* Mutex protects the limit count. */
  	struct mutex mutex;

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

index 2c11bbe6efef..13fbbb90319b 100644
@@ -12,6 +12,8 @@
  #include <linux/kexec.h>
  #include <linux/of.h>
  #include <linux/ima.h>
+#include <linux/reboot.h>
+#include <asm/page.h>
  #include "ima.h"
 
  #ifdef CONFIG_IMA_KEXEC

@@ -19,6 +21,7 @@ struct seq_file ima_kexec_file;
  struct ima_kexec_hdr ima_khdr;
  static void *ima_kexec_buffer;
  static size_t kexec_segment_size;
+static bool ima_kexec_update_registered;
 
  void ima_clear_kexec_file(void)
  {

@@ -221,6 +224,7 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
  	}
  	memcpy(ima_kexec_buffer, buf, buf_size);
  out:
+	kimage_unmap_segment(ima_kexec_buffer);
  	ima_kexec_buffer = NULL;
 
  	if (resume)

@@ -232,6 +236,34 @@ struct notifier_block update_buffer_nb = {
  	.notifier_call = ima_update_kexec_buffer,
  };
 
+/*
+ * Create a mapping for the source pages that contain the IMA buffer
+ * so we can update it later.
+ */
+void ima_kexec_post_load(struct kimage *image)
+{
+	if (ima_kexec_buffer) {
+		kimage_unmap_segment(ima_kexec_buffer);
+		ima_kexec_buffer = NULL;
+	}
+
+	if (!image->ima_buffer_addr)
+		return;
+
+	ima_kexec_buffer = kimage_map_segment(image,
+					      image->ima_buffer_addr,
+					      image->ima_buffer_size);
+	if (!ima_kexec_buffer) {
+		pr_err("%s: Could not map measurements buffer.\n", __func__);
+		return;
+	}

Quoted 101 lines · Tushar Sugandhi

Currently, the mechanism to map and unmap segments to the kimage structure is not available to the subsystems outside of kexec.  This functionality is needed when IMA is allocating the memory segments during kexec 'load' operation.

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

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

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

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

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

---
  include/linux/ima.h                |  3 ++ include/linux/kexec.h              | 13 ++++++ kernel/kexec_core.c                | 73 ++++++++++++++++++++++++++++-- security/integrity/ima/ima_kexec.c | 32 +++++++++++++

  4 files changed, 116 insertions(+), 5 deletions(-)

include/linux/ima.h +3 -0

index 86b57757c7b1..006db20f852d 100644
@@ -49,6 +49,9 @@ static inline void ima_appraise_parse_cmdline(void) {}
   #ifdef CONFIG_IMA_KEXEC
   extern void ima_add_kexec_buffer(struct kimage *image);
+extern void ima_kexec_post_load(struct kimage *image);
+#else
+static inline void ima_kexec_post_load(struct kimage *image) {}
   #endif
   #else

include/linux/kexec.h +0 -0

index 22b5cd24f581..e00b8101b53b 100644
@@ -490,6 +490,15 @@ static inline int 

arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, g
  static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) { } #endif

include/linux/kexec.h +9 -0

+#define for_each_kimage_entry(image, ptr, entry) \
+    for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
+        ptr = (entry & IND_INDIRECTION) ? \
+            boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
+
+extern void *kimage_map_segment(struct kimage *image,
+                unsigned long addr, unsigned long size);
+extern void kimage_unmap_segment(void *buffer);
+

  #else /* !CONFIG_KEXEC_CORE */
  struct pt_regs;
  struct task_struct;

include/linux/kexec.h +4 -0

@@ -497,6 +506,10 @@ static inline void __crash_kexec(struct pt_regs 
 *regs) { }
   static inline void crash_kexec(struct pt_regs *regs) { }
   static inline int kexec_should_crash(struct task_struct *p) { return 0; }
   static inline int kexec_crash_loaded(void) { return 0; }
+static inline void *kimage_map_segment(struct kimage *image,
+                       unsigned long addr, unsigned long size)
+{ return NULL; }
+static inline void kimage_unmap_segment(void *buffer) { }
   #define kexec_in_progress false
   #endif /* CONFIG_KEXEC_CORE */

kernel/kexec_core.c +9 -5

index 3d578c6fefee..e01156f3c404 100644
@@ -594,11 +594,6 @@ void kimage_terminate(struct kimage *image)
       *image->entry = IND_DONE;
   }
-#define for_each_kimage_entry(image, ptr, entry) \
-    for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
-        ptr = (entry & IND_INDIRECTION) ? \
-            boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
-
   static void kimage_free_entry(kimage_entry_t entry)
   {
       struct page *page;

@@ -921,6 +916,74 @@ int kimage_load_segment(struct kimage *image,
       return result;
   }
+void *kimage_map_segment(struct kimage *image,
+             unsigned long addr, unsigned long size)
+{
+    unsigned long eaddr = addr + size;
+    unsigned long src_page_addr, dest_page_addr;
+    struct page **src_pages;
+    int i, npages;
+    kimage_entry_t *ptr, entry;
+    void *vaddr = NULL;

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

Quoted 10 lines

kernel/kexec_core.c +10 -0

+
+    /*
+     * Make sure that we are mapping a whole segment.
+     */
+    for (i = 0; i < image->nr_segments; i++) {
+        if (addr == image->segment[i].mem &&
+            size == image->segment[i].memsz) {
+            break;
+        }
+    }

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

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

and previously kexec_add_buffer() was also called:

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

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

So the one segment should be matching these addresses.

Quoted 30 lines

kernel/kexec_core.c +30 -0

+
+    if (i == image->nr_segments) {
+        pr_err("%s: No segment matching [%lx, %lx)\n", __func__,
+               addr, eaddr);
+        return NULL;
+    }
+
+    /*
+     * Collect the source pages and map them in a contiguous VA range.
+     */
+    npages = PFN_UP(eaddr) - PFN_DOWN(addr);
+    src_pages = kmalloc(sizeof(*src_pages) * npages, GFP_KERNEL);
+    if (!src_pages) {
+        pr_err("%s: Could not allocate ima pages array.\n", __func__);
+        return NULL;
+    }
+
+    i = 0;
+    for_each_kimage_entry(image, ptr, entry) {
+        if (entry & IND_DESTINATION)
+            dest_page_addr = entry & PAGE_MASK;
+        else if (entry & IND_SOURCE) {
+            if (dest_page_addr >= addr && dest_page_addr < eaddr) {
+                src_page_addr = entry & PAGE_MASK;
+                src_pages[i++] =
+                    virt_to_page(__va(src_page_addr));
+                if (i == npages)
+                    break;
+                dest_page_addr += PAGE_SIZE;
+            }

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

Quoted 7 lines

kernel/kexec_core.c +7 -0

+        }
+    }
+
+    /* Sanity check. */
+    WARN_ON(i < npages);
+
+    vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);

On either success or failure kfree(sec_pages); should go right here to not have a memory leak.

Quoted 67 lines

kernel/kexec_core.c +12 -0

+    if (!vaddr) {
+        pr_err("%s: Could not map imap buffer.\n", __func__);
+        kfree(src_pages);
+    }
+    return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+    vunmap(segment_buffer);
+}
+
   struct kexec_load_limit {
       /* Mutex protects the limit count. */
       struct mutex mutex;

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

index 2c11bbe6efef..13fbbb90319b 100644
@@ -12,6 +12,8 @@
   #include <linux/kexec.h>
   #include <linux/of.h>
   #include <linux/ima.h>
+#include <linux/reboot.h>
+#include <asm/page.h>
   #include "ima.h"
   #ifdef CONFIG_IMA_KEXEC

@@ -19,6 +21,7 @@ struct seq_file ima_kexec_file;
   struct ima_kexec_hdr ima_khdr;
   static void *ima_kexec_buffer;
   static size_t kexec_segment_size;
+static bool ima_kexec_update_registered;
   void ima_clear_kexec_file(void)
   {

@@ -221,6 +224,7 @@ static int ima_update_kexec_buffer(struct 
 notifier_block *self,
       }
       memcpy(ima_kexec_buffer, buf, buf_size);

  out:
+    kimage_unmap_segment(ima_kexec_buffer);
      ima_kexec_buffer = NULL;
      if (resume)

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

@@ -232,6 +236,34 @@ struct notifier_block update_buffer_nb = {
       .notifier_call = ima_update_kexec_buffer,
   };
+/*
+ * Create a mapping for the source pages that contain the IMA buffer
+ * so we can update it later.
+ */
+void ima_kexec_post_load(struct kimage *image)
+{
+    if (ima_kexec_buffer) {
+        kimage_unmap_segment(ima_kexec_buffer);
+        ima_kexec_buffer = NULL;
+    }
+
+    if (!image->ima_buffer_addr)
+        return;
+
+    ima_kexec_buffer = kimage_map_segment(image,
+                          image->ima_buffer_addr,
+                          image->ima_buffer_size);
+    if (!ima_kexec_buffer) {
+        pr_err("%s: Could not map measurements buffer.\n", __func__);
+        return;
+    }
+

security/integrity/ima/ima_kexec.c struct notifier_block update_buffer_nb = { · line 269

Quoted 8 lines

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

+    if (!ima_kexec_update_registered) {
+        register_reboot_notifier(&update_buffer_nb);
+        ima_kexec_update_registered = true;
+    }
+}
+
   #endif /* IMA_KEXEC */
   /*

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

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

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

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

---
  security/integrity/ima/Kconfig     |  9 +++++++++
  security/integrity/ima/ima_kexec.c | 13 ++++++++-----
  2 files changed, 17 insertions(+), 5 deletions(-)

security/integrity/ima/Kconfig +0 -0

index 60a511c6b583..1b55cd2bcb36 100644
@@ -338,3 +338,12 @@ config IMA_DISABLE_HTABLE
  	default n
  	help

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

index 13fbbb90319b..6cd5f46a7208 100644
@@ -150,15 +150,18 @@ void ima_add_kexec_buffer(struct kimage *image)
  	int ret;
 
  	/*
-	 * Reserve an extra half page of memory for additional measurements
-	 * added during the kexec load.
+	 * Reserve extra memory for measurements added in the window from
+	 * kexec 'load' to kexec 'execute'.
  	 */
-	binary_runtime_size = ima_get_binary_runtime_size();
+	binary_runtime_size = ima_get_binary_runtime_size() +
+			      sizeof(struct ima_kexec_hdr) +
+			      (CONFIG_IMA_KEXEC_EXTRA_PAGES * PAGE_SIZE);
+

Quoted 31 lines · Tushar Sugandhi

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

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

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

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

---
  security/integrity/ima/Kconfig     |  9 +++++++++ security/integrity/ima/ima_kexec.c | 13 ++++++++-----

  2 files changed, 17 insertions(+), 5 deletions(-)

security/integrity/ima/Kconfig +0 -0

index 60a511c6b583..1b55cd2bcb36 100644
@@ -338,3 +338,12 @@ config IMA_DISABLE_HTABLE

      default n
      help
         This option disables htable to allow measurement of duplicate records.

security/integrity/ima/Kconfig +4 -0

+
+config IMA_KEXEC_EXTRA_PAGES
+    int
+    depends on IMA && IMA_KEXEC

Quoted 5 lines

+    default 16
+    help
+      IMA_KEXEC_EXTRA_PAGES determines the number of extra +      pages to be allocated for IMA measurements added in the +      window from kexec 'load' to kexec 'execute'.

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

Quoted 19 lines

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

index 13fbbb90319b..6cd5f46a7208 100644
@@ -150,15 +150,18 @@ void ima_add_kexec_buffer(struct kimage *image)
       int ret;
       /*
-     * Reserve an extra half page of memory for additional measurements
-     * added during the kexec load.
+     * Reserve extra memory for measurements added in the window from
+     * kexec 'load' to kexec 'execute'.
        */
-    binary_runtime_size = ima_get_binary_runtime_size();
+    binary_runtime_size = ima_get_binary_runtime_size() +
+                  sizeof(struct ima_kexec_hdr) +
+                  (CONFIG_IMA_KEXEC_EXTRA_PAGES * PAGE_SIZE);
+
       if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE)
           kexec_segment_size = ULONG_MAX;

Quoted 8 lines

      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");

Quoted 33 lines · Stefan Berger

Quoted 31 lines · Tushar Sugandhi

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

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

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

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

---
  security/integrity/ima/Kconfig     |  9 +++++++++ security/integrity/ima/ima_kexec.c | 13 ++++++++-----

  2 files changed, 17 insertions(+), 5 deletions(-)

security/integrity/ima/Kconfig +0 -0

index 60a511c6b583..1b55cd2bcb36 100644
@@ -338,3 +338,12 @@ config IMA_DISABLE_HTABLE

      default n
      help
         This option disables htable to allow measurement of duplicate records.

security/integrity/ima/Kconfig +4 -0

+
+config IMA_KEXEC_EXTRA_PAGES
+    int
+    depends on IMA && IMA_KEXEC

Quoted 10 lines

Quoted 5 lines

+    default 16
+    help
+      IMA_KEXEC_EXTRA_PAGES determines the number of extra +      pages to be allocated for IMA measurements added in the +      window from kexec 'load' to kexec 'execute'.

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

Not sure I understand.  Do you mean I should make the default value of the config IMA_KEXEC_EXTRA_PAGES 64 or something?

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

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

~Tushar

Quoted 20 lines

Quoted 20 lines

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

index 13fbbb90319b..6cd5f46a7208 100644
@@ -150,15 +150,18 @@ void ima_add_kexec_buffer(struct kimage *image)
       int ret;
       /*
-     * Reserve an extra half page of memory for additional 

measurements
-     * added during the kexec load.
+     * Reserve extra memory for measurements added in the window from +     * kexec 'load' to kexec 'execute'.
       */
-    binary_runtime_size = ima_get_binary_runtime_size();
+    binary_runtime_size = ima_get_binary_runtime_size() + +                  sizeof(struct ima_kexec_hdr) +
+                  (CONFIG_IMA_KEXEC_EXTRA_PAGES * PAGE_SIZE); +
      if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE) kexec_segment_size = ULONG_MAX;

Quoted 8 lines

Quoted 8 lines

      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");

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

   Stefan

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

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

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

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

@@ -198,6 +208,7 @@ void ima_add_kexec_buffer(struct kimage *image)
  static int ima_update_kexec_buffer(struct notifier_block *self,
  				   unsigned long action, void *data)
  {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
  	void *buf = NULL;
  	size_t buf_size;
  	bool resume = false;

@@ -213,9 +224,31 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
  		return NOTIFY_OK;
  	}
 
+	buf_size = ima_get_binary_runtime_size();
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;",
+		  kexec_segment_size, buf_size);
+
+	/*
+	 * This is one of the very last events measured by IMA before kexec
+	 * soft rebooting into the new Kernal.
+	 * This event can be used as a marker after the system soft reboots
+	 * to the new Kernel to check if there was sufficient memory allocated
+	 * at kexec 'load' to capture the events measured between the window
+	 * of kexec 'load' and 'execute'.
+	 * This event needs to be present in the IMA log, in between the two
+	 * 'boot_aggregate' events that are logged for the previous boot and
+	 * the current soft reboot. If it is not present after the system soft
+	 * reboots into the new Kernel, it would mean the IMA log is not

Quoted 60 lines · Tushar Sugandhi

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

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

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

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

@@ -198,6 +208,7 @@ void ima_add_kexec_buffer(struct kimage *image)
   static int ima_update_kexec_buffer(struct notifier_block *self,
                      unsigned long action, void *data)
   {
+    char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
       void *buf = NULL;
       size_t buf_size;
       bool resume = false;

@@ -213,9 +224,31 @@ static int ima_update_kexec_buffer(struct 
 notifier_block *self,
           return NOTIFY_OK;
       }
+    buf_size = ima_get_binary_runtime_size();
+    scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+          "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;",
+          kexec_segment_size, buf_size);
+
+    /*
+     * This is one of the very last events measured by IMA before kexec
+     * soft rebooting into the new Kernal.
+     * This event can be used as a marker after the system soft reboots
+     * to the new Kernel to check if there was sufficient memory 

allocated
+     * at kexec 'load' to capture the events measured between the window +     * of kexec 'load' and 'execute'.
+     * This event needs to be present in the IMA log, in between the two +     * 'boot_aggregate' events that are logged for the previous boot and +     * the current soft reboot. If it is not present after the system soft
+     * reboots into the new Kernel, it would mean the IMA log is not +     * consistent with the TPM PCR quotes, and the system needs to be +     * cold-booted for the attestation to succeed again.
+     */

Quoted 6 lines

+    ima_measure_critical_data("ima_kexec", "kexec_execute", +                  ima_kexec_event, strlen(ima_kexec_event), +                  false, NULL, 0);
+
      ima_measurements_suspend();
-    buf_size = ima_get_binary_runtime_size();

This should be removed earlier, in 2/7.

Quoted 2 lines

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

Hi Tushar,

Quoted 26 lines · Tushar Sugandhi

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

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

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

Quoted 1 line

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

Adding a new type of critical data is fine. The patch description should describe the reason for it. Please update the Subject line and the patch description accordingly.

Quoted 65 lines

---
  security/integrity/ima/ima_kexec.c | 35 +++++++++++++++++++++++++++++-
  1 file changed, 34 insertions(+), 1 deletion(-)

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

index 6cd5f46a7208..0f9c424fe808 100644
@@ -17,6 +17,8 @@
  #include "ima.h"
 
  #ifdef CONFIG_IMA_KEXEC
+#define IMA_KEXEC_EVENT_LEN 128
+
  struct seq_file ima_kexec_file;
  struct ima_kexec_hdr ima_khdr;
  static void *ima_kexec_buffer;

@@ -34,6 +36,8 @@ void ima_clear_kexec_file(void)
 
  static int ima_alloc_kexec_buf(size_t kexec_segment_size)
  {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
+
  	if ((kexec_segment_size == 0) ||
  	    (kexec_segment_size == ULONG_MAX) ||
  	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {

@@ -64,6 +68,12 @@ static int ima_alloc_kexec_buf(size_t kexec_segment_size)
  	memset(&ima_khdr, 0, sizeof(ima_khdr));
  	ima_khdr.version = 1;
 
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;", kexec_segment_size);
+
+	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
+				  strlen(ima_kexec_event), false, NULL, 0);
+
  	return 0;
  }
 

@@ -198,6 +208,7 @@ void ima_add_kexec_buffer(struct kimage *image)
  static int ima_update_kexec_buffer(struct notifier_block *self,
  				   unsigned long action, void *data)
  {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
  	void *buf = NULL;
  	size_t buf_size;
  	bool resume = false;

@@ -213,9 +224,31 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
  		return NOTIFY_OK;
  	}
 
+	buf_size = ima_get_binary_runtime_size();
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;",
+		  kexec_segment_size, buf_size);
+
+	/*
+	 * This is one of the very last events measured by IMA before kexec
+	 * soft rebooting into the new Kernal.
+	 * This event can be used as a marker after the system soft reboots
+	 * to the new Kernel to check if there was sufficient memory allocated
+	 * at kexec 'load' to capture the events measured between the window
+	 * of kexec 'load' and 'execute'.
+	 * This event needs to be present in the IMA log, in between the two
+	 * 'boot_aggregate' events that are logged for the previous boot and

security/integrity/ima/ima_kexec.c static int ima_update_kexec_buffer(struct notifier_block *self, · line 245

Quoted 5 lines

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

+	 * the current soft reboot. If it is not present after the system soft
+	 * reboots into the new Kernel, it would mean the IMA log is not
+	 * consistent with the TPM PCR quotes, and the system needs to be
+	 * cold-booted for the attestation to succeed again.
+	 */

Comments like this don't belong in the code. Please refer to section "8) Commenting" of Documentation/process/coding-style.rst.

Quoted 11 lines

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

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

Quoted 7 lines · Tushar Sugandhi

The current Kernel behavior is IMA measurements snapshot is taken at kexec 'load' and not at kexec 'execute'. IMA log is then carried over to the new Kernel after kexec 'execute'.

Some systems can be configured to call kexec 'load' first, and followed by kexec 'execute' after some time. (as opposed to calling 'load' and 'execute' in one single kexec command).

Additional measurements may be introduced by the kexec load itself. Saving the measurement list as close as possible to the reboot is beneficial, whether or not the kexec load and kexec execute are executed separately.

Quoted 9 lines

In such scenario, if new IMA
measurements are added between kexec 'load' and kexec 'execute', the TPM PCRs are extended with the IMA events between 'load' and 'execute'. But those IMA events are not carried over to the new Kernel after kexec soft reboot. This results in mismatch between TPM PCR quotes, and the actual IMA measurements list, after the system boots into the new kexec image. This mismatch results in the remote attestation failing for that system.

Quoted 3 lines

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

How about beginning the paragraph with "To solve this problem allocate ... and populate ..."

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

Quoted 4 lines · Tushar Sugandhi

The current Kernel behavior is IMA measurements snapshot is taken at kexec 'load' and not at kexec 'execute'. IMA log is then carried over to the new Kernel after kexec 'execute'.

Quoted 3 lines

Some systems can be configured to call kexec 'load' first, and followed by kexec 'execute' after some time. (as opposed to calling 'load' and 'execute' in one single kexec command).

Additional measurements may be introduced by the kexec load itself. Saving the measurement list as close as possible to the reboot is beneficial, whether or not the kexec load and kexec execute are executed separately.

Quoted 9 lines

In such scenario, if new IMA
measurements are added between kexec 'load' and kexec 'execute', the TPM PCRs are extended with the IMA events between 'load' and 'execute'. But those IMA events are not carried over to the new Kernel after kexec soft reboot. This results in mismatch between TPM PCR quotes, and the actual IMA measurements list, after the system boots into the new kexec image. This mismatch results in the remote attestation failing for that system.

Quoted 3 lines

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

How about beginning the paragraph with "To solve this problem allocate ... and populate ..."