Re: [PATCH v7 3/6] tpm: Support boot measurements
On 3/2/23 14:22, Ilias Apalodimas wrote:
Hi Eddie,
I found the issue. I still think we could squeeze things even more in our
abstraction. Specifically the measure_event() tcg2_agile_log_append()
contain some efi specific bits and I am trying to figure out if we can make
those more generic. However, that's not a show stopper for me.
[...]
+int tcg2_init_log(struct udevice *dev, struct tcg2_event_log *elog);
We have tcg2_init_log() and tcg2_log_init(). This is a bit confusing when
reading the code.
Since tcg2_log_init() is actually initializing the EventLog can we do
tcg2_init_log -> tcg2_prepare_log_buf or something along those lines?
Sure, sounds good.
+
+/**
+ * Begin measurements.
+ *
+ * @devTPM device
[...]
+static int tcg2_log_parse(struct udevice *dev, struct tcg2_event_log *elog)
+{
+ struct tpml_digest_values digest_list;
+ struct tcg_efi_spec_id_event *event;
+ struct tcg_pcr_event *log;
+ u32 calc_size;
+ u32 active;
+ u32 count;
+ u32 evsz;
+ u32 mask;
+ u16 algo;
+ u16 len;
+ int rc;
+ u32 i;
+ u16 j;
+
+ if (elog->log_size <= offsetof(struct tcg_pcr_event, event))
+ return 0;
+
+ log = (struct tcg_pcr_event *)elog->log;
+ if (get_unaligned_le32(>pcr_index) != 0 ||
+ get_unaligned_le32(>event_type) != EV_NO_ACTION)
+ return 0;
+
+ for (i = 0; i < sizeof(log->digest); i++) {
+ if (log->digest[i])
+ return 0;
+ }
+
+ evsz = get_unaligned_le32(>event_size);
+ if (evsz < offsetof(struct tcg_efi_spec_id_event, digest_sizes) ||
+ evsz + offsetof(struct tcg_pcr_event, event) > elog->log_size)
+ return 0;
+
+ event = (struct tcg_efi_spec_id_event *)log->event;
+ if (memcmp(event->signature, TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03,
+ sizeof(TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03)))
+ return 0;
+
+ if (event->spec_version_minor !=
TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2 ||
+ event->spec_version_major !=
TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2)
+ return 0;
+
+ count = get_unaligned_le32(>number_of_algorithms);
+ if (count > ARRAY_SIZE(tpm2_supported_algorithms))
+ return 0;
+
+ calc_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes) +
+ (sizeof(struct tcg_efi_spec_id_event_algorithm_size) * count) +
+ 1;
+ if (evsz != calc_size)
+ return 0;
+
+ rc = tcg2_get_active_pcr_banks(dev, );
+ if (rc)
+ return rc;
There was a check here in the previous version which I can't find. The
previous stage bootloader is creating an EventLog. Since it can't control
the TPM the pcr banks that end up in the EventLog are defined at compile
time. This isn't ideal, but we have 2 options here:
1. Check the hardware active PCR banks and report an error if there's a
mismatch (which is what the older version did)
2. Add the missing function of re-configuring the active banks to
whatever the previous bootloader tells us.
Obviously (2) is a better option, but I am fine if we just report an error
for now.
Yes I found it, and I will add that.
[...]
+ *((u8 *)ev + (event_size - 1)) = 0;
+ elog->log_position = log_size;
+
+ return 0;
+}
+
+static int tcg2_log_find_end(struct tcg2_event_log *elog, struct udevice *dev,
Can we find a better name for this? This basically replays an eventlog we
inherited from a previous stage boot loader into the TPM.
So something like tcg2_replay_eventlog()?
Sure.
+struct tpml_digest_values *digest_list)
+{
+ const u32 offset = offsetof(struct tcg_pcr_event2, digests) +
+ offsetof(struct tpml_digest_values, digests);
+ u32 event_size;
+ u32 count;
+ u16 algo;
+ u32 pcr;
+ u32 pos;
+ u16 len;
+ u8 *log;
+ int rc;
+ u32 i;
+
+ while (elog->log_position + offset < elog->log_size) {
+ log = elog->log + elog->log_position;
+
+ pos = offsetof(struct tcg_pcr_event2, pcr_index);
+ pcr = get_unaligned_le32(log + pos);
+ pos = offsetof(struct tcg_pcr_event2, event_type);
+ if (!get_unaligned_le32(log + pos))
+ return 0;
isn't this an actual error ?
Good point, and all below too. I will return errors there.
+
+ pos = offsetof(struct tcg_pcr_event2, digests) +
+ offsetof(struct tpml_digest_values, count);
+ count = get_unaligned_le32(log + pos);
+ if (count > ARRAY_SIZE(tpm2_supported_algorithms) ||
+ (digest_list->count && digest_list->count != count))
+ return 0;
ditto
+
+ pos = offsetof(struct tcg_pcr_event2,
Re: [PATCH v7 3/6] tpm: Support boot measurements
Hi Eddie,
I found the issue. I still think we could squeeze things even more in our
abstraction. Specifically the measure_event() tcg2_agile_log_append()
contain some efi specific bits and I am trying to figure out if we can make
those more generic. However, that's not a show stopper for me.
[...]
> +int tcg2_init_log(struct udevice *dev, struct tcg2_event_log *elog);
We have tcg2_init_log() and tcg2_log_init(). This is a bit confusing when
reading the code.
Since tcg2_log_init() is actually initializing the EventLog can we do
tcg2_init_log -> tcg2_prepare_log_buf or something along those lines?
> +
> +/**
> + * Begin measurements.
> + *
> + * @dev TPM device
[...]
> +static int tcg2_log_parse(struct udevice *dev, struct tcg2_event_log *elog)
> +{
> + struct tpml_digest_values digest_list;
> + struct tcg_efi_spec_id_event *event;
> + struct tcg_pcr_event *log;
> + u32 calc_size;
> + u32 active;
> + u32 count;
> + u32 evsz;
> + u32 mask;
> + u16 algo;
> + u16 len;
> + int rc;
> + u32 i;
> + u16 j;
> +
> + if (elog->log_size <= offsetof(struct tcg_pcr_event, event))
> + return 0;
> +
> + log = (struct tcg_pcr_event *)elog->log;
> + if (get_unaligned_le32(>pcr_index) != 0 ||
> + get_unaligned_le32(>event_type) != EV_NO_ACTION)
> + return 0;
> +
> + for (i = 0; i < sizeof(log->digest); i++) {
> + if (log->digest[i])
> + return 0;
> + }
> +
> + evsz = get_unaligned_le32(>event_size);
> + if (evsz < offsetof(struct tcg_efi_spec_id_event, digest_sizes) ||
> + evsz + offsetof(struct tcg_pcr_event, event) > elog->log_size)
> + return 0;
> +
> + event = (struct tcg_efi_spec_id_event *)log->event;
> + if (memcmp(event->signature, TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03,
> +sizeof(TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03)))
> + return 0;
> +
> + if (event->spec_version_minor !=
> TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2 ||
> + event->spec_version_major !=
> TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2)
> + return 0;
> +
> + count = get_unaligned_le32(>number_of_algorithms);
> + if (count > ARRAY_SIZE(tpm2_supported_algorithms))
> + return 0;
> +
> + calc_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes) +
> + (sizeof(struct tcg_efi_spec_id_event_algorithm_size) * count) +
> + 1;
> + if (evsz != calc_size)
> + return 0;
> +
> + rc = tcg2_get_active_pcr_banks(dev, );
> + if (rc)
> + return rc;
There was a check here in the previous version which I can't find. The
previous stage bootloader is creating an EventLog. Since it can't control
the TPM the pcr banks that end up in the EventLog are defined at compile
time. This isn't ideal, but we have 2 options here:
1. Check the hardware active PCR banks and report an error if there's a
mismatch (which is what the older version did)
2. Add the missing function of re-configuring the active banks to
whatever the previous bootloader tells us.
Obviously (2) is a better option, but I am fine if we just report an error
for now.
[...]
> + *((u8 *)ev + (event_size - 1)) = 0;
> + elog->log_position = log_size;
> +
> + return 0;
> +}
> +
> +static int tcg2_log_find_end(struct tcg2_event_log *elog, struct udevice
> *dev,
Can we find a better name for this? This basically replays an eventlog we
inherited from a previous stage boot loader into the TPM.
So something like tcg2_replay_eventlog()?
> + struct tpml_digest_values *digest_list)
> +{
> + const u32 offset = offsetof(struct tcg_pcr_event2, digests) +
> + offsetof(struct tpml_digest_values, digests);
> + u32 event_size;
> + u32 count;
> + u16 algo;
> + u32 pcr;
> + u32 pos;
> + u16 len;
> + u8 *log;
> + int rc;
> + u32 i;
> +
> + while (elog->log_position + offset < elog->log_size) {
> + log = elog->log + elog->log_position;
> +
> + pos = offsetof(struct tcg_pcr_event2, pcr_index);
> + pcr = get_unaligned_le32(log + pos);
> + pos = offsetof(struct tcg_pcr_event2, event_type);
> + if (!get_unaligned_le32(log + pos))
> + return 0;
isn't this an actual error ?
> +
> + pos = offsetof(struct tcg_pcr_event2, digests) +
> + offsetof(struct tpml_digest_values, count);
> + count = get_unaligned_le32(log + pos);
> + if (count > ARRAY_SIZE(tpm2_supported_algorithms) ||
> + (digest_list->count && digest_list->count != count))
> + return 0;
ditto
> +
> + pos = offsetof(struct tcg_pcr_event2, digests) +
> + offsetof(struct tpml_digest_values, digests);
> + for (i = 0; i < count; ++i) {
> +
Re: [PATCH v7 3/6] tpm: Support boot measurements
Hi Eddie,
On Thu, 2 Mar 2023 at 16:17, Ilias Apalodimas
wrote:
>
> Hi Eddie,
>
> The good news, is that this generally seems to be working and is really
> close to what I had in mind on code re-usage. Thanks for the patience!
>
> The bad new now is that I think I found one last (famous last words)
> problem
>
> [...]
>
> > + }
> > +
> > + /* Read PCR0 to check if previous firmware extended the PCRs or not.
> > */
> > + rc = tcg2_pcr_read(dev, 0, _list);
> > + if (rc)
> > + return rc;
> > +
>
> This is changing how the code used to work and I think the new way of doing
> it is wrong.
> First of all the check above doesn't check that PCR0 is indeed 0. It
> simply checks we can *read* that PCR.
>
> > + for (i = 0; i < digest_list.count; ++i) {
> > + len = tpm2_algorithm_to_len(digest_list.digests[i].hash_alg);
> > + for (j = 0; j < len; ++j) {
> > + if (digest_list.digests[i].digest.sha512[j])
> > + break;
> > + }
> > +
> > + /* PCR is non-zero; it has been extended, so skip extending.
> > */
>
> I don't think we need this tbh. The previous bootloader would have either
> extended some of the PCRs along with the EventLog construction or he hasn't.
> If it did indeed extend the PCRs then PCR0 should be != 0 since it must
> contain a measurement of EV_S_CRTM_VERSION. So looking at PCR0 should be
> sufficient to trigger replaying the EventLog or not.
> If the previous loader managed to mess up somehow, I don't think it should
> be U-Boot's job to fix the mess :)
I actually misread the patch here. This is indeed only checking PCR0.
I'll go check why one event is missing and get back to you.
>
> > + if (j != len) {
> > + digest_list.count = 0;
> > + break;
> > + }
> > + }
> > +
> > + elog->log_position = offsetof(struct tcg_pcr_event, event) + evsz;
> > + rc = tcg2_log_find_end(elog, dev, _list);
> > + if (rc)
> > + return rc;
> > +
> > + elog->found = true;
> > + return 0;
> > +}
> > +
>
> P.S: I did test this using TF-A and re-using the 'forwarded' EventLog. I
> can see all the events replayed correctly apart from the last one, so i'll
> keep looking in case something else is missing
>
> Regards
> /Ilias
Re: [PATCH v7 3/6] tpm: Support boot measurements
Hi Eddie,
The good news, is that this generally seems to be working and is really
close to what I had in mind on code re-usage. Thanks for the patience!
The bad new now is that I think I found one last (famous last words)
problem
[...]
> + }
> +
> + /* Read PCR0 to check if previous firmware extended the PCRs or not. */
> + rc = tcg2_pcr_read(dev, 0, _list);
> + if (rc)
> + return rc;
> +
This is changing how the code used to work and I think the new way of doing
it is wrong.
First of all the check above doesn't check that PCR0 is indeed 0. It
simply checks we can *read* that PCR.
> + for (i = 0; i < digest_list.count; ++i) {
> + len = tpm2_algorithm_to_len(digest_list.digests[i].hash_alg);
> + for (j = 0; j < len; ++j) {
> + if (digest_list.digests[i].digest.sha512[j])
> + break;
> + }
> +
> + /* PCR is non-zero; it has been extended, so skip extending. */
I don't think we need this tbh. The previous bootloader would have either
extended some of the PCRs along with the EventLog construction or he hasn't.
If it did indeed extend the PCRs then PCR0 should be != 0 since it must
contain a measurement of EV_S_CRTM_VERSION. So looking at PCR0 should be
sufficient to trigger replaying the EventLog or not.
If the previous loader managed to mess up somehow, I don't think it should
be U-Boot's job to fix the mess :)
> + if (j != len) {
> + digest_list.count = 0;
> + break;
> + }
> + }
> +
> + elog->log_position = offsetof(struct tcg_pcr_event, event) + evsz;
> + rc = tcg2_log_find_end(elog, dev, _list);
> + if (rc)
> + return rc;
> +
> + elog->found = true;
> + return 0;
> +}
> +
P.S: I did test this using TF-A and re-using the 'forwarded' EventLog. I
can see all the events replayed correctly apart from the last one, so i'll
keep looking in case something else is missing
Regards
/Ilias
[PATCH v7 3/6] tpm: Support boot measurements
Add TPM2 functions to support boot measurement. This includes
starting up the TPM, initializing/appending the event log, and
measuring the U-Boot version. Much of the code was used in the
EFI subsystem, so remove it there and use the common functions.
Signed-off-by: Eddie James
---
Changes since v6:
- Added Linaro copyright for all the EFI moved code
- Changed tcg2_init_log (and by extension, tcg2_measurement_init) to
copy any discovered event log to the user's log if passed in.
Changes since v5:
- Remove unused platform_get_eventlog in efi_tcg2.c
- First look for tpm_event_log_* properties instead of linux,sml-*
- Fix efi_tcg2.c compilation
- Select SHA* configs
Changes since v4:
- Remove tcg2_measure_event function and check for NULL data in
tcg2_measure_data
- Use tpm_auto_startup
- Fix efi_tcg2.c compilation for removing tcg2_pcr_read function
Changes since v3:
- Reordered headers
- Refactored more of EFI code into common code
Removed digest_info structure and instead used the common alg_to_mask
and alg_to_len
Improved event log parsing in common code to get it equivalent to EFI
Common code now extends PCR if previous bootloader stage couldn't
No need to allocate memory in the common code, so EFI copies the
discovered buffer like it did before
Rename efi measure_event function
Changes since v1:
- Refactor TPM layer functions to allow EFI system to use them, and
remove duplicate EFI functions.
include/efi_tcg2.h| 44 --
include/tpm-v2.h | 248 +
lib/Kconfig |4 +
lib/efi_loader/efi_tcg2.c | 1054 +++--
lib/tpm-v2.c | 804
5 files changed, 1133 insertions(+), 1021 deletions(-)
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
index b1c3abd097..b21c5cb3dd 100644
--- a/include/efi_tcg2.h
+++ b/include/efi_tcg2.h
@@ -129,50 +129,6 @@ struct efi_tcg2_boot_service_capability {
#define BOOT_SERVICE_CAPABILITY_MIN \
offsetof(struct efi_tcg2_boot_service_capability, number_of_pcr_banks)
-#define TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03 "Spec ID Event03"
-#define TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2 2
-#define TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2 0
-#define TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_ERRATA_TPM2 2
-
-/**
- * struct TCG_EfiSpecIdEventAlgorithmSize - hashing algorithm information
- *
- * @algorithm_id: algorithm defined in enum tpm2_algorithms
- * @digest_size: size of the algorithm
- */
-struct tcg_efi_spec_id_event_algorithm_size {
- u16 algorithm_id;
- u16 digest_size;
-} __packed;
-
-/**
- * struct TCG_EfiSpecIDEventStruct - content of the event log header
- *
- * @signature: signature, set to Spec ID Event03
- * @platform_class:class defined in TCG ACPI Specification
- * Client Common Header.
- * @spec_version_minor:minor version
- * @spec_version_major:major version
- * @spec_version_errata: major version
- * @uintn_size:size of the efi_uintn_t fields used in
various
- * data structures used in this specification.
- * 0x01 indicates u32 and 0x02 indicates u64
- * @number_of_algorithms: hashing algorithms used in this event log
- * @digest_sizes: array of number_of_algorithms pairs
- * 1st member defines the algorithm id
- * 2nd member defines the algorithm size
- */
-struct tcg_efi_spec_id_event {
- u8 signature[16];
- u32 platform_class;
- u8 spec_version_minor;
- u8 spec_version_major;
- u8 spec_errata;
- u8 uintn_size;
- u32 number_of_algorithms;
- struct tcg_efi_spec_id_event_algorithm_size digest_sizes[];
-} __packed;
-
/**
* struct tdEFI_TCG2_FINAL_EVENTS_TABLE - log entries after Get Event Log
* @version: version number for this structure
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index 6684033deb..c491a58b02 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -216,6 +216,50 @@ struct tcg_pcr_event2 {
u8 event[];
} __packed;
+/**
+ * struct TCG_EfiSpecIdEventAlgorithmSize - hashing algorithm information
+ *
+ * @algorithm_id: algorithm defined in enum tpm2_algorithms
+ * @digest_size: size of the algorithm
+ */
+struct tcg_efi_spec_id_event_algorithm_size {
+ u16 algorithm_id;
+ u16 digest_size;
+} __packed;
+
+#define TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03 "Spec ID Event03"
+#define TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2 2
+#define TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2 0
+#define TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_ERRATA_TPM2 2
+
+/**
+ * struct TCG_EfiSpecIDEventStruct - content of the event log header
+ *
+ * @signature: signature, set to Spec ID Event03
+ *
