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

[Nayna <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]

Hi Jason,

Thanks for your inputs.. Further responses inline.

On 08/13/2016 08:29 AM, Jason Gunthorpe wrote:
> 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.

Ok.

>
>> 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.

Sure, I have done this change now in my next patch, should be able to 
post soon.
>
>> 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..
>

Ok.

>> 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;
> }

I tried the suggested approach and since ACPI specific functions won't 
be available for arch using CONFIG_OF, so the compilation fails and vice 
versa for CONFIG_ACPI..

Jason, for understanding.. can you explain the issue with existing 
design for read_log, where arch specific read_log is compiled based on 
CONFIG option.. And then, we can just change in Makefile.. where 
currently it is like

ifdef CONFIG_ACPI
        tpm-y += tpm_acpi.o
else
ifdef CONFIG_OF
        tpm-y += tpm_of.o
endif
endif


this can be changed to
tpm-$(CONFIG_ACPI) += tpm_acpi.o
tpm-$(CONFIG_OF) += tpm_of.o


Please let me know if I missed something.

Thanks & Regards,
    - Nayna

>
>> 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
>


------------------------------------------------------------------------------