Re: [PATCH v2 0/3] TPM2.0: Added eventlog support for TPM2.0

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Thu, Aug 11, 2016 at 04:18:34PM +0530, Nayna wrote:
>> #1. And the next somewhat pre-existing issue is that we call
>> tpm_bios_log_setup even if we don't have access to a bios log.
>> 
>> Does the bios log ever change or is it static at boot? Can we move
>> read_log into the chip and do it once so we can tell if it worked
>> before creating security fs stuff??
> 
> <nayna>
> 
> So, by statement "if we don't have access to a bios log?".. Do you mean bios
> log is empty ? or has no properties to access the log.

More along the lines we call down to tpm_of and it just exits if the
properties do not exist. For instance my DT using TPM systems do not
have a bios log.

> Regarding bios log, so it is supposed to be written by each component in
> boot stack during boot.

Only if the boot firmware supports this functionality and indicates it
via DT attributes or ACPI. That is what we need to check earlier.

> As per moving read_log into the chip, do you mean something like as below ?
>
> struct tpm_chip {
> ....
>     struct tpm_bios_log *bios_log;
> ....
> }
> 
> read_log(struct tpm_chip *chip)
> {
> 
>  allocates to chip->bios_log->bios_event_log
>  allocates to chip->bios_log->bios_event_log_end;
> 
>  return 0; // for success;
> 
> }
> 
> tpm_chip_register()
> {
> ...
> if (read_log(chip))
>    tpm_bios_log_setup(chip);
> ...
> }

Yes, exactly. The challenge will be to get the chip into the security
file context, give that a try and we can probably help you. The
locking and lifetime will probably be very subtle.

> And as per readlog is concerned, it just reads the start and max size of the
> log for both ACPI/OF. So, it is possible that readlog returns successfully
> giving start and max allocated size. but actual log size is zero. So, when
> we cat bios_measurements_file, it doesn't show anything.  And I think that
> is fine.

That would be a firmware that says it support a log but generated an
empty one, so that seems reasonble to report to user space.

> 
> #2.
> >  #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) ||
> \
> > -	defined(CONFIG_ACPI)
> > -extern struct dentry **tpm_bios_log_setup(const char *);
> > -extern void tpm_bios_log_teardown(struct dentry **);
> > +	defined(CONFIG_ACPI) || defined(CONFIG_PPC64)
> > +extern struct dentry **tpm_bios_log_setup(struct tpm_chip *chip);
> > +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> 
> I'm really deeply unhappy about these ifdefs and the general scheme
> that was used to force this special difference into the tpm core.
> 
> Please find another way to do it.
> 
> IMHO, 'read_log' should simply try ACPI and OF in sequence to find the
> log.
> 
> <nayna> Can you help me to understand the reason for which it was done this
> way ?

When this was written the TPM subsystem was very old and lacked
infrastructure to do this better, we have largely solved those
problems, event log is the last large area that needs some attention..

> Secondly, did you mean something like (just a sample psudocode)
> read_log()
> {
> read_of_node;
> if (read_of_node_fails)
> 	read_acpi_way
>         if (read_acpi_succeeds)
>            do acpi_way of reading for start of log and max log size.
> 	else
>            return err;
> do of_node way of reading for start of log and max log size
> }

I would do more like:

read_log_of(
{
  if (! chip->pdev->of_node .. has attribute)
     return 0
  .. return 1;
}

read_log()
{
  if ((res = read_log_of(chip)) != 0)
     return res;
  if ((res = read_log_acpi(chip)) != 0)
     return res;
  return 0;
}
   
> So, is it safe to assume that if there is no of_node property , that means
> it is ACPI ?

No, ACPI also reads a property from the ACPI table, the existance of
that should be tested directly.

> #3. Where you able to test this on IBM's 'vtpm' stuff as well?,
> 
> <nayna>This was for patch where I am directly using dev.of_node property.
> Sorry I missed to reply in previous response. And answer is "yes", I did
> test it. </nayna>

That is excellent

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev