On 04/08/25 at 08:23am, Mimi Zohar wrote:

On Tue, 2025-04-08 at 16:18 +0800, Baoquan He wrote:

quoted

On 04/08/25 at 01:03am, Mimi Zohar wrote:

quoted

On Tue, 2025-04-08 at 12:39 +0800, Baoquan He wrote:

quoted

On 04/08/25 at 12:07am, Mimi Zohar wrote:

quoted

On Wed, 2025-04-02 at 05:47 -0700, steven chen wrote:

quoted

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

Repeating this here is unnecessary.

quoted

To allow the buffer allocation and population to be separated into distinct
steps, make the function local seq_file "ima_kexec_file" to a file variable.

This change was already made in [PATCH v11 1/9] ima: rename variable the
set_file "file" to "ima_kexec_file".  Please remove.

quoted

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

Signed-off-by: Tushar Sugandhi <redacted>
Signed-off-by: steven chen <redacted>
---
 security/integrity/ima/ima_kexec.c | 46 +++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 650beb74346c..b12ac3619b8f 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c

@@ -15,26 +15,46 @@
 #include "ima.h"
 
 #ifdef CONFIG_IMA_KEXEC
+static struct seq_file ima_kexec_file;
+
+static void ima_free_kexec_file_buf(struct seq_file *sf)
+{
+	vfree(sf->buf);
+	sf->buf = NULL;
+	sf->size = 0;
+	sf->read_pos = 0;
+	sf->count = 0;
+}
+
+static int ima_alloc_kexec_file_buf(size_t segment_size)
+{
+	ima_free_kexec_file_buf(&ima_kexec_file);

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

I also noticed this, patch 7 will remedy this. Put patch 7 just after
this patch or squash it into this patch?

[PATCH v11 7/9] ima: verify if the segment size has changed

I'm glad you noticed this too!  I've been staring at it for a while, not knowing
what to do.

"ima: verify if the segment size has changed" is new to v11.  It was originally
part of this patch.  My comment on v10 was:

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

and

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

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

Thanks for deliberating on this and the details sharing, Mimi.

It could be fine if we add note in patch 2 log to mention the possible
failure. With my understanding, commit/patch bisectable means it won't
break compiling and block the testing. The failure you are concerned
about is not a blocker, right? And people won't back port partial
patches of this series.

Nore sure if there's another better way we can take or detour.

Right, doing two consecutive kexec loads in a row is not common and won't block
testing.  Patch readability is more important, in this case, at least to me. 
I'm fine with your suggestion.

That's great, thanks for confirming.