[SLOF] [PATCH v3 05/17] Extend firmware API

[Hon c Lo]

Hi Stefan,


I went throuhg the C code in the patch.  There are only some minor comments
below:


> +
> +static uint32_t hash_log_extend_event(const void *hashdata,
> +                  uint32_t hashdatalen,
> +                  struct pcpes *pcpes,
> +                  const char *event, uint32_t event_length,
hash_log_event
> +                  uint32_t pcrindex)
> +{
> +   uint32_t rc;
> +
> +   rc = hash_log_event(hashdata, hashdatalen, pcpes, event,
event_length);
> +
> +   /*
> +    * Like PCCLient spec.: evn if log is full extend the PCR


Typo:  s/evn/even/


> +    */
> +   tpm_extend(pcpes->digest, pcrindex);
> +
> +   return rc;
> +}
> +
> +/*
> + * tpm_hash_log_extend_event: Function for interfacing with then
firmware API
> + */

Typo:   s/then/the/


> +uint32_t tpm_hash_log_extend_event(struct pcpes *pcpes)
> +{
> +   const char *event = NULL;
> +   uint32_t event_length = pcpes->eventdatasize;
> +
> +   if (!has_working_tpm())
> +      return TCGBIOS_GENERAL_ERROR;
> +
> +   if (event_length)
> +      event = (void *)pcpes + offset_of(struct pcpes, event);
> +
> +   return hash_log_extend_event(&pcpes->event, pcpes->eventdatasize,
> +                 pcpes, event, event_length,
> +                 pcpes->pcrindex);
> +}


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
hash_log_event() and 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.



Regards,
Vicky

Hon Ching (Vicky) Lo
Linux Security Development
Notes:  lo1 at us.ibm.com
Office:  845-435-8946 (T/L: 295-8946)



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/slof/attachments/20160223/28a73001/attachment.html>