This series implements some further cleanups I saw while reviewing the TPM code. Some of the TPM functions take several parameters and this series attempts to simplify that.
I've only compile tested this.
The series is also available at:
https://github.com/KevinOConnor/seabios/tree/testing
-Kevin
Kevin O'Connor (8): tpm: Don't pass entry_count around in parameters to/from tpm_extend_acpi_log() tpm: There is no need to pass pcrindex to hash_log_extend_event() tpm: Perform hashing separately from logging tpm: Use pcpes->event[] to pass data to tpm_extend_acpi_log() tpm: Avoid scatter-gather copying in build_and_send_cmd() tpm: Don't implement scatter-gather in transmit() tpm: Merge tpm_log_event() and tpm_extend_acpi_log() tpm: Merge tpm_log_extend_event() and tpm_extend(); extend before logging
src/std/tcg.h | 6 +- src/tcgbios.c | 257 ++++++++++++++++++++-------------------------------------- src/tcgbios.h | 8 -- 3 files changed, 87 insertions(+), 184 deletions(-)
Now that entry_count is in a global variable there is no need to pass it around as function parameters.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/tcgbios.c | 34 +++++++++++----------------------- 1 file changed, 11 insertions(+), 23 deletions(-)
diff --git a/src/tcgbios.c b/src/tcgbios.c index c6782ee..5461a54 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -285,15 +285,13 @@ reset_acpi_log(void) * pcpes : Pointer to the event 'header' to be copied into the log * event : Pointer to the event 'body' to be copied into the log * event_length: Length of the event array - * entry_count : optional pointer to get the current entry count * * Output: * Returns an error code in case of faiure, 0 in case of success */ static u32 tpm_extend_acpi_log(struct pcpes *pcpes, - const char *event, u32 event_length, - u16 *entry_count) + const char *event, u32 event_length) { u32 size;
@@ -321,9 +319,6 @@ tpm_extend_acpi_log(struct pcpes *pcpes, tpm_state.log_area_next_entry += size; tpm_state.entry_count++;
- if (entry_count) - *entry_count = tpm_state.entry_count; - return 0; }
@@ -520,8 +515,7 @@ tpm_extend(u8 *hash, u32 pcrindex) static u32 hash_log_event(const void *hashdata, u32 hashdata_length, struct pcpes *pcpes, - const char *event, u32 event_length, - u16 *entry_count) + const char *event, u32 event_length) { u32 rc = 0;
@@ -537,7 +531,7 @@ hash_log_event(const void *hashdata, u32 hashdata_length, if (!has_working_tpm()) return TCG_GENERAL_ERROR;
- rc = tpm_extend_acpi_log(pcpes, event, event_length, entry_count); + rc = tpm_extend_acpi_log(pcpes, event, event_length); if (rc) tpm_set_failure(); return rc; @@ -547,12 +541,12 @@ static u32 hash_log_extend_event(const void *hashdata, u32 hashdata_length, struct pcpes *pcpes, const char *event, u32 event_length, - u32 pcrindex, u16 *entry_count) + u32 pcrindex) { u32 rc;
rc = hash_log_event(hashdata, hashdata_length, pcpes, - event, event_length, entry_count); + event, event_length); if (rc) return rc;
@@ -580,11 +574,8 @@ tpm_add_measurement_to_log(u32 pcrindex, u32 event_type, .pcrindex = pcrindex, .eventtype = event_type, }; - u16 entry_count; - return hash_log_extend_event(hashdata, hashdata_length, &pcpes, - event, event_length, pcrindex, - &entry_count); + event, event_length, pcrindex); }
@@ -1004,7 +995,7 @@ hash_log_extend_event_int(const struct hleei_short *hleei_s, rc = hash_log_extend_event(hleei_s->hashdataptr, hleei_s->hashdatalen, pcpes, (char *)&pcpes->event, pcpes->eventdatasize, - pcrindex, NULL); + pcrindex); if (rc) goto err_exit;
@@ -1084,7 +1075,6 @@ hash_log_event_int(const struct hlei *hlei, struct hleo *hleo) u32 rc = 0; u16 size; struct pcpes *pcpes; - u16 entry_count;
if (is_preboot_if_shutdown() != 0) { rc = TCG_INTERFACE_SHUTDOWN; @@ -1109,15 +1099,14 @@ hash_log_event_int(const struct hlei *hlei, struct hleo *hleo) }
rc = hash_log_event(hlei->hashdataptr, hlei->hashdatalen, - pcpes, (char *)&pcpes->event, pcpes->eventdatasize, - &entry_count); + pcpes, (char *)&pcpes->event, pcpes->eventdatasize); if (rc) goto err_exit;
/* updating the log was fine */ hleo->opblength = sizeof(struct hleo); hleo->reserved = 0; - hleo->eventnumber = entry_count; + hleo->eventnumber = tpm_state.entry_count;
err_exit: if (rc != 0) { @@ -1174,7 +1163,6 @@ compact_hash_log_extend_event_int(u8 *buffer, .eventdatasize = sizeof(info), .event = info, }; - u16 entry_count;
if (is_preboot_if_shutdown() != 0) return TCG_INTERFACE_SHUTDOWN; @@ -1182,10 +1170,10 @@ compact_hash_log_extend_event_int(u8 *buffer, rc = hash_log_extend_event(buffer, length, &pcpes, (char *)&pcpes.event, pcpes.eventdatasize, - pcpes.pcrindex, &entry_count); + pcpes.pcrindex);
if (rc == 0) - *edx_ptr = entry_count; + *edx_ptr = tpm_state.entry_count;
return rc; }
The pcrindex is already in pcpes->pcrindex, so no need to pass it as a parameter.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/tcgbios.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/tcgbios.c b/src/tcgbios.c index 5461a54..1fb8e5c 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -540,8 +540,7 @@ hash_log_event(const void *hashdata, u32 hashdata_length, static u32 hash_log_extend_event(const void *hashdata, u32 hashdata_length, struct pcpes *pcpes, - const char *event, u32 event_length, - u32 pcrindex) + const char *event, u32 event_length) { u32 rc;
@@ -550,7 +549,7 @@ hash_log_extend_event(const void *hashdata, u32 hashdata_length, if (rc) return rc;
- return tpm_extend(pcpes->digest, pcrindex); + return tpm_extend(pcpes->digest, pcpes->pcrindex); }
/* @@ -575,7 +574,7 @@ tpm_add_measurement_to_log(u32 pcrindex, u32 event_type, .eventtype = event_type, }; return hash_log_extend_event(hashdata, hashdata_length, &pcpes, - event, event_length, pcrindex); + event, event_length); }
@@ -994,8 +993,7 @@ hash_log_extend_event_int(const struct hleei_short *hleei_s,
rc = hash_log_extend_event(hleei_s->hashdataptr, hleei_s->hashdatalen, pcpes, - (char *)&pcpes->event, pcpes->eventdatasize, - pcrindex); + (char *)&pcpes->event, pcpes->eventdatasize); if (rc) goto err_exit;
@@ -1169,8 +1167,7 @@ compact_hash_log_extend_event_int(u8 *buffer,
rc = hash_log_extend_event(buffer, length, &pcpes, - (char *)&pcpes.event, pcpes.eventdatasize, - pcpes.pcrindex); + (char *)&pcpes.event, pcpes.eventdatasize);
if (rc == 0) *edx_ptr = tpm_state.entry_count;
Instead of calculating the hash in hash_log_event(), create a new function (tpm_fill_hash) that will create the hash, and update all callers to use tpm_fill_hash() before calling hash_log_event(). This reduce the number of parameters to hash_log_event().
Rename hash_log_event() and hash_log_extent_event() to tpm_log_event() and tpm_log_extend_event() now that these functions no longer implement the hashing.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/tcgbios.c | 54 +++++++++++++++++++++--------------------------------- 1 file changed, 21 insertions(+), 33 deletions(-)
diff --git a/src/tcgbios.c b/src/tcgbios.c index 1fb8e5c..0c28935 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -513,45 +513,36 @@ tpm_extend(u8 *hash, u32 pcrindex) }
static u32 -hash_log_event(const void *hashdata, u32 hashdata_length, - struct pcpes *pcpes, - const char *event, u32 event_length) +tpm_log_event(struct pcpes *pcpes, const char *event, u32 event_length) { - u32 rc = 0; - if (pcpes->pcrindex >= 24) return TCG_INVALID_INPUT_PARA;
- if (hashdata) { - rc = sha1(hashdata, hashdata_length, pcpes->digest); - if (rc) - return rc; - } - if (!has_working_tpm()) return TCG_GENERAL_ERROR;
- rc = tpm_extend_acpi_log(pcpes, event, event_length); + u32 rc = tpm_extend_acpi_log(pcpes, event, event_length); if (rc) tpm_set_failure(); return rc; }
static u32 -hash_log_extend_event(const void *hashdata, u32 hashdata_length, - struct pcpes *pcpes, - const char *event, u32 event_length) +tpm_log_extend_event(struct pcpes *pcpes, const char *event, u32 event_length) { - u32 rc; - - rc = hash_log_event(hashdata, hashdata_length, pcpes, - event, event_length); + u32 rc = tpm_log_event(pcpes, event, event_length); if (rc) return rc; - return tpm_extend(pcpes->digest, pcpes->pcrindex); }
+static void +tpm_fill_hash(struct pcpes *pcpes, const void *hashdata, u32 hashdata_length) +{ + if (hashdata) + sha1(hashdata, hashdata_length, pcpes->digest); +} + /* * Add a measurement to the log; the data at data_seg:data/length are * appended to the TCG_PCClientPCREventStruct @@ -573,8 +564,8 @@ tpm_add_measurement_to_log(u32 pcrindex, u32 event_type, .pcrindex = pcrindex, .eventtype = event_type, }; - return hash_log_extend_event(hashdata, hashdata_length, &pcpes, - event, event_length); + tpm_fill_hash(&pcpes, hashdata, hashdata_length); + return tpm_log_extend_event(&pcpes, event, event_length); }
@@ -991,9 +982,9 @@ hash_log_extend_event_int(const struct hleei_short *hleei_s, goto err_exit; }
- rc = hash_log_extend_event(hleei_s->hashdataptr, hleei_s->hashdatalen, - pcpes, - (char *)&pcpes->event, pcpes->eventdatasize); + tpm_fill_hash(pcpes, hleei_s->hashdataptr, hleei_s->hashdatalen); + rc = tpm_log_extend_event(pcpes + , (char *)&pcpes->event, pcpes->eventdatasize); if (rc) goto err_exit;
@@ -1008,7 +999,6 @@ err_exit: }
return rc; - }
static u32 @@ -1096,8 +1086,8 @@ hash_log_event_int(const struct hlei *hlei, struct hleo *hleo) goto err_exit; }
- rc = hash_log_event(hlei->hashdataptr, hlei->hashdatalen, - pcpes, (char *)&pcpes->event, pcpes->eventdatasize); + tpm_fill_hash(pcpes, hlei->hashdataptr, hlei->hashdatalen); + rc = tpm_log_event(pcpes, (char *)&pcpes->event, pcpes->eventdatasize); if (rc) goto err_exit;
@@ -1154,7 +1144,6 @@ compact_hash_log_extend_event_int(u8 *buffer, u32 pcrindex, u32 *edx_ptr) { - u32 rc = 0; struct pcpes pcpes = { .pcrindex = pcrindex, .eventtype = EV_COMPACT_HASH, @@ -1165,10 +1154,9 @@ compact_hash_log_extend_event_int(u8 *buffer, if (is_preboot_if_shutdown() != 0) return TCG_INTERFACE_SHUTDOWN;
- rc = hash_log_extend_event(buffer, length, - &pcpes, - (char *)&pcpes.event, pcpes.eventdatasize); - + tpm_fill_hash(&pcpes, buffer, length); + u32 rc = tpm_log_extend_event(&pcpes, + (char *)&pcpes.event, pcpes.eventdatasize); if (rc == 0) *edx_ptr = tpm_state.entry_count;
Instead of passing in the event data to tpm_extend_acpi_log() via parameters, use the event[] field in the pcpes. Update those callers that don't populate the pcpes->event to do so prior to calling tpm_extend_acpi_log().
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/std/tcg.h | 2 +- src/tcgbios.c | 76 +++++++++++++++++++++++++++++------------------------------ 2 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/src/std/tcg.h b/src/std/tcg.h index 88b2688..5f9cad9 100644 --- a/src/std/tcg.h +++ b/src/std/tcg.h @@ -207,7 +207,7 @@ struct pcpes u32 eventtype; u8 digest[SHA1_BUFSIZE]; u32 eventdatasize; - u32 event; + u8 event[0]; } PACKED;
struct pcctes diff --git a/src/tcgbios.c b/src/tcgbios.c index 0c28935..e7adf3f 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -290,18 +290,15 @@ reset_acpi_log(void) * Returns an error code in case of faiure, 0 in case of success */ static u32 -tpm_extend_acpi_log(struct pcpes *pcpes, - const char *event, u32 event_length) +tpm_extend_acpi_log(struct pcpes *pcpes) { - u32 size; - dprintf(DEBUG_tcg, "TCGBIOS: LASA = %p, next entry = %p\n", tpm_state.log_area_start_address, tpm_state.log_area_next_entry);
if (tpm_state.log_area_next_entry == NULL) return TCG_PC_LOGOVERFLOW;
- size = offsetof(struct pcpes, event) + event_length; + u32 size = sizeof(*pcpes) + pcpes->eventdatasize;
if ((tpm_state.log_area_next_entry + size - tpm_state.log_area_start_address) > tpm_state.log_area_minimum_length) { @@ -309,11 +306,7 @@ tpm_extend_acpi_log(struct pcpes *pcpes, return TCG_PC_LOGOVERFLOW; }
- pcpes->eventdatasize = event_length; - - memcpy(tpm_state.log_area_next_entry, pcpes, offsetof(struct pcpes, event)); - memcpy(tpm_state.log_area_next_entry + offsetof(struct pcpes, event), - event, event_length); + memcpy(tpm_state.log_area_next_entry, pcpes, size);
tpm_state.log_area_last_entry = tpm_state.log_area_next_entry; tpm_state.log_area_next_entry += size; @@ -513,7 +506,7 @@ tpm_extend(u8 *hash, u32 pcrindex) }
static u32 -tpm_log_event(struct pcpes *pcpes, const char *event, u32 event_length) +tpm_log_event(struct pcpes *pcpes) { if (pcpes->pcrindex >= 24) return TCG_INVALID_INPUT_PARA; @@ -521,16 +514,16 @@ tpm_log_event(struct pcpes *pcpes, const char *event, u32 event_length) if (!has_working_tpm()) return TCG_GENERAL_ERROR;
- u32 rc = tpm_extend_acpi_log(pcpes, event, event_length); + u32 rc = tpm_extend_acpi_log(pcpes); if (rc) tpm_set_failure(); return rc; }
static u32 -tpm_log_extend_event(struct pcpes *pcpes, const char *event, u32 event_length) +tpm_log_extend_event(struct pcpes *pcpes) { - u32 rc = tpm_log_event(pcpes, event, event_length); + u32 rc = tpm_log_event(pcpes); if (rc) return rc; return tpm_extend(pcpes->digest, pcpes->pcrindex); @@ -560,12 +553,21 @@ tpm_add_measurement_to_log(u32 pcrindex, u32 event_type, const char *event, u32 event_length, const u8 *hashdata, u32 hashdata_length) { - struct pcpes pcpes = { - .pcrindex = pcrindex, - .eventtype = event_type, + struct { + struct pcpes pcpes; + u8 event[128]; + } PACKED ev = { + .pcpes.pcrindex = pcrindex, + .pcpes.eventtype = event_type, + .pcpes.eventdatasize = event_length, }; - tpm_fill_hash(&pcpes, hashdata, hashdata_length); - return tpm_log_extend_event(&pcpes, event, event_length); + if (event_length > sizeof(ev.event)) { + warn_internalerror(); + return TCG_FIRMWARE_ERROR; + } + memcpy(ev.event, event, event_length); + tpm_fill_hash(&ev.pcpes, hashdata, hashdata_length); + return tpm_log_extend_event(&ev.pcpes); }
@@ -975,16 +977,14 @@ hash_log_extend_event_int(const struct hleei_short *hleei_s,
pcpes = (struct pcpes *)logdataptr;
- if (pcpes->pcrindex >= 24 || - pcpes->pcrindex != pcrindex || - logdatalen != offsetof(struct pcpes, event) + pcpes->eventdatasize) { + if (pcpes->pcrindex >= 24 || pcpes->pcrindex != pcrindex + || logdatalen != sizeof(*pcpes) + pcpes->eventdatasize) { rc = TCG_INVALID_INPUT_PARA; goto err_exit; }
tpm_fill_hash(pcpes, hleei_s->hashdataptr, hleei_s->hashdatalen); - rc = tpm_log_extend_event(pcpes - , (char *)&pcpes->event, pcpes->eventdatasize); + rc = tpm_log_extend_event(pcpes); if (rc) goto err_exit;
@@ -1077,17 +1077,15 @@ hash_log_event_int(const struct hlei *hlei, struct hleo *hleo)
pcpes = (struct pcpes *)hlei->logdataptr;
- if (pcpes->pcrindex >= 24 || - pcpes->pcrindex != hlei->pcrindex || - pcpes->eventtype != hlei->logeventtype || - hlei->logdatalen != - offsetof(struct pcpes, event) + pcpes->eventdatasize) { + if (pcpes->pcrindex >= 24 || pcpes->pcrindex != hlei->pcrindex + || pcpes->eventtype != hlei->logeventtype + || hlei->logdatalen != sizeof(*pcpes) + pcpes->eventdatasize) { rc = TCG_INVALID_INPUT_PARA; goto err_exit; }
tpm_fill_hash(pcpes, hlei->hashdataptr, hlei->hashdatalen); - rc = tpm_log_event(pcpes, (char *)&pcpes->event, pcpes->eventdatasize); + rc = tpm_log_event(pcpes); if (rc) goto err_exit;
@@ -1144,19 +1142,21 @@ compact_hash_log_extend_event_int(u8 *buffer, u32 pcrindex, u32 *edx_ptr) { - struct pcpes pcpes = { - .pcrindex = pcrindex, - .eventtype = EV_COMPACT_HASH, - .eventdatasize = sizeof(info), - .event = info, + struct { + struct pcpes pcpes; + u32 event; + } PACKED ev = { + .pcpes.pcrindex = pcrindex, + .pcpes.eventtype = EV_COMPACT_HASH, + .pcpes.eventdatasize = sizeof(info), + .event = info, };
if (is_preboot_if_shutdown() != 0) return TCG_INTERFACE_SHUTDOWN;
- tpm_fill_hash(&pcpes, buffer, length); - u32 rc = tpm_log_extend_event(&pcpes, - (char *)&pcpes.event, pcpes.eventdatasize); + tpm_fill_hash(&ev.pcpes, buffer, length); + u32 rc = tpm_log_extend_event(&ev.pcpes); if (rc == 0) *edx_ptr = tpm_state.entry_count;
On 11/22/2015 08:02 PM, Kevin O'Connor wrote:
Instead of passing in the event data to tpm_extend_acpi_log() via parameters, use the event[] field in the pcpes. Update those callers that don't populate the pcpes->event to do so prior to calling tpm_extend_acpi_log().
Signed-off-by: Kevin O'Connor kevin@koconnor.net
src/std/tcg.h | 2 +- src/tcgbios.c | 76 +++++++++++++++++++++++++++++------------------------------ 2 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/src/std/tcg.h b/src/std/tcg.h index 88b2688..5f9cad9 100644 --- a/src/std/tcg.h +++ b/src/std/tcg.h @@ -207,7 +207,7 @@ struct pcpes u32 eventtype; u8 digest[SHA1_BUFSIZE]; u32 eventdatasize;
- u32 event;
u8 event[0]; } PACKED;
struct pcctes
diff --git a/src/tcgbios.c b/src/tcgbios.c index 0c28935..e7adf3f 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -290,18 +290,15 @@ reset_acpi_log(void)
- Returns an error code in case of faiure, 0 in case of success
*/ static u32 -tpm_extend_acpi_log(struct pcpes *pcpes,
const char *event, u32 event_length)+tpm_extend_acpi_log(struct pcpes *pcpes) {
u32 size;
dprintf(DEBUG_tcg, "TCGBIOS: LASA = %p, next entry = %p\n", tpm_state.log_area_start_address, tpm_state.log_area_next_entry); if (tpm_state.log_area_next_entry == NULL) return TCG_PC_LOGOVERFLOW;size = offsetof(struct pcpes, event) + event_length;
u32 size = sizeof(*pcpes) + pcpes->eventdatasize;
if ((tpm_state.log_area_next_entry + size - tpm_state.log_area_start_address) > tpm_state.log_area_minimum_length) {
@@ -309,11 +306,7 @@ tpm_extend_acpi_log(struct pcpes *pcpes, return TCG_PC_LOGOVERFLOW; }
- pcpes->eventdatasize = event_length;
- memcpy(tpm_state.log_area_next_entry, pcpes, offsetof(struct pcpes, event));
- memcpy(tpm_state.log_area_next_entry + offsetof(struct pcpes, event),
event, event_length);
memcpy(tpm_state.log_area_next_entry, pcpes, size);
tpm_state.log_area_last_entry = tpm_state.log_area_next_entry; tpm_state.log_area_next_entry += size;
@@ -513,7 +506,7 @@ tpm_extend(u8 *hash, u32 pcrindex) }
static u32 -tpm_log_event(struct pcpes *pcpes, const char *event, u32 event_length) +tpm_log_event(struct pcpes *pcpes) { if (pcpes->pcrindex >= 24) return TCG_INVALID_INPUT_PARA; @@ -521,16 +514,16 @@ tpm_log_event(struct pcpes *pcpes, const char *event, u32 event_length) if (!has_working_tpm()) return TCG_GENERAL_ERROR;
- u32 rc = tpm_extend_acpi_log(pcpes, event, event_length);
u32 rc = tpm_extend_acpi_log(pcpes); if (rc) tpm_set_failure(); return rc; }
static u32
-tpm_log_extend_event(struct pcpes *pcpes, const char *event, u32 event_length) +tpm_log_extend_event(struct pcpes *pcpes) {
- u32 rc = tpm_log_event(pcpes, event, event_length);
- u32 rc = tpm_log_event(pcpes); if (rc) return rc; return tpm_extend(pcpes->digest, pcpes->pcrindex);
@@ -560,12 +553,21 @@ tpm_add_measurement_to_log(u32 pcrindex, u32 event_type, const char *event, u32 event_length, const u8 *hashdata, u32 hashdata_length) {
- struct pcpes pcpes = {
.pcrindex = pcrindex,.eventtype = event_type,
- struct {
struct pcpes pcpes;u8 event[128];
You are undoing a previous change now. I had tried to prevent having this event buffer (on the stack) but allow higher layers to pass arbitrary-length event buffers around that get copied into the log at the very end. This was to avoid having to append them here.
Stefan
On Sun, Nov 22, 2015 at 08:31:43PM -0500, Stefan Berger wrote:
On 11/22/2015 08:02 PM, Kevin O'Connor wrote:
Instead of passing in the event data to tpm_extend_acpi_log() via parameters, use the event[] field in the pcpes. Update those callers that don't populate the pcpes->event to do so prior to calling tpm_extend_acpi_log().
[...]
You are undoing a previous change now. I had tried to prevent having this event buffer (on the stack) but allow higher layers to pass arbitrary-length event buffers around that get copied into the log at the very end. This was to avoid having to append them here.
Okay, that was dumb on my part. I only looked deeper into the code after your cleanup series from the 17th, and it looks like I undid a bunch of changes you just made.
I agree that we don't want the internal code to be dependent on the external bios interface. The pcpes is ultimately what goes into the ACPI table though, and so it doesn't seem so bad to use its layout when figuring out what to add to the ACPI table. What's the rational for sending both a pcpes and the content of the pcpes to the lower functions? (The tpm_add_measurement_to_log() code is only ever called by internal functions that only use a small amount of data, so there is no fear of it overflowing - at least in the current code.)
It looks like I also undid the change to build_and_send_cmd() - it would be nice to have transmit() just take a single buffer instead of having to use the iovec[] stuff.
-Kevin
On 11/22/2015 08:52 PM, Kevin O'Connor wrote:
On Sun, Nov 22, 2015 at 08:31:43PM -0500, Stefan Berger wrote:
On 11/22/2015 08:02 PM, Kevin O'Connor wrote:
Instead of passing in the event data to tpm_extend_acpi_log() via parameters, use the event[] field in the pcpes. Update those callers that don't populate the pcpes->event to do so prior to calling tpm_extend_acpi_log().
[...]
You are undoing a previous change now. I had tried to prevent having this event buffer (on the stack) but allow higher layers to pass arbitrary-length event buffers around that get copied into the log at the very end. This was to avoid having to append them here.
Okay, that was dumb on my part. I only looked deeper into the code after your cleanup series from the 17th, and it looks like I undid a bunch of changes you just made.
I agree that we don't want the internal code to be dependent on the external bios interface. The pcpes is ultimately what goes into the ACPI table though, and so it doesn't seem so bad to use its layout when figuring out what to add to the ACPI table. What's the rational for sending both a pcpes and the content of the pcpes to the lower functions? (The tpm_add_measurement_to_log() code is only ever called
The change was driven by trying to avoid that one buffer for the event.
by internal functions that only use a small amount of data, so there is no fear of it overflowing - at least in the current code.)
It looks like I also undid the change to build_and_send_cmd() - it would be nice to have transmit() just take a single buffer instead of having to use the iovec[] stuff.
Ok.
Stefan
Setup the tpm hardware request in a linear area of memory.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/tcgbios.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/src/tcgbios.c b/src/tcgbios.c index e7adf3f..d4f6288 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -331,33 +331,30 @@ build_and_send_cmd(u8 locty, u32 ordinal, const u8 *append, u32 append_size, u8 *resbuffer, u32 return_size, u32 *returnCode, enum tpmDurationType to_t) { - u32 rc; + struct { + struct tpm_req_header trqh; + u8 cmd[64]; + } PACKED req = { + .trqh.tag = cpu_to_be16(TPM_TAG_RQU_CMD), + .trqh.totlen = cpu_to_be32(TPM_REQ_HEADER_SIZE + append_size), + .trqh.ordinal = cpu_to_be32(ordinal), + }; u8 obuffer[64]; - struct tpm_req_header trqh; struct tpm_rsp_header *trsh = (struct tpm_rsp_header *)obuffer; - struct iovec iovec[3] = {{ 0 }}; u32 obuffer_len = sizeof(obuffer); + memset(obuffer, 0x0, sizeof(obuffer));
- if (return_size > sizeof(obuffer)) { - dprintf(DEBUG_tcg, "TCGBIOS: size of requested response too big."); + if (return_size > sizeof(obuffer) || append_size > sizeof(req.cmd)) { + warn_internalerror(); return TCG_FIRMWARE_ERROR; } + if (append_size) + memcpy(req.cmd, append, append_size);
- trqh.tag = cpu_to_be16(TPM_TAG_RQU_CMD); - trqh.totlen = cpu_to_be32(TPM_REQ_HEADER_SIZE + append_size); - trqh.ordinal = cpu_to_be32(ordinal); - - iovec[0].data = &trqh; - iovec[0].length = TPM_REQ_HEADER_SIZE; - - if (append_size) { - iovec[1].data = append; - iovec[1].length = append_size; - } - - memset(obuffer, 0x0, sizeof(obuffer)); - - rc = transmit(locty, iovec, obuffer, &obuffer_len, to_t); + struct iovec iovec[2] = {{ 0 }}; + iovec[0].data = &req; + iovec[0].length = TPM_REQ_HEADER_SIZE + append_size; + u32 rc = transmit(locty, iovec, obuffer, &obuffer_len, to_t); if (rc) return rc;
There are no longer any callers to transmit() that use multiple buffers. Simplify transmit() so that it takes a single request buffer.
The pass_through_to_tpm() wrapper around transmit() is no longer needed. Remove the function and have all callers use transmit() directly.
Now that tpm_extend() function calls transmit directly, it can use TPM_DURATION_TYPE_SHORT duration.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/std/tcg.h | 4 --- src/tcgbios.c | 81 +++++++++++++++-------------------------------------------- src/tcgbios.h | 8 ------ 3 files changed, 20 insertions(+), 73 deletions(-)
diff --git a/src/std/tcg.h b/src/std/tcg.h index 5f9cad9..f6a47c7 100644 --- a/src/std/tcg.h +++ b/src/std/tcg.h @@ -232,15 +232,11 @@ struct pcctes_romex u32 totlen; \ u32 ordinal;
-#define TPM_REQ_HEADER_SIZE (sizeof(u16) + sizeof(u32) + sizeof(u32)) - #define TPM_RSP_HEADER \ u16 tag; \ u32 totlen; \ u32 errcode;
-#define TPM_RSP_HEADER_SIZE (sizeof(u16) + sizeof(u32) + sizeof(u32)) - struct tpm_req_header { TPM_REQ_HEADER; } PACKED; diff --git a/src/tcgbios.c b/src/tcgbios.c index d4f6288..13393a2 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -137,32 +137,24 @@ has_working_tpm(void) }
static u32 -transmit(u8 locty, const struct iovec iovec[], - u8 *respbuffer, u32 *respbufferlen, +transmit(u8 locty, struct tpm_req_header *req, + void *respbuffer, u32 *respbufferlen, enum tpmDurationType to_t) { - u32 rc = 0; - u32 irc; - struct tpm_driver *td; - unsigned int i; - if (tpm_state.tpm_driver_to_use == TPM_INVALID_DRIVER) return TCG_FATAL_COM_ERROR;
- td = &tpm_drivers[tpm_state.tpm_driver_to_use]; + struct tpm_driver *td = &tpm_drivers[tpm_state.tpm_driver_to_use];
- irc = td->activate(locty); + u32 irc = td->activate(locty); if (irc != 0) { /* tpm could not be activated */ return TCG_FATAL_COM_ERROR; }
- for (i = 0; iovec[i].length; i++) { - irc = td->senddata(iovec[i].data, - iovec[i].length); - if (irc != 0) - return TCG_FATAL_COM_ERROR; - } + irc = td->senddata((void*)req, be32_to_cpu(req->totlen)); + if (irc != 0) + return TCG_FATAL_COM_ERROR;
irc = td->waitdatavalid(); if (irc != 0) @@ -172,14 +164,13 @@ transmit(u8 locty, const struct iovec iovec[], if (irc != 0) return TCG_FATAL_COM_ERROR;
- irc = td->readresp(respbuffer, - respbufferlen); + irc = td->readresp(respbuffer, respbufferlen); if (irc != 0) return TCG_FATAL_COM_ERROR;
td->ready();
- return rc; + return 0; }
@@ -336,7 +327,7 @@ build_and_send_cmd(u8 locty, u32 ordinal, const u8 *append, u32 append_size, u8 cmd[64]; } PACKED req = { .trqh.tag = cpu_to_be16(TPM_TAG_RQU_CMD), - .trqh.totlen = cpu_to_be32(TPM_REQ_HEADER_SIZE + append_size), + .trqh.totlen = cpu_to_be32(sizeof(req.trqh) + append_size), .trqh.ordinal = cpu_to_be32(ordinal), }; u8 obuffer[64]; @@ -351,10 +342,7 @@ build_and_send_cmd(u8 locty, u32 ordinal, const u8 *append, u32 append_size, if (append_size) memcpy(req.cmd, append, append_size);
- struct iovec iovec[2] = {{ 0 }}; - iovec[0].data = &req; - iovec[0].length = TPM_REQ_HEADER_SIZE + append_size; - u32 rc = transmit(locty, iovec, obuffer, &obuffer_len, to_t); + u32 rc = transmit(locty, &req.trqh, obuffer, &obuffer_len, to_t); if (rc) return rc;
@@ -457,31 +445,8 @@ err_exit: }
static u32 -pass_through_to_tpm(u8 locty, const u8 *cmd, u32 cmd_length, - u8 *resp, u32 *resp_length) -{ - struct iovec iovec[2] = {{ 0 }}; - const u32 *tmp; - - if (cmd_length < TPM_REQ_HEADER_SIZE) - return TCG_INVALID_INPUT_PARA; - - iovec[0].data = cmd; - tmp = (const u32 *)&((u8 *)iovec[0].data)[2]; - iovec[0].length = cpu_to_be32(*tmp); - - if (cmd_length != iovec[0].length) - return TCG_INVALID_INPUT_PARA; - - return transmit(locty, iovec, resp, resp_length, - TPM_DURATION_TYPE_LONG /* worst case */); - -} - -static u32 tpm_extend(u8 *hash, u32 pcrindex) { - u32 rc; struct tpm_req_extend tre = { .tag = cpu_to_be16(TPM_TAG_RQU_CMD), .totlen = cpu_to_be32(sizeof(tre)), @@ -493,9 +458,8 @@ tpm_extend(u8 *hash, u32 pcrindex)
memcpy(tre.digest, hash, sizeof(tre.digest));
- rc = pass_through_to_tpm(0, (u8 *)&tre, sizeof(tre), - (u8 *)&rsp, &resp_length); - + u32 rc = transmit(0, (void*)&tre, &rsp, &resp_length, + TPM_DURATION_TYPE_SHORT); if (rc || resp_length != sizeof(rsp)) tpm_set_failure();
@@ -1002,29 +966,24 @@ static u32 pass_through_to_tpm_int(struct pttti *pttti, struct pttto *pttto) { u32 rc = 0; - u32 resbuflen = 0; - struct tpm_req_header *trh;
if (is_preboot_if_shutdown()) { rc = TCG_INTERFACE_SHUTDOWN; goto err_exit; }
- trh = (struct tpm_req_header *)pttti->tpmopin; + struct tpm_req_header *trh = (void*)pttti->tpmopin;
- if (pttti->ipblength < sizeof(struct pttti) + TPM_REQ_HEADER_SIZE || - pttti->opblength < sizeof(struct pttto) || - be32_to_cpu(trh->totlen) + sizeof(struct pttti) > pttti->ipblength ) { + if (pttti->ipblength < sizeof(struct pttti) + sizeof(trh) + || pttti->ipblength != sizeof(struct pttti) + be32_to_cpu(trh->totlen) + || pttti->opblength < sizeof(struct pttto)) { rc = TCG_INVALID_INPUT_PARA; goto err_exit; }
- resbuflen = pttti->opblength - offsetof(struct pttto, tpmopout); - - rc = pass_through_to_tpm(0, pttti->tpmopin, - pttti->ipblength - offsetof(struct pttti, tpmopin), - pttto->tpmopout, &resbuflen); - + u32 resbuflen = pttti->opblength - offsetof(struct pttto, tpmopout); + rc = transmit(0, trh, pttto->tpmopout, &resbuflen, + TPM_DURATION_TYPE_LONG /* worst case */); if (rc) goto err_exit;
diff --git a/src/tcgbios.h b/src/tcgbios.h index 0f9d5c3..4d69b1e 100644 --- a/src/tcgbios.h +++ b/src/tcgbios.h @@ -3,14 +3,6 @@
#include "types.h"
-#define STATUS_FLAG_SHUTDOWN (1 << 0) - -struct iovec -{ - size_t length; - const void *data; -}; - struct bregs; void tpm_interrupt_handler32(struct bregs *regs);
Merge tpm_extend_acpi_log() and tpm_log_event(). Move error checking and handling to callers. Don't shutdown the TPM on a failure from the 16bit BIOS interface.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/tcgbios.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/src/tcgbios.c b/src/tcgbios.c index 13393a2..16eb699 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -281,11 +281,14 @@ reset_acpi_log(void) * Returns an error code in case of faiure, 0 in case of success */ static u32 -tpm_extend_acpi_log(struct pcpes *pcpes) +tpm_log_event(struct pcpes *pcpes) { dprintf(DEBUG_tcg, "TCGBIOS: LASA = %p, next entry = %p\n", tpm_state.log_area_start_address, tpm_state.log_area_next_entry);
+ if (pcpes->pcrindex >= 24) + return TCG_INVALID_INPUT_PARA; + if (tpm_state.log_area_next_entry == NULL) return TCG_PC_LOGOVERFLOW;
@@ -467,26 +470,16 @@ tpm_extend(u8 *hash, u32 pcrindex) }
static u32 -tpm_log_event(struct pcpes *pcpes) +tpm_log_extend_event(struct pcpes *pcpes) { - if (pcpes->pcrindex >= 24) - return TCG_INVALID_INPUT_PARA; - if (!has_working_tpm()) return TCG_GENERAL_ERROR;
- u32 rc = tpm_extend_acpi_log(pcpes); - if (rc) - tpm_set_failure(); - return rc; -} - -static u32 -tpm_log_extend_event(struct pcpes *pcpes) -{ u32 rc = tpm_log_event(pcpes); - if (rc) + if (rc) { + tpm_set_failure(); return rc; + } return tpm_extend(pcpes->digest, pcpes->pcrindex); }
Merge tpm_extend() into tpm_log_extend_event(). Also, the spec states that a log entry should only be added if the extend succeeds, so attempt the extend prior to adding to the log.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/tcgbios.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/src/tcgbios.c b/src/tcgbios.c index 16eb699..9184300 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -448,39 +448,32 @@ err_exit: }
static u32 -tpm_extend(u8 *hash, u32 pcrindex) +tpm_log_extend_event(struct pcpes *pcpes) { + if (!has_working_tpm()) + return TCG_GENERAL_ERROR; + struct tpm_req_extend tre = { .tag = cpu_to_be16(TPM_TAG_RQU_CMD), .totlen = cpu_to_be32(sizeof(tre)), .ordinal = cpu_to_be32(TPM_ORD_Extend), - .pcrindex = cpu_to_be32(pcrindex), + .pcrindex = cpu_to_be32(pcpes->pcrindex), }; + memcpy(tre.digest, pcpes->digest, sizeof(tre.digest)); + struct tpm_rsp_extend rsp; u32 resp_length = sizeof(rsp); - - memcpy(tre.digest, hash, sizeof(tre.digest)); - u32 rc = transmit(0, (void*)&tre, &rsp, &resp_length, TPM_DURATION_TYPE_SHORT); - if (rc || resp_length != sizeof(rsp)) - tpm_set_failure(); - - return rc; -} - -static u32 -tpm_log_extend_event(struct pcpes *pcpes) -{ - if (!has_working_tpm()) - return TCG_GENERAL_ERROR; - - u32 rc = tpm_log_event(pcpes); - if (rc) { + if (rc || resp_length != sizeof(rsp)) { tpm_set_failure(); return rc; } - return tpm_extend(pcpes->digest, pcpes->pcrindex); + + rc = tpm_log_event(pcpes); + if (rc) + tpm_set_failure(); + return rc; }
static void
-
Kevin O'Connor -
Stefan Berger