Re: [tpmdd-devel] [PATCH v6 2/2] tpm: add securityfs support for TPM 2.0 firmware event log

[Nayna]

On 11/30/2016 10:29 PM, Nayna wrote:
>
>
> On 11/26/2016 09:17 PM, Jarkko Sakkinen wrote:
>> On Sat, Nov 26, 2016 at 07:45:39AM -0500, Nayna Jain wrote:
>>> Unlike the device driver support for TPM 1.2, the TPM 2.0 does
>>> not support the securityfs pseudo files for displaying the
>>> firmware event log.
>>>
>>> This patch enables support for providing the TPM 2.0 event log in
>>> binary form. TPM 2.0 event log supports a crypto agile format that
>>> records multiple digests, which is different from TPM 1.2. This
>>> patch enables the tpm_bios_log_setup for TPM 2.0  and adds the
>>> event log parser which understand the TPM 2.0 crypto agile format.
>>>
>>> Signed-off-by: Nayna Jain 

>>> diff --git a/drivers/char/tpm/tpm_eventlog.h 
>>> b/drivers/char/tpm/tpm_eventlog.h
>>> index 1660d74..7e33b90 100644
>>> --- a/drivers/char/tpm/tpm_eventlog.h
>>> +++ b/drivers/char/tpm/tpm_eventlog.h
>>> @@ -5,6 +5,9 @@
>>>#define TCG_EVENT_NAME_LEN_MAX   255
>>>#define MAX_TEXT_EVENT   1000/* Max event string length */
>>>#define ACPI_TCPA_SIG"TCPA"  /* 0x41504354 /'TCPA' */
>>> +#define HASH_COUNT 3
>>> +#define MAX_TPM_LOG_MSG128
>>> +#define MAX_DIGEST_SIZE64
>>
>> Where have been the values for these constants derived? You should
>> anyway prefix them with TPM_.
>
> HASH_COUNT is to represent multiple active banks at a time, where SHA1
> and SHA256 are the ones, I kept it 3 with assumption of SHA384/SHA512.
> And with that, I kept MAX_DIGEST_SIZE as 64.
>
> MAX_TPM_LOG_MSG was an assumption by me. I think TCG Spec says max value
> can be 1MB.
>
> I would actually like to know the views on this.

I would like to know your and Jason views on the above given reasoning 
related to how values for these constants are derived.

Thanks & Regards,
- Nayna

>
>>
>>>#ifdef CONFIG_PPC64
>>>#define do_endian_conversion(x) be32_to_cpu(x)
>>> @@ -73,6 +76,73 @@ enum tcpa_pc_event_ids {
>>> HOST_TABLE_OF_DEVICES,
>>>};
>>>
>>> +/*
>>> + * All the structures related to TPM 2.0 Event Log are taken from TCG EFIi
>>> + * Protocol * Specification, Family "2.0". Document is available on link
>>> + * http://www.trustedcomputinggroup.org/tcg-efi-protocol-specification/
>>> + * Information is also available on TCG PC Client Platform Firmware Profile
>>> + * Specification, Family "2.0"
>>> + * Detailed digest structures for TPM 2.0 are defined in document
>>> + * Trusted Platform Module Library Part 2: Structures, Family "2.0".
>>> + */
>>> +
>>> +/* TPM 2.0 Event log header algorithm spec. */
>>> +struct tcg_efi_specid_event_algs {
>>> +   u16 alg_id;
>>> +   u16 digest_size;
>>> +} __packed;
>>> +
>>> +/* TPM 2.0 Event log header data. */
>>> +struct tcg_efi_specid_event {
>>> +   u8  signature[16];
>>> +   u32 platform_class;
>>> +   u8  spec_version_minor;
>>> +   u8  spec_version_major;
>>> +   u8  spec_errata;
>>> +   u8  uintnsize;
>>> +   u32 num_algs;
>>> +   struct tcg_efi_specid_event_algs   digest_sizes[HASH_COUNT];
>>> +   u8  vendor_info_size;
>>> +   u8  vendor_info[0];
>>> +} __packed;
>>> +
>>> +/* TPM 2.0 Event Log Header. */
>>> +struct tcg_pcr_event {
>>> +   u32 pcr_idx;
>>> +   u32 event_type;
>>> +   u8  digest[20];
>>> +   u32 event_size;
>>> +   u8  event[MAX_TPM_LOG_MSG];
>>> +} __packed;
>>> +
>>> +/* TPM 2.0 Crypto agile algorithm and respective digest. */
>>> +struct tpmt_ha {
>>> +   u16 alg_id;
>>> +   u8  digest[MAX_DIGEST_SIZE];
>>> +} __packed;
>>> +
>>> +/* TPM 2.0 Crypto agile digests list. */
>>> +struct tpml_digest_values {
>>> +   u32 count;
>>> +   struct tpmt_ha  digests[HASH_COUNT];
>>> +} __packed;
>>> +
>>> +/* TPM 2.0 Event field structure. */
>>> +struct tcg_event_field {
>>> +   u32 event_size;
>>> +   u8  event[MAX_TPM_LOG_MSG];
>>> +} __packed;
>>> +
>>> +/* TPM 2.0 Crypto agile log entry format. */
>>> +struct tcg_pcr_event2 {
>>> +   u32 pcr_idx;
>>> +   u32 event_type;
>>> +   struct tpml_digest_values digests;
>>> +   struct tcg_event_field  event;
>>> +} __packed;
>>
>> Your alignment is broken. You sometimes align and sometimes do not.
>>
>> For struct fields it does not make sense align fields at all since it
>> does not "scale". It is done in some places in the driver but for new
>> code it is absolutely disallowed. Thus, the right way to fix this is to
>> remove all the aligment.
>>
>> For enums it does make sense and improves readability.
>
> Ok. I didn't change anything in this from previous version, not sure now
> why it looks different. Will verify that.
> I remembered your feedback for my first version. So, now I don't have
> any alignment done for full struct, but have field a tab away from the type.
>
> Thanks & Regards,
>  - Nayna
>>> +
>>> +extern const struct seq_operations tpm2_binary_b_measurements_seqops;
>>> +
>>>#if defined(CONFIG_ACPI)
>>>int 

Re: [tpmdd-devel] [PATCH v6 2/2] tpm: add securityfs support for TPM 2.0 firmware event log

[Nayna]

On 12/01/2016 05:13 PM, Jarkko Sakkinen wrote:
> On Wed, Nov 30, 2016 at 10:29:00PM +0530, Nayna wrote:
>>
>>
>> On 11/26/2016 09:17 PM, Jarkko Sakkinen wrote:
>>> On Sat, Nov 26, 2016 at 07:45:39AM -0500, Nayna Jain wrote:
 Unlike the device driver support for TPM 1.2, the TPM 2.0 does
 not support the securityfs pseudo files for displaying the
 firmware event log.

 This patch enables support for providing the TPM 2.0 event log in
 binary form. TPM 2.0 event log supports a crypto agile format that
 records multiple digests, which is different from TPM 1.2. This
 patch enables the tpm_bios_log_setup for TPM 2.0  and adds the
 event log parser which understand the TPM 2.0 crypto agile format.

 Signed-off-by: Nayna Jain 
>>>
>>> I would not rush with new patch set versions as long as the testing is
>>> almost completely lacking. I didn't even have time to read the previous
>>> version properly before this came out.
>>
>> Sure Jarkko. My apologies for multiple versions. I will wait for testing,
>> before posting my next version.
>
> You could send now a new version because probably anyone who wants
> to review your patches has had a chance to look at it. My point was
> that you wait for at least few days.

Sure Jarkko. Thanks !!

Also Jarkko/Jason, please let me know your views on #defines in 
tpm_eventlog.h for TPM 2.0 event log support. I have responded to the 
related and other feedbacks in my previous mail sent on 30th Nov.

Thanks & Regards,
- Nayna

>
> Maybe Jason could help testing your patches. I don't know when I have
> time to setup environment. He had OF environment available.
>
> /Jarkko
>


--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v6 2/2] tpm: add securityfs support for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Wed, Nov 30, 2016 at 10:29:00PM +0530, Nayna wrote:
> 
> 
> On 11/26/2016 09:17 PM, Jarkko Sakkinen wrote:
> > On Sat, Nov 26, 2016 at 07:45:39AM -0500, Nayna Jain wrote:
> > > Unlike the device driver support for TPM 1.2, the TPM 2.0 does
> > > not support the securityfs pseudo files for displaying the
> > > firmware event log.
> > > 
> > > This patch enables support for providing the TPM 2.0 event log in
> > > binary form. TPM 2.0 event log supports a crypto agile format that
> > > records multiple digests, which is different from TPM 1.2. This
> > > patch enables the tpm_bios_log_setup for TPM 2.0  and adds the
> > > event log parser which understand the TPM 2.0 crypto agile format.
> > > 
> > > Signed-off-by: Nayna Jain 
> > 
> > I would not rush with new patch set versions as long as the testing is
> > almost completely lacking. I didn't even have time to read the previous
> > version properly before this came out.
> 
> Sure Jarkko. My apologies for multiple versions. I will wait for testing,
> before posting my next version.

You could send now a new version because probably anyone who wants
to review your patches has had a chance to look at it. My point was
that you wait for at least few days.

Maybe Jason could help testing your patches. I don't know when I have
time to setup environment. He had OF environment available.

/Jarkko

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH v6 2/2] tpm: add securityfs support for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Sat, Nov 26, 2016 at 07:45:39AM -0500, Nayna Jain wrote:
> Unlike the device driver support for TPM 1.2, the TPM 2.0 does
> not support the securityfs pseudo files for displaying the
> firmware event log.
> 
> This patch enables support for providing the TPM 2.0 event log in
> binary form. TPM 2.0 event log supports a crypto agile format that
> records multiple digests, which is different from TPM 1.2. This
> patch enables the tpm_bios_log_setup for TPM 2.0  and adds the
> event log parser which understand the TPM 2.0 crypto agile format.
> 
> Signed-off-by: Nayna Jain 

I would not rush with new patch set versions as long as the testing is
almost completely lacking. I didn't even have time to read the previous
version properly before this came out.

> ---
>  drivers/char/tpm/Makefile  |   2 +-
>  .../char/tpm/{tpm_eventlog.c => tpm1_eventlog.c}   |  35 ++--
>  drivers/char/tpm/tpm2_eventlog.c   | 214 
> +
>  drivers/char/tpm/tpm_eventlog.h|  70 +++
>  4 files changed, 306 insertions(+), 15 deletions(-)
>  rename drivers/char/tpm/{tpm_eventlog.c => tpm1_eventlog.c} (95%)
>  create mode 100644 drivers/char/tpm/tpm2_eventlog.c
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a05b1eb..3d386a8 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -3,7 +3,7 @@
>  #
>  obj-$(CONFIG_TCG_TPM) += tpm.o
>  tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
> - tpm_eventlog.o
> + tpm1_eventlog.o tpm2_eventlog.o
>  tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
>  tpm-$(CONFIG_OF) += tpm_of.o
>  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> diff --git a/drivers/char/tpm/tpm_eventlog.c 
> b/drivers/char/tpm/tpm1_eventlog.c
> similarity index 95%
> rename from drivers/char/tpm/tpm_eventlog.c
> rename to drivers/char/tpm/tpm1_eventlog.c
> index fe7e3fa..e9a092b 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm1_eventlog.c
> @@ -390,9 +390,6 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>   unsigned int cnt;
>   int rc;
>  
> - if (chip->flags & TPM_CHIP_FLAG_TPM2)
> - return 0;
> -
>   rc = tpm_read_log(chip);
>   if (rc)
>   return rc;
> @@ -407,7 +404,13 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>   cnt++;
>  
>   chip->bin_log_seqops.chip = chip;
> - chip->bin_log_seqops.seqops = _binary_b_measurements_seqops;
> + if (chip->flags & TPM_CHIP_FLAG_TPM2)
> + chip->bin_log_seqops.seqops =
> + _binary_b_measurements_seqops;
> + else
> + chip->bin_log_seqops.seqops =
> + _binary_b_measurements_seqops;
> +
>  
>   chip->bios_dir[cnt] =
>   securityfs_create_file("binary_bios_measurements",
> @@ -418,17 +421,21 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>   goto err;
>   cnt++;
>  
> - chip->ascii_log_seqops.chip = chip;
> - chip->ascii_log_seqops.seqops = _ascii_b_measurements_seqops;
> + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
>  
> - chip->bios_dir[cnt] =
> - securityfs_create_file("ascii_bios_measurements",
> -0440, chip->bios_dir[0],
> -(void *)>ascii_log_seqops,
> -_bios_measurements_ops);
> - if (IS_ERR(chip->bios_dir[cnt]))
> - goto err;
> - cnt++;
> + chip->ascii_log_seqops.chip = chip;
> + chip->ascii_log_seqops.seqops =
> + _ascii_b_measurements_seqops;
> +
> + chip->bios_dir[cnt] =
> + securityfs_create_file("ascii_bios_measurements",
> +0440, chip->bios_dir[0],
> +(void *)>ascii_log_seqops,
> +_bios_measurements_ops);
> + if (IS_ERR(chip->bios_dir[cnt]))
> + goto err;
> + cnt++;
> + }
>  
>   return 0;
>  
> diff --git a/drivers/char/tpm/tpm2_eventlog.c 
> b/drivers/char/tpm/tpm2_eventlog.c
> new file mode 100644
> index 000..cf9fea0
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2_eventlog.c
> @@ -0,0 +1,214 @@
> +/*
> + * Copyright (C) 2016 IBM Corporation
> + *
> + * Authors:
> + *  Nayna Jain 
> + *
> + * Access to TPM 2.0 event log as written by Firmware.
> + * It assumes that writer of event log has followed TCG Spec 2.0
> + * and written the event struct data in little endian. With that,
> + * it doesn't need any endian conversion for structure content.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the