Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute
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(_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
Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute
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
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(_extend_list_mutex);
+ atomic_set(_ima_measurements, 1);
+ mutex_unlock(_extend_list_mutex);
+}
+
+void ima_measurements_resume(void)
+{
+ mutex_lock(_extend_list_mutex);
+ atomic_set(_ima_measurements, 0);
+ mutex_unlock(_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.
Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute
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
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(_extend_list_mutex);
> + atomic_set(_ima_measurements, 1);
> + mutex_unlock(_extend_list_mutex);
> +}
> +
> +void ima_measurements_resume(void)
> +{
> + mutex_lock(_extend_list_mutex);
> + atomic_set(_ima_measurements, 0);
> + mutex_unlock(_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.
--
thanks,
Mimi
Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute
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
---
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(_buffer_size, _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(_size, ,
+ 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;
Re: [PATCH v2 2/7] ima: move ima_dump_measurement_list call from kexec load to execute
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
---
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(_buffer_size, _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(_size, ,
+ 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;
+
