[PATCH v6 0/2] securityfs support for TPM 2.0 firmware event log

[PATCH v6 0/2] securityfs support for TPM 2.0 firmware event log

[Nayna Jain]

The TPM device driver defines ascii and binary methods for
displaying the TPM 1.2 event log via securityfs files, which are
needed for validating a TPM quote. The device driver for TPM 2.0
does not have similar support for displaying the TPM 2.0
event log. This patch set adds the support for displaying
TPM 2.0 event log in binary format.

The parsing mechanism to display the TPM 2.0 event log in binary
format is implemented as defined in the TPM 2.0 TCG specification[1].
If the firmware event log support exists and is successfully read,
the securityfs file is created to provide the event log in binary
format for both the OF device tree and ACPI.

   - Patch 1 adds the device tree bindings support for Physical TPM.
   - Patch 2 adds the support for creating securityfs files and for
     displaying the TPM 2.0 crypto agile event log in binary format.

[1] TCG EFI Protocol Specification, Family "2.0" - Section 5 "Event
Log Structure"

Changelog History

v6:

- Rebased to the Jarkko's latest master branch (e717b5c:tpm: vtpm_proxy: 
  conditionally call tpm_chip_unregister)
- Retained securityfs setup functions in tpm_eventlog.c
- Renamed tpm_eventlog.c to tpm1_eventlog.c
- Fixed tpm_read_log_of() for NULL check and memcpy function.

v5:

- Upstreamed cleanup and fixes as different patchset
- Rebased to the Jarkko's latest master branch (e5be084 tpm: vtpm_proxy:
  Do not access host's event log)
- Patch "tpm: enhance read_log_of() to support Physical TPM event log
  - New Patch.
- Patch "tpm: add securityfs support for TPM 2.0 firmware event log"
  - Moved the changes in read_log_of() to a different patch
  - TPM 2.0 event log data types are declared in tpm_eventlog.h, tpm2.h
  is removed.
  - Included other feedbacks also from Jarkko on aligment and extra
    line

v4:

- Includes feedbacks from Jarkko and Jason.
- Patch "tpm: define a generic open() method for ascii & bios
measurements".
  - Fix indentation issue.
- Patch "tpm: replace the dynamically allocated bios_dir as
  struct dentry array".
  - Continue to use bios_dir_count variable to use is_bad() checks and
    to maintain correct order for securityfs_remove() during teardown.
  - Reset chip->bios_dir_count in teardown() function.
- Patch "tpm: validate the eventlog access before tpm_bios_log_setup".
  - Retain TPM2 check which was removed in previous patch.
  - Add tpm_bios_log_setup failure handling.
  - Remove use of private data from v3 version of patch. Add a
  new member to struct tpm_chip to achieve the same purpose.
- Patch "tpm: redefine the read_log method to check for ACPI/OF 
properties sequentially".
  - Move replacement of CONFIG_TCG_IBMVTPM with CONFIG_OF to this
    patch from patch 3.
  - Replace -1 error code with -ENODEV.
- Patch "tpm: replace the of_find_node_by_name() with dev of_node
property".
  - Uses chip->dev.parent->of_node.
  - Created separate patch for cleanup of pr_err messages.
- Patch "tpm: remove printk error messages".
  - New Patch.
- Patch "tpm: add the securityfs file support for TPM 2.0 eventlog".
  - Parses event digests using event alg_id rather than event log header
    alg_id.
  - Uses of_property_match_string to differentiate tpm/vtpm compatible

v3:

- Includes the review feedbacks as suggested by Jason.
- Split of patches into one patch per idea.
- Generic open() method for ascii/bios measurements.
- Replacement of of **bios_dir with *bios_dir[3].
- Verifying readlog() is successful before creating securityfs entries.
- Generic readlog() to check for ACPI/OF in sequence.
- read_log_of() method now uses of_node propertry rather than
calling find_device_by_name.
- read_log differentiates vtpm/tpm using its compatible property.
- Cleans pr_err with dev_dbg.
- Commit msgs subject line prefixed with tpm.

v2:

- Fixes issues as given in feedback by Jason.
- Adds documentation for device tree.

Nayna Jain (2):
  tpm: enhance read_log_of() to support Physical TPM event log
  tpm: add securityfs support for TPM 2.0 firmware event log

 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 +++++++
 drivers/char/tpm/tpm_of.c                          |  27 ++-
 5 files changed, 329 insertions(+), 19 deletions(-)
 rename drivers/char/tpm/{tpm_eventlog.c => tpm1_eventlog.c} (95%)
 create mode 100644 drivers/char/tpm/tpm2_eventlog.c

-- 
2.5.0

[PATCH v6 1/2] tpm: enhance read_log_of() to support Physical TPM event log

[Nayna Jain]

Physical TPMs use Open Firmware Device Tree bindings that are similar
to the IBM Power virtual TPM to support event log. However, these
properties store the values in different endianness for Physical
and Virtual TPM.

This patch fixes the endianness issue by doing appropriate conversion
based on Physical or Virtual TPM.

Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm_of.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 7dee42d7..4b0d5e6 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -27,6 +27,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	const u32 *sizep;
 	const u64 *basep;
 	struct tpm_bios_log *log;
+	u32 log_size;
+	u64 log_base;
 
 	log = &chip->log;
 	if (chip->dev.parent && chip->dev.parent->of_node)
@@ -41,18 +43,35 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	if (sizep == NULL || basep == NULL)
 		return -EIO;
 
-	if (*sizep == 0) {
+	/*
+	 * For both vtpm/tpm, firmware has log addr and log size in big
+	 * endian format. But in case of vtpm, there is a method called
+	 * sml-handover which is run during kernel init even before
+	 * device tree is setup. This sml-handover function takes care
+	 * of endianness and writes to sml-base and sml-size in little
+	 * endian format. For this reason, vtpm doesn't need conversion
+	 * but physical tpm needs the conversion.
+	 */
+	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
+		log_size = be32_to_cpup(sizep);
+		log_base = be64_to_cpup(basep);
+	} else {
+		log_size = *sizep;
+		log_base = *basep;
+	}
+
+	if (log_size == 0) {
 		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
 		return -EIO;
 	}
 
-	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
+	log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
 	if (!log->bios_event_log)
 		return -ENOMEM;
 
-	log->bios_event_log_end = log->bios_event_log + *sizep;
+	log->bios_event_log_end = log->bios_event_log + log_size;
 
-	memcpy(log->bios_event_log, __va(*basep), *sizep);
+	memcpy(chip->log.bios_event_log, __va(log_base), log_size);
 
 	return 0;
 }
-- 
2.5.0

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

[Nayna Jain]

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>
---
 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 = &tpm_binary_b_measurements_seqops;
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		chip->bin_log_seqops.seqops =
+			&tpm2_binary_b_measurements_seqops;
+	else
+		chip->bin_log_seqops.seqops =
+			&tpm_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 = &tpm_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 *)&chip->ascii_log_seqops,
-				   &tpm_bios_measurements_ops);
-	if (IS_ERR(chip->bios_dir[cnt]))
-		goto err;
-	cnt++;
+		chip->ascii_log_seqops.chip = chip;
+		chip->ascii_log_seqops.seqops =
+			&tpm_ascii_b_measurements_seqops;
+
+		chip->bios_dir[cnt] =
+			securityfs_create_file("ascii_bios_measurements",
+					       0440, chip->bios_dir[0],
+					       (void *)&chip->ascii_log_seqops,
+					       &tpm_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 0000000..cf9fea0
--- /dev/null
+++ b/drivers/char/tpm/tpm2_eventlog.c
@@ -0,0 +1,214 @@
+/*
+ * Copyright (C) 2016 IBM Corporation
+ *
+ * Authors:
+ *      Nayna Jain <nayna@linux.vnet.ibm.com>
+ *
+ * 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 License, or (at your option) any later version.
+ */
+
+#include <linux/seq_file.h>
+#include <linux/fs.h>
+#include <linux/security.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "tpm.h"
+#include "tpm_eventlog.h"
+
+static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
+		struct tcg_pcr_event *event_header)
+{
+	struct tcg_efi_specid_event *efispecid;
+	struct tcg_event_field *event_field;
+	void *marker, *marker_start;
+	int i, j;
+	u16 halg;
+	u32 halg_size;
+	size_t size = 0;
+
+	/*
+	 * NOTE: TPM 2.0 supports extend to multiple PCR Banks. This implies
+	 * event log also has multiple digest values, one for each PCR Bank.
+	 * This is called Crypto Agile Log Entry Format.
+	 * TCG EFI Protocol Specification defines the procedure to parse
+	 * the event log. Below code implements this procedure to parse
+	 * correctly the Crypto agile log entry format.
+	 * Example of Crypto Agile Log Digests Format :
+	 * digest_values.count = 2;
+	 * digest_values.digest[0].alg_id = sha1;
+	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
+	 * digest_values.digest[1].alg_id = sha256;
+	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
+	 * Offset of eventsize is sizeof(count) + sizeof(alg_id) + 20
+	 *			+ sizeof(alg_id) + 32;
+	 *
+	 * Since, offset of event_size can vary based on digests count, offset
+	 * has to be calculated at run time. void *marker is used to traverse
+	 * the dynamic structure and calculate the offset of event_size.
+	 */
+
+	marker = event;
+	marker_start = marker;
+	marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
+		+ sizeof(event->digests.count);
+
+	efispecid = (struct tcg_efi_specid_event *) event_header->event;
+
+	for (i = 0; (i < event->digests.count) && (i < HASH_COUNT); i++) {
+		halg_size = sizeof(event->digests.digests[i].alg_id);
+		memcpy(&halg, marker, halg_size);
+		marker = marker + halg_size;
+		for (j = 0; (j < efispecid->num_algs); j++) {
+			if (halg == efispecid->digest_sizes[j].alg_id) {
+				marker = marker +
+					efispecid->digest_sizes[j].digest_size;
+				break;
+			}
+		}
+	}
+
+	event_field = (struct tcg_event_field *) marker;
+	marker = marker + sizeof(event_field->event_size)
+		+ event_field->event_size;
+	size = marker - marker_start;
+
+	if ((event->event_type == 0) && (event_field->event_size == 0))
+		return 0;
+
+	return size;
+}
+
+static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
+{
+	struct tpm_chip *chip = m->private;
+	struct tpm_bios_log *log = &chip->log;
+	void *addr = log->bios_event_log;
+	void *limit = log->bios_event_log_end;
+	struct tcg_pcr_event *event_header;
+	struct tcg_pcr_event2 *event;
+	int i;
+	size_t size = 0;
+
+	event_header = addr;
+
+	size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event)
+		+ event_header->event_size;
+
+
+	if (*pos == 0) {
+		if (addr + size < limit) {
+			if ((event_header->event_type == 0) &&
+					(event_header->event_size == 0))
+				return NULL;
+			return SEQ_START_TOKEN;
+		}
+	}
+
+	if (*pos > 0) {
+		addr += size;
+		event = addr;
+		size = calc_tpm2_event_size(event, event_header);
+		if ((addr + size >=  limit) || (size == 0))
+			return NULL;
+	}
+
+	/* read over *pos measurements */
+	for (i = 0; i < (*pos - 1); i++) {
+		event = addr;
+		size = calc_tpm2_event_size(event, event_header);
+
+		if ((addr + size >= limit) || (size == 0))
+			return NULL;
+		addr += size;
+	}
+
+	return addr;
+}
+
+static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
+		loff_t *pos)
+{
+	struct tcg_pcr_event *event_header;
+	struct tcg_pcr_event2 *event;
+	struct tpm_chip *chip = m->private;
+	struct tpm_bios_log *log = &chip->log;
+	void *limit = log->bios_event_log_end;
+	void *marker;
+	size_t event_size = 0;
+
+	event_header = log->bios_event_log;
+
+	if (v == SEQ_START_TOKEN) {
+		event_size = sizeof(struct tcg_pcr_event)
+			- sizeof(event_header->event)
+			+ event_header->event_size;
+		marker = event_header;
+	} else {
+		event = v;
+		event_size = calc_tpm2_event_size(event, event_header);
+		if (event_size == 0)
+			return NULL;
+		marker =  event;
+	}
+
+	marker = marker + event_size;
+	if (marker >= limit)
+		return NULL;
+	v = marker;
+	event = v;
+
+	event_size = calc_tpm2_event_size(event, event_header);
+	if (((v + event_size) >= limit) || (event_size == 0))
+		return NULL;
+
+	(*pos)++;
+	return v;
+}
+
+static void tpm2_bios_measurements_stop(struct seq_file *m, void *v)
+{
+}
+
+static int tpm2_binary_bios_measurements_show(struct seq_file *m, void *v)
+{
+	struct tpm_chip *chip = m->private;
+	struct tpm_bios_log *log = &chip->log;
+	struct tcg_pcr_event *event_header = log->bios_event_log;
+	struct tcg_pcr_event2 *event = v;
+	void *temp_ptr;
+	size_t size = 0;
+
+	if (v == SEQ_START_TOKEN) {
+		size = sizeof(struct tcg_pcr_event)
+			- sizeof(event_header->event)
+			+ event_header->event_size;
+
+		temp_ptr = event_header;
+
+		if (size > 0)
+			seq_write(m, temp_ptr, size);
+	} else {
+		size = calc_tpm2_event_size(event, event_header);
+		temp_ptr = event;
+		if (size > 0)
+			seq_write(m, temp_ptr, size);
+	}
+
+	return 0;
+}
+
+const struct seq_operations tpm2_binary_b_measurements_seqops = {
+	.start = tpm2_bios_measurements_start,
+	.next = tpm2_bios_measurements_next,
+	.stop = tpm2_bios_measurements_stop,
+	.show = tpm2_binary_bios_measurements_show,
+};
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
 
 #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;
+
+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

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

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 = &tpm_binary_b_measurements_seqops;
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		chip->bin_log_seqops.seqops =
> +			&tpm2_binary_b_measurements_seqops;
> +	else
> +		chip->bin_log_seqops.seqops =
> +			&tpm_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 = &tpm_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 *)&chip->ascii_log_seqops,
> -				   &tpm_bios_measurements_ops);
> -	if (IS_ERR(chip->bios_dir[cnt]))
> -		goto err;
> -	cnt++;
> +		chip->ascii_log_seqops.chip = chip;
> +		chip->ascii_log_seqops.seqops =
> +			&tpm_ascii_b_measurements_seqops;
> +
> +		chip->bios_dir[cnt] =
> +			securityfs_create_file("ascii_bios_measurements",
> +					       0440, chip->bios_dir[0],
> +					       (void *)&chip->ascii_log_seqops,
> +					       &tpm_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 0000000..cf9fea0
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2_eventlog.c
> @@ -0,0 +1,214 @@
> +/*
> + * Copyright (C) 2016 IBM Corporation
> + *
> + * Authors:
> + *      Nayna Jain <nayna@linux.vnet.ibm.com>
> + *
> + * 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 License, or (at your option) any later version.
> + */
> +
> +#include <linux/seq_file.h>
> +#include <linux/fs.h>
> +#include <linux/security.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include "tpm.h"
> +#include "tpm_eventlog.h"
> +
> +static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
> +		struct tcg_pcr_event *event_header)
> +{
> +	struct tcg_efi_specid_event *efispecid;
> +	struct tcg_event_field *event_field;
> +	void *marker, *marker_start;

Split to two lines.

> +	int i, j;

Split to two lines.

> +	u16 halg;
> +	u32 halg_size;
> +	size_t size = 0;

Please do not initialize variables unless you need to initialize them in
the declaration. It is not a good practice. And in here it is especially
misleading because you don't initialize anything else. I assumed first
that there might be a special reason why size is initialized.

> +
> +	/*
> +	 * NOTE: TPM 2.0 supports extend to multiple PCR Banks. This implies
> +	 * event log also has multiple digest values, one for each PCR Bank.
> +	 * This is called Crypto Agile Log Entry Format.
> +	 * TCG EFI Protocol Specification defines the procedure to parse
> +	 * the event log. Below code implements this procedure to parse
> +	 * correctly the Crypto agile log entry format.
> +	 * Example of Crypto Agile Log Digests Format :
> +	 * digest_values.count = 2;
> +	 * digest_values.digest[0].alg_id = sha1;
> +	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
> +	 * digest_values.digest[1].alg_id = sha256;
> +	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
> +	 * Offset of eventsize is sizeof(count) + sizeof(alg_id) + 20
> +	 *			+ sizeof(alg_id) + 32;
> +	 *

The bellow code does not implement anything. I woud just keep the first
paragraph in this comment.

> +	 * Since, offset of event_size can vary based on digests count, offset
> +	 * has to be calculated at run time. void *marker is used to traverse
> +	 * the dynamic structure and calculate the offset of event_size.
> +	 */
> +
> +	marker = event;
> +	marker_start = marker;
> +	marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
> +		+ sizeof(event->digests.count);
> +
> +	efispecid = (struct tcg_efi_specid_event *) event_header->event;
> +
> +	for (i = 0; (i < event->digests.count) && (i < HASH_COUNT); i++) {
> +		halg_size = sizeof(event->digests.digests[i].alg_id);
> +		memcpy(&halg, marker, halg_size);
> +		marker = marker + halg_size;
> +		for (j = 0; (j < efispecid->num_algs); j++) {
> +			if (halg == efispecid->digest_sizes[j].alg_id) {
> +				marker = marker +
> +					efispecid->digest_sizes[j].digest_size;
> +				break;
> +			}
> +		}
> +	}
> +
> +	event_field = (struct tcg_event_field *) marker;
> +	marker = marker + sizeof(event_field->event_size)
> +		+ event_field->event_size;
> +	size = marker - marker_start;
> +
> +	if ((event->event_type == 0) && (event_field->event_size == 0))
> +		return 0;
> +
> +	return size;
> +}
> +
> +static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
> +{
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
> +	void *addr = log->bios_event_log;
> +	void *limit = log->bios_event_log_end;
> +	struct tcg_pcr_event *event_header;
> +	struct tcg_pcr_event2 *event;
> +	int i;
> +	size_t size = 0;

Again.

> +
> +	event_header = addr;
> +
> +	size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event)
> +		+ event_header->event_size;
> +
> +

Why two newline characters?

> +	if (*pos == 0) {
> +		if (addr + size < limit) {
> +			if ((event_header->event_type == 0) &&
> +					(event_header->event_size == 0))
> +				return NULL;
> +			return SEQ_START_TOKEN;
> +		}
> +	}
> +
> +	if (*pos > 0) {
> +		addr += size;
> +		event = addr;
> +		size = calc_tpm2_event_size(event, event_header);
> +		if ((addr + size >=  limit) || (size == 0))
> +			return NULL;
> +	}
> +
> +	/* read over *pos measurements */

What does this comment tell that the code below does not tell? And in
addition to that it is incorrect. You are reading over *pos - 1
measurements, not *pos measurements.

> +	for (i = 0; i < (*pos - 1); i++) {
> +		event = addr;
> +		size = calc_tpm2_event_size(event, event_header);
> +
> +		if ((addr + size >= limit) || (size == 0))
> +			return NULL;
> +		addr += size;
> +	}
> +
> +	return addr;
> +}
> +
> +static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
> +		loff_t *pos)
> +{
> +	struct tcg_pcr_event *event_header;
> +	struct tcg_pcr_event2 *event;
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
> +	void *limit = log->bios_event_log_end;
> +	void *marker;
> +	size_t event_size = 0;

Again.

> +
> +	event_header = log->bios_event_log;
> +
> +	if (v == SEQ_START_TOKEN) {
> +		event_size = sizeof(struct tcg_pcr_event)
> +			- sizeof(event_header->event)
> +			+ event_header->event_size;
> +		marker = event_header;
> +	} else {
> +		event = v;
> +		event_size = calc_tpm2_event_size(event, event_header);
> +		if (event_size == 0)
> +			return NULL;
> +		marker =  event;
> +	}
> +
> +	marker = marker + event_size;
> +	if (marker >= limit)
> +		return NULL;
> +	v = marker;
> +	event = v;
> +
> +	event_size = calc_tpm2_event_size(event, event_header);
> +	if (((v + event_size) >= limit) || (event_size == 0))
> +		return NULL;
> +
> +	(*pos)++;
> +	return v;
> +}
> +
> +static void tpm2_bios_measurements_stop(struct seq_file *m, void *v)
> +{
> +}
> +
> +static int tpm2_binary_bios_measurements_show(struct seq_file *m, void *v)
> +{
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
> +	struct tcg_pcr_event *event_header = log->bios_event_log;
> +	struct tcg_pcr_event2 *event = v;
> +	void *temp_ptr;
> +	size_t size = 0;
> +
> +	if (v == SEQ_START_TOKEN) {
> +		size = sizeof(struct tcg_pcr_event)
> +			- sizeof(event_header->event)
> +			+ event_header->event_size;
> +
> +		temp_ptr = event_header;
> +
> +		if (size > 0)
> +			seq_write(m, temp_ptr, size);
> +	} else {
> +		size = calc_tpm2_event_size(event, event_header);
> +		temp_ptr = event;
> +		if (size > 0)
> +			seq_write(m, temp_ptr, size);
> +	}
> +
> +	return 0;
> +}
> +
> +const struct seq_operations tpm2_binary_b_measurements_seqops = {
> +	.start = tpm2_bios_measurements_start,
> +	.next = tpm2_bios_measurements_next,
> +	.stop = tpm2_bios_measurements_stop,
> +	.show = tpm2_binary_bios_measurements_show,
> +};
> 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_.

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

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

Re: [PATCH v6 1/2] tpm: enhance read_log_of() to support Physical TPM event log

[Jarkko Sakkinen]

On Sat, Nov 26, 2016 at 07:45:38AM -0500, Nayna Jain wrote:
> Physical TPMs use Open Firmware Device Tree bindings that are similar
> to the IBM Power virtual TPM to support event log. However, these
> properties store the values in different endianness for Physical
> and Virtual TPM.
> 
> This patch fixes the endianness issue by doing appropriate conversion
> based on Physical or Virtual TPM.
> 
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm_of.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 7dee42d7..4b0d5e6 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -27,6 +27,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>  	const u32 *sizep;
>  	const u64 *basep;
>  	struct tpm_bios_log *log;
> +	u32 log_size;
> +	u64 log_base;

Maybe just base and size?

>  
>  	log = &chip->log;
>  	if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -41,18 +43,35 @@ int tpm_read_log_of(struct tpm_chip *chip)
>  	if (sizep == NULL || basep == NULL)
>  		return -EIO;
>  
> -	if (*sizep == 0) {
> +	/*
> +	 * For both vtpm/tpm, firmware has log addr and log size in big
> +	 * endian format. But in case of vtpm, there is a method called
> +	 * sml-handover which is run during kernel init even before
> +	 * device tree is setup. This sml-handover function takes care
> +	 * of endianness and writes to sml-base and sml-size in little
> +	 * endian format. For this reason, vtpm doesn't need conversion
> +	 * but physical tpm needs the conversion.
> +	 */
> +	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
> +		log_size = be32_to_cpup(sizep);
> +		log_base = be64_to_cpup(basep);
> +	} else {
> +		log_size = *sizep;
> +		log_base = *basep;
> +	}
> +
> +	if (log_size == 0) {
>  		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
>  		return -EIO;
>  	}
>  
> -	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> +	log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
>  	if (!log->bios_event_log)
>  		return -ENOMEM;
>  
> -	log->bios_event_log_end = log->bios_event_log + *sizep;
> +	log->bios_event_log_end = log->bios_event_log + log_size;
>  
> -	memcpy(log->bios_event_log, __va(*basep), *sizep);
> +	memcpy(chip->log.bios_event_log, __va(log_base), log_size);

Why did you change the first parameter?

>  
>  	return 0;
>  }
> -- 
> 2.5.0
> 

/Jarkko

Re: [PATCH v6 1/2] tpm: enhance read_log_of() to support Physical TPM event log

[Nayna]

On 11/26/2016 09:45 PM, Jarkko Sakkinen wrote:
> On Sat, Nov 26, 2016 at 07:45:38AM -0500, Nayna Jain wrote:
>> Physical TPMs use Open Firmware Device Tree bindings that are similar
>> to the IBM Power virtual TPM to support event log. However, these
>> properties store the values in different endianness for Physical
>> and Virtual TPM.
>>
>> This patch fixes the endianness issue by doing appropriate conversion
>> based on Physical or Virtual TPM.
>>
>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>> ---
>>   drivers/char/tpm/tpm_of.c | 27 +++++++++++++++++++++++----
>>   1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
>> index 7dee42d7..4b0d5e6 100644
>> --- a/drivers/char/tpm/tpm_of.c
>> +++ b/drivers/char/tpm/tpm_of.c
>> @@ -27,6 +27,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>>   	const u32 *sizep;
>>   	const u64 *basep;
>>   	struct tpm_bios_log *log;
>> +	u32 log_size;
>> +	u64 log_base;
>
> Maybe just base and size?

Hmm.. okay to keep names short, I can change to base/size. And since 
they are used within log function, probably it is implicit that, these 
refer to Log  base address and size.

>
>>
>>   	log = &chip->log;
>>   	if (chip->dev.parent && chip->dev.parent->of_node)
>> @@ -41,18 +43,35 @@ int tpm_read_log_of(struct tpm_chip *chip)
>>   	if (sizep == NULL || basep == NULL)
>>   		return -EIO;
>>
>> -	if (*sizep == 0) {
>> +	/*
>> +	 * For both vtpm/tpm, firmware has log addr and log size in big
>> +	 * endian format. But in case of vtpm, there is a method called
>> +	 * sml-handover which is run during kernel init even before
>> +	 * device tree is setup. This sml-handover function takes care
>> +	 * of endianness and writes to sml-base and sml-size in little
>> +	 * endian format. For this reason, vtpm doesn't need conversion
>> +	 * but physical tpm needs the conversion.
>> +	 */
>> +	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
>> +		log_size = be32_to_cpup(sizep);
>> +		log_base = be64_to_cpup(basep);
>> +	} else {
>> +		log_size = *sizep;
>> +		log_base = *basep;
>> +	}
>> +
>> +	if (log_size == 0) {
>>   		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
>>   		return -EIO;
>>   	}
>>
>> -	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
>> +	log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
>>   	if (!log->bios_event_log)
>>   		return -ENOMEM;
>>
>> -	log->bios_event_log_end = log->bios_event_log + *sizep;
>> +	log->bios_event_log_end = log->bios_event_log + log_size;
>>
>> -	memcpy(log->bios_event_log, __va(*basep), *sizep);
>> +	memcpy(chip->log.bios_event_log, __va(log_base), log_size);
>
> Why did you change the first parameter?

Hmm.. yeah..shouldn't be.  Sorry, I think while redoing, I mixed up with 
my previous versions. Will change it.
Thanks for noticing it.

Thanks & Regards,
    - Nayna

>
>>
>>   	return 0;
>>   }
>> --
>> 2.5.0
>>
>
> /Jarkko
>

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

[Nayna]

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

>
>> ---
>>   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 = &tpm_binary_b_measurements_seqops;
>> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>> +		chip->bin_log_seqops.seqops =
>> +			&tpm2_binary_b_measurements_seqops;
>> +	else
>> +		chip->bin_log_seqops.seqops =
>> +			&tpm_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 = &tpm_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 *)&chip->ascii_log_seqops,
>> -				   &tpm_bios_measurements_ops);
>> -	if (IS_ERR(chip->bios_dir[cnt]))
>> -		goto err;
>> -	cnt++;
>> +		chip->ascii_log_seqops.chip = chip;
>> +		chip->ascii_log_seqops.seqops =
>> +			&tpm_ascii_b_measurements_seqops;
>> +
>> +		chip->bios_dir[cnt] =
>> +			securityfs_create_file("ascii_bios_measurements",
>> +					       0440, chip->bios_dir[0],
>> +					       (void *)&chip->ascii_log_seqops,
>> +					       &tpm_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 0000000..cf9fea0
>> --- /dev/null
>> +++ b/drivers/char/tpm/tpm2_eventlog.c
>> @@ -0,0 +1,214 @@
>> +/*
>> + * Copyright (C) 2016 IBM Corporation
>> + *
>> + * Authors:
>> + *      Nayna Jain <nayna@linux.vnet.ibm.com>
>> + *
>> + * 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 License, or (at your option) any later version.
>> + */
>> +
>> +#include <linux/seq_file.h>
>> +#include <linux/fs.h>
>> +#include <linux/security.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +
>> +#include "tpm.h"
>> +#include "tpm_eventlog.h"
>> +
>> +static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
>> +		struct tcg_pcr_event *event_header)
>> +{
>> +	struct tcg_efi_specid_event *efispecid;
>> +	struct tcg_event_field *event_field;
>> +	void *marker, *marker_start;
>
> Split to two lines.

Will fix it.

>
>> +	int i, j;
>
> Split to two lines.

Sure.
>
>> +	u16 halg;
>> +	u32 halg_size;
>> +	size_t size = 0;
>
> Please do not initialize variables unless you need to initialize them in
> the declaration. It is not a good practice. And in here it is especially
> misleading because you don't initialize anything else. I assumed first
> that there might be a special reason why size is initialized.
>

Sure. Will fix.

>> +
>> +	/*
>> +	 * NOTE: TPM 2.0 supports extend to multiple PCR Banks. This implies
>> +	 * event log also has multiple digest values, one for each PCR Bank.
>> +	 * This is called Crypto Agile Log Entry Format.
>> +	 * TCG EFI Protocol Specification defines the procedure to parse
>> +	 * the event log. Below code implements this procedure to parse
>> +	 * correctly the Crypto agile log entry format.
>> +	 * Example of Crypto Agile Log Digests Format :
>> +	 * digest_values.count = 2;
>> +	 * digest_values.digest[0].alg_id = sha1;
>> +	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
>> +	 * digest_values.digest[1].alg_id = sha256;
>> +	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
>> +	 * Offset of eventsize is sizeof(count) + sizeof(alg_id) + 20
>> +	 *			+ sizeof(alg_id) + 32;
>> +	 *
>
> The bellow code does not implement anything. I woud just keep the first
> paragraph in this comment.

Ok. Thanks for looking on this. I will keep only first paragraph.

>
>> +	 * Since, offset of event_size can vary based on digests count, offset
>> +	 * has to be calculated at run time. void *marker is used to traverse
>> +	 * the dynamic structure and calculate the offset of event_size.
>> +	 */
>> +
>> +	marker = event;
>> +	marker_start = marker;
>> +	marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
>> +		+ sizeof(event->digests.count);
>> +
>> +	efispecid = (struct tcg_efi_specid_event *) event_header->event;
>> +
>> +	for (i = 0; (i < event->digests.count) && (i < HASH_COUNT); i++) {
>> +		halg_size = sizeof(event->digests.digests[i].alg_id);
>> +		memcpy(&halg, marker, halg_size);
>> +		marker = marker + halg_size;
>> +		for (j = 0; (j < efispecid->num_algs); j++) {
>> +			if (halg == efispecid->digest_sizes[j].alg_id) {
>> +				marker = marker +
>> +					efispecid->digest_sizes[j].digest_size;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	event_field = (struct tcg_event_field *) marker;
>> +	marker = marker + sizeof(event_field->event_size)
>> +		+ event_field->event_size;
>> +	size = marker - marker_start;
>> +
>> +	if ((event->event_type == 0) && (event_field->event_size == 0))
>> +		return 0;
>> +
>> +	return size;
>> +}
>> +
>> +static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
>> +{
>> +	struct tpm_chip *chip = m->private;
>> +	struct tpm_bios_log *log = &chip->log;
>> +	void *addr = log->bios_event_log;
>> +	void *limit = log->bios_event_log_end;
>> +	struct tcg_pcr_event *event_header;
>> +	struct tcg_pcr_event2 *event;
>> +	int i;
>> +	size_t size = 0;
>
> Again.

Will fix.

>
>> +
>> +	event_header = addr;
>> +
>> +	size = sizeof(struct tcg_pcr_event) - sizeof(event_header->event)
>> +		+ event_header->event_size;
>> +
>> +
>
> Why two newline characters?

Will fix.
>
>> +	if (*pos == 0) {
>> +		if (addr + size < limit) {
>> +			if ((event_header->event_type == 0) &&
>> +					(event_header->event_size == 0))
>> +				return NULL;
>> +			return SEQ_START_TOKEN;
>> +		}
>> +	}
>> +
>> +	if (*pos > 0) {
>> +		addr += size;
>> +		event = addr;
>> +		size = calc_tpm2_event_size(event, event_header);
>> +		if ((addr + size >=  limit) || (size == 0))
>> +			return NULL;
>> +	}
>> +
>> +	/* read over *pos measurements */
>
> What does this comment tell that the code below does not tell? And in
> addition to that it is incorrect. You are reading over *pos - 1
> measurements, not *pos measurements.

Hmm.. Ok.. looks now like probably not needed.

>
>> +	for (i = 0; i < (*pos - 1); i++) {
>> +		event = addr;
>> +		size = calc_tpm2_event_size(event, event_header);
>> +
>> +		if ((addr + size >= limit) || (size == 0))
>> +			return NULL;
>> +		addr += size;
>> +	}
>> +
>> +	return addr;
>> +}
>> +
>> +static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
>> +		loff_t *pos)
>> +{
>> +	struct tcg_pcr_event *event_header;
>> +	struct tcg_pcr_event2 *event;
>> +	struct tpm_chip *chip = m->private;
>> +	struct tpm_bios_log *log = &chip->log;
>> +	void *limit = log->bios_event_log_end;
>> +	void *marker;
>> +	size_t event_size = 0;
>
> Again.

Will fix.
>
>> +
>> +	event_header = log->bios_event_log;
>> +
>> +	if (v == SEQ_START_TOKEN) {
>> +		event_size = sizeof(struct tcg_pcr_event)
>> +			- sizeof(event_header->event)
>> +			+ event_header->event_size;
>> +		marker = event_header;
>> +	} else {
>> +		event = v;
>> +		event_size = calc_tpm2_event_size(event, event_header);
>> +		if (event_size == 0)
>> +			return NULL;
>> +		marker =  event;
>> +	}
>> +
>> +	marker = marker + event_size;
>> +	if (marker >= limit)
>> +		return NULL;
>> +	v = marker;
>> +	event = v;
>> +
>> +	event_size = calc_tpm2_event_size(event, event_header);
>> +	if (((v + event_size) >= limit) || (event_size == 0))
>> +		return NULL;
>> +
>> +	(*pos)++;
>> +	return v;
>> +}
>> +
>> +static void tpm2_bios_measurements_stop(struct seq_file *m, void *v)
>> +{
>> +}
>> +
>> +static int tpm2_binary_bios_measurements_show(struct seq_file *m, void *v)
>> +{
>> +	struct tpm_chip *chip = m->private;
>> +	struct tpm_bios_log *log = &chip->log;
>> +	struct tcg_pcr_event *event_header = log->bios_event_log;
>> +	struct tcg_pcr_event2 *event = v;
>> +	void *temp_ptr;
>> +	size_t size = 0;
>> +
>> +	if (v == SEQ_START_TOKEN) {
>> +		size = sizeof(struct tcg_pcr_event)
>> +			- sizeof(event_header->event)
>> +			+ event_header->event_size;
>> +
>> +		temp_ptr = event_header;
>> +
>> +		if (size > 0)
>> +			seq_write(m, temp_ptr, size);
>> +	} else {
>> +		size = calc_tpm2_event_size(event, event_header);
>> +		temp_ptr = event;
>> +		if (size > 0)
>> +			seq_write(m, temp_ptr, size);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +const struct seq_operations tpm2_binary_b_measurements_seqops = {
>> +	.start = tpm2_bios_measurements_start,
>> +	.next = tpm2_bios_measurements_next,
>> +	.stop = tpm2_bios_measurements_stop,
>> +	.show = tpm2_binary_bios_measurements_show,
>> +};
>> 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.

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

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

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

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