All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/7] ima: kexec: measure events between kexec load and execute
@ 2024-01-22 18:37 ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:37 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

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

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

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

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

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

 - kexec functionality for mapping the segments from the current Kernel
   to the subsequent one, 

 - necessary changes to the kexec_file_load syscall, enabling it to call
   the ima functions,

 - registering a reboot notifier which gets called during kexec 
   'execute',

 - introducing a new Kconfig option to configure the extra memory to be
   allocated for passing IMA log from the current Kernel to the next,
   
 - introducing two new events to be measured by IMA during kexec, to
   help diagnose if the IMA log was copied fully or partially, from the
   current Kernel to the next,

 - excluding IMA segment while calculating and storing digest in function
   kexec_calculate_store_digests(), since IMA segment can be modified
   after the digest is computed during kexec 'load'.  This will ensure
   that the segment is not added to the 'purgatory_sha_regions', and thus
   not verified by verify_sha256_digest().

The changes proposed in this series ensure the integrity of the IMA
measurements is preserved across kexec soft reboots, thus significantly
improving the security of the Kernel post kexec soft reboots.

There were previous attempts to fix this issue [1], [2], [3].  But they
were not merged into the mainline Kernel.

We took inspiration from the past work [1] and [2] while working on this
patch series.

V3 of this series is available here[5] for reference.

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

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

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

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

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

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

Change Log v4:
 - Incorporated feedback from the community (Stefan Berger and
   Mimi Zohar) on v3 of this series[5].
 - Rearranged patches so that they remain bisect-safe i.e. the
   system can go through kexec soft reboot, and IMA log is carried
   over after each patch.
 - Verified all the patches are bisect-safe by booting into each
   patch and verifying kexec soft reboot works, and IMA log gets
   carried over.
 - Suspend-resume measurements is now a separate patch (patch #5)
   and all the relevant code is part of the same patch.
 - Excluding IMA segment from segment digest verification is now a
   separate patch. (patch #3).
 - Registering reboot notifier and functions related to move ima 
   log copy from kexec load to execute are now part of the same
   patch (patch #4) to protect bisect-safeness of the series.
 - Updated the title of patch #6 as per the feedback.
 - The default value of kexec extra memory for IMA measurements
   is set to half the PAGESIZE to maintain backwards compatibility.
 - Added number of IMA measurement records as part of 'kexec_load' 
   and 'kexec_execute' IMA critical data events.
 - Updated patch descriptions as necessary.
Change Log v3:
 - Incorporated feedback from the community (Stefan Berger and
   Mimi Zohar) on v2 of this series[4].
 - Renamed functions and removed extraneous checks and code comments.
 - Updated patch descriptions and titles as necessary.
 - Updated kexec_calculate_store_digests() in patch 2/7 to exclude ima
   segment from calculating and storing digest.
 - Updated patch 3/7 to use kmalloc_array instead of kmalloc and freed
   memory early to avoid potential memory leak.
 - Updated patch 6/7 to change Kconfig option IMA_KEXEC_EXTRA_PAGES to
   IMA_KEXEC_EXTRA_MEMORY_KB to allocate the memory in kb rather than
   in number of pages.
 - Optimized patch 7/7 not to free and alloc memory if the buffer size
   hasn't changed during multiple kexec 'load' operations.
 - Fixed a bug in patch 7/7 to measure multiple 'kexec_load' events even
   if buffer size hasn't changed.
 - Verified the patches are bisect-safe by compiling and booting into
   each patch individually.


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


Tushar Sugandhi (7):
  ima: define and call ima_alloc_kexec_file_buf
  kexec: define functions to map and unmap segments
  ima: kexec: skip IMA segment validation after kexec soft reboot
  ima: kexec: move ima log copy from kexec load to execute
  ima: suspend measurements during buffer copy at kexec execute
  ima: make the kexec extra memory configurable
  ima: measure kexec load and exec events as critical data

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

-- 
2.25.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH v4 0/7] ima: kexec: measure events between kexec load and execute
@ 2024-01-22 18:37 ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:37 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

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

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

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

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

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

 - kexec functionality for mapping the segments from the current Kernel
   to the subsequent one, 

 - necessary changes to the kexec_file_load syscall, enabling it to call
   the ima functions,

 - registering a reboot notifier which gets called during kexec 
   'execute',

 - introducing a new Kconfig option to configure the extra memory to be
   allocated for passing IMA log from the current Kernel to the next,
   
 - introducing two new events to be measured by IMA during kexec, to
   help diagnose if the IMA log was copied fully or partially, from the
   current Kernel to the next,

 - excluding IMA segment while calculating and storing digest in function
   kexec_calculate_store_digests(), since IMA segment can be modified
   after the digest is computed during kexec 'load'.  This will ensure
   that the segment is not added to the 'purgatory_sha_regions', and thus
   not verified by verify_sha256_digest().

The changes proposed in this series ensure the integrity of the IMA
measurements is preserved across kexec soft reboots, thus significantly
improving the security of the Kernel post kexec soft reboots.

There were previous attempts to fix this issue [1], [2], [3].  But they
were not merged into the mainline Kernel.

We took inspiration from the past work [1] and [2] while working on this
patch series.

V3 of this series is available here[5] for reference.

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

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

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

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

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

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

Change Log v4:
 - Incorporated feedback from the community (Stefan Berger and
   Mimi Zohar) on v3 of this series[5].
 - Rearranged patches so that they remain bisect-safe i.e. the
   system can go through kexec soft reboot, and IMA log is carried
   over after each patch.
 - Verified all the patches are bisect-safe by booting into each
   patch and verifying kexec soft reboot works, and IMA log gets
   carried over.
 - Suspend-resume measurements is now a separate patch (patch #5)
   and all the relevant code is part of the same patch.
 - Excluding IMA segment from segment digest verification is now a
   separate patch. (patch #3).
 - Registering reboot notifier and functions related to move ima 
   log copy from kexec load to execute are now part of the same
   patch (patch #4) to protect bisect-safeness of the series.
 - Updated the title of patch #6 as per the feedback.
 - The default value of kexec extra memory for IMA measurements
   is set to half the PAGESIZE to maintain backwards compatibility.
 - Added number of IMA measurement records as part of 'kexec_load' 
   and 'kexec_execute' IMA critical data events.
 - Updated patch descriptions as necessary.
Change Log v3:
 - Incorporated feedback from the community (Stefan Berger and
   Mimi Zohar) on v2 of this series[4].
 - Renamed functions and removed extraneous checks and code comments.
 - Updated patch descriptions and titles as necessary.
 - Updated kexec_calculate_store_digests() in patch 2/7 to exclude ima
   segment from calculating and storing digest.
 - Updated patch 3/7 to use kmalloc_array instead of kmalloc and freed
   memory early to avoid potential memory leak.
 - Updated patch 6/7 to change Kconfig option IMA_KEXEC_EXTRA_PAGES to
   IMA_KEXEC_EXTRA_MEMORY_KB to allocate the memory in kb rather than
   in number of pages.
 - Optimized patch 7/7 not to free and alloc memory if the buffer size
   hasn't changed during multiple kexec 'load' operations.
 - Fixed a bug in patch 7/7 to measure multiple 'kexec_load' events even
   if buffer size hasn't changed.
 - Verified the patches are bisect-safe by compiling and booting into
   each patch individually.


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


Tushar Sugandhi (7):
  ima: define and call ima_alloc_kexec_file_buf
  kexec: define functions to map and unmap segments
  ima: kexec: skip IMA segment validation after kexec soft reboot
  ima: kexec: move ima log copy from kexec load to execute
  ima: suspend measurements during buffer copy at kexec execute
  ima: make the kexec extra memory configurable
  ima: measure kexec load and exec events as critical data

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

-- 
2.25.1


^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
  2024-01-22 18:37 ` Tushar Sugandhi
@ 2024-01-22 18:37   ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:37 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

Refactor ima_dump_measurement_list() to move the memory allocation part
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 function ima_dump_measurement_list() as 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 <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 29 deletions(-)

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
@ 2024-01-22 18:37   ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:37 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

Refactor ima_dump_measurement_list() to move the memory allocation part
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 function ima_dump_measurement_list() as 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 <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 29 deletions(-)

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


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 2/7] kexec: define functions to map and unmap segments
  2024-01-22 18:37 ` Tushar Sugandhi
@ 2024-01-22 18:37   ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:37 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

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

Implement kimage_unmap_segment() for unmapping segments
using vunmap().  Relocate 'for_each_kimage_entry()' macro from
kexec_core.c to kexec.h for broader accessibility.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 include/linux/kexec.h              | 13 +++++++
 kernel/kexec_core.c                | 59 +++++++++++++++++++++++++++---
 security/integrity/ima/ima_kexec.c |  1 +
 3 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 22b5cd24f581..e00b8101b53b 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -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 */
 
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 3d578c6fefee..26978ad02676 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -594,11 +594,6 @@ void kimage_terminate(struct kimage *image)
 	*image->entry = IND_DONE;
 }
 
-#define for_each_kimage_entry(image, ptr, entry) \
-	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
-		ptr = (entry & IND_INDIRECTION) ? \
-			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
-
 static void kimage_free_entry(kimage_entry_t entry)
 {
 	struct page *page;
@@ -921,6 +916,60 @@ int kimage_load_segment(struct kimage *image,
 	return result;
 }
 
+void *kimage_map_segment(struct kimage *image,
+			 unsigned long addr, unsigned long size)
+{
+	unsigned long eaddr = addr + size;
+	unsigned long src_page_addr, dest_page_addr;
+	unsigned int npages;
+	struct page **src_pages;
+	int i;
+	kimage_entry_t *ptr, entry;
+	void *vaddr = NULL;
+
+	/*
+	 * Collect the source pages and map them in a contiguous VA range.
+	 */
+	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
+	src_pages = kmalloc_array(npages, sizeof(*src_pages), GFP_KERNEL);
+	if (!src_pages) {
+		pr_err("%s: Could not allocate ima pages array.\n", __func__);
+		return NULL;
+	}
+
+	i = 0;
+	for_each_kimage_entry(image, ptr, entry) {
+		if (entry & IND_DESTINATION)
+			dest_page_addr = entry & PAGE_MASK;
+		else if (entry & IND_SOURCE) {
+			if (dest_page_addr >= addr && dest_page_addr < eaddr) {
+				src_page_addr = entry & PAGE_MASK;
+				src_pages[i++] =
+					virt_to_page(__va(src_page_addr));
+				if (i == npages)
+					break;
+				dest_page_addr += PAGE_SIZE;
+			}
+		}
+	}
+
+	/* Sanity check. */
+	WARN_ON(i < npages);
+
+	vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
+	kfree(src_pages);
+
+	if (!vaddr)
+		pr_err("%s: Could not map imap buffer.\n", __func__);
+
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
 struct kexec_load_limit {
 	/* Mutex protects the limit count. */
 	struct mutex mutex;
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 99daac355c70..4f944c9b4168 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -170,6 +170,7 @@ void ima_add_kexec_buffer(struct kimage *image)
 	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		 kbuf.mem);
 }
+
 #endif /* IMA_KEXEC */
 
 /*
-- 
2.25.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 2/7] kexec: define functions to map and unmap segments
@ 2024-01-22 18:37   ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:37 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

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

Implement kimage_unmap_segment() for unmapping segments
using vunmap().  Relocate 'for_each_kimage_entry()' macro from
kexec_core.c to kexec.h for broader accessibility.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 include/linux/kexec.h              | 13 +++++++
 kernel/kexec_core.c                | 59 +++++++++++++++++++++++++++---
 security/integrity/ima/ima_kexec.c |  1 +
 3 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 22b5cd24f581..e00b8101b53b 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -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 */
 
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 3d578c6fefee..26978ad02676 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -594,11 +594,6 @@ void kimage_terminate(struct kimage *image)
 	*image->entry = IND_DONE;
 }
 
-#define for_each_kimage_entry(image, ptr, entry) \
-	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
-		ptr = (entry & IND_INDIRECTION) ? \
-			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
-
 static void kimage_free_entry(kimage_entry_t entry)
 {
 	struct page *page;
@@ -921,6 +916,60 @@ int kimage_load_segment(struct kimage *image,
 	return result;
 }
 
+void *kimage_map_segment(struct kimage *image,
+			 unsigned long addr, unsigned long size)
+{
+	unsigned long eaddr = addr + size;
+	unsigned long src_page_addr, dest_page_addr;
+	unsigned int npages;
+	struct page **src_pages;
+	int i;
+	kimage_entry_t *ptr, entry;
+	void *vaddr = NULL;
+
+	/*
+	 * Collect the source pages and map them in a contiguous VA range.
+	 */
+	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
+	src_pages = kmalloc_array(npages, sizeof(*src_pages), GFP_KERNEL);
+	if (!src_pages) {
+		pr_err("%s: Could not allocate ima pages array.\n", __func__);
+		return NULL;
+	}
+
+	i = 0;
+	for_each_kimage_entry(image, ptr, entry) {
+		if (entry & IND_DESTINATION)
+			dest_page_addr = entry & PAGE_MASK;
+		else if (entry & IND_SOURCE) {
+			if (dest_page_addr >= addr && dest_page_addr < eaddr) {
+				src_page_addr = entry & PAGE_MASK;
+				src_pages[i++] =
+					virt_to_page(__va(src_page_addr));
+				if (i == npages)
+					break;
+				dest_page_addr += PAGE_SIZE;
+			}
+		}
+	}
+
+	/* Sanity check. */
+	WARN_ON(i < npages);
+
+	vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
+	kfree(src_pages);
+
+	if (!vaddr)
+		pr_err("%s: Could not map imap buffer.\n", __func__);
+
+	return vaddr;
+}
+
+void kimage_unmap_segment(void *segment_buffer)
+{
+	vunmap(segment_buffer);
+}
+
 struct kexec_load_limit {
 	/* Mutex protects the limit count. */
 	struct mutex mutex;
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 99daac355c70..4f944c9b4168 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -170,6 +170,7 @@ void ima_add_kexec_buffer(struct kimage *image)
 	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		 kbuf.mem);
 }
+
 #endif /* IMA_KEXEC */
 
 /*
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 3/7] ima: kexec: skip IMA segment validation after kexec soft reboot
  2024-01-22 18:37 ` Tushar Sugandhi
@ 2024-01-22 18:38   ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

kexec_calculate_store_digests() calculates and stores the digest of the
segment at kexec_file_load syscall where the IMA segment is also
allocated.  With this series, the IMA segment will be updated with the
measurement log at kexec soft reboot.  Therefore, it may fail digest
verification in verify_sha256_digest() after kexec soft reboot into the
new Kernel.

Skip IMA segment from calculating and storing digest in function
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 <tusharsu@linux.microsoft.com>
---
 include/linux/kexec.h              | 3 +++
 kernel/kexec_file.c                | 8 ++++++++
 security/integrity/ima/ima_kexec.c | 3 +++
 3 files changed, 14 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index e00b8101b53b..eb98aca7f4c7 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -366,6 +366,9 @@ struct kimage {
 
 	phys_addr_t ima_buffer_addr;
 	size_t ima_buffer_size;
+
+	unsigned long ima_segment_index;
+	bool is_ima_segment_index_set;
 #endif
 
 	/* Core ELF header buffer */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f989f5f1933b..bf758fd5062c 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -734,6 +734,14 @@ static int kexec_calculate_store_digests(struct kimage *image)
 		if (ksegment->kbuf == pi->purgatory_buf)
 			continue;
 
+		/*
+		 * Skip the segment if ima_segment_index is set and matches
+		 * the current index
+		 */
+		if (image->is_ima_segment_index_set &&
+		    i == image->ima_segment_index)
+			continue;
+
 		ret = crypto_shash_update(desc, ksegment->kbuf,
 					  ksegment->bufsz);
 		if (ret)
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 4f944c9b4168..d92a48284cc4 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -156,6 +156,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");
@@ -166,6 +167,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;
 
 	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		 kbuf.mem);
-- 
2.25.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 3/7] ima: kexec: skip IMA segment validation after kexec soft reboot
@ 2024-01-22 18:38   ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

kexec_calculate_store_digests() calculates and stores the digest of the
segment at kexec_file_load syscall where the IMA segment is also
allocated.  With this series, the IMA segment will be updated with the
measurement log at kexec soft reboot.  Therefore, it may fail digest
verification in verify_sha256_digest() after kexec soft reboot into the
new Kernel.

Skip IMA segment from calculating and storing digest in function
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 <tusharsu@linux.microsoft.com>
---
 include/linux/kexec.h              | 3 +++
 kernel/kexec_file.c                | 8 ++++++++
 security/integrity/ima/ima_kexec.c | 3 +++
 3 files changed, 14 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index e00b8101b53b..eb98aca7f4c7 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -366,6 +366,9 @@ struct kimage {
 
 	phys_addr_t ima_buffer_addr;
 	size_t ima_buffer_size;
+
+	unsigned long ima_segment_index;
+	bool is_ima_segment_index_set;
 #endif
 
 	/* Core ELF header buffer */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f989f5f1933b..bf758fd5062c 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -734,6 +734,14 @@ static int kexec_calculate_store_digests(struct kimage *image)
 		if (ksegment->kbuf == pi->purgatory_buf)
 			continue;
 
+		/*
+		 * Skip the segment if ima_segment_index is set and matches
+		 * the current index
+		 */
+		if (image->is_ima_segment_index_set &&
+		    i == image->ima_segment_index)
+			continue;
+
 		ret = crypto_shash_update(desc, ksegment->kbuf,
 					  ksegment->bufsz);
 		if (ret)
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 4f944c9b4168..d92a48284cc4 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -156,6 +156,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");
@@ -166,6 +167,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;
 
 	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
 		 kbuf.mem);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 4/7] ima: kexec: move ima log copy from kexec load to execute
  2024-01-22 18:37 ` Tushar Sugandhi
@ 2024-01-22 18:38   ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

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

The below changes need to be part of the same patch to ensure this
patch series remains bisect-safe by ensuring the IMA log gets copied over
during kexec soft reboot both before and after this patch.

Implement ima_update_kexec_buffer() to be called during kexec 'execute'.
Move ima_dump_measurement_list() from ima_add_kexec_buffer() to
ima_update_kexec_buffer().  Make the necessary variables local static to
the file, so that they are accessible during both kexec 'load' - where
the memory is allocated and mapped to a segment in the new Kernel, and
during 'execute' - where the IMA log gets copied over.

Implement kimage_file_post_load() and ima_kexec_post_load() to be invoked
after the new Kernel image has been loaded for kexec.
ima_kexec_post_load() will map the IMA buffer to a segment in the newly
loaded Kernel.  It will also register the reboot notifier_block to trigger
ima_update_kexec_buffer() at exec 'execute'.

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

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

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 86b57757c7b1..006db20f852d 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -49,6 +49,9 @@ static inline void ima_appraise_parse_cmdline(void) {}
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
+extern void ima_kexec_post_load(struct kimage *image);
+#else
+static inline void ima_kexec_post_load(struct kimage *image) {}
 #endif
 
 #else
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index bf758fd5062c..ee38799ff1a3 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -184,6 +184,11 @@ kimage_validate_signature(struct kimage *image)
 }
 #endif
 
+void kimage_file_post_load(struct kimage *image)
+{
+	ima_kexec_post_load(image);
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
@@ -399,6 +404,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 
 	kimage_terminate(image);
 
+	if (!(flags & KEXEC_FILE_ON_CRASH))
+		kimage_file_post_load(image);
+
 	ret = machine_kexec_post_load(image);
 	if (ret)
 		goto out;
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index d92a48284cc4..25150bfc7129 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -12,10 +12,15 @@
 #include <linux/kexec.h>
 #include <linux/of.h>
 #include <linux/ima.h>
+#include <linux/reboot.h>
+#include <asm/page.h>
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
 static struct seq_file ima_kexec_file;
+static void *ima_kexec_buffer;
+static size_t kexec_segment_size;
+static bool ima_kexec_update_registered;
 
 static void ima_free_kexec_file_buf(struct seq_file *sf)
 {
@@ -120,7 +125,6 @@ void ima_add_kexec_buffer(struct kimage *image)
 	/* use more understandable variable names than defined in kbuf */
 	void *kexec_buffer = NULL;
 	size_t kexec_buffer_size;
-	size_t kexec_segment_size;
 	int ret;
 
 	/*
@@ -145,14 +149,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;
@@ -174,6 +170,74 @@ void ima_add_kexec_buffer(struct kimage *image)
 		 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;
+	int ret = NOTIFY_OK;
+
+	if (!kexec_in_progress) {
+		pr_info("%s: No kexec in progress.\n", __func__);
+		return ret;
+	}
+
+	if (!ima_kexec_buffer) {
+		pr_err("%s: Kexec buffer not set.\n", __func__);
+		return ret;
+	}
+
+	ret = ima_dump_measurement_list(&buf_size, &buf,
+					kexec_segment_size);
+
+	if (!buf) {
+		pr_err("%s: Dump measurements failed. Error:%d\n",
+		       __func__, ret);
+		goto out;
+	}
+	memcpy(ima_kexec_buffer, buf, buf_size);
+out:
+	kimage_unmap_segment(ima_kexec_buffer);
+	ima_kexec_buffer = NULL;
+
+	return ret;
+}
+
+struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+};
+
+/*
+ * Create a mapping for the source pages that contain the IMA buffer
+ * so we can update it later.
+ */
+void ima_kexec_post_load(struct kimage *image)
+{
+	if (ima_kexec_buffer) {
+		kimage_unmap_segment(ima_kexec_buffer);
+		ima_kexec_buffer = NULL;
+	}
+
+	if (!image->ima_buffer_addr)
+		return;
+
+	ima_kexec_buffer = kimage_map_segment(image,
+					      image->ima_buffer_addr,
+					      image->ima_buffer_size);
+	if (!ima_kexec_buffer) {
+		pr_err("%s: Could not map measurements buffer.\n", __func__);
+		return;
+	}
+
+	if (!ima_kexec_update_registered) {
+		register_reboot_notifier(&update_buffer_nb);
+		ima_kexec_update_registered = true;
+	}
+}
+
 #endif /* IMA_KEXEC */
 
 /*
-- 
2.25.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 4/7] ima: kexec: move ima log copy from kexec load to execute
@ 2024-01-22 18:38   ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

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

The below changes need to be part of the same patch to ensure this
patch series remains bisect-safe by ensuring the IMA log gets copied over
during kexec soft reboot both before and after this patch.

Implement ima_update_kexec_buffer() to be called during kexec 'execute'.
Move ima_dump_measurement_list() from ima_add_kexec_buffer() to
ima_update_kexec_buffer().  Make the necessary variables local static to
the file, so that they are accessible during both kexec 'load' - where
the memory is allocated and mapped to a segment in the new Kernel, and
during 'execute' - where the IMA log gets copied over.

Implement kimage_file_post_load() and ima_kexec_post_load() to be invoked
after the new Kernel image has been loaded for kexec.
ima_kexec_post_load() will map the IMA buffer to a segment in the newly
loaded Kernel.  It will also register the reboot notifier_block to trigger
ima_update_kexec_buffer() at exec 'execute'.

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

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

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 86b57757c7b1..006db20f852d 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -49,6 +49,9 @@ static inline void ima_appraise_parse_cmdline(void) {}
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
+extern void ima_kexec_post_load(struct kimage *image);
+#else
+static inline void ima_kexec_post_load(struct kimage *image) {}
 #endif
 
 #else
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index bf758fd5062c..ee38799ff1a3 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -184,6 +184,11 @@ kimage_validate_signature(struct kimage *image)
 }
 #endif
 
+void kimage_file_post_load(struct kimage *image)
+{
+	ima_kexec_post_load(image);
+}
+
 /*
  * In file mode list of segments is prepared by kernel. Copy relevant
  * data from user space, do error checking, prepare segment list
@@ -399,6 +404,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 
 	kimage_terminate(image);
 
+	if (!(flags & KEXEC_FILE_ON_CRASH))
+		kimage_file_post_load(image);
+
 	ret = machine_kexec_post_load(image);
 	if (ret)
 		goto out;
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index d92a48284cc4..25150bfc7129 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -12,10 +12,15 @@
 #include <linux/kexec.h>
 #include <linux/of.h>
 #include <linux/ima.h>
+#include <linux/reboot.h>
+#include <asm/page.h>
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
 static struct seq_file ima_kexec_file;
+static void *ima_kexec_buffer;
+static size_t kexec_segment_size;
+static bool ima_kexec_update_registered;
 
 static void ima_free_kexec_file_buf(struct seq_file *sf)
 {
@@ -120,7 +125,6 @@ void ima_add_kexec_buffer(struct kimage *image)
 	/* use more understandable variable names than defined in kbuf */
 	void *kexec_buffer = NULL;
 	size_t kexec_buffer_size;
-	size_t kexec_segment_size;
 	int ret;
 
 	/*
@@ -145,14 +149,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;
@@ -174,6 +170,74 @@ void ima_add_kexec_buffer(struct kimage *image)
 		 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;
+	int ret = NOTIFY_OK;
+
+	if (!kexec_in_progress) {
+		pr_info("%s: No kexec in progress.\n", __func__);
+		return ret;
+	}
+
+	if (!ima_kexec_buffer) {
+		pr_err("%s: Kexec buffer not set.\n", __func__);
+		return ret;
+	}
+
+	ret = ima_dump_measurement_list(&buf_size, &buf,
+					kexec_segment_size);
+
+	if (!buf) {
+		pr_err("%s: Dump measurements failed. Error:%d\n",
+		       __func__, ret);
+		goto out;
+	}
+	memcpy(ima_kexec_buffer, buf, buf_size);
+out:
+	kimage_unmap_segment(ima_kexec_buffer);
+	ima_kexec_buffer = NULL;
+
+	return ret;
+}
+
+struct notifier_block update_buffer_nb = {
+	.notifier_call = ima_update_kexec_buffer,
+};
+
+/*
+ * Create a mapping for the source pages that contain the IMA buffer
+ * so we can update it later.
+ */
+void ima_kexec_post_load(struct kimage *image)
+{
+	if (ima_kexec_buffer) {
+		kimage_unmap_segment(ima_kexec_buffer);
+		ima_kexec_buffer = NULL;
+	}
+
+	if (!image->ima_buffer_addr)
+		return;
+
+	ima_kexec_buffer = kimage_map_segment(image,
+					      image->ima_buffer_addr,
+					      image->ima_buffer_size);
+	if (!ima_kexec_buffer) {
+		pr_err("%s: Could not map measurements buffer.\n", __func__);
+		return;
+	}
+
+	if (!ima_kexec_update_registered) {
+		register_reboot_notifier(&update_buffer_nb);
+		ima_kexec_update_registered = true;
+	}
+}
+
 #endif /* IMA_KEXEC */
 
 /*
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 5/7] ima: suspend measurements during buffer copy at kexec execute
  2024-01-22 18:37 ` Tushar Sugandhi
@ 2024-01-22 18:38   ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

New measurements added to the IMA log while the log is being copied
during the kexec 'execute' may not get copied over.  This can cause the
measurement log to be out of sync with the IMA TPM PCR, which could
result in breaking the integrity of the measurements after kexec soft
reboot.

Implement and call the functions ima_measurements_suspend() and 
ima_measurements_resume() from ima_update_kexec_buffer().

Add a check in the ima_add_template_entry() function not to measure
events when 'suspend_ima_measurements' flag is set.

This ensures the integrity of the IMA log while it is being copied over
to the new Kernel during kexec 'execute'.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima.h       |  2 ++
 security/integrity/ima/ima_kexec.c |  7 +++++++
 security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c29db699c996..49a6047dd8eb 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
 int ima_restore_measurement_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_list(loff_t bufsize, void *buf);
 int ima_measurements_show(struct seq_file *m, void *v);
+void ima_measurements_suspend(void);
+void ima_measurements_resume(void);
 unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(void);
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 25150bfc7129..f26b5b342d84 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -179,6 +179,7 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 	void *buf = NULL;
 	size_t buf_size;
 	int ret = NOTIFY_OK;
+	bool resume = false;
 
 	if (!kexec_in_progress) {
 		pr_info("%s: No kexec in progress.\n", __func__);
@@ -190,12 +191,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 		return ret;
 	}
 
+	ima_measurements_suspend();
+
 	ret = ima_dump_measurement_list(&buf_size, &buf,
 					kexec_segment_size);
 
 	if (!buf) {
 		pr_err("%s: Dump measurements failed. Error:%d\n",
 		       __func__, ret);
+		resume = true;
 		goto out;
 	}
 	memcpy(ima_kexec_buffer, buf, buf_size);
@@ -203,6 +207,9 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 	kimage_unmap_segment(ima_kexec_buffer);
 	ima_kexec_buffer = NULL;
 
+	if (resume)
+		ima_measurements_resume();
+
 	return ret;
 }
 
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 532da87ce519..5946a26a2849 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
  */
 static DEFINE_MUTEX(ima_extend_list_mutex);
 
+/*
+ * Used internally by the kernel to suspend-resume ima measurements.
+ */
+static atomic_t suspend_ima_measurements;
+
 /* lookup up the digest value in the hash table, and return the entry */
 static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 						       int pcr)
@@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 	return result;
 }
 
+void ima_measurements_suspend(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 1);
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
+void ima_measurements_resume(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 0);
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
 /*
  * Add template entry to the measurement list and hash table, and
  * extend the pcr.
@@ -176,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 		}
 	}
 
+	/*
+	 * suspend_ima_measurements will be set if the system is
+	 * undergoing kexec soft boot to a new kernel.
+	 * suspending measurements in this short window ensures the
+	 * consistency of the IMA measurement list during copying
+	 * of the kexec buffer.
+	 */
+	if (atomic_read(&suspend_ima_measurements)) {
+		audit_cause = "measurements_suspended";
+		audit_info = 0;
+		goto out;
+	}
+
 	result = ima_add_digest_entry(entry,
 				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
 	if (result < 0) {
-- 
2.25.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 5/7] ima: suspend measurements during buffer copy at kexec execute
@ 2024-01-22 18:38   ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

New measurements added to the IMA log while the log is being copied
during the kexec 'execute' may not get copied over.  This can cause the
measurement log to be out of sync with the IMA TPM PCR, which could
result in breaking the integrity of the measurements after kexec soft
reboot.

Implement and call the functions ima_measurements_suspend() and 
ima_measurements_resume() from ima_update_kexec_buffer().

Add a check in the ima_add_template_entry() function not to measure
events when 'suspend_ima_measurements' flag is set.

This ensures the integrity of the IMA log while it is being copied over
to the new Kernel during kexec 'execute'.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima.h       |  2 ++
 security/integrity/ima/ima_kexec.c |  7 +++++++
 security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c29db699c996..49a6047dd8eb 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
 int ima_restore_measurement_entry(struct ima_template_entry *entry);
 int ima_restore_measurement_list(loff_t bufsize, void *buf);
 int ima_measurements_show(struct seq_file *m, void *v);
+void ima_measurements_suspend(void);
+void ima_measurements_resume(void);
 unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(void);
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 25150bfc7129..f26b5b342d84 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -179,6 +179,7 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 	void *buf = NULL;
 	size_t buf_size;
 	int ret = NOTIFY_OK;
+	bool resume = false;
 
 	if (!kexec_in_progress) {
 		pr_info("%s: No kexec in progress.\n", __func__);
@@ -190,12 +191,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 		return ret;
 	}
 
+	ima_measurements_suspend();
+
 	ret = ima_dump_measurement_list(&buf_size, &buf,
 					kexec_segment_size);
 
 	if (!buf) {
 		pr_err("%s: Dump measurements failed. Error:%d\n",
 		       __func__, ret);
+		resume = true;
 		goto out;
 	}
 	memcpy(ima_kexec_buffer, buf, buf_size);
@@ -203,6 +207,9 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 	kimage_unmap_segment(ima_kexec_buffer);
 	ima_kexec_buffer = NULL;
 
+	if (resume)
+		ima_measurements_resume();
+
 	return ret;
 }
 
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 532da87ce519..5946a26a2849 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
  */
 static DEFINE_MUTEX(ima_extend_list_mutex);
 
+/*
+ * Used internally by the kernel to suspend-resume ima measurements.
+ */
+static atomic_t suspend_ima_measurements;
+
 /* lookup up the digest value in the hash table, and return the entry */
 static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 						       int pcr)
@@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 	return result;
 }
 
+void ima_measurements_suspend(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 1);
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
+void ima_measurements_resume(void)
+{
+	mutex_lock(&ima_extend_list_mutex);
+	atomic_set(&suspend_ima_measurements, 0);
+	mutex_unlock(&ima_extend_list_mutex);
+}
+
 /*
  * Add template entry to the measurement list and hash table, and
  * extend the pcr.
@@ -176,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 		}
 	}
 
+	/*
+	 * suspend_ima_measurements will be set if the system is
+	 * undergoing kexec soft boot to a new kernel.
+	 * suspending measurements in this short window ensures the
+	 * consistency of the IMA measurement list during copying
+	 * of the kexec buffer.
+	 */
+	if (atomic_read(&suspend_ima_measurements)) {
+		audit_cause = "measurements_suspended";
+		audit_info = 0;
+		goto out;
+	}
+
 	result = ima_add_digest_entry(entry,
 				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
 	if (result < 0) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 6/7] ima: make the kexec extra memory configurable
  2024-01-22 18:37 ` Tushar Sugandhi
@ 2024-01-22 18:38   ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

The extra memory allocated for carrying the IMA measurement list across
kexec is hardcoded 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
to maintain backwards compatibility.

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

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/Kconfig     | 11 +++++++++++
 security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 60a511c6b583..fc103288852b 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE
 	default n
 	help
 	   This option disables htable to allow measurement of duplicate records.
+
+config IMA_KEXEC_EXTRA_MEMORY_KB
+	int
+	depends on IMA && IMA_KEXEC
+	default 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, an extra half page of memory for
+	  additional measurements will be allocated to maintain backwards
+	  compatibility.
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index f26b5b342d84..c126aa6494e9 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -121,6 +121,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_size;
 
 	/* use more understandable variable names than defined in kbuf */
 	void *kexec_buffer = NULL;
@@ -128,15 +129,19 @@ 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_size = PAGE_SIZE / 2;
+	else
+		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
+	binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
+
 	if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE)
 		kexec_segment_size = ULONG_MAX;
 	else
-		kexec_segment_size = ALIGN(ima_get_binary_runtime_size() +
-					   PAGE_SIZE / 2, PAGE_SIZE);
+		kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE);
+
 	if ((kexec_segment_size == ULONG_MAX) ||
 	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
 		pr_err("Binary measurement list too large.\n");
-- 
2.25.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 6/7] ima: make the kexec extra memory configurable
@ 2024-01-22 18:38   ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

The extra memory allocated for carrying the IMA measurement list across
kexec is hardcoded 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
to maintain backwards compatibility.

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

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/Kconfig     | 11 +++++++++++
 security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 60a511c6b583..fc103288852b 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE
 	default n
 	help
 	   This option disables htable to allow measurement of duplicate records.
+
+config IMA_KEXEC_EXTRA_MEMORY_KB
+	int
+	depends on IMA && IMA_KEXEC
+	default 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, an extra half page of memory for
+	  additional measurements will be allocated to maintain backwards
+	  compatibility.
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index f26b5b342d84..c126aa6494e9 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -121,6 +121,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_size;
 
 	/* use more understandable variable names than defined in kbuf */
 	void *kexec_buffer = NULL;
@@ -128,15 +129,19 @@ 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_size = PAGE_SIZE / 2;
+	else
+		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
+	binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
+
 	if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE)
 		kexec_segment_size = ULONG_MAX;
 	else
-		kexec_segment_size = ALIGN(ima_get_binary_runtime_size() +
-					   PAGE_SIZE / 2, PAGE_SIZE);
+		kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE);
+
 	if ((kexec_segment_size == ULONG_MAX) ||
 	    ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) {
 		pr_err("Binary measurement list too large.\n");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 7/7] ima: measure kexec load and exec events as critical data
  2024-01-22 18:37 ` Tushar Sugandhi
@ 2024-01-22 18:38   ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

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

Define two new IMA events, 'kexec_load' and 'kexec_execute', to be 
measured as critical data at kexec 'load' and 'execute' respectively.
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.

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

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index c126aa6494e9..1f8b697f4ed0 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -17,6 +17,8 @@
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+#define IMA_KEXEC_EVENT_LEN 128
+
 static struct seq_file ima_kexec_file;
 static void *ima_kexec_buffer;
 static size_t kexec_segment_size;
@@ -33,6 +35,10 @@ static void ima_free_kexec_file_buf(struct seq_file *sf)
 
 static int ima_alloc_kexec_file_buf(size_t segment_size)
 {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
+	size_t buf_size;
+	long len;
+
 	/*
 	 * kexec 'load' may be called multiple times.
 	 * Free and realloc the buffer only if the segment_size is
@@ -42,7 +48,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
 	    ima_kexec_file.size == segment_size &&
 	    ima_kexec_file.read_pos == 0 &&
 	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
-		return 0;
+		goto out;
 
 	ima_free_kexec_file_buf(&ima_kexec_file);
 
@@ -55,6 +61,18 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
 	ima_kexec_file.read_pos = 0;
 	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
 
+out:
+	buf_size = ima_get_binary_runtime_size();
+	len = atomic_long_read(&ima_htable.len);
+
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;"
+		  "ima_runtime_measurements_count=%ld;",
+		  segment_size, buf_size, len);
+
+	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
+				  strlen(ima_kexec_event), false, NULL, 0);
+
 	return 0;
 }
 
@@ -181,10 +199,12 @@ 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;
 	int ret = NOTIFY_OK;
 	bool resume = false;
+	long len;
 
 	if (!kexec_in_progress) {
 		pr_info("%s: No kexec in progress.\n", __func__);
@@ -196,6 +216,18 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 		return ret;
 	}
 
+	buf_size = ima_get_binary_runtime_size();
+	len = atomic_long_read(&ima_htable.len);
+
+	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", "kexec_execute",
+				  ima_kexec_event, strlen(ima_kexec_event),
+				  false, NULL, 0);
+
 	ima_measurements_suspend();
 
 	ret = ima_dump_measurement_list(&buf_size, &buf,
-- 
2.25.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v4 7/7] ima: measure kexec load and exec events as critical data
@ 2024-01-22 18:38   ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-22 18:38 UTC (permalink / raw)
  To: zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

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

Define two new IMA events, 'kexec_load' and 'kexec_execute', to be 
measured as critical data at kexec 'load' and 'execute' respectively.
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.

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

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index c126aa6494e9..1f8b697f4ed0 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -17,6 +17,8 @@
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+#define IMA_KEXEC_EVENT_LEN 128
+
 static struct seq_file ima_kexec_file;
 static void *ima_kexec_buffer;
 static size_t kexec_segment_size;
@@ -33,6 +35,10 @@ static void ima_free_kexec_file_buf(struct seq_file *sf)
 
 static int ima_alloc_kexec_file_buf(size_t segment_size)
 {
+	char ima_kexec_event[IMA_KEXEC_EVENT_LEN];
+	size_t buf_size;
+	long len;
+
 	/*
 	 * kexec 'load' may be called multiple times.
 	 * Free and realloc the buffer only if the segment_size is
@@ -42,7 +48,7 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
 	    ima_kexec_file.size == segment_size &&
 	    ima_kexec_file.read_pos == 0 &&
 	    ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
-		return 0;
+		goto out;
 
 	ima_free_kexec_file_buf(&ima_kexec_file);
 
@@ -55,6 +61,18 @@ static int ima_alloc_kexec_file_buf(size_t segment_size)
 	ima_kexec_file.read_pos = 0;
 	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
 
+out:
+	buf_size = ima_get_binary_runtime_size();
+	len = atomic_long_read(&ima_htable.len);
+
+	scnprintf(ima_kexec_event, IMA_KEXEC_EVENT_LEN,
+		  "kexec_segment_size=%lu;ima_binary_runtime_size=%lu;"
+		  "ima_runtime_measurements_count=%ld;",
+		  segment_size, buf_size, len);
+
+	ima_measure_critical_data("ima_kexec", "kexec_load", ima_kexec_event,
+				  strlen(ima_kexec_event), false, NULL, 0);
+
 	return 0;
 }
 
@@ -181,10 +199,12 @@ 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;
 	int ret = NOTIFY_OK;
 	bool resume = false;
+	long len;
 
 	if (!kexec_in_progress) {
 		pr_info("%s: No kexec in progress.\n", __func__);
@@ -196,6 +216,18 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
 		return ret;
 	}
 
+	buf_size = ima_get_binary_runtime_size();
+	len = atomic_long_read(&ima_htable.len);
+
+	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", "kexec_execute",
+				  ima_kexec_event, strlen(ima_kexec_event),
+				  false, NULL, 0);
+
 	ima_measurements_suspend();
 
 	ret = ima_dump_measurement_list(&buf_size, &buf,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 2/7] kexec: define functions to map and unmap segments
  2024-01-22 18:37   ` Tushar Sugandhi
@ 2024-01-23 17:03     ` Stefan Berger
  -1 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-23 17:03 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/22/24 13:37, Tushar Sugandhi wrote:
> Implement kimage_map_segment() to enable mapping of IMA buffer source
> pages to the kimage structure post kexec 'load'.  This function,
> accepting a kimage pointer, an address, and a size, will gather the
> source pages within the specified address range, create an array of page
> pointers, and map these to a contiguous virtual address range.  The
> function returns the start of this range if successful, or NULL if
> unsuccessful.
> 
> Implement kimage_unmap_segment() for unmapping segments
> using vunmap().  Relocate 'for_each_kimage_entry()' macro from
> kexec_core.c to kexec.h for broader accessibility.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>   include/linux/kexec.h              | 13 +++++++
>   kernel/kexec_core.c                | 59 +++++++++++++++++++++++++++---
>   security/integrity/ima/ima_kexec.c |  1 +
>   3 files changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 22b5cd24f581..e00b8101b53b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -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);
> +

This series applies to v6.5. You may want to rebase to 6.7.

>   #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 */
>   
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 3d578c6fefee..26978ad02676 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -594,11 +594,6 @@ void kimage_terminate(struct kimage *image)
>   	*image->entry = IND_DONE;
>   }
>   
> -#define for_each_kimage_entry(image, ptr, entry) \
> -	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
> -		ptr = (entry & IND_INDIRECTION) ? \
> -			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
> -
>   static void kimage_free_entry(kimage_entry_t entry)
>   {
>   	struct page *page;
> @@ -921,6 +916,60 @@ int kimage_load_segment(struct kimage *image,
>   	return result;
>   }
>   
> +void *kimage_map_segment(struct kimage *image,
> +			 unsigned long addr, unsigned long size)
> +{
> +	unsigned long eaddr = addr + size;
> +	unsigned long src_page_addr, dest_page_addr;
> +	unsigned int npages;
> +	struct page **src_pages;
> +	int i;
> +	kimage_entry_t *ptr, entry;
> +	void *vaddr = NULL;
> +
> +	/*
> +	 * Collect the source pages and map them in a contiguous VA range.
> +	 */
> +	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
> +	src_pages = kmalloc_array(npages, sizeof(*src_pages), GFP_KERNEL);
> +	if (!src_pages) {
> +		pr_err("%s: Could not allocate ima pages array.\n", __func__);
> +		return NULL;
> +	}
> +
> +	i = 0;
> +	for_each_kimage_entry(image, ptr, entry) {
> +		if (entry & IND_DESTINATION)
> +			dest_page_addr = entry & PAGE_MASK;
> +		else if (entry & IND_SOURCE) {
> +			if (dest_page_addr >= addr && dest_page_addr < eaddr) {
> +				src_page_addr = entry & PAGE_MASK;
> +				src_pages[i++] =
> +					virt_to_page(__va(src_page_addr));
> +				if (i == npages)
> +					break;
> +				dest_page_addr += PAGE_SIZE;
> +			}
> +		}
> +	}
> +
> +	/* Sanity check. */
> +	WARN_ON(i < npages);
> +
> +	vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
> +	kfree(src_pages);
> +
> +	if (!vaddr)
> +		pr_err("%s: Could not map imap buffer.\n", __func__);

imap -> ima

> +
> +	return vaddr;
> +}
> +
> +void kimage_unmap_segment(void *segment_buffer)
> +{
> +	vunmap(segment_buffer);
> +}
> +
>   struct kexec_load_limit {
>   	/* Mutex protects the limit count. */
>   	struct mutex mutex;
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 99daac355c70..4f944c9b4168 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -170,6 +170,7 @@ void ima_add_kexec_buffer(struct kimage *image)
>   	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
>   		 kbuf.mem);
>   }
> +

remove

>   #endif /* IMA_KEXEC */
>   
>   /*

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 2/7] kexec: define functions to map and unmap segments
@ 2024-01-23 17:03     ` Stefan Berger
  0 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-23 17:03 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/22/24 13:37, Tushar Sugandhi wrote:
> Implement kimage_map_segment() to enable mapping of IMA buffer source
> pages to the kimage structure post kexec 'load'.  This function,
> accepting a kimage pointer, an address, and a size, will gather the
> source pages within the specified address range, create an array of page
> pointers, and map these to a contiguous virtual address range.  The
> function returns the start of this range if successful, or NULL if
> unsuccessful.
> 
> Implement kimage_unmap_segment() for unmapping segments
> using vunmap().  Relocate 'for_each_kimage_entry()' macro from
> kexec_core.c to kexec.h for broader accessibility.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>   include/linux/kexec.h              | 13 +++++++
>   kernel/kexec_core.c                | 59 +++++++++++++++++++++++++++---
>   security/integrity/ima/ima_kexec.c |  1 +
>   3 files changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 22b5cd24f581..e00b8101b53b 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -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);
> +

This series applies to v6.5. You may want to rebase to 6.7.

>   #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 */
>   
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 3d578c6fefee..26978ad02676 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -594,11 +594,6 @@ void kimage_terminate(struct kimage *image)
>   	*image->entry = IND_DONE;
>   }
>   
> -#define for_each_kimage_entry(image, ptr, entry) \
> -	for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
> -		ptr = (entry & IND_INDIRECTION) ? \
> -			boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
> -
>   static void kimage_free_entry(kimage_entry_t entry)
>   {
>   	struct page *page;
> @@ -921,6 +916,60 @@ int kimage_load_segment(struct kimage *image,
>   	return result;
>   }
>   
> +void *kimage_map_segment(struct kimage *image,
> +			 unsigned long addr, unsigned long size)
> +{
> +	unsigned long eaddr = addr + size;
> +	unsigned long src_page_addr, dest_page_addr;
> +	unsigned int npages;
> +	struct page **src_pages;
> +	int i;
> +	kimage_entry_t *ptr, entry;
> +	void *vaddr = NULL;
> +
> +	/*
> +	 * Collect the source pages and map them in a contiguous VA range.
> +	 */
> +	npages = PFN_UP(eaddr) - PFN_DOWN(addr);
> +	src_pages = kmalloc_array(npages, sizeof(*src_pages), GFP_KERNEL);
> +	if (!src_pages) {
> +		pr_err("%s: Could not allocate ima pages array.\n", __func__);
> +		return NULL;
> +	}
> +
> +	i = 0;
> +	for_each_kimage_entry(image, ptr, entry) {
> +		if (entry & IND_DESTINATION)
> +			dest_page_addr = entry & PAGE_MASK;
> +		else if (entry & IND_SOURCE) {
> +			if (dest_page_addr >= addr && dest_page_addr < eaddr) {
> +				src_page_addr = entry & PAGE_MASK;
> +				src_pages[i++] =
> +					virt_to_page(__va(src_page_addr));
> +				if (i == npages)
> +					break;
> +				dest_page_addr += PAGE_SIZE;
> +			}
> +		}
> +	}
> +
> +	/* Sanity check. */
> +	WARN_ON(i < npages);
> +
> +	vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
> +	kfree(src_pages);
> +
> +	if (!vaddr)
> +		pr_err("%s: Could not map imap buffer.\n", __func__);

imap -> ima

> +
> +	return vaddr;
> +}
> +
> +void kimage_unmap_segment(void *segment_buffer)
> +{
> +	vunmap(segment_buffer);
> +}
> +
>   struct kexec_load_limit {
>   	/* Mutex protects the limit count. */
>   	struct mutex mutex;
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 99daac355c70..4f944c9b4168 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -170,6 +170,7 @@ void ima_add_kexec_buffer(struct kimage *image)
>   	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
>   		 kbuf.mem);
>   }
> +

remove

>   #endif /* IMA_KEXEC */
>   
>   /*

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 5/7] ima: suspend measurements during buffer copy at kexec execute
  2024-01-22 18:38   ` Tushar Sugandhi
@ 2024-01-23 18:18     ` Stefan Berger
  -1 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-23 18:18 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/22/24 13:38, Tushar Sugandhi wrote:
> New measurements added to the IMA log while the log is being copied
> during the kexec 'execute' may not get copied over.  This can cause the
> measurement log to be out of sync with the IMA TPM PCR, which could

It could be out of sync with any PCR that IMA extends, most often only 
the IMA TPM PCR.

> result in breaking the integrity of the measurements after kexec soft
> reboot.
> 
> Implement and call the functions ima_measurements_suspend() and
> ima_measurements_resume() from ima_update_kexec_buffer().
> 
> Add a check in the ima_add_template_entry() function not to measure
> events when 'suspend_ima_measurements' flag is set.
> 
> This ensures the integrity of the IMA log while it is being copied over
> to the new Kernel during kexec 'execute'.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   security/integrity/ima/ima.h       |  2 ++
>   security/integrity/ima/ima_kexec.c |  7 +++++++
>   security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
>   3 files changed, 41 insertions(+)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c29db699c996..49a6047dd8eb 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
>   int ima_restore_measurement_entry(struct ima_template_entry *entry);
>   int ima_restore_measurement_list(loff_t bufsize, void *buf);
>   int ima_measurements_show(struct seq_file *m, void *v);
> +void ima_measurements_suspend(void);
> +void ima_measurements_resume(void);
>   unsigned long ima_get_binary_runtime_size(void);
>   int ima_init_template(void);
>   void ima_init_template_list(void);
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 25150bfc7129..f26b5b342d84 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -179,6 +179,7 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>   	void *buf = NULL;
>   	size_t buf_size;
>   	int ret = NOTIFY_OK;
> +	bool resume = false;
>   
>   	if (!kexec_in_progress) {
>   		pr_info("%s: No kexec in progress.\n", __func__);
> @@ -190,12 +191,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>   		return ret;
>   	}
>   
> +	ima_measurements_suspend();
> +
>   	ret = ima_dump_measurement_list(&buf_size, &buf,
>   					kexec_segment_size);
>   
>   	if (!buf) {
>   		pr_err("%s: Dump measurements failed. Error:%d\n",
>   		       __func__, ret);
> +		resume = true;
>   		goto out;
>   	}
>   	memcpy(ima_kexec_buffer, buf, buf_size);
> @@ -203,6 +207,9 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>   	kimage_unmap_segment(ima_kexec_buffer);
>   	ima_kexec_buffer = NULL;
>   
> +	if (resume)
> +		ima_measurements_resume();
> +
>   	return ret;
>   }
>   
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index 532da87ce519..5946a26a2849 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
>    */
>   static DEFINE_MUTEX(ima_extend_list_mutex);
>   
> +/*
> + * Used internally by the kernel to suspend-resume ima measurements.
> + */
> +static atomic_t suspend_ima_measurements;
> +
>   /* lookup up the digest value in the hash table, and return the entry */
>   static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
>   						       int pcr)
> @@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
>   	return result;
>   }
>   
> +void ima_measurements_suspend(void)
> +{
> +	mutex_lock(&ima_extend_list_mutex);
> +	atomic_set(&suspend_ima_measurements, 1);
> +	mutex_unlock(&ima_extend_list_mutex);
> +}
> +
> +void ima_measurements_resume(void)
> +{
> +	mutex_lock(&ima_extend_list_mutex);
> +	atomic_set(&suspend_ima_measurements, 0);
> +	mutex_unlock(&ima_extend_list_mutex);
> +}
> +
>   /*
>    * Add template entry to the measurement list and hash table, and
>    * extend the pcr.
> @@ -176,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>   		}
>   	}
>   
> +	/*
> +	 * suspend_ima_measurements will be set if the system is
> +	 * undergoing kexec soft boot to a new kernel.
> +	 * suspending measurements in this short window ensures the
> +	 * consistency of the IMA measurement list during copying
> +	 * of the kexec buffer.
> +	 */
> +	if (atomic_read(&suspend_ima_measurements)) {
> +		audit_cause = "measurements_suspended";
> +		audit_info = 0;
> +		goto out;
> +	}
> +
>   	result = ima_add_digest_entry(entry,
>   				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
>   	if (result < 0) {

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 5/7] ima: suspend measurements during buffer copy at kexec execute
@ 2024-01-23 18:18     ` Stefan Berger
  0 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-23 18:18 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/22/24 13:38, Tushar Sugandhi wrote:
> New measurements added to the IMA log while the log is being copied
> during the kexec 'execute' may not get copied over.  This can cause the
> measurement log to be out of sync with the IMA TPM PCR, which could

It could be out of sync with any PCR that IMA extends, most often only 
the IMA TPM PCR.

> result in breaking the integrity of the measurements after kexec soft
> reboot.
> 
> Implement and call the functions ima_measurements_suspend() and
> ima_measurements_resume() from ima_update_kexec_buffer().
> 
> Add a check in the ima_add_template_entry() function not to measure
> events when 'suspend_ima_measurements' flag is set.
> 
> This ensures the integrity of the IMA log while it is being copied over
> to the new Kernel during kexec 'execute'.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   security/integrity/ima/ima.h       |  2 ++
>   security/integrity/ima/ima_kexec.c |  7 +++++++
>   security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
>   3 files changed, 41 insertions(+)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index c29db699c996..49a6047dd8eb 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
>   int ima_restore_measurement_entry(struct ima_template_entry *entry);
>   int ima_restore_measurement_list(loff_t bufsize, void *buf);
>   int ima_measurements_show(struct seq_file *m, void *v);
> +void ima_measurements_suspend(void);
> +void ima_measurements_resume(void);
>   unsigned long ima_get_binary_runtime_size(void);
>   int ima_init_template(void);
>   void ima_init_template_list(void);
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 25150bfc7129..f26b5b342d84 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -179,6 +179,7 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>   	void *buf = NULL;
>   	size_t buf_size;
>   	int ret = NOTIFY_OK;
> +	bool resume = false;
>   
>   	if (!kexec_in_progress) {
>   		pr_info("%s: No kexec in progress.\n", __func__);
> @@ -190,12 +191,15 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>   		return ret;
>   	}
>   
> +	ima_measurements_suspend();
> +
>   	ret = ima_dump_measurement_list(&buf_size, &buf,
>   					kexec_segment_size);
>   
>   	if (!buf) {
>   		pr_err("%s: Dump measurements failed. Error:%d\n",
>   		       __func__, ret);
> +		resume = true;
>   		goto out;
>   	}
>   	memcpy(ima_kexec_buffer, buf, buf_size);
> @@ -203,6 +207,9 @@ static int ima_update_kexec_buffer(struct notifier_block *self,
>   	kimage_unmap_segment(ima_kexec_buffer);
>   	ima_kexec_buffer = NULL;
>   
> +	if (resume)
> +		ima_measurements_resume();
> +
>   	return ret;
>   }
>   
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index 532da87ce519..5946a26a2849 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
>    */
>   static DEFINE_MUTEX(ima_extend_list_mutex);
>   
> +/*
> + * Used internally by the kernel to suspend-resume ima measurements.
> + */
> +static atomic_t suspend_ima_measurements;
> +
>   /* lookup up the digest value in the hash table, and return the entry */
>   static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
>   						       int pcr)
> @@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
>   	return result;
>   }
>   
> +void ima_measurements_suspend(void)
> +{
> +	mutex_lock(&ima_extend_list_mutex);
> +	atomic_set(&suspend_ima_measurements, 1);
> +	mutex_unlock(&ima_extend_list_mutex);
> +}
> +
> +void ima_measurements_resume(void)
> +{
> +	mutex_lock(&ima_extend_list_mutex);
> +	atomic_set(&suspend_ima_measurements, 0);
> +	mutex_unlock(&ima_extend_list_mutex);
> +}
> +
>   /*
>    * Add template entry to the measurement list and hash table, and
>    * extend the pcr.
> @@ -176,6 +195,19 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>   		}
>   	}
>   
> +	/*
> +	 * suspend_ima_measurements will be set if the system is
> +	 * undergoing kexec soft boot to a new kernel.
> +	 * suspending measurements in this short window ensures the
> +	 * consistency of the IMA measurement list during copying
> +	 * of the kexec buffer.
> +	 */
> +	if (atomic_read(&suspend_ima_measurements)) {
> +		audit_cause = "measurements_suspended";
> +		audit_info = 0;
> +		goto out;
> +	}
> +
>   	result = ima_add_digest_entry(entry,
>   				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
>   	if (result < 0) {

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
  2024-01-22 18:38   ` Tushar Sugandhi
@ 2024-01-23 19:02     ` Stefan Berger
  -1 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-23 19:02 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/22/24 13:38, Tushar Sugandhi wrote:
> The extra memory allocated for carrying the IMA measurement list across
> kexec is hardcoded 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
> to maintain backwards compatibility.
> 
> Update ima_add_kexec_buffer() function to allocate memory based on the
> Kconfig option value, rather than the currently hardcoded one.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>   security/integrity/ima/Kconfig     | 11 +++++++++++
>   security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
>   2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 60a511c6b583..fc103288852b 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE
>   	default n
>   	help
>   	   This option disables htable to allow measurement of duplicate records.
> +
> +config IMA_KEXEC_EXTRA_MEMORY_KB
> +	int
> +	depends on IMA && IMA_KEXEC
> +	default 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, an extra half page of memory for
> +	  additional measurements will be allocated to maintain backwards
> +	  compatibility.

Is there really an issue with 'backwards compatibility' that the user 
needs to know about ? From looking at the code it seems more important 
that a bit of additional memory is reserved now to fit the kexec 'load' 
and 'exec' critical data events but that's not 'backwards compatibility'.

Also mention this as a guidance on either how kexec load+exec need to be 
used or as a hint that it may potentially require a lot of memory? :

The amount of extra memory that is necessary to carry all measurements 
across a kexec depends on memory requirements of the measurements taken 
between the kexec 'load' and 'exec' stages. The more measurements are 
taken, the more extra memory is required. Large amounts of extra memory 
can be avoided by running kexec 'load' and 'exec' in short sequence.

> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index f26b5b342d84..c126aa6494e9 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -121,6 +121,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_size;
>   
>   	/* use more understandable variable names than defined in kbuf */
>   	void *kexec_buffer = NULL;
> @@ -128,15 +129,19 @@ 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_size = PAGE_SIZE / 2;
> +	else
> +		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
> +	binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
> +
>   	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");

Code LGTM

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
@ 2024-01-23 19:02     ` Stefan Berger
  0 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-23 19:02 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/22/24 13:38, Tushar Sugandhi wrote:
> The extra memory allocated for carrying the IMA measurement list across
> kexec is hardcoded 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
> to maintain backwards compatibility.
> 
> Update ima_add_kexec_buffer() function to allocate memory based on the
> Kconfig option value, rather than the currently hardcoded one.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>   security/integrity/ima/Kconfig     | 11 +++++++++++
>   security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
>   2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 60a511c6b583..fc103288852b 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE
>   	default n
>   	help
>   	   This option disables htable to allow measurement of duplicate records.
> +
> +config IMA_KEXEC_EXTRA_MEMORY_KB
> +	int
> +	depends on IMA && IMA_KEXEC
> +	default 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, an extra half page of memory for
> +	  additional measurements will be allocated to maintain backwards
> +	  compatibility.

Is there really an issue with 'backwards compatibility' that the user 
needs to know about ? From looking at the code it seems more important 
that a bit of additional memory is reserved now to fit the kexec 'load' 
and 'exec' critical data events but that's not 'backwards compatibility'.

Also mention this as a guidance on either how kexec load+exec need to be 
used or as a hint that it may potentially require a lot of memory? :

The amount of extra memory that is necessary to carry all measurements 
across a kexec depends on memory requirements of the measurements taken 
between the kexec 'load' and 'exec' stages. The more measurements are 
taken, the more extra memory is required. Large amounts of extra memory 
can be avoided by running kexec 'load' and 'exec' in short sequence.

> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index f26b5b342d84..c126aa6494e9 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -121,6 +121,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_size;
>   
>   	/* use more understandable variable names than defined in kbuf */
>   	void *kexec_buffer = NULL;
> @@ -128,15 +129,19 @@ 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_size = PAGE_SIZE / 2;
> +	else
> +		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
> +	binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
> +
>   	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");

Code LGTM

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 2/7] kexec: define functions to map and unmap segments
  2024-01-23 17:03     ` Stefan Berger
@ 2024-01-23 20:39       ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-23 20:39 UTC (permalink / raw)
  To: Stefan Berger, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul

Thanks Stefan for taking a look.

On 1/23/24 09:03, Stefan Berger wrote:
> 
> 
> On 1/22/24 13:37, Tushar Sugandhi wrote:
>> Implement kimage_map_segment() to enable mapping of IMA buffer source
>> pages to the kimage structure post kexec 'load'.  This function,
>> accepting a kimage pointer, an address, and a size, will gather the
>> source pages within the specified address range, create an array of page
>> pointers, and map these to a contiguous virtual address range.  The
>> function returns the start of this range if successful, or NULL if
>> unsuccessful.
>>
>> Implement kimage_unmap_segment() for unmapping segments
>> using vunmap().  Relocate 'for_each_kimage_entry()' macro from
>> kexec_core.c to kexec.h for broader accessibility.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   include/linux/kexec.h              | 13 +++++++
>>   kernel/kexec_core.c                | 59 +++++++++++++++++++++++++++---
>>   security/integrity/ima/ima_kexec.c |  1 +
>>   3 files changed, 68 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 22b5cd24f581..e00b8101b53b 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -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);
>> +
> 
> This series applies to v6.5. You may want to rebase to 6.7.
> 
Will rebase. Thanks for catching this.
>>   #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 */
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index 3d578c6fefee..26978ad02676 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -594,11 +594,6 @@ void kimage_terminate(struct kimage *image)
>>       *image->entry = IND_DONE;
>>   }
>> -#define for_each_kimage_entry(image, ptr, entry) \
>> -    for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
>> -        ptr = (entry & IND_INDIRECTION) ? \
>> -            boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
>> -
>>   static void kimage_free_entry(kimage_entry_t entry)
>>   {
>>       struct page *page;
>> @@ -921,6 +916,60 @@ int kimage_load_segment(struct kimage *image,
>>       return result;
>>   }
>> +void *kimage_map_segment(struct kimage *image,
>> +             unsigned long addr, unsigned long size)
>> +{
>> +    unsigned long eaddr = addr + size;
>> +    unsigned long src_page_addr, dest_page_addr;
>> +    unsigned int npages;
>> +    struct page **src_pages;
>> +    int i;
>> +    kimage_entry_t *ptr, entry;
>> +    void *vaddr = NULL;
>> +
>> +    /*
>> +     * Collect the source pages and map them in a contiguous VA range.
>> +     */
>> +    npages = PFN_UP(eaddr) - PFN_DOWN(addr);
>> +    src_pages = kmalloc_array(npages, sizeof(*src_pages), GFP_KERNEL);
>> +    if (!src_pages) {
>> +        pr_err("%s: Could not allocate ima pages array.\n", __func__);
>> +        return NULL;
>> +    }
>> +
>> +    i = 0;
>> +    for_each_kimage_entry(image, ptr, entry) {
>> +        if (entry & IND_DESTINATION)
>> +            dest_page_addr = entry & PAGE_MASK;
>> +        else if (entry & IND_SOURCE) {
>> +            if (dest_page_addr >= addr && dest_page_addr < eaddr) {
>> +                src_page_addr = entry & PAGE_MASK;
>> +                src_pages[i++] =
>> +                    virt_to_page(__va(src_page_addr));
>> +                if (i == npages)
>> +                    break;
>> +                dest_page_addr += PAGE_SIZE;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* Sanity check. */
>> +    WARN_ON(i < npages);
>> +
>> +    vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
>> +    kfree(src_pages);
>> +
>> +    if (!vaddr)
>> +        pr_err("%s: Could not map imap buffer.\n", __func__);
> 
> imap -> ima
> 
Good eye catching this.
Will fix.
>> +
>> +    return vaddr;
>> +}
>> +
>> +void kimage_unmap_segment(void *segment_buffer)
>> +{
>> +    vunmap(segment_buffer);
>> +}
>> +
>>   struct kexec_load_limit {
>>       /* Mutex protects the limit count. */
>>       struct mutex mutex;
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 99daac355c70..4f944c9b4168 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -170,6 +170,7 @@ void ima_add_kexec_buffer(struct kimage *image)
>>       pr_debug("kexec measurement buffer for the loaded kernel at 
>> 0x%lx.\n",
>>            kbuf.mem);
>>   }
>> +
> 
> remove
>
Will do.

~Tushar

>>   #endif /* IMA_KEXEC */
>>   /*

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 2/7] kexec: define functions to map and unmap segments
@ 2024-01-23 20:39       ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-23 20:39 UTC (permalink / raw)
  To: Stefan Berger, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul

Thanks Stefan for taking a look.

On 1/23/24 09:03, Stefan Berger wrote:
> 
> 
> On 1/22/24 13:37, Tushar Sugandhi wrote:
>> Implement kimage_map_segment() to enable mapping of IMA buffer source
>> pages to the kimage structure post kexec 'load'.  This function,
>> accepting a kimage pointer, an address, and a size, will gather the
>> source pages within the specified address range, create an array of page
>> pointers, and map these to a contiguous virtual address range.  The
>> function returns the start of this range if successful, or NULL if
>> unsuccessful.
>>
>> Implement kimage_unmap_segment() for unmapping segments
>> using vunmap().  Relocate 'for_each_kimage_entry()' macro from
>> kexec_core.c to kexec.h for broader accessibility.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   include/linux/kexec.h              | 13 +++++++
>>   kernel/kexec_core.c                | 59 +++++++++++++++++++++++++++---
>>   security/integrity/ima/ima_kexec.c |  1 +
>>   3 files changed, 68 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 22b5cd24f581..e00b8101b53b 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -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);
>> +
> 
> This series applies to v6.5. You may want to rebase to 6.7.
> 
Will rebase. Thanks for catching this.
>>   #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 */
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index 3d578c6fefee..26978ad02676 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -594,11 +594,6 @@ void kimage_terminate(struct kimage *image)
>>       *image->entry = IND_DONE;
>>   }
>> -#define for_each_kimage_entry(image, ptr, entry) \
>> -    for (ptr = &image->head; (entry = *ptr) && !(entry & IND_DONE); \
>> -        ptr = (entry & IND_INDIRECTION) ? \
>> -            boot_phys_to_virt((entry & PAGE_MASK)) : ptr + 1)
>> -
>>   static void kimage_free_entry(kimage_entry_t entry)
>>   {
>>       struct page *page;
>> @@ -921,6 +916,60 @@ int kimage_load_segment(struct kimage *image,
>>       return result;
>>   }
>> +void *kimage_map_segment(struct kimage *image,
>> +             unsigned long addr, unsigned long size)
>> +{
>> +    unsigned long eaddr = addr + size;
>> +    unsigned long src_page_addr, dest_page_addr;
>> +    unsigned int npages;
>> +    struct page **src_pages;
>> +    int i;
>> +    kimage_entry_t *ptr, entry;
>> +    void *vaddr = NULL;
>> +
>> +    /*
>> +     * Collect the source pages and map them in a contiguous VA range.
>> +     */
>> +    npages = PFN_UP(eaddr) - PFN_DOWN(addr);
>> +    src_pages = kmalloc_array(npages, sizeof(*src_pages), GFP_KERNEL);
>> +    if (!src_pages) {
>> +        pr_err("%s: Could not allocate ima pages array.\n", __func__);
>> +        return NULL;
>> +    }
>> +
>> +    i = 0;
>> +    for_each_kimage_entry(image, ptr, entry) {
>> +        if (entry & IND_DESTINATION)
>> +            dest_page_addr = entry & PAGE_MASK;
>> +        else if (entry & IND_SOURCE) {
>> +            if (dest_page_addr >= addr && dest_page_addr < eaddr) {
>> +                src_page_addr = entry & PAGE_MASK;
>> +                src_pages[i++] =
>> +                    virt_to_page(__va(src_page_addr));
>> +                if (i == npages)
>> +                    break;
>> +                dest_page_addr += PAGE_SIZE;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* Sanity check. */
>> +    WARN_ON(i < npages);
>> +
>> +    vaddr = vmap(src_pages, npages, VM_MAP, PAGE_KERNEL);
>> +    kfree(src_pages);
>> +
>> +    if (!vaddr)
>> +        pr_err("%s: Could not map imap buffer.\n", __func__);
> 
> imap -> ima
> 
Good eye catching this.
Will fix.
>> +
>> +    return vaddr;
>> +}
>> +
>> +void kimage_unmap_segment(void *segment_buffer)
>> +{
>> +    vunmap(segment_buffer);
>> +}
>> +
>>   struct kexec_load_limit {
>>       /* Mutex protects the limit count. */
>>       struct mutex mutex;
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 99daac355c70..4f944c9b4168 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -170,6 +170,7 @@ void ima_add_kexec_buffer(struct kimage *image)
>>       pr_debug("kexec measurement buffer for the loaded kernel at 
>> 0x%lx.\n",
>>            kbuf.mem);
>>   }
>> +
> 
> remove
>
Will do.

~Tushar

>>   #endif /* IMA_KEXEC */
>>   /*

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 5/7] ima: suspend measurements during buffer copy at kexec execute
  2024-01-23 18:18     ` Stefan Berger
@ 2024-01-23 20:55       ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-23 20:55 UTC (permalink / raw)
  To: Stefan Berger, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/23/24 10:18, Stefan Berger wrote:
> 
> 
> On 1/22/24 13:38, Tushar Sugandhi wrote:
>> New measurements added to the IMA log while the log is being copied
>> during the kexec 'execute' may not get copied over.  This can cause the
>> measurement log to be out of sync with the IMA TPM PCR, which could
> 
> It could be out of sync with any PCR that IMA extends, most often only 
> the IMA TPM PCR.
> 
Agreed.  That's what I meant. But I can see my above wording could be 
misleading.
I will reword it to "This can cause the measurement log to be out of 
sync with the TPM PCRs that IMA extends, which could result in..."

>> result in breaking the integrity of the measurements after kexec soft
>> reboot.
>>
>> Implement and call the functions ima_measurements_suspend() and
>> ima_measurements_resume() from ima_update_kexec_buffer().
>>
>> Add a check in the ima_add_template_entry() function not to measure
>> events when 'suspend_ima_measurements' flag is set.
>>
>> This ensures the integrity of the IMA log while it is being copied over
>> to the new Kernel during kexec 'execute'.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 
Thanks for the tag. I will add it to this patch in the next iteration of 
the series.
Please let me know if you want it to be added to any other patches.

~Tushar
>> ---
>>   security/integrity/ima/ima.h       |  2 ++
>>   security/integrity/ima/ima_kexec.c |  7 +++++++
>>   security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
>>   3 files changed, 41 insertions(+)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index c29db699c996..49a6047dd8eb 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct 
>> ima_template_desc *ima_template);
>>   int ima_restore_measurement_entry(struct ima_template_entry *entry);
>>   int ima_restore_measurement_list(loff_t bufsize, void *buf);
>>   int ima_measurements_show(struct seq_file *m, void *v);
>> +void ima_measurements_suspend(void);
>> +void ima_measurements_resume(void);
>>   unsigned long ima_get_binary_runtime_size(void);
>>   int ima_init_template(void);
>>   void ima_init_template_list(void);
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 25150bfc7129..f26b5b342d84 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -179,6 +179,7 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>       void *buf = NULL;
>>       size_t buf_size;
>>       int ret = NOTIFY_OK;
>> +    bool resume = false;
>>       if (!kexec_in_progress) {
>>           pr_info("%s: No kexec in progress.\n", __func__);
>> @@ -190,12 +191,15 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>           return ret;
>>       }
>> +    ima_measurements_suspend();
>> +
>>       ret = ima_dump_measurement_list(&buf_size, &buf,
>>                       kexec_segment_size);
>>       if (!buf) {
>>           pr_err("%s: Dump measurements failed. Error:%d\n",
>>                  __func__, ret);
>> +        resume = true;
>>           goto out;
>>       }
>>       memcpy(ima_kexec_buffer, buf, buf_size);
>> @@ -203,6 +207,9 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>       kimage_unmap_segment(ima_kexec_buffer);
>>       ima_kexec_buffer = NULL;
>> +    if (resume)
>> +        ima_measurements_resume();
>> +
>>       return ret;
>>   }
>> diff --git a/security/integrity/ima/ima_queue.c 
>> b/security/integrity/ima/ima_queue.c
>> index 532da87ce519..5946a26a2849 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
>>    */
>>   static DEFINE_MUTEX(ima_extend_list_mutex);
>> +/*
>> + * Used internally by the kernel to suspend-resume ima measurements.
>> + */
>> +static atomic_t suspend_ima_measurements;
>> +
>>   /* lookup up the digest value in the hash table, and return the 
>> entry */
>>   static struct ima_queue_entry *ima_lookup_digest_entry(u8 
>> *digest_value,
>>                                  int pcr)
>> @@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest 
>> *digests_arg, int pcr)
>>       return result;
>>   }
>> +void ima_measurements_suspend(void)
>> +{
>> +    mutex_lock(&ima_extend_list_mutex);
>> +    atomic_set(&suspend_ima_measurements, 1);
>> +    mutex_unlock(&ima_extend_list_mutex);
>> +}
>> +
>> +void ima_measurements_resume(void)
>> +{
>> +    mutex_lock(&ima_extend_list_mutex);
>> +    atomic_set(&suspend_ima_measurements, 0);
>> +    mutex_unlock(&ima_extend_list_mutex);
>> +}
>> +
>>   /*
>>    * Add template entry to the measurement list and hash table, and
>>    * extend the pcr.
>> @@ -176,6 +195,19 @@ int ima_add_template_entry(struct 
>> ima_template_entry *entry, int violation,
>>           }
>>       }
>> +    /*
>> +     * suspend_ima_measurements will be set if the system is
>> +     * undergoing kexec soft boot to a new kernel.
>> +     * suspending measurements in this short window ensures the
>> +     * consistency of the IMA measurement list during copying
>> +     * of the kexec buffer.
>> +     */
>> +    if (atomic_read(&suspend_ima_measurements)) {
>> +        audit_cause = "measurements_suspended";
>> +        audit_info = 0;
>> +        goto out;
>> +    }
>> +
>>       result = ima_add_digest_entry(entry,
>>                         !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
>>       if (result < 0) {

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 5/7] ima: suspend measurements during buffer copy at kexec execute
@ 2024-01-23 20:55       ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-23 20:55 UTC (permalink / raw)
  To: Stefan Berger, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/23/24 10:18, Stefan Berger wrote:
> 
> 
> On 1/22/24 13:38, Tushar Sugandhi wrote:
>> New measurements added to the IMA log while the log is being copied
>> during the kexec 'execute' may not get copied over.  This can cause the
>> measurement log to be out of sync with the IMA TPM PCR, which could
> 
> It could be out of sync with any PCR that IMA extends, most often only 
> the IMA TPM PCR.
> 
Agreed.  That's what I meant. But I can see my above wording could be 
misleading.
I will reword it to "This can cause the measurement log to be out of 
sync with the TPM PCRs that IMA extends, which could result in..."

>> result in breaking the integrity of the measurements after kexec soft
>> reboot.
>>
>> Implement and call the functions ima_measurements_suspend() and
>> ima_measurements_resume() from ima_update_kexec_buffer().
>>
>> Add a check in the ima_add_template_entry() function not to measure
>> events when 'suspend_ima_measurements' flag is set.
>>
>> This ensures the integrity of the IMA log while it is being copied over
>> to the new Kernel during kexec 'execute'.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 
Thanks for the tag. I will add it to this patch in the next iteration of 
the series.
Please let me know if you want it to be added to any other patches.

~Tushar
>> ---
>>   security/integrity/ima/ima.h       |  2 ++
>>   security/integrity/ima/ima_kexec.c |  7 +++++++
>>   security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
>>   3 files changed, 41 insertions(+)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index c29db699c996..49a6047dd8eb 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -161,6 +161,8 @@ bool ima_template_has_modsig(const struct 
>> ima_template_desc *ima_template);
>>   int ima_restore_measurement_entry(struct ima_template_entry *entry);
>>   int ima_restore_measurement_list(loff_t bufsize, void *buf);
>>   int ima_measurements_show(struct seq_file *m, void *v);
>> +void ima_measurements_suspend(void);
>> +void ima_measurements_resume(void);
>>   unsigned long ima_get_binary_runtime_size(void);
>>   int ima_init_template(void);
>>   void ima_init_template_list(void);
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 25150bfc7129..f26b5b342d84 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -179,6 +179,7 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>       void *buf = NULL;
>>       size_t buf_size;
>>       int ret = NOTIFY_OK;
>> +    bool resume = false;
>>       if (!kexec_in_progress) {
>>           pr_info("%s: No kexec in progress.\n", __func__);
>> @@ -190,12 +191,15 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>           return ret;
>>       }
>> +    ima_measurements_suspend();
>> +
>>       ret = ima_dump_measurement_list(&buf_size, &buf,
>>                       kexec_segment_size);
>>       if (!buf) {
>>           pr_err("%s: Dump measurements failed. Error:%d\n",
>>                  __func__, ret);
>> +        resume = true;
>>           goto out;
>>       }
>>       memcpy(ima_kexec_buffer, buf, buf_size);
>> @@ -203,6 +207,9 @@ static int ima_update_kexec_buffer(struct 
>> notifier_block *self,
>>       kimage_unmap_segment(ima_kexec_buffer);
>>       ima_kexec_buffer = NULL;
>> +    if (resume)
>> +        ima_measurements_resume();
>> +
>>       return ret;
>>   }
>> diff --git a/security/integrity/ima/ima_queue.c 
>> b/security/integrity/ima/ima_queue.c
>> index 532da87ce519..5946a26a2849 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = {
>>    */
>>   static DEFINE_MUTEX(ima_extend_list_mutex);
>> +/*
>> + * Used internally by the kernel to suspend-resume ima measurements.
>> + */
>> +static atomic_t suspend_ima_measurements;
>> +
>>   /* lookup up the digest value in the hash table, and return the 
>> entry */
>>   static struct ima_queue_entry *ima_lookup_digest_entry(u8 
>> *digest_value,
>>                                  int pcr)
>> @@ -148,6 +153,20 @@ static int ima_pcr_extend(struct tpm_digest 
>> *digests_arg, int pcr)
>>       return result;
>>   }
>> +void ima_measurements_suspend(void)
>> +{
>> +    mutex_lock(&ima_extend_list_mutex);
>> +    atomic_set(&suspend_ima_measurements, 1);
>> +    mutex_unlock(&ima_extend_list_mutex);
>> +}
>> +
>> +void ima_measurements_resume(void)
>> +{
>> +    mutex_lock(&ima_extend_list_mutex);
>> +    atomic_set(&suspend_ima_measurements, 0);
>> +    mutex_unlock(&ima_extend_list_mutex);
>> +}
>> +
>>   /*
>>    * Add template entry to the measurement list and hash table, and
>>    * extend the pcr.
>> @@ -176,6 +195,19 @@ int ima_add_template_entry(struct 
>> ima_template_entry *entry, int violation,
>>           }
>>       }
>> +    /*
>> +     * suspend_ima_measurements will be set if the system is
>> +     * undergoing kexec soft boot to a new kernel.
>> +     * suspending measurements in this short window ensures the
>> +     * consistency of the IMA measurement list during copying
>> +     * of the kexec buffer.
>> +     */
>> +    if (atomic_read(&suspend_ima_measurements)) {
>> +        audit_cause = "measurements_suspended";
>> +        audit_info = 0;
>> +        goto out;
>> +    }
>> +
>>       result = ima_add_digest_entry(entry,
>>                         !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
>>       if (result < 0) {

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
  2024-01-23 19:02     ` Stefan Berger
@ 2024-01-23 21:19       ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-23 21:19 UTC (permalink / raw)
  To: Stefan Berger, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul

Thanks again Stefan for taking a look.

On 1/23/24 11:02, Stefan Berger wrote:
> 
> 
> On 1/22/24 13:38, Tushar Sugandhi wrote:
>> The extra memory allocated for carrying the IMA measurement list across
>> kexec is hardcoded 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
>> to maintain backwards compatibility.
>>
>> Update ima_add_kexec_buffer() function to allocate memory based on the
>> Kconfig option value, rather than the currently hardcoded one.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/Kconfig     | 11 +++++++++++
>>   security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
>>   2 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/security/integrity/ima/Kconfig 
>> b/security/integrity/ima/Kconfig
>> index 60a511c6b583..fc103288852b 100644
>> --- a/security/integrity/ima/Kconfig
>> +++ b/security/integrity/ima/Kconfig
>> @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE
>>       default n
>>       help
>>          This option disables htable to allow measurement of duplicate 
>> records.
>> +
>> +config IMA_KEXEC_EXTRA_MEMORY_KB
>> +    int
>> +    depends on IMA && IMA_KEXEC
>> +    default 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, an extra half page of memory for
>> +      additional measurements will be allocated to maintain backwards
>> +      compatibility.
> 
> Is there really an issue with 'backwards compatibility' that the user 
> needs to know about ? From looking at the code it seems more important 
> that a bit of additional memory is reserved now to fit the kexec 'load' 
> and 'exec' critical data events but that's not 'backwards compatibility'.
> 
I was contemplating how to put the right description in place 
considering the conversation we had in v3 of this series[1].
Without that context[1] default 0 could be equally confusing to the end 
user.  With the phrase 'backwards compatibility', I was trying to 
address the potential confusion around the default value 0 in the config 
- that it represents half-a-page as default. And that particular value 
choice ( half-a-page) is for backwards compatibility.
You are right, I the user shouldn't care about it.  But I had to start 
somewhere so that we can have this discussion on this thread. :)

Let me know how this description looks after removing the phrase 
'backwards compatibility':

" 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, an extra half a page of memory for those
   additional measurements will be allocated."

Lastly, do you want me to add suggested-by and/or reviewed-by tag to 
this patch? Let me know. I will be happy to do so.

Thanks,
Tushar

---
[1] Subject: Re: [PATCH v3 6/7] ima: configure memory to log events 
between kexec load and execute
https://lore.kernel.org/all/a09613f2-0d9f-41a3-b78d-b1b1fd35614c@linux.microsoft.com/

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

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


> Also mention this as a guidance on either how kexec load+exec need to be 
> used or as a hint that it may potentially require a lot of memory? :
> 
> The amount of extra memory that is necessary to carry all measurements 
> across a kexec depends on memory requirements of the measurements taken 
> between the kexec 'load' and 'exec' stages. The more measurements are 
> taken, the more extra memory is required. Large amounts of extra memory 
> can be avoided by running kexec 'load' and 'exec' in short sequence.
> 
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index f26b5b342d84..c126aa6494e9 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -121,6 +121,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_size;
>>       /* use more understandable variable names than defined in kbuf */
>>       void *kexec_buffer = NULL;
>> @@ -128,15 +129,19 @@ 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_size = PAGE_SIZE / 2;
>> +    else
>> +        extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
>> +    binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
>> +
>>       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");
> 
> Code LGTM

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
@ 2024-01-23 21:19       ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-23 21:19 UTC (permalink / raw)
  To: Stefan Berger, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul

Thanks again Stefan for taking a look.

On 1/23/24 11:02, Stefan Berger wrote:
> 
> 
> On 1/22/24 13:38, Tushar Sugandhi wrote:
>> The extra memory allocated for carrying the IMA measurement list across
>> kexec is hardcoded 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
>> to maintain backwards compatibility.
>>
>> Update ima_add_kexec_buffer() function to allocate memory based on the
>> Kconfig option value, rather than the currently hardcoded one.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/Kconfig     | 11 +++++++++++
>>   security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
>>   2 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/security/integrity/ima/Kconfig 
>> b/security/integrity/ima/Kconfig
>> index 60a511c6b583..fc103288852b 100644
>> --- a/security/integrity/ima/Kconfig
>> +++ b/security/integrity/ima/Kconfig
>> @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE
>>       default n
>>       help
>>          This option disables htable to allow measurement of duplicate 
>> records.
>> +
>> +config IMA_KEXEC_EXTRA_MEMORY_KB
>> +    int
>> +    depends on IMA && IMA_KEXEC
>> +    default 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, an extra half page of memory for
>> +      additional measurements will be allocated to maintain backwards
>> +      compatibility.
> 
> Is there really an issue with 'backwards compatibility' that the user 
> needs to know about ? From looking at the code it seems more important 
> that a bit of additional memory is reserved now to fit the kexec 'load' 
> and 'exec' critical data events but that's not 'backwards compatibility'.
> 
I was contemplating how to put the right description in place 
considering the conversation we had in v3 of this series[1].
Without that context[1] default 0 could be equally confusing to the end 
user.  With the phrase 'backwards compatibility', I was trying to 
address the potential confusion around the default value 0 in the config 
- that it represents half-a-page as default. And that particular value 
choice ( half-a-page) is for backwards compatibility.
You are right, I the user shouldn't care about it.  But I had to start 
somewhere so that we can have this discussion on this thread. :)

Let me know how this description looks after removing the phrase 
'backwards compatibility':

" 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, an extra half a page of memory for those
   additional measurements will be allocated."

Lastly, do you want me to add suggested-by and/or reviewed-by tag to 
this patch? Let me know. I will be happy to do so.

Thanks,
Tushar

---
[1] Subject: Re: [PATCH v3 6/7] ima: configure memory to log events 
between kexec load and execute
https://lore.kernel.org/all/a09613f2-0d9f-41a3-b78d-b1b1fd35614c@linux.microsoft.com/

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

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


> Also mention this as a guidance on either how kexec load+exec need to be 
> used or as a hint that it may potentially require a lot of memory? :
> 
> The amount of extra memory that is necessary to carry all measurements 
> across a kexec depends on memory requirements of the measurements taken 
> between the kexec 'load' and 'exec' stages. The more measurements are 
> taken, the more extra memory is required. Large amounts of extra memory 
> can be avoided by running kexec 'load' and 'exec' in short sequence.
> 
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index f26b5b342d84..c126aa6494e9 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -121,6 +121,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_size;
>>       /* use more understandable variable names than defined in kbuf */
>>       void *kexec_buffer = NULL;
>> @@ -128,15 +129,19 @@ 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_size = PAGE_SIZE / 2;
>> +    else
>> +        extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
>> +    binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
>> +
>>       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");
> 
> Code LGTM

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
  2024-01-23 21:19       ` Tushar Sugandhi
@ 2024-01-24  1:48         ` Stefan Berger
  -1 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-24  1:48 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/23/24 16:19, Tushar Sugandhi wrote:
> Thanks again Stefan for taking a look.
> 
> On 1/23/24 11:02, Stefan Berger wrote:
>>
>>
>> On 1/22/24 13:38, Tushar Sugandhi wrote:
>>> The extra memory allocated for carrying the IMA measurement list across
>>> kexec is hardcoded 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
>>> to maintain backwards compatibility.
>>>
>>> Update ima_add_kexec_buffer() function to allocate memory based on the
>>> Kconfig option value, rather than the currently hardcoded one.
>>>
>>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>>> ---
>>>   security/integrity/ima/Kconfig     | 11 +++++++++++
>>>   security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
>>>   2 files changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/security/integrity/ima/Kconfig 
>>> b/security/integrity/ima/Kconfig
>>> index 60a511c6b583..fc103288852b 100644
>>> --- a/security/integrity/ima/Kconfig
>>> +++ b/security/integrity/ima/Kconfig
>>> @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE
>>>       default n
>>>       help
>>>          This option disables htable to allow measurement of 
>>> duplicate records.
>>> +
>>> +config IMA_KEXEC_EXTRA_MEMORY_KB
>>> +    int
>>> +    depends on IMA && IMA_KEXEC
>>> +    default 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, an extra half page of memory for
>>> +      additional measurements will be allocated to maintain backwards
>>> +      compatibility.
>>
>> Is there really an issue with 'backwards compatibility' that the user 
>> needs to know about ? From looking at the code it seems more important 
>> that a bit of additional memory is reserved now to fit the kexec 
>> 'load' and 'exec' critical data events but that's not 'backwards 
>> compatibility'.
>>
> I was contemplating how to put the right description in place 
> considering the conversation we had in v3 of this series[1].
> Without that context[1] default 0 could be equally confusing to the end 
> user.  With the phrase 'backwards compatibility', I was trying to 
> address the potential confusion around the default value 0 in the config 
> - that it represents half-a-page as default. And that particular value 
> choice ( half-a-page) is for backwards compatibility.
> You are right, I the user shouldn't care about it.  But I had to start 
> somewhere so that we can have this discussion on this thread. :)
> 
> Let me know how this description looks after removing the phrase 
> 'backwards compatibility':
> 
> " 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, an extra half a page of memory for those
>    additional measurements will be allocated."

Sounds good to me.
> 
> Lastly, do you want me to add suggested-by and/or reviewed-by tag to 
> this patch? Let me know. I will be happy to do so.

Either way is fine by me.

> 
> Thanks,
> Tushar
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
@ 2024-01-24  1:48         ` Stefan Berger
  0 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-24  1:48 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/23/24 16:19, Tushar Sugandhi wrote:
> Thanks again Stefan for taking a look.
> 
> On 1/23/24 11:02, Stefan Berger wrote:
>>
>>
>> On 1/22/24 13:38, Tushar Sugandhi wrote:
>>> The extra memory allocated for carrying the IMA measurement list across
>>> kexec is hardcoded 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
>>> to maintain backwards compatibility.
>>>
>>> Update ima_add_kexec_buffer() function to allocate memory based on the
>>> Kconfig option value, rather than the currently hardcoded one.
>>>
>>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>>> ---
>>>   security/integrity/ima/Kconfig     | 11 +++++++++++
>>>   security/integrity/ima/ima_kexec.c | 15 ++++++++++-----
>>>   2 files changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/security/integrity/ima/Kconfig 
>>> b/security/integrity/ima/Kconfig
>>> index 60a511c6b583..fc103288852b 100644
>>> --- a/security/integrity/ima/Kconfig
>>> +++ b/security/integrity/ima/Kconfig
>>> @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE
>>>       default n
>>>       help
>>>          This option disables htable to allow measurement of 
>>> duplicate records.
>>> +
>>> +config IMA_KEXEC_EXTRA_MEMORY_KB
>>> +    int
>>> +    depends on IMA && IMA_KEXEC
>>> +    default 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, an extra half page of memory for
>>> +      additional measurements will be allocated to maintain backwards
>>> +      compatibility.
>>
>> Is there really an issue with 'backwards compatibility' that the user 
>> needs to know about ? From looking at the code it seems more important 
>> that a bit of additional memory is reserved now to fit the kexec 
>> 'load' and 'exec' critical data events but that's not 'backwards 
>> compatibility'.
>>
> I was contemplating how to put the right description in place 
> considering the conversation we had in v3 of this series[1].
> Without that context[1] default 0 could be equally confusing to the end 
> user.  With the phrase 'backwards compatibility', I was trying to 
> address the potential confusion around the default value 0 in the config 
> - that it represents half-a-page as default. And that particular value 
> choice ( half-a-page) is for backwards compatibility.
> You are right, I the user shouldn't care about it.  But I had to start 
> somewhere so that we can have this discussion on this thread. :)
> 
> Let me know how this description looks after removing the phrase 
> 'backwards compatibility':
> 
> " 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, an extra half a page of memory for those
>    additional measurements will be allocated."

Sounds good to me.
> 
> Lastly, do you want me to add suggested-by and/or reviewed-by tag to 
> this patch? Let me know. I will be happy to do so.

Either way is fine by me.

> 
> Thanks,
> Tushar
> 

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
  2024-01-22 18:37   ` Tushar Sugandhi
@ 2024-01-24  2:54     ` Stefan Berger
  -1 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-24  2:54 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



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


A dent with this patch when only applying this patch:

Two consecutive kexec loads lead to this here:

[   30.670330] IMA buffer at 0x3fff10000, size = 0xf0000
[   32.519618] ------------[ cut here ]------------
[   32.519669] Trying to vfree() nonexistent vm area (00000000093ae29c)
[   32.519762] WARNING: CPU: 11 PID: 1796 at mm/vmalloc.c:2826 
vfree+0x254/0x340
[   32.519786] Modules linked in: bonding tls rfkill sunrpc 
virtio_console virtio_balloon crct10dif_vpmsum fuse loop zram bochs 
drm_vram_helper drm_kms_helper drm_ttm_helper ttm ibmvscsi 
scsi_transport_srp drm virtio_blk virtio_net vmx_crypto net_failover 
crc32c_vpmsum failover pseries_wdt drm_panel_orientation_quirks 
scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_multipath
[   32.519939] CPU: 11 PID: 1796 Comm: kexec Not tainted 6.5.0+ #112
[   32.519953] Hardware name: IBM pSeries (emulated by qemu) POWER8E 
(raw) 0x4b0201 0xf000004 of:SLOF,git-5b4c5a hv:linux,kvm pSeries
[   32.519973] NIP:  c0000000004bd004 LR: c0000000004bd000 CTR: 
c00000000017ef00
[   32.519986] REGS: c00000004593b670 TRAP: 0700   Not tainted  (6.5.0+)
[   32.519999] MSR:  8000000002029033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 
44424842  XER: 00000000
[   32.520023] CFAR: c0000000001515b0 IRQMASK: 0
                GPR00: c0000000004bd000 c00000004593b910 
c000000001e17000 0000000000000038
                GPR04: 00000000ffffbfff c00000004593b6e8 
c00000004593b6e0 00000003f9580000
                GPR08: 0000000000000027 c0000003fb707010 
0000000000000001 0000000044424842
                GPR12: c00000000017ef00 c00000003fff1300 
0000000000000000 0000000000000000
                GPR16: 0000000000000000 0000000000000000 
0000000000000000 0000000000000000
                GPR20: 0000000000000000 0000000000000000 
0000000000000003 0000000000000004
                GPR24: 00007fffeab0f68f 000000000000004c 
0000000000000000 c00000002bdce400
                GPR28: c000000002bf28f0 0000000000000000 
c008000004770000 0000000000000000
[   32.520180] NIP [c0000000004bd004] vfree+0x254/0x340
[   32.520212] LR [c0000000004bd000] vfree+0x250/0x340
[   32.520225] Call Trace:
[   32.520232] [c00000004593b910] [c0000000004bd000] vfree+0x250/0x340 
(unreliable)
[   32.520250] [c00000004593b990] [c00000000091d590] 
ima_add_kexec_buffer+0xe0/0x3c0
[   32.520296] [c00000004593ba90] [c000000000280968] 
sys_kexec_file_load+0x148/0x9b0
[   32.520333] [c00000004593bb70] [c00000000002ea84] 
system_call_exception+0x174/0x320
[   32.520372] [c00000004593be50] [c00000000000d6a0] 
system_call_common+0x160/0x2c4
[   32.520408] --- interrupt: c00 at 0x7fffa52e7ae4
[   32.520420] NIP:  00007fffa52e7ae4 LR: 0000000108481d8c CTR: 
0000000000000000
[   32.520452] REGS: c00000004593be80 TRAP: 0c00   Not tainted  (6.5.0+)
[   32.520483] MSR:  800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE>  CR: 
24424202  XER: 00000000
[   32.520507] IRQMASK: 0
                GPR00: 000000000000017e 00007fffeab09470 
00007fffa53f6f00 0000000000000003
                GPR04: 0000000000000004 000000000000004c 
00007fffeab0f68f 0000000000000000
                GPR08: 0000000000000000 0000000000000000 
0000000000000000 0000000000000000
                GPR12: 0000000000000000 00007fffa559b280 
0000000000000002 0000000000000001
                GPR16: 0000000000000000 0000000000000000 
0000000000000000 0000000000000000
                GPR20: 00007fffa53f0454 00007fffa53f0458 
0000000000000000 0000000000000001
                GPR24: 0000000000000000 00007fffeab0f64d 
0000000000000006 0000000000000000
                GPR28: 0000000000000003 00007fffeab09530 
00007fffeab09b08 0000000000000007
[   32.520767] NIP [00007fffa52e7ae4] 0x7fffa52e7ae4
[   32.521192] LR [0000000108481d8c] 0x108481d8c
[   32.521587] --- interrupt: c00
[   32.521981] Code: 3884c208 4bfc20f1 60000000 0fe00000 60000000 
60000000 60420000 3c62ff94 7fc4f378 38632b20 4bc944cd 60000000 
<0fe00000> eba10068 4bffff34 2c080000
[   32.522823] ---[ end trace 0000000000000000 ]---
[   32.536347] Removed old IMA buffer reservation.
[   32.536473] IMA buffer at 0x3fff10000, size = 0xf0000

This vfree here probably has to go:

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
@ 2024-01-24  2:54     ` Stefan Berger
  0 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-24  2:54 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



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


A dent with this patch when only applying this patch:

Two consecutive kexec loads lead to this here:

[   30.670330] IMA buffer at 0x3fff10000, size = 0xf0000
[   32.519618] ------------[ cut here ]------------
[   32.519669] Trying to vfree() nonexistent vm area (00000000093ae29c)
[   32.519762] WARNING: CPU: 11 PID: 1796 at mm/vmalloc.c:2826 
vfree+0x254/0x340
[   32.519786] Modules linked in: bonding tls rfkill sunrpc 
virtio_console virtio_balloon crct10dif_vpmsum fuse loop zram bochs 
drm_vram_helper drm_kms_helper drm_ttm_helper ttm ibmvscsi 
scsi_transport_srp drm virtio_blk virtio_net vmx_crypto net_failover 
crc32c_vpmsum failover pseries_wdt drm_panel_orientation_quirks 
scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_multipath
[   32.519939] CPU: 11 PID: 1796 Comm: kexec Not tainted 6.5.0+ #112
[   32.519953] Hardware name: IBM pSeries (emulated by qemu) POWER8E 
(raw) 0x4b0201 0xf000004 of:SLOF,git-5b4c5a hv:linux,kvm pSeries
[   32.519973] NIP:  c0000000004bd004 LR: c0000000004bd000 CTR: 
c00000000017ef00
[   32.519986] REGS: c00000004593b670 TRAP: 0700   Not tainted  (6.5.0+)
[   32.519999] MSR:  8000000002029033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 
44424842  XER: 00000000
[   32.520023] CFAR: c0000000001515b0 IRQMASK: 0
                GPR00: c0000000004bd000 c00000004593b910 
c000000001e17000 0000000000000038
                GPR04: 00000000ffffbfff c00000004593b6e8 
c00000004593b6e0 00000003f9580000
                GPR08: 0000000000000027 c0000003fb707010 
0000000000000001 0000000044424842
                GPR12: c00000000017ef00 c00000003fff1300 
0000000000000000 0000000000000000
                GPR16: 0000000000000000 0000000000000000 
0000000000000000 0000000000000000
                GPR20: 0000000000000000 0000000000000000 
0000000000000003 0000000000000004
                GPR24: 00007fffeab0f68f 000000000000004c 
0000000000000000 c00000002bdce400
                GPR28: c000000002bf28f0 0000000000000000 
c008000004770000 0000000000000000
[   32.520180] NIP [c0000000004bd004] vfree+0x254/0x340
[   32.520212] LR [c0000000004bd000] vfree+0x250/0x340
[   32.520225] Call Trace:
[   32.520232] [c00000004593b910] [c0000000004bd000] vfree+0x250/0x340 
(unreliable)
[   32.520250] [c00000004593b990] [c00000000091d590] 
ima_add_kexec_buffer+0xe0/0x3c0
[   32.520296] [c00000004593ba90] [c000000000280968] 
sys_kexec_file_load+0x148/0x9b0
[   32.520333] [c00000004593bb70] [c00000000002ea84] 
system_call_exception+0x174/0x320
[   32.520372] [c00000004593be50] [c00000000000d6a0] 
system_call_common+0x160/0x2c4
[   32.520408] --- interrupt: c00 at 0x7fffa52e7ae4
[   32.520420] NIP:  00007fffa52e7ae4 LR: 0000000108481d8c CTR: 
0000000000000000
[   32.520452] REGS: c00000004593be80 TRAP: 0c00   Not tainted  (6.5.0+)
[   32.520483] MSR:  800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE>  CR: 
24424202  XER: 00000000
[   32.520507] IRQMASK: 0
                GPR00: 000000000000017e 00007fffeab09470 
00007fffa53f6f00 0000000000000003
                GPR04: 0000000000000004 000000000000004c 
00007fffeab0f68f 0000000000000000
                GPR08: 0000000000000000 0000000000000000 
0000000000000000 0000000000000000
                GPR12: 0000000000000000 00007fffa559b280 
0000000000000002 0000000000000001
                GPR16: 0000000000000000 0000000000000000 
0000000000000000 0000000000000000
                GPR20: 00007fffa53f0454 00007fffa53f0458 
0000000000000000 0000000000000001
                GPR24: 0000000000000000 00007fffeab0f64d 
0000000000000006 0000000000000000
                GPR28: 0000000000000003 00007fffeab09530 
00007fffeab09b08 0000000000000007
[   32.520767] NIP [00007fffa52e7ae4] 0x7fffa52e7ae4
[   32.521192] LR [0000000108481d8c] 0x108481d8c
[   32.521587] --- interrupt: c00
[   32.521981] Code: 3884c208 4bfc20f1 60000000 0fe00000 60000000 
60000000 60420000 3c62ff94 7fc4f378 38632b20 4bc944cd 60000000 
<0fe00000> eba10068 4bffff34 2c080000
[   32.522823] ---[ end trace 0000000000000000 ]---
[   32.536347] Removed old IMA buffer reservation.
[   32.536473] IMA buffer at 0x3fff10000, size = 0xf0000

This vfree here probably has to go:

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

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
  2024-01-24  2:54     ` Stefan Berger
@ 2024-01-24  3:38       ` Stefan Berger
  -1 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-24  3:38 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/23/24 21:54, Stefan Berger wrote:
> 
> 
> On 1/22/24 13:37, Tushar Sugandhi wrote:
>> Refactor ima_dump_measurement_list() to move the memory allocation part
>> 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 function ima_dump_measurement_list() as 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 <tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++---------
>>   1 file changed, 67 insertions(+), 29 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 419dc405c831..99daac355c70 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -15,62 +15,93 @@
>>   #include "ima.h"
>>   #ifdef CONFIG_IMA_KEXEC
>> +static struct seq_file ima_kexec_file;
>> +
>> +static void ima_free_kexec_file_buf(struct seq_file *sf)
>> +{
>> +    vfree(sf->buf);
>> +    sf->buf = NULL;
>> +    sf->size = 0;
>> +    sf->read_pos = 0;
>> +    sf->count = 0;
>> +}
>> +
>> +static int ima_alloc_kexec_file_buf(size_t segment_size)
>> +{
>> +    /*
>> +     * kexec 'load' may be called multiple times.
>> +     * Free and realloc the buffer only if the segment_size is
>> +     * changed from the previous kexec 'load' call.
>> +     */
>> +    if (ima_kexec_file.buf &&
>> +        ima_kexec_file.size == segment_size &&
>> +        ima_kexec_file.read_pos == 0 &&
>> +        ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
>> +        return 0;
>> +
>> +    ima_free_kexec_file_buf(&ima_kexec_file);
>> +
>> +    /* segment size can't change between kexec load and execute */
>> +    ima_kexec_file.buf = vmalloc(segment_size);
>> +    if (!ima_kexec_file.buf)
>> +        return -ENOMEM;
>> +
>> +    ima_kexec_file.size = segment_size;
>> +    ima_kexec_file.read_pos = 0;
>> +    ima_kexec_file.count = sizeof(struct ima_kexec_hdr);    /* 
>> reserved space */
>> +
>> +    return 0;
>> +}
>> +
>>   static int ima_dump_measurement_list(unsigned long *buffer_size, 
>> void **buffer,
>>                        unsigned long segment_size)
>>   {
>>       struct ima_queue_entry *qe;
>> -    struct seq_file file;
>>       struct ima_kexec_hdr khdr;
>> -    int ret = 0;
>> -    /* segment size can't change between kexec load and execute */
>> -    file.buf = vmalloc(segment_size);
>> -    if (!file.buf) {
>> -        ret = -ENOMEM;
>> -        goto out;
>> +    if (!ima_kexec_file.buf) {
>> +        *buffer_size = 0;
>> +        *buffer = NULL;
>> +        pr_err("%s: Kexec file buf not allocated\n", __func__);
>> +        return -EINVAL;
>>       }
>> -    file.size = segment_size;
>> -    file.read_pos = 0;
>> -    file.count = sizeof(khdr);    /* reserved space */
>> -
>>       memset(&khdr, 0, sizeof(khdr));
>>       khdr.version = 1;
>> +
>> +    /* Copy as many IMA measurements list records as possible */
>>       list_for_each_entry_rcu(qe, &ima_measurements, later) {
>> -        if (file.count < file.size) {
>> +        if (ima_kexec_file.count < ima_kexec_file.size) {
>>               khdr.count++;
>> -            ima_measurements_show(&file, qe);
>> +            ima_measurements_show(&ima_kexec_file, qe);
>>           } else {
>> -            ret = -EINVAL;
>> +            pr_err("%s: IMA log file is too big for Kexec buf\n",
>> +                   __func__);
>>               break;
>>           }
>>       }
>> -    if (ret < 0)
>> -        goto out;
>> -
>>       /*
>>        * fill in reserved space with some buffer details
>>        * (eg. version, buffer size, number of measurements)
>>        */
>> -    khdr.buffer_size = file.count;
>> +    khdr.buffer_size = ima_kexec_file.count;
>>       if (ima_canonical_fmt) {
>>           khdr.version = cpu_to_le16(khdr.version);
>>           khdr.count = cpu_to_le64(khdr.count);
>>           khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
>>       }
>> -    memcpy(file.buf, &khdr, sizeof(khdr));
>> +    memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr));
>>       print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
>> -                 file.buf, file.count < 100 ? file.count : 100,
>> +                 ima_kexec_file.buf, ima_kexec_file.count < 100 ?
>> +                 ima_kexec_file.count : 100,
>>                    true);
>> -    *buffer_size = file.count;
>> -    *buffer = file.buf;
>> -out:
>> -    if (ret == -EINVAL)
>> -        vfree(file.buf);
>> -    return ret;
>> +    *buffer_size = ima_kexec_file.count;
>> +    *buffer = ima_kexec_file.buf;
>> +
>> +    return 0;
>>   }
>>   /*
>> @@ -108,13 +139,20 @@ void ima_add_kexec_buffer(struct kimage *image)
>>           return;
>>       }
>> -    ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
>> -                  kexec_segment_size);
>> -    if (!kexec_buffer) {
>> +    ret = ima_alloc_kexec_file_buf(kexec_segment_size);
>> +    if (ret < 0) {
>>           pr_err("Not enough memory for the kexec measurement 
>> buffer.\n");
>>           return;
>>       }
>> +    ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
>> +                    kexec_segment_size);
>> +    if (ret < 0) {
>> +        pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
>> +               __func__, ret);
>> +        return;
>> +    }
>> +
>>       kbuf.buffer = kexec_buffer;
>>       kbuf.bufsz = kexec_buffer_size;
>>       kbuf.memsz = kexec_segment_size;
> 
> 
> A dent with this patch when only applying this patch:
> 
> Two consecutive kexec loads lead to this here:
> 
> [   30.670330] IMA buffer at 0x3fff10000, size = 0xf0000
> [   32.519618] ------------[ cut here ]------------
> [   32.519669] Trying to vfree() nonexistent vm area (00000000093ae29c)
> [   32.519762] WARNING: CPU: 11 PID: 1796 at mm/vmalloc.c:2826 
> vfree+0x254/0x340
> [   32.519786] Modules linked in: bonding tls rfkill sunrpc 
> virtio_console virtio_balloon crct10dif_vpmsum fuse loop zram bochs 
> drm_vram_helper drm_kms_helper drm_ttm_helper ttm ibmvscsi 
> scsi_transport_srp drm virtio_blk virtio_net vmx_crypto net_failover 
> crc32c_vpmsum failover pseries_wdt drm_panel_orientation_quirks 
> scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_multipath
> [   32.519939] CPU: 11 PID: 1796 Comm: kexec Not tainted 6.5.0+ #112
> [   32.519953] Hardware name: IBM pSeries (emulated by qemu) POWER8E 
> (raw) 0x4b0201 0xf000004 of:SLOF,git-5b4c5a hv:linux,kvm pSeries
> [   32.519973] NIP:  c0000000004bd004 LR: c0000000004bd000 CTR: 
> c00000000017ef00
> [   32.519986] REGS: c00000004593b670 TRAP: 0700   Not tainted  (6.5.0+)
> [   32.519999] MSR:  8000000002029033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 
> 44424842  XER: 00000000
> [   32.520023] CFAR: c0000000001515b0 IRQMASK: 0
>                 GPR00: c0000000004bd000 c00000004593b910 
> c000000001e17000 0000000000000038
>                 GPR04: 00000000ffffbfff c00000004593b6e8 
> c00000004593b6e0 00000003f9580000
>                 GPR08: 0000000000000027 c0000003fb707010 
> 0000000000000001 0000000044424842
>                 GPR12: c00000000017ef00 c00000003fff1300 
> 0000000000000000 0000000000000000
>                 GPR16: 0000000000000000 0000000000000000 
> 0000000000000000 0000000000000000
>                 GPR20: 0000000000000000 0000000000000000 
> 0000000000000003 0000000000000004
>                 GPR24: 00007fffeab0f68f 000000000000004c 
> 0000000000000000 c00000002bdce400
>                 GPR28: c000000002bf28f0 0000000000000000 
> c008000004770000 0000000000000000
> [   32.520180] NIP [c0000000004bd004] vfree+0x254/0x340
> [   32.520212] LR [c0000000004bd000] vfree+0x250/0x340
> [   32.520225] Call Trace:
> [   32.520232] [c00000004593b910] [c0000000004bd000] vfree+0x250/0x340 
> (unreliable)
> [   32.520250] [c00000004593b990] [c00000000091d590] 
> ima_add_kexec_buffer+0xe0/0x3c0
> [   32.520296] [c00000004593ba90] [c000000000280968] 
> sys_kexec_file_load+0x148/0x9b0
> [   32.520333] [c00000004593bb70] [c00000000002ea84] 
> system_call_exception+0x174/0x320
> [   32.520372] [c00000004593be50] [c00000000000d6a0] 
> system_call_common+0x160/0x2c4
> [   32.520408] --- interrupt: c00 at 0x7fffa52e7ae4
> [   32.520420] NIP:  00007fffa52e7ae4 LR: 0000000108481d8c CTR: 
> 0000000000000000
> [   32.520452] REGS: c00000004593be80 TRAP: 0c00   Not tainted  (6.5.0+)
> [   32.520483] MSR:  800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE>  CR: 
> 24424202  XER: 00000000
> [   32.520507] IRQMASK: 0
>                 GPR00: 000000000000017e 00007fffeab09470 
> 00007fffa53f6f00 0000000000000003
>                 GPR04: 0000000000000004 000000000000004c 
> 00007fffeab0f68f 0000000000000000
>                 GPR08: 0000000000000000 0000000000000000 
> 0000000000000000 0000000000000000
>                 GPR12: 0000000000000000 00007fffa559b280 
> 0000000000000002 0000000000000001
>                 GPR16: 0000000000000000 0000000000000000 
> 0000000000000000 0000000000000000
>                 GPR20: 00007fffa53f0454 00007fffa53f0458 
> 0000000000000000 0000000000000001
>                 GPR24: 0000000000000000 00007fffeab0f64d 
> 0000000000000006 0000000000000000
>                 GPR28: 0000000000000003 00007fffeab09530 
> 00007fffeab09b08 0000000000000007
> [   32.520767] NIP [00007fffa52e7ae4] 0x7fffa52e7ae4
> [   32.521192] LR [0000000108481d8c] 0x108481d8c
> [   32.521587] --- interrupt: c00
> [   32.521981] Code: 3884c208 4bfc20f1 60000000 0fe00000 60000000 
> 60000000 60420000 3c62ff94 7fc4f378 38632b20 4bc944cd 60000000 
> <0fe00000> eba10068 4bffff34 2c080000
> [   32.522823] ---[ end trace 0000000000000000 ]---
> [   32.536347] Removed old IMA buffer reservation.
> [   32.536473] IMA buffer at 0x3fff10000, size = 0xf0000
> 
> This vfree here probably has to go:
> 
>          ret = kexec_add_buffer(&kbuf);
>          if (ret) {
>                  pr_err("Error passing over kexec measurement buffer.\n");
>                  vfree(kexec_buffer);
>                  return;
>          }
> 

The vfree may need to be removed or replaced with 
ima_free_kexec_file_buf() but it doesn't seem to solve the problem alone.
I got rid of this issue later with this:

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

[...]

@@ -170,6 +175,9 @@ 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 since kexec_add_buffer and will 
vfree() it */
+       ima_reset_kexec_file(&ima_kexec_file);
+
         pr_debug("kexec measurement buffer for the loaded kernel at 
0x%lx.\n",
                  kbuf.mem);


> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
@ 2024-01-24  3:38       ` Stefan Berger
  0 siblings, 0 replies; 52+ messages in thread
From: Stefan Berger @ 2024-01-24  3:38 UTC (permalink / raw)
  To: Tushar Sugandhi, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul



On 1/23/24 21:54, Stefan Berger wrote:
> 
> 
> On 1/22/24 13:37, Tushar Sugandhi wrote:
>> Refactor ima_dump_measurement_list() to move the memory allocation part
>> 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 function ima_dump_measurement_list() as 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 <tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima_kexec.c | 96 +++++++++++++++++++++---------
>>   1 file changed, 67 insertions(+), 29 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_kexec.c 
>> b/security/integrity/ima/ima_kexec.c
>> index 419dc405c831..99daac355c70 100644
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -15,62 +15,93 @@
>>   #include "ima.h"
>>   #ifdef CONFIG_IMA_KEXEC
>> +static struct seq_file ima_kexec_file;
>> +
>> +static void ima_free_kexec_file_buf(struct seq_file *sf)
>> +{
>> +    vfree(sf->buf);
>> +    sf->buf = NULL;
>> +    sf->size = 0;
>> +    sf->read_pos = 0;
>> +    sf->count = 0;
>> +}
>> +
>> +static int ima_alloc_kexec_file_buf(size_t segment_size)
>> +{
>> +    /*
>> +     * kexec 'load' may be called multiple times.
>> +     * Free and realloc the buffer only if the segment_size is
>> +     * changed from the previous kexec 'load' call.
>> +     */
>> +    if (ima_kexec_file.buf &&
>> +        ima_kexec_file.size == segment_size &&
>> +        ima_kexec_file.read_pos == 0 &&
>> +        ima_kexec_file.count == sizeof(struct ima_kexec_hdr))
>> +        return 0;
>> +
>> +    ima_free_kexec_file_buf(&ima_kexec_file);
>> +
>> +    /* segment size can't change between kexec load and execute */
>> +    ima_kexec_file.buf = vmalloc(segment_size);
>> +    if (!ima_kexec_file.buf)
>> +        return -ENOMEM;
>> +
>> +    ima_kexec_file.size = segment_size;
>> +    ima_kexec_file.read_pos = 0;
>> +    ima_kexec_file.count = sizeof(struct ima_kexec_hdr);    /* 
>> reserved space */
>> +
>> +    return 0;
>> +}
>> +
>>   static int ima_dump_measurement_list(unsigned long *buffer_size, 
>> void **buffer,
>>                        unsigned long segment_size)
>>   {
>>       struct ima_queue_entry *qe;
>> -    struct seq_file file;
>>       struct ima_kexec_hdr khdr;
>> -    int ret = 0;
>> -    /* segment size can't change between kexec load and execute */
>> -    file.buf = vmalloc(segment_size);
>> -    if (!file.buf) {
>> -        ret = -ENOMEM;
>> -        goto out;
>> +    if (!ima_kexec_file.buf) {
>> +        *buffer_size = 0;
>> +        *buffer = NULL;
>> +        pr_err("%s: Kexec file buf not allocated\n", __func__);
>> +        return -EINVAL;
>>       }
>> -    file.size = segment_size;
>> -    file.read_pos = 0;
>> -    file.count = sizeof(khdr);    /* reserved space */
>> -
>>       memset(&khdr, 0, sizeof(khdr));
>>       khdr.version = 1;
>> +
>> +    /* Copy as many IMA measurements list records as possible */
>>       list_for_each_entry_rcu(qe, &ima_measurements, later) {
>> -        if (file.count < file.size) {
>> +        if (ima_kexec_file.count < ima_kexec_file.size) {
>>               khdr.count++;
>> -            ima_measurements_show(&file, qe);
>> +            ima_measurements_show(&ima_kexec_file, qe);
>>           } else {
>> -            ret = -EINVAL;
>> +            pr_err("%s: IMA log file is too big for Kexec buf\n",
>> +                   __func__);
>>               break;
>>           }
>>       }
>> -    if (ret < 0)
>> -        goto out;
>> -
>>       /*
>>        * fill in reserved space with some buffer details
>>        * (eg. version, buffer size, number of measurements)
>>        */
>> -    khdr.buffer_size = file.count;
>> +    khdr.buffer_size = ima_kexec_file.count;
>>       if (ima_canonical_fmt) {
>>           khdr.version = cpu_to_le16(khdr.version);
>>           khdr.count = cpu_to_le64(khdr.count);
>>           khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
>>       }
>> -    memcpy(file.buf, &khdr, sizeof(khdr));
>> +    memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr));
>>       print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
>> -                 file.buf, file.count < 100 ? file.count : 100,
>> +                 ima_kexec_file.buf, ima_kexec_file.count < 100 ?
>> +                 ima_kexec_file.count : 100,
>>                    true);
>> -    *buffer_size = file.count;
>> -    *buffer = file.buf;
>> -out:
>> -    if (ret == -EINVAL)
>> -        vfree(file.buf);
>> -    return ret;
>> +    *buffer_size = ima_kexec_file.count;
>> +    *buffer = ima_kexec_file.buf;
>> +
>> +    return 0;
>>   }
>>   /*
>> @@ -108,13 +139,20 @@ void ima_add_kexec_buffer(struct kimage *image)
>>           return;
>>       }
>> -    ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
>> -                  kexec_segment_size);
>> -    if (!kexec_buffer) {
>> +    ret = ima_alloc_kexec_file_buf(kexec_segment_size);
>> +    if (ret < 0) {
>>           pr_err("Not enough memory for the kexec measurement 
>> buffer.\n");
>>           return;
>>       }
>> +    ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
>> +                    kexec_segment_size);
>> +    if (ret < 0) {
>> +        pr_err("%s: Failed to dump IMA measurements. Error:%d.\n",
>> +               __func__, ret);
>> +        return;
>> +    }
>> +
>>       kbuf.buffer = kexec_buffer;
>>       kbuf.bufsz = kexec_buffer_size;
>>       kbuf.memsz = kexec_segment_size;
> 
> 
> A dent with this patch when only applying this patch:
> 
> Two consecutive kexec loads lead to this here:
> 
> [   30.670330] IMA buffer at 0x3fff10000, size = 0xf0000
> [   32.519618] ------------[ cut here ]------------
> [   32.519669] Trying to vfree() nonexistent vm area (00000000093ae29c)
> [   32.519762] WARNING: CPU: 11 PID: 1796 at mm/vmalloc.c:2826 
> vfree+0x254/0x340
> [   32.519786] Modules linked in: bonding tls rfkill sunrpc 
> virtio_console virtio_balloon crct10dif_vpmsum fuse loop zram bochs 
> drm_vram_helper drm_kms_helper drm_ttm_helper ttm ibmvscsi 
> scsi_transport_srp drm virtio_blk virtio_net vmx_crypto net_failover 
> crc32c_vpmsum failover pseries_wdt drm_panel_orientation_quirks 
> scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_multipath
> [   32.519939] CPU: 11 PID: 1796 Comm: kexec Not tainted 6.5.0+ #112
> [   32.519953] Hardware name: IBM pSeries (emulated by qemu) POWER8E 
> (raw) 0x4b0201 0xf000004 of:SLOF,git-5b4c5a hv:linux,kvm pSeries
> [   32.519973] NIP:  c0000000004bd004 LR: c0000000004bd000 CTR: 
> c00000000017ef00
> [   32.519986] REGS: c00000004593b670 TRAP: 0700   Not tainted  (6.5.0+)
> [   32.519999] MSR:  8000000002029033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 
> 44424842  XER: 00000000
> [   32.520023] CFAR: c0000000001515b0 IRQMASK: 0
>                 GPR00: c0000000004bd000 c00000004593b910 
> c000000001e17000 0000000000000038
>                 GPR04: 00000000ffffbfff c00000004593b6e8 
> c00000004593b6e0 00000003f9580000
>                 GPR08: 0000000000000027 c0000003fb707010 
> 0000000000000001 0000000044424842
>                 GPR12: c00000000017ef00 c00000003fff1300 
> 0000000000000000 0000000000000000
>                 GPR16: 0000000000000000 0000000000000000 
> 0000000000000000 0000000000000000
>                 GPR20: 0000000000000000 0000000000000000 
> 0000000000000003 0000000000000004
>                 GPR24: 00007fffeab0f68f 000000000000004c 
> 0000000000000000 c00000002bdce400
>                 GPR28: c000000002bf28f0 0000000000000000 
> c008000004770000 0000000000000000
> [   32.520180] NIP [c0000000004bd004] vfree+0x254/0x340
> [   32.520212] LR [c0000000004bd000] vfree+0x250/0x340
> [   32.520225] Call Trace:
> [   32.520232] [c00000004593b910] [c0000000004bd000] vfree+0x250/0x340 
> (unreliable)
> [   32.520250] [c00000004593b990] [c00000000091d590] 
> ima_add_kexec_buffer+0xe0/0x3c0
> [   32.520296] [c00000004593ba90] [c000000000280968] 
> sys_kexec_file_load+0x148/0x9b0
> [   32.520333] [c00000004593bb70] [c00000000002ea84] 
> system_call_exception+0x174/0x320
> [   32.520372] [c00000004593be50] [c00000000000d6a0] 
> system_call_common+0x160/0x2c4
> [   32.520408] --- interrupt: c00 at 0x7fffa52e7ae4
> [   32.520420] NIP:  00007fffa52e7ae4 LR: 0000000108481d8c CTR: 
> 0000000000000000
> [   32.520452] REGS: c00000004593be80 TRAP: 0c00   Not tainted  (6.5.0+)
> [   32.520483] MSR:  800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE>  CR: 
> 24424202  XER: 00000000
> [   32.520507] IRQMASK: 0
>                 GPR00: 000000000000017e 00007fffeab09470 
> 00007fffa53f6f00 0000000000000003
>                 GPR04: 0000000000000004 000000000000004c 
> 00007fffeab0f68f 0000000000000000
>                 GPR08: 0000000000000000 0000000000000000 
> 0000000000000000 0000000000000000
>                 GPR12: 0000000000000000 00007fffa559b280 
> 0000000000000002 0000000000000001
>                 GPR16: 0000000000000000 0000000000000000 
> 0000000000000000 0000000000000000
>                 GPR20: 00007fffa53f0454 00007fffa53f0458 
> 0000000000000000 0000000000000001
>                 GPR24: 0000000000000000 00007fffeab0f64d 
> 0000000000000006 0000000000000000
>                 GPR28: 0000000000000003 00007fffeab09530 
> 00007fffeab09b08 0000000000000007
> [   32.520767] NIP [00007fffa52e7ae4] 0x7fffa52e7ae4
> [   32.521192] LR [0000000108481d8c] 0x108481d8c
> [   32.521587] --- interrupt: c00
> [   32.521981] Code: 3884c208 4bfc20f1 60000000 0fe00000 60000000 
> 60000000 60420000 3c62ff94 7fc4f378 38632b20 4bc944cd 60000000 
> <0fe00000> eba10068 4bffff34 2c080000
> [   32.522823] ---[ end trace 0000000000000000 ]---
> [   32.536347] Removed old IMA buffer reservation.
> [   32.536473] IMA buffer at 0x3fff10000, size = 0xf0000
> 
> This vfree here probably has to go:
> 
>          ret = kexec_add_buffer(&kbuf);
>          if (ret) {
>                  pr_err("Error passing over kexec measurement buffer.\n");
>                  vfree(kexec_buffer);
>                  return;
>          }
> 

The vfree may need to be removed or replaced with 
ima_free_kexec_file_buf() but it doesn't seem to solve the problem alone.
I got rid of this issue later with this:

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

[...]

@@ -170,6 +175,9 @@ 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 since kexec_add_buffer and will 
vfree() it */
+       ima_reset_kexec_file(&ima_kexec_file);
+
         pr_debug("kexec measurement buffer for the loaded kernel at 
0x%lx.\n",
                  kbuf.mem);


> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
  2024-01-22 18:37   ` Tushar Sugandhi
@ 2024-01-24 13:33     ` Mimi Zohar
  -1 siblings, 0 replies; 52+ messages in thread
From: Mimi Zohar @ 2024-01-24 13:33 UTC (permalink / raw)
  To: Tushar Sugandhi, roberto.sassu, roberto.sassu, eric.snowberg,
	stefanb, ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

Hi Tushar,

On Mon, 2024-01-22 at 10:37 -0800, Tushar Sugandhi wrote:

Missing from this and the other patch descriptions is the problem
description.  Please refer to the section titled  "Describe your changes" in 
https://docs.kernel.org/process/submitting-patches.html.

"Describe your problem.  Whether your patch is a one-line bug fix or 5000 lines
of a new feature, there must be an underlying problem that motivated you to do
this work.  Convince the reviewer that there is a problem worth fixing and that
it makes sense for them to read past the first paragraph."

In this case, "why" you need to refactor ima_dump_measurement_list() is the
problem.

For example:

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

"Once the problem is established, describe what you are actually doing about it
in technical detail.  It's important to describe the change in plain English for
the reviewer to verify that the code is behaving as you intend it to."

> Refactor ima_dump_measurement_list() to move the memory allocation part
> 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 function ima_dump_measurement_list() as 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.

Please make this into an unordered list.

-- 
thanks,

Mimi


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
@ 2024-01-24 13:33     ` Mimi Zohar
  0 siblings, 0 replies; 52+ messages in thread
From: Mimi Zohar @ 2024-01-24 13:33 UTC (permalink / raw)
  To: Tushar Sugandhi, roberto.sassu, roberto.sassu, eric.snowberg,
	stefanb, ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

Hi Tushar,

On Mon, 2024-01-22 at 10:37 -0800, Tushar Sugandhi wrote:

Missing from this and the other patch descriptions is the problem
description.  Please refer to the section titled  "Describe your changes" in 
https://docs.kernel.org/process/submitting-patches.html.

"Describe your problem.  Whether your patch is a one-line bug fix or 5000 lines
of a new feature, there must be an underlying problem that motivated you to do
this work.  Convince the reviewer that there is a problem worth fixing and that
it makes sense for them to read past the first paragraph."

In this case, "why" you need to refactor ima_dump_measurement_list() is the
problem.

For example:

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

"Once the problem is established, describe what you are actually doing about it
in technical detail.  It's important to describe the change in plain English for
the reviewer to verify that the code is behaving as you intend it to."

> Refactor ima_dump_measurement_list() to move the memory allocation part
> 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 function ima_dump_measurement_list() as 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.

Please make this into an unordered list.

-- 
thanks,

Mimi


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
  2024-01-22 18:38   ` Tushar Sugandhi
@ 2024-01-24 14:07     ` Mimi Zohar
  -1 siblings, 0 replies; 52+ messages in thread
From: Mimi Zohar @ 2024-01-24 14:07 UTC (permalink / raw)
  To: Tushar Sugandhi, roberto.sassu, roberto.sassu, eric.snowberg,
	stefanb, ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul


> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -121,6 +121,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_size;
>  
>  	/* use more understandable variable names than defined in kbuf */
>  	void *kexec_buffer = NULL;
> @@ -128,15 +129,19 @@ 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.
>  	 */

The memory is still being allocated at kexec "load",  so the extra memory is for
additional measurement records "since" kexec load.

Mimi

> -	binary_runtime_size = ima_get_binary_runtime_size();
> +	if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0)
> +		extra_size = PAGE_SIZE / 2;
> +	else
> +		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
> +	binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
> +
>  	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");


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
@ 2024-01-24 14:07     ` Mimi Zohar
  0 siblings, 0 replies; 52+ messages in thread
From: Mimi Zohar @ 2024-01-24 14:07 UTC (permalink / raw)
  To: Tushar Sugandhi, roberto.sassu, roberto.sassu, eric.snowberg,
	stefanb, ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul


> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -121,6 +121,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_size;
>  
>  	/* use more understandable variable names than defined in kbuf */
>  	void *kexec_buffer = NULL;
> @@ -128,15 +129,19 @@ 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.
>  	 */

The memory is still being allocated at kexec "load",  so the extra memory is for
additional measurement records "since" kexec load.

Mimi

> -	binary_runtime_size = ima_get_binary_runtime_size();
> +	if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0)
> +		extra_size = PAGE_SIZE / 2;
> +	else
> +		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
> +	binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
> +
>  	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");


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 7/7] ima: measure kexec load and exec events as critical data
  2024-01-22 18:38   ` Tushar Sugandhi
@ 2024-01-24 14:35     ` Mimi Zohar
  -1 siblings, 0 replies; 52+ messages in thread
From: Mimi Zohar @ 2024-01-24 14:35 UTC (permalink / raw)
  To: Tushar Sugandhi, roberto.sassu, roberto.sassu, eric.snowberg,
	stefanb, ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote:

The problem statement could be written as:

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.

Mimi

> There could be a potential mismatch between IMA measurements and TPM PCR
> quotes caused by the indeterminate interval between kexec 'load' and
> 'execute'.  The extra memory allocated at kexec 'load' for IMA log buffer
> may run out.  It can lead to missing events in the IMA log after a soft
> reboot to the new Kernel, resulting in TPM PCR quotes mismatch and remote
> attestation failures.
> 
> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be 
> measured as critical data at kexec 'load' and 'execute' respectively.
> 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.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 7/7] ima: measure kexec load and exec events as critical data
@ 2024-01-24 14:35     ` Mimi Zohar
  0 siblings, 0 replies; 52+ messages in thread
From: Mimi Zohar @ 2024-01-24 14:35 UTC (permalink / raw)
  To: Tushar Sugandhi, roberto.sassu, roberto.sassu, eric.snowberg,
	stefanb, ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote:

The problem statement could be written as:

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.

Mimi

> There could be a potential mismatch between IMA measurements and TPM PCR
> quotes caused by the indeterminate interval between kexec 'load' and
> 'execute'.  The extra memory allocated at kexec 'load' for IMA log buffer
> may run out.  It can lead to missing events in the IMA log after a soft
> reboot to the new Kernel, resulting in TPM PCR quotes mismatch and remote
> attestation failures.
> 
> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be 
> measured as critical data at kexec 'load' and 'execute' respectively.
> 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.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>



^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 4/7] ima: kexec: move ima log copy from kexec load to execute
  2024-01-22 18:38   ` Tushar Sugandhi
@ 2024-01-24 16:11     ` Mimi Zohar
  -1 siblings, 0 replies; 52+ messages in thread
From: Mimi Zohar @ 2024-01-24 16:11 UTC (permalink / raw)
  To: Tushar Sugandhi, roberto.sassu, roberto.sassu, eric.snowberg,
	stefanb, ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote:
> ima_dump_measurement_list() is called during kexec 'load', which may
> result in loss of IMA measurements during kexec soft reboot.  It needs
> to be called during kexec 'execute'.
> 
> The below changes need to be part of the same patch to ensure this
> patch series remains bisect-safe by ensuring the IMA log gets copied over
> during kexec soft reboot both before and after this patch.
> 
> Implement ima_update_kexec_buffer() to be called during kexec 'execute'.
> Move ima_dump_measurement_list() from ima_add_kexec_buffer() to
> ima_update_kexec_buffer().  Make the necessary variables local static to
> the file, so that they are accessible during both kexec 'load' - where
> the memory is allocated and mapped to a segment in the new Kernel, and
> during 'execute' - where the IMA log gets copied over.
> 
> Implement kimage_file_post_load() and ima_kexec_post_load() to be invoked
> after the new Kernel image has been loaded for kexec.
> ima_kexec_post_load() will map the IMA buffer to a segment in the newly
> loaded Kernel.  It will also register the reboot notifier_block to trigger
> ima_update_kexec_buffer() at exec 'execute'.

This defines two new IMA hooks - ima_kexec_post_load() and
ima_update_kexec_buffer().  They shouldn't be hidden here in the move of copying
the measurement list from kexec load to execute.

If "ima_update_kexec_buffer()" was initially defined as a stub function, the
infrastructure could be set up ahead of time.  This patch could then be limited
to just moving the copy from kexec "load" to "execute", by replacing the stub
function with the real function.

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

-- 
thanks,

Mimi


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 4/7] ima: kexec: move ima log copy from kexec load to execute
@ 2024-01-24 16:11     ` Mimi Zohar
  0 siblings, 0 replies; 52+ messages in thread
From: Mimi Zohar @ 2024-01-24 16:11 UTC (permalink / raw)
  To: Tushar Sugandhi, roberto.sassu, roberto.sassu, eric.snowberg,
	stefanb, ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote:
> ima_dump_measurement_list() is called during kexec 'load', which may
> result in loss of IMA measurements during kexec soft reboot.  It needs
> to be called during kexec 'execute'.
> 
> The below changes need to be part of the same patch to ensure this
> patch series remains bisect-safe by ensuring the IMA log gets copied over
> during kexec soft reboot both before and after this patch.
> 
> Implement ima_update_kexec_buffer() to be called during kexec 'execute'.
> Move ima_dump_measurement_list() from ima_add_kexec_buffer() to
> ima_update_kexec_buffer().  Make the necessary variables local static to
> the file, so that they are accessible during both kexec 'load' - where
> the memory is allocated and mapped to a segment in the new Kernel, and
> during 'execute' - where the IMA log gets copied over.
> 
> Implement kimage_file_post_load() and ima_kexec_post_load() to be invoked
> after the new Kernel image has been loaded for kexec.
> ima_kexec_post_load() will map the IMA buffer to a segment in the newly
> loaded Kernel.  It will also register the reboot notifier_block to trigger
> ima_update_kexec_buffer() at exec 'execute'.

This defines two new IMA hooks - ima_kexec_post_load() and
ima_update_kexec_buffer().  They shouldn't be hidden here in the move of copying
the measurement list from kexec load to execute.

If "ima_update_kexec_buffer()" was initially defined as a stub function, the
infrastructure could be set up ahead of time.  This patch could then be limited
to just moving the copy from kexec "load" to "execute", by replacing the stub
function with the real function.

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

-- 
thanks,

Mimi


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
  2024-01-24 13:33     ` Mimi Zohar
@ 2024-01-25 19:03       ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-25 19:03 UTC (permalink / raw)
  To: Mimi Zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

Thanks Mimi.


On 1/24/24 05:33, Mimi Zohar wrote:
> Hi Tushar,
> 
> On Mon, 2024-01-22 at 10:37 -0800, Tushar Sugandhi wrote:
> 
> Missing from this and the other patch descriptions is the problem
> description.  Please refer to the section titled  "Describe your changes" in
> https://docs.kernel.org/process/submitting-patches.html.
> 
> "Describe your problem.  Whether your patch is a one-line bug fix or 5000 lines
> of a new feature, there must be an underlying problem that motivated you to do
> this work.  Convince the reviewer that there is a problem worth fixing and that
> it makes sense for them to read past the first paragraph."
> 
> In this case, "why" you need to refactor ima_dump_measurement_list() is the
> problem.
> 
Thanks.  I will revisit all the patch descriptions in this series to 
take into account the 'why' specific to that particular patch.

> For example:
> 
> 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".
> 
Appreciate you giving an example in this case.
I will try to follow it in other patches too.

> "Once the problem is established, describe what you are actually doing about it
> in technical detail.  It's important to describe the change in plain English for
> the reviewer to verify that the code is behaving as you intend it to."
> 
>> Refactor ima_dump_measurement_list() to move the memory allocation part
>> 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 function ima_dump_measurement_list() as 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.
> 
> Please make this into an unordered list.
> 
Will do.

Thanks again.

~Tushar

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
@ 2024-01-25 19:03       ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-25 19:03 UTC (permalink / raw)
  To: Mimi Zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul

Thanks Mimi.


On 1/24/24 05:33, Mimi Zohar wrote:
> Hi Tushar,
> 
> On Mon, 2024-01-22 at 10:37 -0800, Tushar Sugandhi wrote:
> 
> Missing from this and the other patch descriptions is the problem
> description.  Please refer to the section titled  "Describe your changes" in
> https://docs.kernel.org/process/submitting-patches.html.
> 
> "Describe your problem.  Whether your patch is a one-line bug fix or 5000 lines
> of a new feature, there must be an underlying problem that motivated you to do
> this work.  Convince the reviewer that there is a problem worth fixing and that
> it makes sense for them to read past the first paragraph."
> 
> In this case, "why" you need to refactor ima_dump_measurement_list() is the
> problem.
> 
Thanks.  I will revisit all the patch descriptions in this series to 
take into account the 'why' specific to that particular patch.

> For example:
> 
> 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".
> 
Appreciate you giving an example in this case.
I will try to follow it in other patches too.

> "Once the problem is established, describe what you are actually doing about it
> in technical detail.  It's important to describe the change in plain English for
> the reviewer to verify that the code is behaving as you intend it to."
> 
>> Refactor ima_dump_measurement_list() to move the memory allocation part
>> 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 function ima_dump_measurement_list() as 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.
> 
> Please make this into an unordered list.
> 
Will do.

Thanks again.

~Tushar

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 4/7] ima: kexec: move ima log copy from kexec load to execute
  2024-01-24 16:11     ` Mimi Zohar
@ 2024-01-25 19:06       ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-25 19:06 UTC (permalink / raw)
  To: Mimi Zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul



On 1/24/24 08:11, Mimi Zohar wrote:
> On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote:
>> ima_dump_measurement_list() is called during kexec 'load', which may
>> result in loss of IMA measurements during kexec soft reboot.  It needs
>> to be called during kexec 'execute'.
>>
>> The below changes need to be part of the same patch to ensure this
>> patch series remains bisect-safe by ensuring the IMA log gets copied over
>> during kexec soft reboot both before and after this patch.
>>
>> Implement ima_update_kexec_buffer() to be called during kexec 'execute'.
>> Move ima_dump_measurement_list() from ima_add_kexec_buffer() to
>> ima_update_kexec_buffer().  Make the necessary variables local static to
>> the file, so that they are accessible during both kexec 'load' - where
>> the memory is allocated and mapped to a segment in the new Kernel, and
>> during 'execute' - where the IMA log gets copied over.
>>
>> Implement kimage_file_post_load() and ima_kexec_post_load() to be invoked
>> after the new Kernel image has been loaded for kexec.
>> ima_kexec_post_load() will map the IMA buffer to a segment in the newly
>> loaded Kernel.  It will also register the reboot notifier_block to trigger
>> ima_update_kexec_buffer() at exec 'execute'.
> 
> This defines two new IMA hooks - ima_kexec_post_load() and
> ima_update_kexec_buffer().  They shouldn't be hidden here in the move of copying
> the measurement list from kexec load to execute.
> 
> If "ima_update_kexec_buffer()" was initially defined as a stub function, the
> infrastructure could be set up ahead of time.  This patch could then be limited
> to just moving the copy from kexec "load" to "execute", by replacing the stub
> function with the real function.
> 
Agreed.  Making ima_kexec_post_load() and ima_update_kexec_buffer() as 
stubs/hooks did cross my mind.  Thanks for confirming that.

I will split this patch (4/7) into two.

First will define the stubs, setup the infrastructure.
And second will move the copy from 'load' to 'execute'.

~Tushar
>> Modify kexec_file_load() syscall to call kimage_file_post_load() after the
>> image has been loaded and prepared for kexec.  Call it only on kexec soft
>> reboot and not for KEXEC_FILE_ON_CRASH.
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 4/7] ima: kexec: move ima log copy from kexec load to execute
@ 2024-01-25 19:06       ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-25 19:06 UTC (permalink / raw)
  To: Mimi Zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul



On 1/24/24 08:11, Mimi Zohar wrote:
> On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote:
>> ima_dump_measurement_list() is called during kexec 'load', which may
>> result in loss of IMA measurements during kexec soft reboot.  It needs
>> to be called during kexec 'execute'.
>>
>> The below changes need to be part of the same patch to ensure this
>> patch series remains bisect-safe by ensuring the IMA log gets copied over
>> during kexec soft reboot both before and after this patch.
>>
>> Implement ima_update_kexec_buffer() to be called during kexec 'execute'.
>> Move ima_dump_measurement_list() from ima_add_kexec_buffer() to
>> ima_update_kexec_buffer().  Make the necessary variables local static to
>> the file, so that they are accessible during both kexec 'load' - where
>> the memory is allocated and mapped to a segment in the new Kernel, and
>> during 'execute' - where the IMA log gets copied over.
>>
>> Implement kimage_file_post_load() and ima_kexec_post_load() to be invoked
>> after the new Kernel image has been loaded for kexec.
>> ima_kexec_post_load() will map the IMA buffer to a segment in the newly
>> loaded Kernel.  It will also register the reboot notifier_block to trigger
>> ima_update_kexec_buffer() at exec 'execute'.
> 
> This defines two new IMA hooks - ima_kexec_post_load() and
> ima_update_kexec_buffer().  They shouldn't be hidden here in the move of copying
> the measurement list from kexec load to execute.
> 
> If "ima_update_kexec_buffer()" was initially defined as a stub function, the
> infrastructure could be set up ahead of time.  This patch could then be limited
> to just moving the copy from kexec "load" to "execute", by replacing the stub
> function with the real function.
> 
Agreed.  Making ima_kexec_post_load() and ima_update_kexec_buffer() as 
stubs/hooks did cross my mind.  Thanks for confirming that.

I will split this patch (4/7) into two.

First will define the stubs, setup the infrastructure.
And second will move the copy from 'load' to 'execute'.

~Tushar
>> Modify kexec_file_load() syscall to call kimage_file_post_load() after the
>> image has been loaded and prepared for kexec.  Call it only on kexec soft
>> reboot and not for KEXEC_FILE_ON_CRASH.
> 

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
  2024-01-24 14:07     ` Mimi Zohar
@ 2024-01-25 19:14       ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-25 19:14 UTC (permalink / raw)
  To: Mimi Zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul



On 1/24/24 06:07, Mimi Zohar wrote:
> 
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -121,6 +121,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_size;
>>   
>>   	/* use more understandable variable names than defined in kbuf */
>>   	void *kexec_buffer = NULL;
>> @@ -128,15 +129,19 @@ 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.
>>   	 */
> 
> The memory is still being allocated at kexec "load",  so the extra memory is for
> additional measurement records "since" kexec load.
> 
> Mimi
> 
This wording was an attempt to address the comment in v3[1].
So I tried to make the comment generic.  But maybe I made it too generic.
I will update.

[1] Re: [PATCH v3 6/7] ima: configure memory to log events between kexec 
load and execute
https://lore.kernel.org/all/fbe6aa7577875b23a9913a39f858f06f1d2aa903.camel@linux.ibm.com/

"Additional records could be added as a result of the kexec
load itself.
...
Please remove any references to measurements between kexec load and
execute."

~Tushar

>> -	binary_runtime_size = ima_get_binary_runtime_size();
>> +	if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0)
>> +		extra_size = PAGE_SIZE / 2;
>> +	else
>> +		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
>> +	binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
>> +
>>   	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");
> 

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 6/7] ima: make the kexec extra memory configurable
@ 2024-01-25 19:14       ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-25 19:14 UTC (permalink / raw)
  To: Mimi Zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul



On 1/24/24 06:07, Mimi Zohar wrote:
> 
>> --- a/security/integrity/ima/ima_kexec.c
>> +++ b/security/integrity/ima/ima_kexec.c
>> @@ -121,6 +121,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_size;
>>   
>>   	/* use more understandable variable names than defined in kbuf */
>>   	void *kexec_buffer = NULL;
>> @@ -128,15 +129,19 @@ 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.
>>   	 */
> 
> The memory is still being allocated at kexec "load",  so the extra memory is for
> additional measurement records "since" kexec load.
> 
> Mimi
> 
This wording was an attempt to address the comment in v3[1].
So I tried to make the comment generic.  But maybe I made it too generic.
I will update.

[1] Re: [PATCH v3 6/7] ima: configure memory to log events between kexec 
load and execute
https://lore.kernel.org/all/fbe6aa7577875b23a9913a39f858f06f1d2aa903.camel@linux.ibm.com/

"Additional records could be added as a result of the kexec
load itself.
...
Please remove any references to measurements between kexec load and
execute."

~Tushar

>> -	binary_runtime_size = ima_get_binary_runtime_size();
>> +	if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0)
>> +		extra_size = PAGE_SIZE / 2;
>> +	else
>> +		extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024;
>> +	binary_runtime_size = ima_get_binary_runtime_size() + extra_size;
>> +
>>   	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");
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 7/7] ima: measure kexec load and exec events as critical data
  2024-01-24 14:35     ` Mimi Zohar
@ 2024-01-25 19:16       ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-25 19:16 UTC (permalink / raw)
  To: Mimi Zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul



On 1/24/24 06:35, Mimi Zohar wrote:
> On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote:
> 
> The problem statement could be written as:
> 
> 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.
> 
> Mimi
> 
Thank you again for taking efforts to write one more example for me.
Appreciate it.

Will do.

~Tushar

>> There could be a potential mismatch between IMA measurements and TPM PCR
>> quotes caused by the indeterminate interval between kexec 'load' and
>> 'execute'.  The extra memory allocated at kexec 'load' for IMA log buffer
>> may run out.  It can lead to missing events in the IMA log after a soft
>> reboot to the new Kernel, resulting in TPM PCR quotes mismatch and remote
>> attestation failures.
>>
>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
>> measured as critical data at kexec 'load' and 'execute' respectively.
>> 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.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 7/7] ima: measure kexec load and exec events as critical data
@ 2024-01-25 19:16       ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-25 19:16 UTC (permalink / raw)
  To: Mimi Zohar, roberto.sassu, roberto.sassu, eric.snowberg, stefanb,
	ebiederm, noodles, bauermann, linux-integrity, kexec
  Cc: code, nramas, paul



On 1/24/24 06:35, Mimi Zohar wrote:
> On Mon, 2024-01-22 at 10:38 -0800, Tushar Sugandhi wrote:
> 
> The problem statement could be written as:
> 
> 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.
> 
> Mimi
> 
Thank you again for taking efforts to write one more example for me.
Appreciate it.

Will do.

~Tushar

>> There could be a potential mismatch between IMA measurements and TPM PCR
>> quotes caused by the indeterminate interval between kexec 'load' and
>> 'execute'.  The extra memory allocated at kexec 'load' for IMA log buffer
>> may run out.  It can lead to missing events in the IMA log after a soft
>> reboot to the new Kernel, resulting in TPM PCR quotes mismatch and remote
>> attestation failures.
>>
>> Define two new IMA events, 'kexec_load' and 'kexec_execute', to be
>> measured as critical data at kexec 'load' and 'execute' respectively.
>> 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.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> 

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
  2024-01-24  3:38       ` Stefan Berger
@ 2024-01-26 22:14         ` Tushar Sugandhi
  -1 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-26 22:14 UTC (permalink / raw)
  To: Stefan Berger, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul

Thanks for catching this Stefan.

On 1/23/24 19:38, Stefan Berger wrote:
>>>       kbuf.buffer = kexec_buffer;
>>>       kbuf.bufsz = kexec_buffer_size;
>>>       kbuf.memsz = kexec_segment_size;
>>
>>
>> A dent with this patch when only applying this patch:
>>
>> Two consecutive kexec loads lead to this here:
>>
>> [   30.670330] IMA buffer at 0x3fff10000, size = 0xf0000
>> [   32.519618] ------------[ cut here ]------------
>> [   32.519669] Trying to vfree() nonexistent vm area (00000000093ae29c)
>> [   32.519762] WARNING: CPU: 11 PID: 1796 at mm/vmalloc.c:2826 
>> vfree+0x254/0x340
>> [   32.519786] Modules linked in: bonding tls rfkill sunrpc 
>> virtio_console virtio_balloon crct10dif_vpmsum fuse loop zram bochs 
>> drm_vram_helper drm_kms_helper drm_ttm_helper ttm ibmvscsi 
>> scsi_transport_srp drm virtio_blk virtio_net vmx_crypto net_failover 
>> crc32c_vpmsum failover pseries_wdt drm_panel_orientation_quirks 
>> scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_multipath
>> [   32.519939] CPU: 11 PID: 1796 Comm: kexec Not tainted 6.5.0+ #112
>> [   32.519953] Hardware name: IBM pSeries (emulated by qemu) POWER8E 
>> (raw) 0x4b0201 0xf000004 of:SLOF,git-5b4c5a hv:linux,kvm pSeries
>> [   32.519973] NIP:  c0000000004bd004 LR: c0000000004bd000 CTR: 
>> c00000000017ef00
>> [   32.519986] REGS: c00000004593b670 TRAP: 0700   Not tainted  (6.5.0+)
>> [   32.519999] MSR:  8000000002029033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 
>> 44424842  XER: 00000000
>> [   32.520023] CFAR: c0000000001515b0 IRQMASK: 0
>>                 GPR00: c0000000004bd000 c00000004593b910 
>> c000000001e17000 0000000000000038
>>                 GPR04: 00000000ffffbfff c00000004593b6e8 
>> c00000004593b6e0 00000003f9580000
>>                 GPR08: 0000000000000027 c0000003fb707010 
>> 0000000000000001 0000000044424842
>>                 GPR12: c00000000017ef00 c00000003fff1300 
>> 0000000000000000 0000000000000000
>>                 GPR16: 0000000000000000 0000000000000000 
>> 0000000000000000 0000000000000000
>>                 GPR20: 0000000000000000 0000000000000000 
>> 0000000000000003 0000000000000004
>>                 GPR24: 00007fffeab0f68f 000000000000004c 
>> 0000000000000000 c00000002bdce400
>>                 GPR28: c000000002bf28f0 0000000000000000 
>> c008000004770000 0000000000000000
>> [   32.520180] NIP [c0000000004bd004] vfree+0x254/0x340
>> [   32.520212] LR [c0000000004bd000] vfree+0x250/0x340
>> [   32.520225] Call Trace:
>> [   32.520232] [c00000004593b910] [c0000000004bd000] vfree+0x250/0x340 
>> (unreliable)
>> [   32.520250] [c00000004593b990] [c00000000091d590] 
>> ima_add_kexec_buffer+0xe0/0x3c0
>> [   32.520296] [c00000004593ba90] [c000000000280968] 
>> sys_kexec_file_load+0x148/0x9b0
>> [   32.520333] [c00000004593bb70] [c00000000002ea84] 
>> system_call_exception+0x174/0x320
>> [   32.520372] [c00000004593be50] [c00000000000d6a0] 
>> system_call_common+0x160/0x2c4
>> [   32.520408] --- interrupt: c00 at 0x7fffa52e7ae4
>> [   32.520420] NIP:  00007fffa52e7ae4 LR: 0000000108481d8c CTR: 
>> 0000000000000000
>> [   32.520452] REGS: c00000004593be80 TRAP: 0c00   Not tainted  (6.5.0+)
>> [   32.520483] MSR:  800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE>  
>> CR: 24424202  XER: 00000000
>> [   32.520507] IRQMASK: 0
>>                 GPR00: 000000000000017e 00007fffeab09470 
>> 00007fffa53f6f00 0000000000000003
>>                 GPR04: 0000000000000004 000000000000004c 
>> 00007fffeab0f68f 0000000000000000
>>                 GPR08: 0000000000000000 0000000000000000 
>> 0000000000000000 0000000000000000
>>                 GPR12: 0000000000000000 00007fffa559b280 
>> 0000000000000002 0000000000000001
>>                 GPR16: 0000000000000000 0000000000000000 
>> 0000000000000000 0000000000000000
>>                 GPR20: 00007fffa53f0454 00007fffa53f0458 
>> 0000000000000000 0000000000000001
>>                 GPR24: 0000000000000000 00007fffeab0f64d 
>> 0000000000000006 0000000000000000
>>                 GPR28: 0000000000000003 00007fffeab09530 
>> 00007fffeab09b08 0000000000000007
>> [   32.520767] NIP [00007fffa52e7ae4] 0x7fffa52e7ae4
>> [   32.521192] LR [0000000108481d8c] 0x108481d8c
>> [   32.521587] --- interrupt: c00
>> [   32.521981] Code: 3884c208 4bfc20f1 60000000 0fe00000 60000000 
>> 60000000 60420000 3c62ff94 7fc4f378 38632b20 4bc944cd 60000000 
>> <0fe00000> eba10068 4bffff34 2c080000
>> [   32.522823] ---[ end trace 0000000000000000 ]---
>> [   32.536347] Removed old IMA buffer reservation.
>> [   32.536473] IMA buffer at 0x3fff10000, size = 0xf0000
>>
>> This vfree here probably has to go:
>>
>>          ret = kexec_add_buffer(&kbuf);
>>          if (ret) {
>>                  pr_err("Error passing over kexec measurement 
>> buffer.\n");
>>                  vfree(kexec_buffer);
>>                  return;
>>          }
>>
> 
> The vfree may need to be removed or replaced with 
> ima_free_kexec_file_buf() but it doesn't seem to solve the problem alone.
> I got rid of this issue later with this:
> 
> 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);
> }
> 
> [...]
> 
I was able to repro the issue by calling kexec 'load' multiple times on 
patch #1.  Good catch, thanks.

Earlier I was testing the multiple 'load' scenario on the last patch only.
Here onward I will call it on each of the patch individually.

> @@ -170,6 +175,9 @@ 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 since kexec_add_buffer and will 
> vfree() it */
> +       ima_reset_kexec_file(&ima_kexec_file);
> +
>          pr_debug("kexec measurement buffer for the loaded kernel at 
> 0x%lx.\n",
>                   kbuf.mem);
> 
> 
Thanks for the suggested solution. Looks like you applied it on top of
patch #3.
I applied it on patch #1, and it seems to be working.

I will dig deeper to ensure it doesn't cause any memory leaks, and will 
incorporate it in v5.

Thanks again.

~Tushar

>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf
@ 2024-01-26 22:14         ` Tushar Sugandhi
  0 siblings, 0 replies; 52+ messages in thread
From: Tushar Sugandhi @ 2024-01-26 22:14 UTC (permalink / raw)
  To: Stefan Berger, zohar, roberto.sassu, roberto.sassu,
	eric.snowberg, ebiederm, noodles, bauermann, linux-integrity,
	kexec
  Cc: code, nramas, paul

Thanks for catching this Stefan.

On 1/23/24 19:38, Stefan Berger wrote:
>>>       kbuf.buffer = kexec_buffer;
>>>       kbuf.bufsz = kexec_buffer_size;
>>>       kbuf.memsz = kexec_segment_size;
>>
>>
>> A dent with this patch when only applying this patch:
>>
>> Two consecutive kexec loads lead to this here:
>>
>> [   30.670330] IMA buffer at 0x3fff10000, size = 0xf0000
>> [   32.519618] ------------[ cut here ]------------
>> [   32.519669] Trying to vfree() nonexistent vm area (00000000093ae29c)
>> [   32.519762] WARNING: CPU: 11 PID: 1796 at mm/vmalloc.c:2826 
>> vfree+0x254/0x340
>> [   32.519786] Modules linked in: bonding tls rfkill sunrpc 
>> virtio_console virtio_balloon crct10dif_vpmsum fuse loop zram bochs 
>> drm_vram_helper drm_kms_helper drm_ttm_helper ttm ibmvscsi 
>> scsi_transport_srp drm virtio_blk virtio_net vmx_crypto net_failover 
>> crc32c_vpmsum failover pseries_wdt drm_panel_orientation_quirks 
>> scsi_dh_rdac scsi_dh_emc scsi_dh_alua dm_multipath
>> [   32.519939] CPU: 11 PID: 1796 Comm: kexec Not tainted 6.5.0+ #112
>> [   32.519953] Hardware name: IBM pSeries (emulated by qemu) POWER8E 
>> (raw) 0x4b0201 0xf000004 of:SLOF,git-5b4c5a hv:linux,kvm pSeries
>> [   32.519973] NIP:  c0000000004bd004 LR: c0000000004bd000 CTR: 
>> c00000000017ef00
>> [   32.519986] REGS: c00000004593b670 TRAP: 0700   Not tainted  (6.5.0+)
>> [   32.519999] MSR:  8000000002029033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 
>> 44424842  XER: 00000000
>> [   32.520023] CFAR: c0000000001515b0 IRQMASK: 0
>>                 GPR00: c0000000004bd000 c00000004593b910 
>> c000000001e17000 0000000000000038
>>                 GPR04: 00000000ffffbfff c00000004593b6e8 
>> c00000004593b6e0 00000003f9580000
>>                 GPR08: 0000000000000027 c0000003fb707010 
>> 0000000000000001 0000000044424842
>>                 GPR12: c00000000017ef00 c00000003fff1300 
>> 0000000000000000 0000000000000000
>>                 GPR16: 0000000000000000 0000000000000000 
>> 0000000000000000 0000000000000000
>>                 GPR20: 0000000000000000 0000000000000000 
>> 0000000000000003 0000000000000004
>>                 GPR24: 00007fffeab0f68f 000000000000004c 
>> 0000000000000000 c00000002bdce400
>>                 GPR28: c000000002bf28f0 0000000000000000 
>> c008000004770000 0000000000000000
>> [   32.520180] NIP [c0000000004bd004] vfree+0x254/0x340
>> [   32.520212] LR [c0000000004bd000] vfree+0x250/0x340
>> [   32.520225] Call Trace:
>> [   32.520232] [c00000004593b910] [c0000000004bd000] vfree+0x250/0x340 
>> (unreliable)
>> [   32.520250] [c00000004593b990] [c00000000091d590] 
>> ima_add_kexec_buffer+0xe0/0x3c0
>> [   32.520296] [c00000004593ba90] [c000000000280968] 
>> sys_kexec_file_load+0x148/0x9b0
>> [   32.520333] [c00000004593bb70] [c00000000002ea84] 
>> system_call_exception+0x174/0x320
>> [   32.520372] [c00000004593be50] [c00000000000d6a0] 
>> system_call_common+0x160/0x2c4
>> [   32.520408] --- interrupt: c00 at 0x7fffa52e7ae4
>> [   32.520420] NIP:  00007fffa52e7ae4 LR: 0000000108481d8c CTR: 
>> 0000000000000000
>> [   32.520452] REGS: c00000004593be80 TRAP: 0c00   Not tainted  (6.5.0+)
>> [   32.520483] MSR:  800000000000f033 <SF,EE,PR,FP,ME,IR,DR,RI,LE>  
>> CR: 24424202  XER: 00000000
>> [   32.520507] IRQMASK: 0
>>                 GPR00: 000000000000017e 00007fffeab09470 
>> 00007fffa53f6f00 0000000000000003
>>                 GPR04: 0000000000000004 000000000000004c 
>> 00007fffeab0f68f 0000000000000000
>>                 GPR08: 0000000000000000 0000000000000000 
>> 0000000000000000 0000000000000000
>>                 GPR12: 0000000000000000 00007fffa559b280 
>> 0000000000000002 0000000000000001
>>                 GPR16: 0000000000000000 0000000000000000 
>> 0000000000000000 0000000000000000
>>                 GPR20: 00007fffa53f0454 00007fffa53f0458 
>> 0000000000000000 0000000000000001
>>                 GPR24: 0000000000000000 00007fffeab0f64d 
>> 0000000000000006 0000000000000000
>>                 GPR28: 0000000000000003 00007fffeab09530 
>> 00007fffeab09b08 0000000000000007
>> [   32.520767] NIP [00007fffa52e7ae4] 0x7fffa52e7ae4
>> [   32.521192] LR [0000000108481d8c] 0x108481d8c
>> [   32.521587] --- interrupt: c00
>> [   32.521981] Code: 3884c208 4bfc20f1 60000000 0fe00000 60000000 
>> 60000000 60420000 3c62ff94 7fc4f378 38632b20 4bc944cd 60000000 
>> <0fe00000> eba10068 4bffff34 2c080000
>> [   32.522823] ---[ end trace 0000000000000000 ]---
>> [   32.536347] Removed old IMA buffer reservation.
>> [   32.536473] IMA buffer at 0x3fff10000, size = 0xf0000
>>
>> This vfree here probably has to go:
>>
>>          ret = kexec_add_buffer(&kbuf);
>>          if (ret) {
>>                  pr_err("Error passing over kexec measurement 
>> buffer.\n");
>>                  vfree(kexec_buffer);
>>                  return;
>>          }
>>
> 
> The vfree may need to be removed or replaced with 
> ima_free_kexec_file_buf() but it doesn't seem to solve the problem alone.
> I got rid of this issue later with this:
> 
> 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);
> }
> 
> [...]
> 
I was able to repro the issue by calling kexec 'load' multiple times on 
patch #1.  Good catch, thanks.

Earlier I was testing the multiple 'load' scenario on the last patch only.
Here onward I will call it on each of the patch individually.

> @@ -170,6 +175,9 @@ 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 since kexec_add_buffer and will 
> vfree() it */
> +       ima_reset_kexec_file(&ima_kexec_file);
> +
>          pr_debug("kexec measurement buffer for the loaded kernel at 
> 0x%lx.\n",
>                   kbuf.mem);
> 
> 
Thanks for the suggested solution. Looks like you applied it on top of
patch #3.
I applied it on patch #1, and it seems to be working.

I will dig deeper to ensure it doesn't cause any memory leaks, and will 
incorporate it in v5.

Thanks again.

~Tushar

>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec 

^ permalink raw reply	[flat|nested] 52+ messages in thread
end of thread, other threads:[~2024-01-26 22:14 UTC | newest]

Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-22 18:37 [PATCH v4 0/7] ima: kexec: measure events between kexec load and execute Tushar Sugandhi
2024-01-22 18:37 ` Tushar Sugandhi
2024-01-22 18:37 ` [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf Tushar Sugandhi
2024-01-22 18:37   ` Tushar Sugandhi
2024-01-24  2:54   ` Stefan Berger
2024-01-24  2:54     ` Stefan Berger
2024-01-24  3:38     ` Stefan Berger
2024-01-24  3:38       ` Stefan Berger
2024-01-26 22:14       ` Tushar Sugandhi
2024-01-26 22:14         ` Tushar Sugandhi
2024-01-24 13:33   ` Mimi Zohar
2024-01-24 13:33     ` Mimi Zohar
2024-01-25 19:03     ` Tushar Sugandhi
2024-01-25 19:03       ` Tushar Sugandhi
2024-01-22 18:37 ` [PATCH v4 2/7] kexec: define functions to map and unmap segments Tushar Sugandhi
2024-01-22 18:37   ` Tushar Sugandhi
2024-01-23 17:03   ` Stefan Berger
2024-01-23 17:03     ` Stefan Berger
2024-01-23 20:39     ` Tushar Sugandhi
2024-01-23 20:39       ` Tushar Sugandhi
2024-01-22 18:38 ` [PATCH v4 3/7] ima: kexec: skip IMA segment validation after kexec soft reboot Tushar Sugandhi
2024-01-22 18:38   ` Tushar Sugandhi
2024-01-22 18:38 ` [PATCH v4 4/7] ima: kexec: move ima log copy from kexec load to execute Tushar Sugandhi
2024-01-22 18:38   ` Tushar Sugandhi
2024-01-24 16:11   ` Mimi Zohar
2024-01-24 16:11     ` Mimi Zohar
2024-01-25 19:06     ` Tushar Sugandhi
2024-01-25 19:06       ` Tushar Sugandhi
2024-01-22 18:38 ` [PATCH v4 5/7] ima: suspend measurements during buffer copy at kexec execute Tushar Sugandhi
2024-01-22 18:38   ` Tushar Sugandhi
2024-01-23 18:18   ` Stefan Berger
2024-01-23 18:18     ` Stefan Berger
2024-01-23 20:55     ` Tushar Sugandhi
2024-01-23 20:55       ` Tushar Sugandhi
2024-01-22 18:38 ` [PATCH v4 6/7] ima: make the kexec extra memory configurable Tushar Sugandhi
2024-01-22 18:38   ` Tushar Sugandhi
2024-01-23 19:02   ` Stefan Berger
2024-01-23 19:02     ` Stefan Berger
2024-01-23 21:19     ` Tushar Sugandhi
2024-01-23 21:19       ` Tushar Sugandhi
2024-01-24  1:48       ` Stefan Berger
2024-01-24  1:48         ` Stefan Berger
2024-01-24 14:07   ` Mimi Zohar
2024-01-24 14:07     ` Mimi Zohar
2024-01-25 19:14     ` Tushar Sugandhi
2024-01-25 19:14       ` Tushar Sugandhi
2024-01-22 18:38 ` [PATCH v4 7/7] ima: measure kexec load and exec events as critical data Tushar Sugandhi
2024-01-22 18:38   ` Tushar Sugandhi
2024-01-24 14:35   ` Mimi Zohar
2024-01-24 14:35     ` Mimi Zohar
2024-01-25 19:16     ` Tushar Sugandhi
2024-01-25 19:16       ` Tushar Sugandhi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.