<html><body><p><tt>Hi Stefan,</tt><br><br><br><tt>I went throuhg the C code in the patch. There are only some minor comments below:</tt><br><br><tt><br>> +<br>> +static uint32_t hash_log_extend_event(const void *hashdata,<br>> + uint32_t hashdatalen,<br>> + struct pcpes *pcpes,<br>> + const char *event, uint32_t event_length, hash_log_event<br>> + uint32_t pcrindex)<br>> +{<br>> + uint32_t rc;<br>> +<br>> + rc = hash_log_event(hashdata, hashdatalen, pcpes, event, event_length);<br>> +<br>> + /*<br>> + * Like PCCLient spec.: evn if log is full extend the PCR<br></tt><br><br><tt>Typo: s/evn/even/</tt><br><br><br><tt>> + */<br>> + tpm_extend(pcpes->digest, pcrindex);<br>> +<br>> + return rc;<br>> +}<br>> +<br>> +/*<br>> + * tpm_hash_log_extend_event: Function for interfacing with then firmware API</tt><br><tt>> + */</tt><br><br><tt>Typo: s/then/the/</tt><br><tt><br><br>> +uint32_t tpm_hash_log_extend_event(struct pcpes *pcpes)<br>> +{<br>> + const char *event = NULL;<br>> + uint32_t event_length = pcpes->eventdatasize;<br>> +<br>> + if (!has_working_tpm())<br>> + return TCGBIOS_GENERAL_ERROR;<br>> +<br>> + if (event_length)<br>> + event = (void *)pcpes + offset_of(struct pcpes, event);<br>> +<br>> + return hash_log_extend_event(&pcpes->event, pcpes->eventdatasize,<br>> + pcpes, event, event_length,<br>> + pcpes->pcrindex);<br>> +}</tt><br><br><br><tt>hash_log_extend_event() was implemented with six paramters; it looks redundant, as it could have been passed with the pcpe structure alone. The only reason I could think of doing it this way here is that you would like </tt><tt>hash_log_event() and </tt><tt>hash_log_extend_event() to be reused in other ways in the future. I suspect that's the case here; if so, I'm fine with it.</tt><br><br><tt><br></tt><br>Regards,<br>Vicky<br><br>Hon Ching (Vicky) Lo<br>Linux Security Development<br>Notes: lo1@us.ibm.com<br>Office: 845-435-8946 (T/L: 295-8946)<br><br><br><br><BR>
</body></html>