Re: [PATCH v3 2/7] ima: kexec: move ima log copy from kexec load to execute

[Tushar Sugandhi <tusharsu@linux.microsoft.com>]

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@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