Re: [PATCH 5/6] ima: measure TPM update counter at ima_init

From: Mimi Zohar <zohar@linux.ibm.com>
To: Tushar Sugandhi <tusharsu@linux.microsoft.com>,
	noodles@fb.com, bauermann@kolabnow.com, ebiederm@xmission.com,
	bhe@redhat.com, vgoyal@redhat.com, dyoung@redhat.com,
	peterhuewe@gmx.de, jarkko@kernel.org, jgg@ziepe.ca,
	kexec@lists.infradead.org, linux-integrity@vger.kernel.org
Cc: code@tyhicks.com, nramas@linux.microsoft.com, paul@paul-moore.com
Subject: Re: [PATCH 5/6] ima: measure TPM update counter at ima_init
Date: Thu, 03 Aug 2023 21:18:57 -0400	[thread overview]
Message-ID: <e9ab98a823c2e6c536fb2acbe86229a0085bd34a.camel@linux.ibm.com> (raw)
In-Reply-To: <da91b56a-83c8-d12e-71f8-887b6701f021@linux.microsoft.com>

On Thu, 2023-08-03 at 16:34 -0700, Tushar Sugandhi wrote:

> >> +++ b/security/integrity/ima/ima_init.c
> >> @@ -154,5 +154,8 @@ int __init ima_init(void)
> >>   				  UTS_RELEASE, strlen(UTS_RELEASE), false,
> >>   				  NULL, 0);
> >>   
> >> +	/* Measures TPM update counter at ima_init */
> >> +	ima_measure_update_counter("ima_init_tpm_update_counter");
> >> +
> > With "ima_policy=critical_data" on the boot command line, the IMA
> > measurement list record looks like:
> >
> > 6e190cc643ff0b718485966a0300473baedface735 ima_init_tpm_update_counter 7570646174655f636f756e7465723d3330383b
> >
> > Please change the "ima_init_tpm_update_counter" to something shorter
> > and the hex encoded ascii string and pcr counter to something readable.
> I believe you are seeing the above line in ascill_runtime_measurements log.

Yes, the ascii_runtime_measurements are suppose to be readable to the
end user.

> The ascii logging format is consistent with other event data for 
> critical_data event e.g. kernel_version.

Then you got it wrong.

> 10 8f449175bbf88bc55fc1127466628c39a3957d15 ima-buf 
> sha1:4acab4fbb08db663b7b7b4528e8729187d726782 kernel_version 
> 362e332e302d7263332b
> 10 f10678b63c4b2529339dff02282e63d9c6bb0385 ima-buf 
> sha1:d8c187524412f74a961f2051a9529c009e700337 
> ima_init_tpm_update_counter 7570646174655f636f756e7465723d3133303b
> 
> Entries in the binary runtime measurements look readable to me.

You've inverted the meaning of the ascii and binary runtime measurement
lists.  For comparison look at the ima-ng/ima-sig records.

> ima_init_tpm_update_counter update_counter=130;
> ...
> kexec_load_tpm_update_counte rupdate_counter=133;
> 
> Please let me know if you still want me to change the format.

OI course the ascii measurement list should be human readable.

> > Perhaps name this critical-data "tpm" and "tpm-info", similar to the
>  From patch 4/6:
> +    result = ima_measure_critical_data("tpm_pcr_update_counter", 
> event_name,
> +                  buf, buf_len, false, NULL, 0);
> 
> The critical_data event_label value is currently set to 
> "tpm_pcr_update_counter".

Why is the string so long?   Long strings or variables don't make the
code or logs more understandable.  Please shorten both the strings and
variables.

> I can rename event_label to "tpm-info", so that the admins can filter the
> event in IMA policy based on the label if needed.

The new record needs to be self containd and verifiable.  The
additional info I suggested were just examples.  Please take the time
to consider what needs to be included in the new record.  Decide
whether this is a TPM security critical data record.  Only then, decide
on the naming.

-- 
thanks,

Mimi

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec