[PATCH v3 2/7] ima: kexec: move ima log copy from kexec load to execute
Tushar Sugandhi
tusharsu at linux.microsoft.com
Thu Jan 11 15:29:10 PST 2024
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 at linux.microsoft.com>
>
> 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@linux.microsoft.com/
/* 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
More information about the kexec
mailing list