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

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.

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 <tusharsu@linux.microsoft.com>
Signed-off-by: steven chen <chenste@linux.microsoft.com>
---
 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);
+
+	/* segment size can't change between kexec load and execute */
+	ima_kexec_file.buf = vmalloc(segment_size);
+	if (!ima_kexec_file.buf)
+		return -ENOMEM;
+
+	ima_kexec_file.size = segment_size;
+	ima_kexec_file.read_pos = 0;
+	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
+
+	return 0;
+}
+
 static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 				     unsigned long segment_size)
 {
-	struct seq_file ima_kexec_file;
 	struct ima_queue_entry *qe;
 	struct ima_kexec_hdr khdr;
 	int ret = 0;
 
 	/* segment size can't change between kexec load and execute */
-	ima_kexec_file.buf = vmalloc(segment_size);
 	if (!ima_kexec_file.buf) {
-		ret = -ENOMEM;
-		goto out;
+		pr_err("Kexec file buf not allocated\n");
+		return -EINVAL;
 	}
 
-	ima_kexec_file.file = NULL;
-	ima_kexec_file.size = segment_size;
-	ima_kexec_file.read_pos = 0;
-	ima_kexec_file.count = sizeof(khdr);	/* reserved space */
-
 	memset(&khdr, 0, sizeof(khdr));
 	khdr.version = 1;
 	/* This is an append-only list, no need to hold the RCU read lock */
@@ -71,8 +91,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 	*buffer_size = ima_kexec_file.count;
 	*buffer = ima_kexec_file.buf;
 out:
-	if (ret == -EINVAL)
-		vfree(ima_kexec_file.buf);
 	return ret;
 }
 
@@ -111,6 +129,12 @@ void ima_add_kexec_buffer(struct kimage *image)
 		return;
 	}
 
+	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
+	if (ret < 0) {
+		pr_err("Not enough memory for the kexec measurement buffer.\n");
+		return;
+	}
+
 	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
 				  kexec_segment_size);
 	if (!kexec_buffer) {
-- 
2.25.1

On Wed, 2025-04-02 at 05:47 -0700, steven chen wrote:
> 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.
> 
> 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.

> 
> 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 <tusharsu@linux.microsoft.com>
> Signed-off-by: steven chen <chenste@linux.microsoft.com>
> ---
>  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.

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

On 04/08/25 at 12:07am, Mimi Zohar wrote:
> On Wed, 2025-04-02 at 05:47 -0700, steven chen wrote:
> > 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.
> > 
> > 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.
> 
> > 
> > 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 <tusharsu@linux.microsoft.com>
> > Signed-off-by: steven chen <chenste@linux.microsoft.com>
> > ---
> >  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

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

On Tue, 2025-04-08 at 12:39 +0800, Baoquan He wrote:
> On 04/08/25 at 12:07am, Mimi Zohar wrote:
> > On Wed, 2025-04-02 at 05:47 -0700, steven chen wrote:
> > > 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.
> > > 
> > > 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.
> > 
> > > 
> > > 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 <tusharsu@linux.microsoft.com>
> > > Signed-off-by: steven chen <chenste@linux.microsoft.com>
> > > ---
> > >  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().

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

On 04/08/25 at 01:03am, Mimi Zohar wrote:
> On Tue, 2025-04-08 at 12:39 +0800, Baoquan He wrote:
> > On 04/08/25 at 12:07am, Mimi Zohar wrote:
> > > On Wed, 2025-04-02 at 05:47 -0700, steven chen wrote:
> > > > 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.
> > > > 
> > > > 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.
> > > 
> > > > 
> > > > 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 <tusharsu@linux.microsoft.com>
> > > > Signed-off-by: steven chen <chenste@linux.microsoft.com>
> > > > ---
> > > >  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.

Thanks
Baoquan
> 
> > 
> > > 
> > > > +
> > > > +	/* 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;
> > > > +}
> > > > +
> > > 
> > 
> > 
>

On Tue, 2025-04-08 at 16:18 +0800, Baoquan He wrote:
> On 04/08/25 at 01:03am, Mimi Zohar wrote:
> > On Tue, 2025-04-08 at 12:39 +0800, Baoquan He wrote:
> > > On 04/08/25 at 12:07am, Mimi Zohar wrote:
> > > > On Wed, 2025-04-02 at 05:47 -0700, steven chen wrote:
> > > > > 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.
> > > > > 
> > > > > 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.
> > > > 
> > > > > 
> > > > > 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 <tusharsu@linux.microsoft.com>
> > > > > Signed-off-by: steven chen <chenste@linux.microsoft.com>
> > > > > ---
> > > > >  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.

Thanks, Boaquan.

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

On 04/08/25 at 08:23am, Mimi Zohar wrote:
> On Tue, 2025-04-08 at 16:18 +0800, Baoquan He wrote:
> > On 04/08/25 at 01:03am, Mimi Zohar wrote:
> > > On Tue, 2025-04-08 at 12:39 +0800, Baoquan He wrote:
> > > > On 04/08/25 at 12:07am, Mimi Zohar wrote:
> > > > > On Wed, 2025-04-02 at 05:47 -0700, steven chen wrote:
> > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > > 
> > > > > > 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 <tusharsu@linux.microsoft.com>
> > > > > > Signed-off-by: steven chen <chenste@linux.microsoft.com>
> > > > > > ---
> > > > > >  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.

On 04/02/25 at 05:47am, steven chen wrote:
> 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
> 
> 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.
> 
> 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 <tusharsu@linux.microsoft.com>
> Signed-off-by: steven chen <chenste@linux.microsoft.com>
> ---
>  security/integrity/ima/ima_kexec.c | 46 +++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 11 deletions(-)

LGTM,

Acked-by: Baoquan He <bhe@redhat.com>

> 
> 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);
> +
> +	/* segment size can't change between kexec load and execute */
> +	ima_kexec_file.buf = vmalloc(segment_size);
> +	if (!ima_kexec_file.buf)
> +		return -ENOMEM;
> +
> +	ima_kexec_file.size = segment_size;
> +	ima_kexec_file.read_pos = 0;
> +	ima_kexec_file.count = sizeof(struct ima_kexec_hdr);	/* reserved space */
> +
> +	return 0;
> +}
> +
>  static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
>  				     unsigned long segment_size)
>  {
> -	struct seq_file ima_kexec_file;
>  	struct ima_queue_entry *qe;
>  	struct ima_kexec_hdr khdr;
>  	int ret = 0;
>  
>  	/* segment size can't change between kexec load and execute */
> -	ima_kexec_file.buf = vmalloc(segment_size);
>  	if (!ima_kexec_file.buf) {
> -		ret = -ENOMEM;
> -		goto out;
> +		pr_err("Kexec file buf not allocated\n");
> +		return -EINVAL;
>  	}
>  
> -	ima_kexec_file.file = NULL;
> -	ima_kexec_file.size = segment_size;
> -	ima_kexec_file.read_pos = 0;
> -	ima_kexec_file.count = sizeof(khdr);	/* reserved space */
> -
>  	memset(&khdr, 0, sizeof(khdr));
>  	khdr.version = 1;
>  	/* This is an append-only list, no need to hold the RCU read lock */
> @@ -71,8 +91,6 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
>  	*buffer_size = ima_kexec_file.count;
>  	*buffer = ima_kexec_file.buf;
>  out:
> -	if (ret == -EINVAL)
> -		vfree(ima_kexec_file.buf);
>  	return ret;
>  }
>  
> @@ -111,6 +129,12 @@ void ima_add_kexec_buffer(struct kimage *image)
>  		return;
>  	}
>  
> +	ret = ima_alloc_kexec_file_buf(kexec_segment_size);
> +	if (ret < 0) {
> +		pr_err("Not enough memory for the kexec measurement buffer.\n");
> +		return;
> +	}
> +
>  	ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer,
>  				  kexec_segment_size);
>  	if (!kexec_buffer) {
> -- 
> 2.25.1
>