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'. However, the time gap
between kexec load and kexec reboot can be very long. During this
time window, new events extended into TPM PCRs miss the chance
to be carried over to the second kernel.
To address the above, the following approach is proposed:
- Allocate the necessary buffer during the kexec load phase.
- Populate this buffer with the IMA measurements during
the kexec execute phase.
In the current implementation, a local variable "file" of type seq_file
is used in the API ima_dump_measurement_list() to store the IMA measurements
to be carried over across kexec system call. To make this buffer accessible
at kexec 'execute' time, rename it to "ima_kexec_file" before making it
a file variable to better reflect its purpose.
Renaming the local variable "file" of type seq_file defined in the
ima_dump_measurement_list function to "ima_kexec_file" will improve code
readability and maintainability by making the variable's role more explicit.
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: steven chen <chenste@linux.microsoft.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
security/integrity/ima/ima_kexec.c | 31 +++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index 9d45f4d26f73..650beb74346c 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -18,30 +18,30 @@
static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
unsigned long segment_size)
{
+ struct seq_file ima_kexec_file;
struct ima_queue_entry *qe;
- struct seq_file file;
struct ima_kexec_hdr khdr;
int ret = 0;
/* segment size can't change between kexec load and execute */
- file.buf = vmalloc(segment_size);
- if (!file.buf) {
+ ima_kexec_file.buf = vmalloc(segment_size);
+ if (!ima_kexec_file.buf) {
ret = -ENOMEM;
goto out;
}
- file.file = NULL;
- file.size = segment_size;
- file.read_pos = 0;
- file.count = sizeof(khdr); /* reserved space */
+ ima_kexec_file.file = NULL;
+ ima_kexec_file.size = segment_size;
+ ima_kexec_file.read_pos = 0;
+ ima_kexec_file.count = sizeof(khdr); /* reserved space */
memset(&khdr, 0, sizeof(khdr));
khdr.version = 1;
/* This is an append-only list, no need to hold the RCU read lock */
list_for_each_entry_rcu(qe, &ima_measurements, later, true) {
- if (file.count < file.size) {
+ if (ima_kexec_file.count < ima_kexec_file.size) {
khdr.count++;
- ima_measurements_show(&file, qe);
+ ima_measurements_show(&ima_kexec_file, qe);
} else {
ret = -EINVAL;
break;
@@ -55,23 +55,24 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
* fill in reserved space with some buffer details
* (eg. version, buffer size, number of measurements)
*/
- khdr.buffer_size = file.count;
+ khdr.buffer_size = ima_kexec_file.count;
if (ima_canonical_fmt) {
khdr.version = cpu_to_le16(khdr.version);
khdr.count = cpu_to_le64(khdr.count);
khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
}
- memcpy(file.buf, &khdr, sizeof(khdr));
+ memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr));
print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
- file.buf, file.count < 100 ? file.count : 100,
+ ima_kexec_file.buf, ima_kexec_file.count < 100 ?
+ ima_kexec_file.count : 100,
true);
- *buffer_size = file.count;
- *buffer = file.buf;
+ *buffer_size = ima_kexec_file.count;
+ *buffer = ima_kexec_file.buf;
out:
if (ret == -EINVAL)
- vfree(file.buf);
+ vfree(ima_kexec_file.buf);
return ret;
}
--
2.25.1Please fix the Subject line: change set_file -> seq_file On Wed, 2025-04-02 at 05:47 -0700, steven chen wrote: > 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'. However, the time gap > between kexec load and kexec reboot can be very long. During this > time window, new events extended into TPM PCRs miss the chance > to be carried over to the second kernel. > > To address the above, the following approach is proposed: > - Allocate the necessary buffer during the kexec load phase. > - Populate this buffer with the IMA measurements during > the kexec execute phase. > > In the current implementation, a local variable "file" of type seq_file > is used in the API ima_dump_measurement_list() to store the IMA measurements > to be carried over across kexec system call. To make this buffer accessible > at kexec 'execute' time, rename it to "ima_kexec_file" before making it > a file variable to better reflect its purpose. Only the reason for the variable name change is needed. Please remove everything else. > > Renaming the local variable "file" of type seq_file defined in the > ima_dump_measurement_list function to "ima_kexec_file" will improve code > readability and maintainability by making the variable's role more explicit. As previously mentioned, there is a difference when naming local and file static global variables. Variable naming is described https://www.kernel.org/doc/html/v4.10/process/coding-style.html#naming -> Before making the local ima_dump_measurement_list() seq_file "file" variable file static global, rename it to ima_kexec_file. thanks, Mimi
On 04/02/25 at 05:47am, steven chen wrote:
> 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'. However, the time gap
> between kexec load and kexec reboot can be very long. During this
> time window, new events extended into TPM PCRs miss the chance
> to be carried over to the second kernel.
>
> To address the above, the following approach is proposed:
> - Allocate the necessary buffer during the kexec load phase.
> - Populate this buffer with the IMA measurements during
> the kexec execute phase.
>
> In the current implementation, a local variable "file" of type seq_file
> is used in the API ima_dump_measurement_list() to store the IMA measurements
> to be carried over across kexec system call. To make this buffer accessible
> at kexec 'execute' time, rename it to "ima_kexec_file" before making it
> a file variable to better reflect its purpose.
>
> Renaming the local variable "file" of type seq_file defined in the
> ima_dump_measurement_list function to "ima_kexec_file" will improve code
> readability and maintainability by making the variable's role more explicit.
Seems it's clearer with below paragraph to replace the whole log:
=====
Rename the local variable "file" of type seq_file defined in the
ima_dump_measurement_list function to "ima_kexec_file" to improve code
readability and maintainability by making the variable's role more explicit.
=====
The code change looks good to me.
>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: steven chen <chenste@linux.microsoft.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
If there's code change in patch content, the reviewing tag should be
reset so that reviewing is taken again on the new change.
> ---
> security/integrity/ima/ima_kexec.c | 31 +++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 9d45f4d26f73..650beb74346c 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -18,30 +18,30 @@
> static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
> unsigned long segment_size)
> {
> + struct seq_file ima_kexec_file;
> struct ima_queue_entry *qe;
> - struct seq_file file;
> struct ima_kexec_hdr khdr;
> int ret = 0;
>
> /* segment size can't change between kexec load and execute */
> - file.buf = vmalloc(segment_size);
> - if (!file.buf) {
> + ima_kexec_file.buf = vmalloc(segment_size);
> + if (!ima_kexec_file.buf) {
> ret = -ENOMEM;
> goto out;
> }
>
> - file.file = NULL;
> - file.size = segment_size;
> - file.read_pos = 0;
> - file.count = sizeof(khdr); /* reserved space */
> + ima_kexec_file.file = NULL;
> + ima_kexec_file.size = segment_size;
> + ima_kexec_file.read_pos = 0;
> + ima_kexec_file.count = sizeof(khdr); /* reserved space */
>
> memset(&khdr, 0, sizeof(khdr));
> khdr.version = 1;
> /* This is an append-only list, no need to hold the RCU read lock */
> list_for_each_entry_rcu(qe, &ima_measurements, later, true) {
> - if (file.count < file.size) {
> + if (ima_kexec_file.count < ima_kexec_file.size) {
> khdr.count++;
> - ima_measurements_show(&file, qe);
> + ima_measurements_show(&ima_kexec_file, qe);
> } else {
> ret = -EINVAL;
> break;
> @@ -55,23 +55,24 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
> * fill in reserved space with some buffer details
> * (eg. version, buffer size, number of measurements)
> */
> - khdr.buffer_size = file.count;
> + khdr.buffer_size = ima_kexec_file.count;
> if (ima_canonical_fmt) {
> khdr.version = cpu_to_le16(khdr.version);
> khdr.count = cpu_to_le64(khdr.count);
> khdr.buffer_size = cpu_to_le64(khdr.buffer_size);
> }
> - memcpy(file.buf, &khdr, sizeof(khdr));
> + memcpy(ima_kexec_file.buf, &khdr, sizeof(khdr));
>
> print_hex_dump_debug("ima dump: ", DUMP_PREFIX_NONE, 16, 1,
> - file.buf, file.count < 100 ? file.count : 100,
> + ima_kexec_file.buf, ima_kexec_file.count < 100 ?
> + ima_kexec_file.count : 100,
> true);
>
> - *buffer_size = file.count;
> - *buffer = file.buf;
> + *buffer_size = ima_kexec_file.count;
> + *buffer = ima_kexec_file.buf;
> out:
> if (ret == -EINVAL)
> - vfree(file.buf);
> + vfree(ima_kexec_file.buf);
> return ret;
> }
>
> --
> 2.25.1
>
© 2016 - 2025 Red Hat, Inc.