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

[Mimi Zohar <zohar@linux.ibm.com>]

Hi Tushar,

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

Thanks!

> >> 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().
> 
> > Please make ignoring the IMA segment a separate patch.
> > 
> Sure. Will do.

Thank you for the explanation.  Please include in the patch description a
statement about the "sha_regions" not including the IMA segment, so nothing is
needed on the verify side.

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

Thanks!

Mimi