| Message ID | 20250318010448.954-2-chenste@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On 3/17/25 9:04 PM, steven chen wrote: > The name of the local variable "file" of type seq_file defined in the > ima_dump_measurement_list function is too generic. To better reflect the > purpose of the variable, rename it to "ima_kexec_file". This change will > help improve code readability and maintainability by making the variable's > role more explicit. > > The variable ima_kexec_file is indeed the memory allocated for copying IMA > measurement records. The ima_dump_measurement_list function calculates the > actual memory occupied by the IMA logs and compares it with the allocated > memory. If there is enough memory, it copies all IMA measurement records; > otherwise, it does not copy any records, which would result in a failure > of remote attestation. > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > Signed-off-by: steven chen <chenste@linux.microsoft.com> > --- > security/integrity/ima/ima_kexec.c | 39 ++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 15 deletions(-) > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 9d45f4d26f73..8567619889d1 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,33 +15,41 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +/* > + * Copy the measurement list to the allocated memory > + * compare the size of IMA measurement list with the size of the allocated memory Compare the size of the IMA ... memory. > + * if the size of the allocated memory is not less than the size of IMA measurement list > + * copy the measurement list to the allocated memory. > + * else > + * return error If the size of the allocated memory is not less than the size of IMA measurement list, copy the measurement list to the allocated memory, return an error otherwise. Looked a bit unusual. With this nit: Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > + */ > 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 +63,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; > } >
On 03/18/25 at 11:10am, Stefan Berger wrote: > > > On 3/17/25 9:04 PM, steven chen wrote: > > The name of the local variable "file" of type seq_file defined in the > > ima_dump_measurement_list function is too generic. To better reflect the > > purpose of the variable, rename it to "ima_kexec_file". This change will > > help improve code readability and maintainability by making the variable's > > role more explicit. > > > > The variable ima_kexec_file is indeed the memory allocated for copying IMA > > measurement records. The ima_dump_measurement_list function calculates the > > actual memory occupied by the IMA logs and compares it with the allocated > > memory. If there is enough memory, it copies all IMA measurement records; > > otherwise, it does not copy any records, which would result in a failure > > of remote attestation. > > > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > > Signed-off-by: steven chen <chenste@linux.microsoft.com> > > > > --- > > security/integrity/ima/ima_kexec.c | 39 ++++++++++++++++++------------ > > 1 file changed, 24 insertions(+), 15 deletions(-) > > > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > > index 9d45f4d26f73..8567619889d1 100644 > > --- a/security/integrity/ima/ima_kexec.c > > +++ b/security/integrity/ima/ima_kexec.c > > @@ -15,33 +15,41 @@ > > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > > +/* > > + * Copy the measurement list to the allocated memory > > + * compare the size of IMA measurement list with the size of the allocated memory > > Compare the size of the IMA ... memory. > > > > + * if the size of the allocated memory is not less than the size of IMA measurement list > > + * copy the measurement list to the allocated memory. > > + * else > > + * return error > > If the size of the allocated memory is not less than the size of IMA > measurement list, copy the measurement list to the allocated memory, return > an error otherwise. Ack the suggested change.
Fix spelling: set_file On Mon, 2025-03-17 at 18:04 -0700, steven chen wrote: > The name of the local variable "file" of type seq_file defined in the > ima_dump_measurement_list function is too generic. To better reflect the > purpose of the variable, rename it to "ima_kexec_file". This change will > help improve code readability and maintainability by making the variable's > role more explicit. The reason for making the variable name change is the variable scope. -> Before making the function local seq_file "file" variable global, rename it to ima_kexec_file. > > The variable ima_kexec_file is indeed the memory allocated for copying IMA > measurement records. The ima_dump_measurement_list function calculates the > actual memory occupied by the IMA logs and compares it with the allocated > memory. If there is enough memory, it copies all IMA measurement records; > otherwise, it does not copy any records, which would result in a failure > of remote attestation. This paragraph is not applicable to the patch change. > > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> > Signed-off-by: steven chen <chenste@linux.microsoft.com> > --- > security/integrity/ima/ima_kexec.c | 39 ++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 15 deletions(-) > > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index 9d45f4d26f73..8567619889d1 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,33 +15,41 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +/* > + * Copy the measurement list to the allocated memory > + * compare the size of IMA measurement list with the size of the allocated memory > + * if the size of the allocated memory is not less than the size of IMA measurement list > + * copy the measurement list to the allocated memory. > + * else > + * return error > + */ Please minimize patch changes. Before posting, please look at the patch(s) and remove anything not applicable to it. In this case, the comment is not applicable to the variable name change. thanks, Mimi > 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 +63,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; > } >
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 9d45f4d26f73..8567619889d1 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -15,33 +15,41 @@ #include "ima.h" #ifdef CONFIG_IMA_KEXEC +/* + * Copy the measurement list to the allocated memory + * compare the size of IMA measurement list with the size of the allocated memory + * if the size of the allocated memory is not less than the size of IMA measurement list + * copy the measurement list to the allocated memory. + * else + * return error + */ 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 +63,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; }
The name of the local variable "file" of type seq_file defined in the ima_dump_measurement_list function is too generic. To better reflect the purpose of the variable, rename it to "ima_kexec_file". This change will help improve code readability and maintainability by making the variable's role more explicit. The variable ima_kexec_file is indeed the memory allocated for copying IMA measurement records. The ima_dump_measurement_list function calculates the actual memory occupied by the IMA logs and compares it with the allocated memory. If there is enough memory, it copies all IMA measurement records; otherwise, it does not copy any records, which would result in a failure of remote attestation. Suggested-by: Mimi Zohar <zohar@linux.ibm.com> Signed-off-by: steven chen <chenste@linux.microsoft.com> --- security/integrity/ima/ima_kexec.c | 39 ++++++++++++++++++------------ 1 file changed, 24 insertions(+), 15 deletions(-)