| Message ID | 20231005182602.634615-3-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On 10/5/23 14:25, Tushar Sugandhi wrote: > In the current IMA implementation, ima_dump_measurement_list() is called > during the kexec 'load' operation. This can result in loss of IMA > measurements taken between the 'load' and 'execute' phases when the > system goes through Kexec soft reboot to a new Kernel. The call to the > function ima_dump_measurement_list() needs to be moved out of the > function ima_add_kexec_buffer() and needs to be called during the kexec > 'execute' operation. > > Implement a function ima_update_kexec_buffer() that is called during > kexec 'execute', allowing IMA to update the measurement list with the > events between kexec 'load' and 'execute'. Move the > ima_dump_measurement_list() call from ima_add_kexec_buffer() to > ima_update_kexec_buffer(). Make ima_kexec_buffer and kexec_segment_size > variables global, so that they can be accessed during both kexec 'load' > and 'execute'. Add functions ima_measurements_suspend() and > ima_measurements_resume() to set and reset the 'suspend_ima_measurements' > variable respectively, to suspend/resume IMA measurements. Use > the existing 'ima_extend_list_mutex' to ensure that the operations are > thread-safe. These function calls will help maintaining the integrity > of the IMA log while it is being copied to the new Kernel's buffer. > Add a reboot notifier_block 'update_buffer_nb' to ensure > the function ima_update_kexec_buffer() gets called during kexec > soft-reboot. > > Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com> > --- > security/integrity/ima/ima.h | 2 ++ > security/integrity/ima/ima_kexec.c | 58 +++++++++++++++++++++++++----- > security/integrity/ima/ima_queue.c | 18 ++++++++++ > 3 files changed, 69 insertions(+), 9 deletions(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index c29db699c996..49a6047dd8eb 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -161,6 +161,8 @@ 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); > +void ima_measurements_suspend(void); > +void ima_measurements_resume(void); > 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 307e07991865..2c11bbe6efef 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -17,6 +17,8 @@ > #ifdef CONFIG_IMA_KEXEC > struct seq_file ima_kexec_file; > struct ima_kexec_hdr ima_khdr; > +static void *ima_kexec_buffer; > +static size_t kexec_segment_size; > > void ima_clear_kexec_file(void) > { > @@ -142,7 +144,6 @@ 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_segment_size; > int ret; > > /* > @@ -167,14 +168,6 @@ void ima_add_kexec_buffer(struct kimage *image) > return; > } > > - ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, > - kexec_segment_size); > - if (ret < 0) { > - pr_err("%s: Failed to dump IMA measurements. Error:%d.\n", > - __func__, ret); > - return; > - } > - > kbuf.buffer = kexec_buffer; > kbuf.bufsz = kexec_buffer_size; > kbuf.memsz = kexec_segment_size; > @@ -192,6 +185,53 @@ void ima_add_kexec_buffer(struct kimage *image) > pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n", > kbuf.mem); > } > + > +/* > + * Called during kexec execute so that IMA can update the measurement list. > + */ > +static int ima_update_kexec_buffer(struct notifier_block *self, > + unsigned long action, void *data) > +{ > + void *buf = NULL; > + size_t buf_size; > + bool resume = false; > + int ret; > + > + if (!kexec_in_progress) { > + pr_info("%s: No kexec in progress.\n", __func__); > + return NOTIFY_OK; > + } > + > + if (!ima_kexec_buffer) { > + pr_err("%s: Kexec buffer not set.\n", __func__); > + return NOTIFY_OK; > + } > + > + ima_measurements_suspend(); > + > + buf_size = ima_get_binary_runtime_size(); There doesn't seem to be a need to call this function and pass in the binary runtime size into the dump function. You should be able to remove it. > + ret = ima_dump_measurement_list(&buf_size, &buf, > + kexec_segment_size); > + > + if (!buf || ret < 0) { I don't think this function can (or should) ever return ret >= 0 with buf == NULL. > + pr_err("%s: Dump measurements failed. Error:%d\n", > + __func__, ret); > + resume = true; > + goto out; > + } > + memcpy(ima_kexec_buffer, buf, buf_size); > +out: > + ima_kexec_buffer = NULL; > + > + if (resume) > + ima_measurements_resume(); > + > + return NOTIFY_OK; > +} > +struct notifier_block update_buffer_nb = { > + .notifier_call = ima_update_kexec_buffer, > +}; > + > #endif /* IMA_KEXEC */ > > /* > diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c > index 532da87ce519..9e7d1196006e 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = { > */ > static DEFINE_MUTEX(ima_extend_list_mutex); > > +/* > + * Used internally by the kernel to suspend-resume ima measurements. > + */ > +static atomic_t suspend_ima_measurements; > + > /* lookup up the digest value in the hash table, and return the entry */ > static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value, > int pcr) > @@ -147,6 +152,19 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr) > pr_err("Error Communicating to TPM chip, result: %d\n", result); > return result; > } > +void ima_measurements_suspend(void) > +{ > + mutex_lock(&ima_extend_list_mutex); > + atomic_set(&suspend_ima_measurements, 1); > + mutex_unlock(&ima_extend_list_mutex); > +} > + > +void ima_measurements_resume(void) > +{ > + mutex_lock(&ima_extend_list_mutex); > + atomic_set(&suspend_ima_measurements, 0); > + mutex_unlock(&ima_extend_list_mutex); > +} > > /* > * Add template entry to the measurement list and hash table, and
On 10/12/23 17:28, Stefan Berger wrote: > > On 10/5/23 14:25, Tushar Sugandhi wrote: >> In the current IMA implementation, ima_dump_measurement_list() is called >> during the kexec 'load' operation. This can result in loss of IMA >> measurements taken between the 'load' and 'execute' phases when the >> system goes through Kexec soft reboot to a new Kernel. The call to the >> function ima_dump_measurement_list() needs to be moved out of the >> function ima_add_kexec_buffer() and needs to be called during the kexec >> 'execute' operation. >> >> Implement a function ima_update_kexec_buffer() that is called during >> kexec 'execute', allowing IMA to update the measurement list with the >> events between kexec 'load' and 'execute'. Move the >> ima_dump_measurement_list() call from ima_add_kexec_buffer() to >> ima_update_kexec_buffer(). Make ima_kexec_buffer and kexec_segment_size >> variables global, so that they can be accessed during both kexec 'load' >> and 'execute'. Add functions ima_measurements_suspend() and >> ima_measurements_resume() to set and reset the 'suspend_ima_measurements' >> variable respectively, to suspend/resume IMA measurements. Use >> the existing 'ima_extend_list_mutex' to ensure that the operations are >> thread-safe. These function calls will help maintaining the integrity >> of the IMA log while it is being copied to the new Kernel's buffer. >> Add a reboot notifier_block 'update_buffer_nb' to ensure >> the function ima_update_kexec_buffer() gets called during kexec >> soft-reboot. >> >> Signed-off-by: Tushar Sugandhi<tusharsu@linux.microsoft.com> >> --- >> security/integrity/ima/ima.h | 2 ++ >> security/integrity/ima/ima_kexec.c | 58 +++++++++++++++++++++++++----- >> security/integrity/ima/ima_queue.c | 18 ++++++++++ >> 3 files changed, 69 insertions(+), 9 deletions(-) >> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >> index c29db699c996..49a6047dd8eb 100644 >> --- a/security/integrity/ima/ima.h >> +++ b/security/integrity/ima/ima.h >> @@ -161,6 +161,8 @@ 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); >> +void ima_measurements_suspend(void); >> +void ima_measurements_resume(void); >> 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 307e07991865..2c11bbe6efef 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -17,6 +17,8 @@ >> #ifdef CONFIG_IMA_KEXEC >> struct seq_file ima_kexec_file; >> struct ima_kexec_hdr ima_khdr; >> +static void *ima_kexec_buffer; >> +static size_t kexec_segment_size; >> void ima_clear_kexec_file(void) >> { >> @@ -142,7 +144,6 @@ 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_segment_size; >> int ret; >> /* >> @@ -167,14 +168,6 @@ void ima_add_kexec_buffer(struct kimage *image) >> return; >> } >> - ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, >> - kexec_segment_size); >> - if (ret < 0) { >> - pr_err("%s: Failed to dump IMA measurements. Error:%d.\n", >> - __func__, ret); >> - return; >> - } >> - >> kbuf.buffer = kexec_buffer; >> kbuf.bufsz = kexec_buffer_size; >> kbuf.memsz = kexec_segment_size; >> @@ -192,6 +185,53 @@ void ima_add_kexec_buffer(struct kimage *image) >> pr_debug("kexec measurement buffer for the loaded kernel at >> 0x%lx.\n", >> kbuf.mem); >> } >> + >> +/* >> + * Called during kexec execute so that IMA can update the measurement >> list. >> + */ >> +static int ima_update_kexec_buffer(struct notifier_block *self, >> + unsigned long action, void *data) >> +{ >> + void *buf = NULL; >> + size_t buf_size; >> + bool resume = false; >> + int ret; >> + >> + if (!kexec_in_progress) { >> + pr_info("%s: No kexec in progress.\n", __func__); >> + return NOTIFY_OK; >> + } >> + >> + if (!ima_kexec_buffer) { >> + pr_err("%s: Kexec buffer not set.\n", __func__); >> + return NOTIFY_OK; >> + } >> + >> + ima_measurements_suspend(); >> + >> + buf_size = ima_get_binary_runtime_size(); > There doesn't seem to be a need to call this function and pass in the > binary runtime size into the dump function. You should be able to remove > it. Oh. This was an oversight. This line is removed in patch 7/7. It should have been removed here itself. Thanks for catching it. Will fix. >> + ret = ima_dump_measurement_list(&buf_size, &buf, >> + kexec_segment_size); >> + >> + if (!buf || ret < 0) { > I don't think this function can (or should) ever return ret >= 0 with > buf == NULL. I will simplify this check. Thanks. ~Tushar >> + pr_err("%s: Dump measurements failed. Error:%d\n", >> + __func__, ret); >> + resume = true; >> + goto out; >> + } >> + memcpy(ima_kexec_buffer, buf, buf_size); >> +out: >> + ima_kexec_buffer = NULL; >> + >> + if (resume) >> + ima_measurements_resume(); >> + >> + return NOTIFY_OK; >> +} >> +struct notifier_block update_buffer_nb = { >> + .notifier_call = ima_update_kexec_buffer, >> +}; >> + >> #endif /* IMA_KEXEC */ >> /* >> diff --git a/security/integrity/ima/ima_queue.c >> b/security/integrity/ima/ima_queue.c >> index 532da87ce519..9e7d1196006e 100644 >> --- a/security/integrity/ima/ima_queue.c >> +++ b/security/integrity/ima/ima_queue.c >> @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = { >> */ >> static DEFINE_MUTEX(ima_extend_list_mutex); >> +/* >> + * Used internally by the kernel to suspend-resume ima measurements. >> + */ >> +static atomic_t suspend_ima_measurements; >> + >> /* lookup up the digest value in the hash table, and return the >> entry */ >> static struct ima_queue_entry *ima_lookup_digest_entry(u8 >> *digest_value, >> int pcr) >> @@ -147,6 +152,19 @@ static int ima_pcr_extend(struct tpm_digest >> *digests_arg, int pcr) >> pr_err("Error Communicating to TPM chip, result: %d\n", >> result); >> return result; >> } >> +void ima_measurements_suspend(void) >> +{ >> + mutex_lock(&ima_extend_list_mutex); >> + atomic_set(&suspend_ima_measurements, 1); >> + mutex_unlock(&ima_extend_list_mutex); >> +} >> + >> +void ima_measurements_resume(void) >> +{ >> + mutex_lock(&ima_extend_list_mutex); >> + atomic_set(&suspend_ima_measurements, 0); >> + mutex_unlock(&ima_extend_list_mutex); >> +} >> /* >> * Add template entry to the measurement list and hash table, and
On 10/27/23 06:08, Mimi Zohar wrote: > Hi Tushar, > > On Thu, 2023-10-05 at 11:25 -0700, Tushar Sugandhi wrote: >> In the current IMA implementation, ima_dump_measurement_list() is called >> during the kexec 'load' operation. This can result in loss of IMA >> measurements taken between the 'load' and 'execute' phases when the >> system goes through Kexec soft reboot to a new Kernel. The call to the >> function ima_dump_measurement_list() needs to be moved out of the >> function ima_add_kexec_buffer() and needs to be called during the kexec >> 'execute' operation. >> >> Implement a function ima_update_kexec_buffer() that is called during >> kexec 'execute', allowing IMA to update the measurement list with the >> events between kexec 'load' and 'execute'. Move the >> ima_dump_measurement_list() call from ima_add_kexec_buffer() to >> ima_update_kexec_buffer(). Make ima_kexec_buffer and kexec_segment_size >> variables global, so that they can be accessed during both kexec 'load' >> and 'execute'. Add functions ima_measurements_suspend() and >> ima_measurements_resume() to set and reset the 'suspend_ima_measurements' >> variable respectively, to suspend/resume IMA measurements. Use >> the existing 'ima_extend_list_mutex' to ensure that the operations are >> thread-safe. These function calls will help maintaining the integrity >> of the IMA log while it is being copied to the new Kernel's buffer. >> Add a reboot notifier_block 'update_buffer_nb' to ensure >> the function ima_update_kexec_buffer() gets called during kexec >> soft-reboot. >> >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > > The lengthiness and complexity of the patch description is an > indication that the patch needs to be broken up. Please refer to > Documentation/process/submitting-patches.rst for further info. > > In addition, this patch moves the function ima_dump_measurement_list() > to a new function named ima_update_kexec_buffer(), which is never > called. The patch set is thus not bisect safe. > > [...] >> +void ima_measurements_suspend(void) >> +{ >> + mutex_lock(&ima_extend_list_mutex); >> + atomic_set(&suspend_ima_measurements, 1); >> + mutex_unlock(&ima_extend_list_mutex); >> +} >> + >> +void ima_measurements_resume(void) >> +{ >> + mutex_lock(&ima_extend_list_mutex); >> + atomic_set(&suspend_ima_measurements, 0); >> + mutex_unlock(&ima_extend_list_mutex); >> +} > > These function are being defined and called here, but are not enforced > until a later patch. It would make more sense to introduce and > enforce these functions in a single patch with an explanation as to why > suspend/resume is needed. > > The cover letter describes the problem as "... new IMA measurements are > added between kexec 'load' and kexec 'execute'". Please include in > the patch description the reason for needing suspend/resume, since > saving the measurement records is done during kexec execute. > Since I am introducing a few new functions in this patch set, I am having hard time keeping the patches bisect safe and at the same time managing the length/complexity of the individual patches in the series. I will go back and revisit the bisect-safeness of the patches in this series again. Thanks for the feedback, appreciate it.
On 11/14/23 14:43, Tushar Sugandhi wrote: > > In addition, this patch moves the function ima_dump_measurement_list() > to a new function named ima_update_kexec_buffer(), which is never > called. The patch set is thus not bisect safe. BTW, ima_update_kexec_buffer() is part of the notifier_block. which is part of the same patch 2/7. +struct notifier_block update_buffer_nb = { + .notifier_call = ima_update_kexec_buffer, +}; + update_buffer_nb is being registered to the reboot notifiers in patch 3/7 of this series. So ima_update_kexec_buffer() is being called. +void ima_kexec_post_load(struct kimage *image) +{ ... ... + + if (!ima_kexec_update_registered) { + register_reboot_notifier(&update_buffer_nb); + ima_kexec_update_registered = true; + } +} Maybe you meant 'update_buffer_nb' variable needs to be defined and used in the same patch and not defined in 2/7 and used in 3/7. Anyways, I think I took care of the majority of the bisect-safe issues from V1->V2 of this series. But maybe I missed a few. I will look at this with fresh perspective, to see if I missed anything, when I publish V3 of the series. Thanks, Tushar
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index c29db699c996..49a6047dd8eb 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -161,6 +161,8 @@ 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); +void ima_measurements_suspend(void); +void ima_measurements_resume(void); 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 307e07991865..2c11bbe6efef 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -17,6 +17,8 @@ #ifdef CONFIG_IMA_KEXEC struct seq_file ima_kexec_file; struct ima_kexec_hdr ima_khdr; +static void *ima_kexec_buffer; +static size_t kexec_segment_size; void ima_clear_kexec_file(void) { @@ -142,7 +144,6 @@ 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_segment_size; int ret; /* @@ -167,14 +168,6 @@ void ima_add_kexec_buffer(struct kimage *image) return; } - ret = ima_dump_measurement_list(&kexec_buffer_size, &kexec_buffer, - kexec_segment_size); - if (ret < 0) { - pr_err("%s: Failed to dump IMA measurements. Error:%d.\n", - __func__, ret); - return; - } - kbuf.buffer = kexec_buffer; kbuf.bufsz = kexec_buffer_size; kbuf.memsz = kexec_segment_size; @@ -192,6 +185,53 @@ void ima_add_kexec_buffer(struct kimage *image) pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n", kbuf.mem); } + +/* + * Called during kexec execute so that IMA can update the measurement list. + */ +static int ima_update_kexec_buffer(struct notifier_block *self, + unsigned long action, void *data) +{ + void *buf = NULL; + size_t buf_size; + bool resume = false; + int ret; + + if (!kexec_in_progress) { + pr_info("%s: No kexec in progress.\n", __func__); + return NOTIFY_OK; + } + + if (!ima_kexec_buffer) { + pr_err("%s: Kexec buffer not set.\n", __func__); + return NOTIFY_OK; + } + + ima_measurements_suspend(); + + buf_size = ima_get_binary_runtime_size(); + ret = ima_dump_measurement_list(&buf_size, &buf, + kexec_segment_size); + + if (!buf || ret < 0) { + pr_err("%s: Dump measurements failed. Error:%d\n", + __func__, ret); + resume = true; + goto out; + } + memcpy(ima_kexec_buffer, buf, buf_size); +out: + ima_kexec_buffer = NULL; + + if (resume) + ima_measurements_resume(); + + return NOTIFY_OK; +} +struct notifier_block update_buffer_nb = { + .notifier_call = ima_update_kexec_buffer, +}; + #endif /* IMA_KEXEC */ /* diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 532da87ce519..9e7d1196006e 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -44,6 +44,11 @@ struct ima_h_table ima_htable = { */ static DEFINE_MUTEX(ima_extend_list_mutex); +/* + * Used internally by the kernel to suspend-resume ima measurements. + */ +static atomic_t suspend_ima_measurements; + /* lookup up the digest value in the hash table, and return the entry */ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value, int pcr) @@ -147,6 +152,19 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr) pr_err("Error Communicating to TPM chip, result: %d\n", result); return result; } +void ima_measurements_suspend(void) +{ + mutex_lock(&ima_extend_list_mutex); + atomic_set(&suspend_ima_measurements, 1); + mutex_unlock(&ima_extend_list_mutex); +} + +void ima_measurements_resume(void) +{ + mutex_lock(&ima_extend_list_mutex); + atomic_set(&suspend_ima_measurements, 0); + mutex_unlock(&ima_extend_list_mutex); +} /* * Add template entry to the measurement list and hash table, and
In the current IMA implementation, ima_dump_measurement_list() is called during the kexec 'load' operation. This can result in loss of IMA measurements taken between the 'load' and 'execute' phases when the system goes through Kexec soft reboot to a new Kernel. The call to the function ima_dump_measurement_list() needs to be moved out of the function ima_add_kexec_buffer() and needs to be called during the kexec 'execute' operation. Implement a function ima_update_kexec_buffer() that is called during kexec 'execute', allowing IMA to update the measurement list with the events between kexec 'load' and 'execute'. Move the ima_dump_measurement_list() call from ima_add_kexec_buffer() to ima_update_kexec_buffer(). Make ima_kexec_buffer and kexec_segment_size variables global, so that they can be accessed during both kexec 'load' and 'execute'. Add functions ima_measurements_suspend() and ima_measurements_resume() to set and reset the 'suspend_ima_measurements' variable respectively, to suspend/resume IMA measurements. Use the existing 'ima_extend_list_mutex' to ensure that the operations are thread-safe. These function calls will help maintaining the integrity of the IMA log while it is being copied to the new Kernel's buffer. Add a reboot notifier_block 'update_buffer_nb' to ensure the function ima_update_kexec_buffer() gets called during kexec soft-reboot. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- security/integrity/ima/ima.h | 2 ++ security/integrity/ima/ima_kexec.c | 58 +++++++++++++++++++++++++----- security/integrity/ima/ima_queue.c | 18 ++++++++++ 3 files changed, 69 insertions(+), 9 deletions(-)