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

[Nayna <nayna@linux.vnet.ibm.com>]

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 <nayna@linux.vnet.ibm.com>

>>> 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_MSG		128
>>> +#define MAX_DIGEST_SIZE		64
>>
>> 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 tpm_read_log_acpi(struct tpm_chip *chip);
>>>    #else
>>> --
>>> 2.5.0
>>
>> /Jarkko
>>
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>