Apologies for the late response on this particular patch (v3 2/7) Mimi.
I was on vacation in December.
I was meaning to respond to this one when I came back, but I was caught
in between other work items last few days. Sorry if it caused any
confusion.
Responses below.
On 12/20/23 11:02, Mimi Zohar wrote:
Hi Tushar,
On Fri, 2023-12-15 at 17:07 -0800, Tushar Sugandhi wrote:
ima_dump_measurement_list() is called from ima_add_kexec_buffer() during
kexec 'load', which may result in loss of IMA measurements between kexec
'load' and 'execute'. It needs to be called during kexec 'execute'.
Implement ima_update_kexec_buffer(), to be called during kexec 'execute'.
Move ima_dump_measurement_list() function call from ima_add_kexec_buffer()
to ima_update_kexec_buffer(). Make the needed variables global for
accessibility during kexec 'load' and 'execute'. Implement and call
ima_measurements_suspend() and ima_measurements_resume() to help ensure
the integrity of the IMA log during copy. Add a reboot notifier_block to
trigger ima_update_kexec_buffer() during kexec soft-reboot. Exclude ima
segment from calculating and storing digest in function
kexec_calculate_store_digests(), since ima segment can be modified
after the digest is computed during kexec 'load'.
Signed-off-by: Tushar Sugandhi <tusharsu@xxxxxxxxxxxxxxxxxxx>
Wow! That's quite a bit for a single patch.
This patch moves the ima_dump_measurement_list() call from kexec load
to exec, but doesn't register the reboot notifier in this patch. I
don't see how it is possible with just the previous and this patch
applied that the measurement list is carried across kexec.
Ah. That's a good catch.
I was only checking if I can boot into the Kernel for testing
bisect-safe readiness for each patch. I will ensure the move of
ima_dump_measurement_list() and registering the reboot notifier at
execute stays an atomic operation in a single patch.
Please test after applying each patch in the patch set to make sure
that the measurement list is properly carried across kexec.
Yup. I was only checking if I can boot into the Kernel after each patch.
My bad. :(
Going forward, I will check each patch for the measurement list carry
over after kexec.
Additional inline comments below.
---
include/linux/kexec.h | 3 ++
kernel/kexec_file.c | 8 ++++
security/integrity/ima/ima.h | 2 +
security/integrity/ima/ima_kexec.c | 61 +++++++++++++++++++++++++-----
security/integrity/ima/ima_queue.c | 19 ++++++++++
5 files changed, 84 insertions(+), 9 deletions(-)
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 22b5cd24f581..fd94404acc66 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -366,6 +366,9 @@ struct kimage {
phys_addr_t ima_buffer_addr;
size_t ima_buffer_size;
+
+ unsigned long ima_segment_index;
+ bool is_ima_segment_index_set;
#endif
/* Core ELF header buffer */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f989f5f1933b..bf758fd5062c 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -734,6 +734,14 @@ static int kexec_calculate_store_digests(struct kimage *image)
if (ksegment->kbuf == pi->purgatory_buf)
continue;
+ /*
+ * Skip the segment if ima_segment_index is set and matches
+ * the current index
+ */
+ if (image->is_ima_segment_index_set &&
+ i == image->ima_segment_index)
+ continue;
With this change, the IMA segment is not included in the digest
calculation, nor should it be included in the digest verification.
However, I'm not seeing the matching code change in the digest
verification.
Fair question.
But I don't think anything else needs to be done here.
The way kexec_calculate_store_digests() and verify_sha256_digest()
are implemented, it already skips verification of the segments if
the segment is not part of 'purgatory_sha_regions'.
In kexec_calculate_store_digests(), my change is to 'continue' when the
segment is the IMA segment when the function is going through all the
segments in a for loop [1].
Therefore in kexec_calculate_store_digests() -
- crypto_shash_update() is not called for IMA segment [1].
- sha_regions[j] is not updated with IMA segment [1].
- This 'sha_regions' variable later becomes 'purgatory_sha_regions'
in kexec_calculate_store_digests [1].
- and verify_sha256_digest() only verifies 'purgatory_sha_regions'[2].
Since IMA segment is not part of the 'purgatory_sha_regions', it is
not included in the verification as part of verify_sha256_digest().
I have pasted the relevant code below for quick reference [1][2].
Please make ignoring the IMA segment a separate patch.
Sure. Will do.
ret = crypto_shash_update(desc, ksegment->kbuf,
ksegment->bufsz);
if (ret)
...
...
...
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c29db699c996..49a6047dd8eb 100644
Suspending and resuming extending the measurement list should be a
separate patch as well, with its own patch description.
Sure. Will do.
/*
* Add template entry to the measurement list and hash table, and
* extend the pcr.
----------------------------------------------------------------------------------
Reference code in the context of kexec_calculate_store_digests()
and verify_sha256_digest() conversation above.
----------------------------------------------------------------------------------
[1]
https://lore.kernel.org/all/20231216010729.2904751-3-tusharsu@xxxxxxxxxxxxxxxxxxx/
/* Calculate and store the digest of segments */
static int kexec_calculate_store_digests(struct kimage *image)
{
...
...
for (j = i = 0; i < image->nr_segments; i++) {
struct kexec_segment *ksegment;
ksegment = &image->segment[i];
/*
* Skip purgatory as it will be modified once we put digest
* info in purgatory.
*/
if (ksegment->kbuf == pi->purgatory_buf)
continue;
+ /*
+ * Skip the segment if ima_segment_index is set and matches
+ * the current index
+ */
+ if (image->is_ima_segment_index_set &&
+ i == image->ima_segment_index)
+ continue;
+
ret = crypto_shash_update(desc, ksegment->kbuf,
ksegment->bufsz);
if (ret)
break;
/*
* Assume rest of the buffer is filled with zero and
* update digest accordingly.
*/
nullsz = ksegment->memsz - ksegment->bufsz;
while (nullsz) {
unsigned long bytes = nullsz;
if (bytes > zero_buf_sz)
bytes = zero_buf_sz;
ret = crypto_shash_update(desc, zero_buf, bytes);
if (ret)
break;
nullsz -= bytes;
}
if (ret)
break;
sha_regions[j].start = ksegment->mem;
sha_regions[j].len = ksegment->memsz;
j++;
}
if (!ret) {
ret = crypto_shash_final(desc, digest);
if (ret)
goto out_free_digest;
ret = kexec_purgatory_get_set_symbol(image,
"purgatory_sha_regions",
sha_regions, sha_region_sz, 0);
if (ret)
goto out_free_digest;
ret = kexec_purgatory_get_set_symbol(image,
"purgatory_sha256_digest",
digest, SHA256_DIGEST_SIZE, 0);
if (ret)
goto out_free_digest;
}
out_free_digest:
kfree(digest);
out_free_sha_regions:
vfree(sha_regions);
out_free_desc:
kfree(desc);
out_free_tfm:
kfree(tfm);
out:
return ret;
}
---------------------------------------------------------------------------
[2]
https://elixir.bootlin.com/linux/latest/source/arch/x86/purgatory/purgatory.c#L24
u8 purgatory_sha256_digest[SHA256_DIGEST_SIZE]
__section(".kexec-purgatory");
struct kexec_sha_region purgatory_sha_regions[KEXEC_SEGMENT_MAX]
__section(".kexec-purgatory");
static int verify_sha256_digest(void)
{
struct kexec_sha_region *ptr, *end;
u8 digest[SHA256_DIGEST_SIZE];
struct sha256_state sctx;
sha256_init(&sctx);
end = purgatory_sha_regions + ARRAY_SIZE(purgatory_sha_regions);
for (ptr = purgatory_sha_regions; ptr < end; ptr++)
sha256_update(&sctx, (uint8_t *)(ptr->start), ptr->len);
sha256_final(&sctx, digest);
if (memcmp(digest, purgatory_sha256_digest, sizeof(digest)))
return 1;
return 0;
}
Thanks,
Tushar
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
