Re: [PATCH] tpm: Add debug information for device protocol and eventlog

[Tianjia Zhang <tianjia.zhang@linux.alibaba.com>]

Thanks for pointing it out, this patch is too careless, very sorry, I 
will revise another version.

Best regards,
Tianjia

On 10/28/20 4:58 AM, Daniel Kiper wrote:
> Hi,
> 
> First of all, sorry for late reply...
> 
> On Wed, Jul 29, 2020 at 09:33:27PM +0800, Tianjia Zhang wrote:
>> Add a number of debug logs to the tpm module. The condition tag
>> for opening debugging is `tpm`. On TPM machines, this will bring
>> great convenience to diagnosis and debugging.
>>
>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
>> ---
>>   grub-core/commands/efi/tpm.c | 21 +++++++++++++++++----
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/grub-core/commands/efi/tpm.c b/grub-core/commands/efi/tpm.c
>> index b03b6b296..9b2cd43f1 100644
>> --- a/grub-core/commands/efi/tpm.c
>> +++ b/grub-core/commands/efi/tpm.c
>> @@ -56,9 +56,12 @@ grub_tpm1_present (grub_efi_tpm_protocol_t *tpm)
>>
>>     if (status != GRUB_EFI_SUCCESS || caps.TPMDeactivatedFlag
>>         || !caps.TPMPresentFlag)
>> -    return tpm1_present = 0;
>> +    tpm1_present = 0;
>>
>> -  return tpm1_present = 1;
>> +  tpm1_present = 1;
>> +
>> +  grub_dprintf ("tpm", "tpm1 %s present\n", tpm1_present ? "" : "NOT");
>> +  return (grub_efi_boolean_t) tpm1_present;
> 
> This does not work as you expect. tpm1 will be always reported as present.
> 
>>   }
>>
>>   static grub_efi_boolean_t
>> @@ -75,9 +78,12 @@ grub_tpm2_present (grub_efi_tpm2_protocol_t *tpm)
>>     status = efi_call_2 (tpm->get_capability, tpm, &caps);
>>
>>     if (status != GRUB_EFI_SUCCESS || !caps.TPMPresentFlag)
>> -    return tpm2_present = 0;
>> +    tpm2_present = 0;
>> +
>> +  tpm2_present = 1;
>>
>> -  return tpm2_present = 1;
>> +  grub_dprintf ("tpm", "tpm2 %s present\n", tpm2_present ? "" : "NOT");
>> +  return (grub_efi_boolean_t) tpm2_present;
>>   }
> 
> Ditto except WRT tpm2.
> 
>>   static grub_efi_boolean_t
>> @@ -102,6 +108,7 @@ grub_tpm_handle_find (grub_efi_handle_t *tpm_handle,
>>         *tpm_handle = handles[0];
>>         grub_tpm_version = 1;
>>         *protocol_version = 1;
>> +      grub_dprintf ("tpm", "TPM handle Found, version: 1\n");
>>         return 1;
>>       }
>>
>> @@ -113,9 +120,11 @@ grub_tpm_handle_find (grub_efi_handle_t *tpm_handle,
>>         *tpm_handle = handles[0];
>>         grub_tpm_version = 2;
>>         *protocol_version = 2;
>> +      grub_dprintf ("tpm", "TPM handle Found, version: 2\n");
>>         return 1;
>>       }
>>
>> +  grub_dprintf ("tpm", "NO TPM handle Found\n");
>>     return 0;
>>   }
>>
>> @@ -147,6 +156,8 @@ grub_tpm1_log_event (grub_efi_handle_t tpm_handle, unsigned char *buf,
>>     event->EventSize = grub_strlen (description) + 1;
>>     grub_memcpy (event->Event, description, event->EventSize);
>>
>> +  grub_dprintf ("tpm", "tpm1 log_extend_event, pcr = %d, size = %d, %s\n",
>> +                pcr, (int)size, description);
> 
> Could you use PRIxGRUB_* macro from include/grub/types.h instead of "%d" for size here?
> 
>>     algorithm = TCG_ALG_SHA;
>>     status = efi_call_7 (tpm->log_extend_event, tpm, (grub_addr_t) buf, (grub_uint64_t) size,
>>   		       algorithm, event, &eventnum, &lastevent);
>> @@ -199,6 +210,8 @@ grub_tpm2_log_event (grub_efi_handle_t tpm_handle, unsigned char *buf,
>>       sizeof (*event) - sizeof (event->Event) + grub_strlen (description) + 1;
>>     grub_memcpy (event->Event, description, grub_strlen (description) + 1);
>>
>> +  grub_dprintf ("tpm", "tpm2 log_extend_event, pcr = %d, size = %d, %s\n",
>> +                pcr, (int)size, description);
> 
> Ditto.
> 
> However, I would prefer if you print this only from grub_tpm_measure()
> function without tpm1/tpm2 prefix.
> 
> Daniel
>