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

[Tianjia Zhang]

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