| Message ID | 20250218225502.747963-2-chenste@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On Tue, 2025-02-18 at 14:54 -0800, steven chen wrote: > 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'. > > This patch includes the following changes: > - Refactor ima_dump_measurement_list() to move the memory allocation > 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 ima_dump_measurement_list() > a local static to the file, so that it can be accessed from > ima_alloc_kexec_file_buf(). Compare actual memory required to ensure > there is enough memory for the entire measurement record. > - Copy as many measurement events as possible. > - Make necessary changes to the function ima_add_kexec_buffer() to call > the above two functions. > - Compared the memory size allocated with memory size of the entire > measurement record. If there is not enough memory, it will copy as many > IMA measurement records as possible, and this situation will result > in a failure of remote attestation. > > Author: Tushar Sugandhi <tusharsu@linux.microsoft.com> Steven, thank you again for picking up this patch set. As previously explained, there is no tag named "Author" in https://www.kernel.org/doc/Documentation/process/submitting-patches.rst. To give credit to the original author use "Co-developed-by". The "Co-developed-by:" tag is immediately followed by the original author's "Signed-off-by:" tag. Please refer to the document for an example. > Suggested-by: Mimi Zohar <zohar@linux.ibm.com> <--- "Co-developed-by:" would go here. > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > Signed-off-by: steven chen <chenste@linux.microsoft.com> > --- > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_kexec.c | 102 +++++++++++++++++++++-------- > security/integrity/ima/ima_queue.c | 4 +- > 3 files changed, 77 insertions(+), 30 deletions(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 24d09ea91b87..4428fcf42167 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -274,6 +274,7 @@ 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); > +int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry); > 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 9d45f4d26f73..89088f1fa989 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -15,63 +15,97 @@ > #include "ima.h" > > #ifdef CONFIG_IMA_KEXEC > +static struct seq_file ima_kexec_file; > + > +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); > +} > + > +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) > + goto out; > + > + 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; > + > +out: > + ima_kexec_file.read_pos = 0; > + ima_kexec_file.count = sizeof(struct ima_kexec_hdr); /* reserved space */ > + > + return 0; > +} > + <--- ima_dump_measurement_list() function comment goes here. > 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; > + size_t entry_size = 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) { > + pr_err("Kexec file buf not allocated\n"); > + return -EINVAL; > } > > - file.file = NULL; > - file.size = segment_size; > - file.read_pos = 0; > - 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 */ > + /* Copy as many IMA measurements list records as possible */ Having two consecutive comments like this looks weird. Please refer to section "8) Commenting" of https://www.kernel.org/doc/Documentation/process/coding-style.rst. The first comment is particular to list_for_each_entry_rcu() and should remain here. The latter comment is more generic and should be included as part of a function comment. > list_for_each_entry_rcu(qe, &ima_measurements, later, true) { > - if (file.count < file.size) { > + entry_size += ima_get_binary_runtime_entry_size(qe->entry); > + if (entry_size <= segment_size) { As much as possible splitting a function shouldn't change the existing code. It makes it harder to review. This sort of change should be a separate patch with the Subject topic line something like "ima: copy only complete measurement records across kexec". Making this change as the first patch in the patch set will also allow it to be backported. > khdr.count++; > - ima_measurements_show(&file, qe); > + ima_measurements_show(&ima_kexec_file, qe); > } else { > ret = -EINVAL; > + pr_err("IMA log file is too big for Kexec buf\n"); > break; We really DO want to "Copy as many IMA measurements list records as possible" as possible. However, the code doesn't match the comment, since the caller of ima_dump_measurement_list() treats -EINVAL as an error and bails. > } > } > > - 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); > + *buffer_size = ima_kexec_file.count; > + *buffer = ima_kexec_file.buf; > + > return ret; > } > > @@ -90,7 +124,7 @@ 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_buffer_size = 0; > size_t kexec_segment_size; > int ret; > > @@ -110,13 +144,19 @@ 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("Failed to dump IMA measurements. Error:%d.\n", ret); > + return; > + } > + As mentioned above, we really do want to copy as many measurement records as possible across kexec. > kbuf.buffer = kexec_buffer; > kbuf.bufsz = kexec_buffer_size; > kbuf.memsz = kexec_segment_size; > @@ -131,6 +171,12 @@ void ima_add_kexec_buffer(struct kimage *image) > image->ima_buffer_size = kexec_segment_size; > image->ima_buffer = kexec_buffer; > > + /* > + * kexec owns kexec_buffer after kexec_add_buffer() is called > + * and it will vfree() that buffer. > + */ > + ima_reset_kexec_file(&ima_kexec_file); > + > kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n", > kbuf.mem); > } > diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c > index 83d53824aa98..3dfd178d4292 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -78,7 +78,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 > *digest_value, > * binary_runtime_measurement list entry, which contains a > * couple of variable length fields (e.g template name and data). > */ > -static int get_binary_runtime_size(struct ima_template_entry *entry) > +int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry) > { > int size = 0; > > @@ -122,7 +122,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry, > if (binary_runtime_size != ULONG_MAX) { > int size; > > - size = get_binary_runtime_size(entry); > + size = ima_get_binary_runtime_entry_size(entry); > binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ? > binary_runtime_size + size : ULONG_MAX; > } This change would be included in the new first patch named something like "ima: copy only complete measurement records across kexec". Thanks, Mimi
On Thu, 2025-02-20 at 09:53 -0500, Mimi Zohar wrote: > On Tue, 2025-02-18 at 14:54 -0800, steven chen wrote: [... > > Author: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > Steven, thank you again for picking up this patch set. > > As previously explained, there is no tag named "Author" in > https://www.kernel.org/doc/Documentation/process/submitting-patches.rst > . To give credit to the original author use "Co-developed-by". Just on this, only use the co-developed-by if you actually *modified* the patch. If you're just transmitting the patch unmodified you can give original author credit by including a From: original author <email> Followed by a blank line at the beginning of the email. That makes the git author field contan whatever the From: line says. You still need a signoff from yourself in the original patch because you transmitted it. Some people also consider minor modifications to be insufficient to disturb the original copyright ownership and simply document what they did in square brackets under their signoff, like this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b5d1e6ee761a109400e97ac6a1b91c57d0f6a43a Regards, James
On Thu, 2025-02-20 at 10:04 -0500, James Bottomley wrote: > On Thu, 2025-02-20 at 09:53 -0500, Mimi Zohar wrote: > > On Tue, 2025-02-18 at 14:54 -0800, steven chen wrote: > [... > > > Author: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > > > Steven, thank you again for picking up this patch set. > > > > As previously explained, there is no tag named "Author" in > > https://www.kernel.org/doc/Documentation/process/submitting-patches.rst > > . To give credit to the original author use "Co-developed-by". > > Just on this, only use the co-developed-by if you actually *modified* > the patch. If you're just transmitting the patch unmodified you can > give original author credit by including a > > From: original author <email> > > Followed by a blank line at the beginning of the email. That makes the > git author field contan whatever the From: line says. You still need a > signoff from yourself in the original patch because you transmitted it. > > Some people also consider minor modifications to be insufficient to > disturb the original copyright ownership and simply document what they > did in square brackets under their signoff, like this: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b5d1e6ee761a109400e97ac6a1b91c57d0f6a43a Originally I had said: > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > Signed-off-by: steven chen <chenste@linux.microsoft.com> Before the "Co-developed-by" tag was defined, it was implied simply by this ordering of the "Signed-off-by" tags. For those patches you didn't modify, simply import Tushar's patch with him as the author and add your Signed-off-by tag after his. Thanks, James, for the explanation of using "From: original author <email>" to force the author to be Tushar. Mimi
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 24d09ea91b87..4428fcf42167 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -274,6 +274,7 @@ 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); +int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry); 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 9d45f4d26f73..89088f1fa989 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -15,63 +15,97 @@ #include "ima.h" #ifdef CONFIG_IMA_KEXEC +static struct seq_file ima_kexec_file; + +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); +} + +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) + goto out; + + 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; + +out: + 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; + size_t entry_size = 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) { + pr_err("Kexec file buf not allocated\n"); + return -EINVAL; } - file.file = NULL; - file.size = segment_size; - file.read_pos = 0; - 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 */ + /* Copy as many IMA measurements list records as possible */ list_for_each_entry_rcu(qe, &ima_measurements, later, true) { - if (file.count < file.size) { + entry_size += ima_get_binary_runtime_entry_size(qe->entry); + if (entry_size <= segment_size) { khdr.count++; - ima_measurements_show(&file, qe); + ima_measurements_show(&ima_kexec_file, qe); } else { ret = -EINVAL; + pr_err("IMA log file is too big for Kexec buf\n"); 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); + *buffer_size = ima_kexec_file.count; + *buffer = ima_kexec_file.buf; + return ret; } @@ -90,7 +124,7 @@ 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_buffer_size = 0; size_t kexec_segment_size; int ret; @@ -110,13 +144,19 @@ 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("Failed to dump IMA measurements. Error:%d.\n", ret); + return; + } + kbuf.buffer = kexec_buffer; kbuf.bufsz = kexec_buffer_size; kbuf.memsz = kexec_segment_size; @@ -131,6 +171,12 @@ void ima_add_kexec_buffer(struct kimage *image) image->ima_buffer_size = kexec_segment_size; image->ima_buffer = kexec_buffer; + /* + * kexec owns kexec_buffer after kexec_add_buffer() is called + * and it will vfree() that buffer. + */ + ima_reset_kexec_file(&ima_kexec_file); + kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n", kbuf.mem); } diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 83d53824aa98..3dfd178d4292 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -78,7 +78,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value, * binary_runtime_measurement list entry, which contains a * couple of variable length fields (e.g template name and data). */ -static int get_binary_runtime_size(struct ima_template_entry *entry) +int ima_get_binary_runtime_entry_size(struct ima_template_entry *entry) { int size = 0; @@ -122,7 +122,7 @@ static int ima_add_digest_entry(struct ima_template_entry *entry, if (binary_runtime_size != ULONG_MAX) { int size; - size = get_binary_runtime_size(entry); + size = ima_get_binary_runtime_entry_size(entry); binary_runtime_size = (binary_runtime_size < ULONG_MAX - size) ? binary_runtime_size + size : ULONG_MAX; }