[PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Nayna Jain]

Existing TPM2.0 support lacks the support for eventlog securityfs file.
This patch adds the binary_bios_measurements to TPM2.0 eventlog
securityfs file.

Additionally, it also includes the review feedbacks as suggested by
Jason.

Further, commit msg subject line is prefixed with tpm as was suggested
by Jarkko.

Changelog 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

Nayna Jain (7):
  tpm: Define a generic open() method for ascii & bios measurements.
  tpm: Replace the dynamically allocated bios_dir as struct dentry
    array.
  tpm: Validate the eventlog access before tpm_bios_log_setup
  tpm: Redefine the read_log method to check for ACPI/OF properties
    sequentially
  tpm: Replace the of_find_node_by_name() with dev of_node property
  tpm: Moves the eventlog init functions to tpm_eventlog_init.c
  tpm: Adds securityfs support for TPM2.0 eventlog

 drivers/char/tpm/Makefile            |  13 +-
 drivers/char/tpm/tpm-chip.c          |  21 +---
 drivers/char/tpm/tpm.h               |   7 +-
 drivers/char/tpm/tpm2.h              |  85 +++++++++++++
 drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_acpi.c          |  19 +--
 drivers/char/tpm/tpm_eventlog.c      | 154 +-----------------------
 drivers/char/tpm/tpm_eventlog.h      |  26 ++--
 drivers/char/tpm/tpm_eventlog_init.c | 153 ++++++++++++++++++++++++
 drivers/char/tpm/tpm_of.c            |  65 ++++++----
 10 files changed, 543 insertions(+), 224 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2.h
 create mode 100644 drivers/char/tpm/tpm2_eventlog.c
 create mode 100644 drivers/char/tpm/tpm_eventlog_init.c

-- 
2.5.0


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

[PATCH v3 1/7] tpm: Define a generic open() method for ascii & bios measurements.

[Nayna Jain]

Open methods for eventlog ascii and binary bios measurements file
operations are very similar. This patch refactors the code into
single open() call by passing seq_operations as i_node->private data.

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm_eventlog.c | 59 +++++++++--------------------------------
 1 file changed, 13 insertions(+), 46 deletions(-)

diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index e722886..b0a4d02 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -7,6 +7,7 @@
  *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
  *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
  *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *	Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
  *
  * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
  *
@@ -318,12 +319,14 @@ static const struct seq_operations tpm_binary_b_measurments_seqops = {
 	.show = tpm_binary_bios_measurements_show,
 };
 
-static int tpm_ascii_bios_measurements_open(struct inode *inode,
+static int tpm_bios_measurements_open(struct inode *inode,
 					    struct file *file)
 {
 	int err;
 	struct tpm_bios_log *log;
 	struct seq_file *seq;
+	const struct seq_operations *seqops =
+	(const struct seq_operations *)inode->i_private;
 
 	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
 	if (!log)
@@ -333,7 +336,7 @@ static int tpm_ascii_bios_measurements_open(struct inode *inode,
 		goto out_free;
 
 	/* now register seq file */
-	err = seq_open(file, &tpm_ascii_b_measurments_seqops);
+	err = seq_open(file, seqops);
 	if (!err) {
 		seq = file->private_data;
 		seq->private = log;
@@ -349,46 +352,8 @@ out_free:
 	goto out;
 }
 
-static const struct file_operations tpm_ascii_bios_measurements_ops = {
-	.open = tpm_ascii_bios_measurements_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = tpm_bios_measurements_release,
-};
-
-static int tpm_binary_bios_measurements_open(struct inode *inode,
-					     struct file *file)
-{
-	int err;
-	struct tpm_bios_log *log;
-	struct seq_file *seq;
-
-	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
-	if (!log)
-		return -ENOMEM;
-
-	if ((err = read_log(log)))
-		goto out_free;
-
-	/* now register seq file */
-	err = seq_open(file, &tpm_binary_b_measurments_seqops);
-	if (!err) {
-		seq = file->private_data;
-		seq->private = log;
-	} else {
-		goto out_free;
-	}
-
-out:
-	return err;
-out_free:
-	kfree(log->bios_event_log);
-	kfree(log);
-	goto out;
-}
-
-static const struct file_operations tpm_binary_bios_measurements_ops = {
-	.open = tpm_binary_bios_measurements_open,
+static const struct file_operations tpm_bios_measurements_ops = {
+	.open = tpm_bios_measurements_open,
 	.read = seq_read,
 	.llseek = seq_lseek,
 	.release = tpm_bios_measurements_release,
@@ -413,15 +378,17 @@ struct dentry **tpm_bios_log_setup(const char *name)
 
 	bin_file =
 	    securityfs_create_file("binary_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
-				   &tpm_binary_bios_measurements_ops);
+				   S_IRUSR | S_IRGRP, tpm_dir,
+				   (void *)&tpm_binary_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
 	if (is_bad(bin_file))
 		goto out_tpm;
 
 	ascii_file =
 	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
-				   &tpm_ascii_bios_measurements_ops);
+				   S_IRUSR | S_IRGRP, tpm_dir,
+				   (void *)&tpm_ascii_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
 	if (is_bad(ascii_file))
 		goto out_bin;
 
-- 
2.5.0


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

[PATCH v3 2/7] tpm: Replace the dynamically allocated bios_dir as struct dentry array.

[Nayna Jain]

bios_dir is defined as struct dentry **bios_dir, which results in
dynamic allocation and possible memory leak. This patch replaces
it with struct dentry array i.e. struct dentry *bios_dir[3]
similar to what is done for sysfs groups.

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c     |  5 ++--
 drivers/char/tpm/tpm.h          |  3 ++-
 drivers/char/tpm/tpm_eventlog.c | 60 ++++++++++++++++++-----------------------
 drivers/char/tpm/tpm_eventlog.h | 10 +++----
 4 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e595013..1cd1238 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -283,7 +283,7 @@ static int tpm1_chip_register(struct tpm_chip *chip)
 
 	tpm_sysfs_add_device(chip);
 
-	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
+	tpm_bios_log_setup(chip);
 
 	return 0;
 }
@@ -293,8 +293,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return;
 
-	if (chip->bios_dir)
-		tpm_bios_log_teardown(chip->bios_dir);
+	tpm_bios_log_teardown(chip);
 }
 
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 6e002c4..603a661 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -171,7 +171,8 @@ struct tpm_chip {
 	unsigned long duration[3]; /* jiffies */
 	bool duration_adjusted;
 
-	struct dentry **bios_dir;
+	struct dentry *bios_dir[3];
+	unsigned int bios_dir_count;
 
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index b0a4d02..9dd69a7 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -332,7 +332,8 @@ static int tpm_bios_measurements_open(struct inode *inode,
 	if (!log)
 		return -ENOMEM;
 
-	if ((err = read_log(log)))
+	err = read_log(log);
+	if (err)
 		goto out_free;
 
 	/* now register seq file */
@@ -368,54 +369,45 @@ static int is_bad(void *p)
 	return 0;
 }
 
-struct dentry **tpm_bios_log_setup(const char *name)
+void tpm_bios_log_setup(struct tpm_chip *chip)
 {
-	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
+	const char *name = dev_name(&chip->dev);
 
-	tpm_dir = securityfs_create_dir(name, NULL);
-	if (is_bad(tpm_dir))
-		goto out;
+	chip->bios_dir_count = 0;
+	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
+	NULL);
+	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+		goto err;
+	chip->bios_dir_count++;
 
-	bin_file =
+	chip->bios_dir[chip->bios_dir_count] =
 	    securityfs_create_file("binary_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir,
+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
 				   (void *)&tpm_binary_b_measurments_seqops,
 				   &tpm_bios_measurements_ops);
-	if (is_bad(bin_file))
-		goto out_tpm;
+	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+		goto err;
+	chip->bios_dir_count++;
 
-	ascii_file =
+	chip->bios_dir[chip->bios_dir_count] =
 	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, tpm_dir,
+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
 				   (void *)&tpm_ascii_b_measurments_seqops,
 				   &tpm_bios_measurements_ops);
-	if (is_bad(ascii_file))
-		goto out_bin;
+	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+		goto err;
+	chip->bios_dir_count++;
 
-	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
-	if (!ret)
-		goto out_ascii;
+	return;
 
-	ret[0] = ascii_file;
-	ret[1] = bin_file;
-	ret[2] = tpm_dir;
-
-	return ret;
-
-out_ascii:
-	securityfs_remove(ascii_file);
-out_bin:
-	securityfs_remove(bin_file);
-out_tpm:
-	securityfs_remove(tpm_dir);
-out:
-	return NULL;
+err:
+	tpm_bios_log_teardown(chip);
 }
 
-void tpm_bios_log_teardown(struct dentry **lst)
+void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 	int i;
 
-	for (i = 0; i < 3; i++)
-		securityfs_remove(lst[i]);
+	for (i = chip->bios_dir_count; i > 0; --i)
+		securityfs_remove(chip->bios_dir[i-1]);
 }
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 8de62b0..67621c9 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -77,14 +77,14 @@ int read_log(struct tpm_bios_log *log);
 
 #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 **);
+extern void tpm_bios_log_setup(struct tpm_chip *chip);
+extern void tpm_bios_log_teardown(struct tpm_chip *chip);
 #else
-static inline struct dentry **tpm_bios_log_setup(const char *name)
+static inline void tpm_bios_log_setup(struct tpm_chip *chip)
 {
-	return NULL;
+	chip->bios_dir_count = 0;
 }
-static inline void tpm_bios_log_teardown(struct dentry **dir)
+static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 }
 #endif
-- 
2.5.0


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

[PATCH v3 3/7] tpm: Validate the eventlog access before tpm_bios_log_setup

[Nayna Jain]

Currently, securityfs files for eventlog is created irrespective of
logs properties exist or not i.e. event log base address and
size.

This patch will create ascii and bios measurements file
only if readlog() is successful.

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/Makefile       | 10 ++++++----
 drivers/char/tpm/tpm-chip.c     | 18 ++++--------------
 drivers/char/tpm/tpm.h          |  4 ++++
 drivers/char/tpm/tpm_acpi.c     | 14 +++++++-------
 drivers/char/tpm/tpm_eventlog.c | 36 ++++++++++++------------------------
 drivers/char/tpm/tpm_eventlog.h | 17 +++--------------
 drivers/char/tpm/tpm_of.c       | 12 ++++++------
 7 files changed, 42 insertions(+), 69 deletions(-)

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a385fb8..00e48e4 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -2,14 +2,16 @@
 # Makefile for the kernel tpm device drivers.
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
-tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
+tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
+	tpm_eventlog.o
+
 tpm-$(CONFIG_ACPI) += tpm_ppi.o
 
 ifdef CONFIG_ACPI
-	tpm-y += tpm_eventlog.o tpm_acpi.o
+	tpm-y += tpm_acpi.o
 else
-ifdef CONFIG_TCG_IBMVTPM
-	tpm-y += tpm_eventlog.o tpm_of.o
+ifdef CONFIG_OF
+	tpm-y += tpm_of.o
 endif
 endif
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 1cd1238..307130e 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -283,19 +283,9 @@ static int tpm1_chip_register(struct tpm_chip *chip)
 
 	tpm_sysfs_add_device(chip);
 
-	tpm_bios_log_setup(chip);
-
 	return 0;
 }
 
-static void tpm1_chip_unregister(struct tpm_chip *chip)
-{
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		return;
-
-	tpm_bios_log_teardown(chip);
-}
-
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
 {
 	struct attribute **i;
@@ -369,10 +359,8 @@ int tpm_chip_register(struct tpm_chip *chip)
 	tpm_add_ppi(chip);
 
 	rc = tpm_add_char_device(chip);
-	if (rc) {
-		tpm1_chip_unregister(chip);
+	if (rc)
 		return rc;
-	}
 
 	chip->flags |= TPM_CHIP_FLAG_REGISTERED;
 
@@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip)
 		return rc;
 	}
 
+	tpm_bios_log_setup(chip);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_chip_register);
@@ -406,7 +396,7 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 
 	tpm_del_legacy_sysfs(chip);
 
-	tpm1_chip_unregister(chip);
+	tpm_bios_log_teardown(chip);
 	tpm_del_char_device(chip);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_unregister);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 603a661..032eb7d 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -35,6 +35,8 @@
 #include <linux/cdev.h>
 #include <linux/highmem.h>
 
+#include "tpm_eventlog.h"
+
 enum tpm_const {
 	TPM_MINOR = 224,	/* officially assigned */
 	TPM_BUFSIZE = 4096,
@@ -156,6 +158,8 @@ struct tpm_chip {
 	struct rw_semaphore ops_sem;
 	const struct tpm_class_ops *ops;
 
+	struct tpm_bios_log log;
+
 	unsigned int flags;
 
 	int dev_num;		/* /dev/tpm# */
diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 565a947..05b4e8a 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -45,14 +45,14 @@ struct acpi_tcpa {
 };
 
 /* read binary bios log */
-int read_log(struct tpm_bios_log *log)
+int read_log(struct tpm_chip *chip)
 {
 	struct acpi_tcpa *buff;
 	acpi_status status;
 	void __iomem *virt;
 	u64 len, start;
 
-	if (log->bios_event_log != NULL) {
+	if (chip->log.bios_event_log != NULL) {
 		printk(KERN_ERR
 		       "%s: ERROR - Eventlog already initialized\n",
 		       __func__);
@@ -86,23 +86,23 @@ int read_log(struct tpm_bios_log *log)
 	}
 
 	/* malloc EventLog space */
-	log->bios_event_log = kmalloc(len, GFP_KERNEL);
-	if (!log->bios_event_log) {
+	chip->log.bios_event_log = kmalloc(len, GFP_KERNEL);
+	if (!chip->log.bios_event_log) {
 		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
 			__func__);
 		return -ENOMEM;
 	}
 
-	log->bios_event_log_end = log->bios_event_log + len;
+	chip->log.bios_event_log_end = chip->log.bios_event_log + len;
 
 	virt = acpi_os_map_iomem(start, len);
 	if (!virt) {
-		kfree(log->bios_event_log);
+		kfree(chip->log.bios_event_log);
 		printk("%s: ERROR - Unable to map memory\n", __func__);
 		return -EIO;
 	}
 
-	memcpy_fromio(log->bios_event_log, virt, len);
+	memcpy_fromio(chip->log.bios_event_log, virt, len);
 
 	acpi_os_unmap_iomem(virt, len);
 	return 0;
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index 9dd69a7..d6f2477 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -261,14 +261,6 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
 static int tpm_bios_measurements_release(struct inode *inode,
 					 struct file *file)
 {
-	struct seq_file *seq = file->private_data;
-	struct tpm_bios_log *log = seq->private;
-
-	if (log) {
-		kfree(log->bios_event_log);
-		kfree(log);
-	}
-
 	return seq_release(inode, file);
 }
 
@@ -323,34 +315,22 @@ static int tpm_bios_measurements_open(struct inode *inode,
 					    struct file *file)
 {
 	int err;
-	struct tpm_bios_log *log;
 	struct seq_file *seq;
+	struct tpm_chip *chip;
 	const struct seq_operations *seqops =
 	(const struct seq_operations *)inode->i_private;
 
-	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
-	if (!log)
-		return -ENOMEM;
-
-	err = read_log(log);
-	if (err)
-		goto out_free;
+	chip = (struct tpm_chip
+	*)file->f_path.dentry->d_parent->d_inode->i_private;
 
 	/* now register seq file */
 	err = seq_open(file, seqops);
 	if (!err) {
 		seq = file->private_data;
-		seq->private = log;
-	} else {
-		goto out_free;
+		seq->private = &chip->log;
 	}
 
-out:
 	return err;
-out_free:
-	kfree(log->bios_event_log);
-	kfree(log);
-	goto out;
 }
 
 static const struct file_operations tpm_bios_measurements_ops = {
@@ -372,12 +352,18 @@ static int is_bad(void *p)
 void tpm_bios_log_setup(struct tpm_chip *chip)
 {
 	const char *name = dev_name(&chip->dev);
+	int rc = 0;
+
+	rc = read_log(chip);
+	if (rc < 0)
+		return;
 
 	chip->bios_dir_count = 0;
 	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
 	NULL);
 	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
 		goto err;
+	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip;
 	chip->bios_dir_count++;
 
 	chip->bios_dir[chip->bios_dir_count] =
@@ -410,4 +396,6 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
 
 	for (i = chip->bios_dir_count; i > 0; --i)
 		securityfs_remove(chip->bios_dir[i-1]);
+
+	kfree(chip->log.bios_event_log);
 }
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 67621c9..6a01d43 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -73,20 +73,9 @@ enum tcpa_pc_event_ids {
 	HOST_TABLE_OF_DEVICES,
 };
 
-int read_log(struct tpm_bios_log *log);
+int read_log(struct tpm_chip *chip);
 
-#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
-	defined(CONFIG_ACPI)
-extern void tpm_bios_log_setup(struct tpm_chip *chip);
-extern void tpm_bios_log_teardown(struct tpm_chip *chip);
-#else
-static inline void tpm_bios_log_setup(struct tpm_chip *chip)
-{
-	chip->bios_dir_count = 0;
-}
-static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
-{
-}
-#endif
+void tpm_bios_log_setup(struct tpm_chip *chip);
+void tpm_bios_log_teardown(struct tpm_chip *chip);
 
 #endif
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 570f30c..8e77976 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -20,13 +20,13 @@
 #include "tpm.h"
 #include "tpm_eventlog.h"
 
-int read_log(struct tpm_bios_log *log)
+int read_log(struct tpm_chip *chip)
 {
 	struct device_node *np;
 	const u32 *sizep;
 	const u64 *basep;
 
-	if (log->bios_event_log != NULL) {
+	if (chip->log.bios_event_log != NULL) {
 		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
 		return -EFAULT;
 	}
@@ -53,17 +53,17 @@ int read_log(struct tpm_bios_log *log)
 		goto cleanup_eio;
 	}
 
-	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
-	if (!log->bios_event_log) {
+	chip->log.bios_event_log = kmalloc(*sizep, GFP_KERNEL);
+	if (!chip->log.bios_event_log) {
 		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
 		       __func__);
 		of_node_put(np);
 		return -ENOMEM;
 	}
 
-	log->bios_event_log_end = log->bios_event_log + *sizep;
+	chip->log.bios_event_log_end = chip->log.bios_event_log + *sizep;
 
-	memcpy(log->bios_event_log, __va(*basep), *sizep);
+	memcpy(chip->log.bios_event_log, __va(*basep), *sizep);
 	of_node_put(np);
 
 	return 0;
-- 
2.5.0


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

[PATCH v3 4/7] tpm: Redefine the read_log method to check for ACPI/OF properties sequentially

[Nayna Jain]

Currently, the difference in read_log method for ACPI/OF based platforms
is handled by defining respective read_log method and handing
them using CONFIG based #ifdef condition in Makefile which is not
the recommended approach.

This patch cleans up the ifdef condition in Makefile by defining
single read_log method which checks for ACPI/OF event log memory in
sequence.

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/Makefile       | 11 ++---------
 drivers/char/tpm/tpm_acpi.c     |  9 +--------
 drivers/char/tpm/tpm_eventlog.c | 17 +++++++++++++++++
 drivers/char/tpm/tpm_eventlog.h | 18 +++++++++++++++++-
 drivers/char/tpm/tpm_of.c       | 11 +++++------
 5 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 00e48e4..e8c7b4d 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -5,15 +5,8 @@ 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
 
-tpm-$(CONFIG_ACPI) += tpm_ppi.o
-
-ifdef CONFIG_ACPI
-	tpm-y += tpm_acpi.o
-else
-ifdef CONFIG_OF
-	tpm-y += tpm_of.o
-endif
-endif
+tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
+tpm-$(CONFIG_OF) += tpm_of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
 obj-$(CONFIG_TCG_TIS) += tpm_tis.o
 obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 05b4e8a..a670c4f 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -45,20 +45,13 @@ struct acpi_tcpa {
 };
 
 /* read binary bios log */
-int read_log(struct tpm_chip *chip)
+int read_log_acpi(struct tpm_chip *chip)
 {
 	struct acpi_tcpa *buff;
 	acpi_status status;
 	void __iomem *virt;
 	u64 len, start;
 
-	if (chip->log.bios_event_log != NULL) {
-		printk(KERN_ERR
-		       "%s: ERROR - Eventlog already initialized\n",
-		       __func__);
-		return -EFAULT;
-	}
-
 	/* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
 	status = acpi_get_table(ACPI_SIG_TCPA, 1,
 				(struct acpi_table_header **)&buff);
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index d6f2477..f84ce71 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -349,6 +349,23 @@ static int is_bad(void *p)
 	return 0;
 }
 
+int read_log(struct tpm_chip *chip)
+{
+	int rc;
+
+	if (chip->log.bios_event_log != NULL) {
+		dev_dbg(&chip->dev, "%s:ERROR - Eventlog already initialized\n",
+			__func__);
+		return -EFAULT;
+	}
+
+	rc = read_log_acpi(chip);
+	if (rc == 0)
+		return rc;
+	rc = read_log_of(chip);
+	return rc;
+}
+
 void tpm_bios_log_setup(struct tpm_chip *chip)
 {
 	const char *name = dev_name(&chip->dev);
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 6a01d43..0e599ab 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -73,7 +73,23 @@ enum tcpa_pc_event_ids {
 	HOST_TABLE_OF_DEVICES,
 };
 
-int read_log(struct tpm_chip *chip);
+#if defined(CONFIG_ACPI)
+int read_log_acpi(struct tpm_chip *chip);
+#else
+static inline int read_log_acpi(struct tpm_chip *chip)
+{
+	return -1;
+}
+#endif
+
+#if defined(CONFIG_OF)
+int read_log_of(struct tpm_chip *chip);
+#else
+static inline int read_log_of(struct tpm_chip *chip)
+{
+	return -1;
+}
+#endif
 
 void tpm_bios_log_setup(struct tpm_chip *chip);
 void tpm_bios_log_teardown(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 8e77976..5067a86 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -14,23 +14,22 @@
  *
  */
 
+#include <linux/seq_file.h>
+#include <linux/fs.h>
+#include <linux/security.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 
 #include "tpm.h"
 #include "tpm_eventlog.h"
 
-int read_log(struct tpm_chip *chip)
+int read_log_of(struct tpm_chip *chip)
 {
 	struct device_node *np;
 	const u32 *sizep;
 	const u64 *basep;
 
-	if (chip->log.bios_event_log != NULL) {
-		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
-		return -EFAULT;
-	}
-
 	np = of_find_node_by_name(NULL, "vtpm");
 	if (!np) {
 		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
-- 
2.5.0


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

[PATCH v3 5/7] tpm: Replace the of_find_node_by_name() with dev of_node property

[Nayna Jain]

Using device of_node property is better way to refer to device node
rather than of_find_node_by_name().

Additionally, this patch replaces all currently used pr_err()  with
recommended dev_dbg().

Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c |  2 ++
 drivers/char/tpm/tpm_of.c   | 20 ++++++++++----------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 307130e..a040080 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 	chip->dev.release = tpm_dev_release;
 	chip->dev.parent = dev;
 	chip->dev.groups = chip->groups;
+	if (dev->of_node)
+		chip->dev.of_node = chip->dev.parent->of_node;
 
 	if (chip->dev_num == 0)
 		chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 5067a86..4e4eed7 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -2,6 +2,7 @@
  * Copyright 2012 IBM Corporation
  *
  * Author: Ashley Lai <ashleydlai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *         Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
  *
  * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
  *
@@ -30,44 +31,43 @@ int read_log_of(struct tpm_chip *chip)
 	const u32 *sizep;
 	const u64 *basep;
 
-	np = of_find_node_by_name(NULL, "vtpm");
+	if (chip->dev.of_node)
+		np = chip->dev.of_node;
 	if (!np) {
-		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - IBMVTPM not supported\n",
+			__func__);
 		return -ENODEV;
 	}
 
 	sizep = of_get_property(np, "linux,sml-size", NULL);
 	if (sizep == NULL) {
-		pr_err("%s: ERROR - SML size not found\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
+			__func__);
 		goto cleanup_eio;
 	}
 	if (*sizep == 0) {
-		pr_err("%s: ERROR - event log area empty\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
+			__func__);
 		goto cleanup_eio;
 	}
 
 	basep = of_get_property(np, "linux,sml-base", NULL);
 	if (basep == NULL) {
-		pr_err("%s: ERROR - SML not found\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n", __func__);
 		goto cleanup_eio;
 	}
 
 	chip->log.bios_event_log = kmalloc(*sizep, GFP_KERNEL);
 	if (!chip->log.bios_event_log) {
-		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
-		       __func__);
-		of_node_put(np);
 		return -ENOMEM;
 	}
 
 	chip->log.bios_event_log_end = chip->log.bios_event_log + *sizep;
 
 	memcpy(chip->log.bios_event_log, __va(*basep), *sizep);
-	of_node_put(np);
 
 	return 0;
 
 cleanup_eio:
-	of_node_put(np);
 	return -EIO;
 }
-- 
2.5.0


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

[PATCH v3 6/7] tpm: Moves the eventlog init functions to tpm_eventlog_init.c

[Nayna Jain]

Eventlog initialization functions are common for TPM1.2 and TPM2.0
Currently, they are defined in tpm_eventlog.c which does parsing of
TPM1.2 specific eventlog.

Since initialization functions are common for TPM2.0 also, have
moved the init functions to tpm_eventlog_init.c.

Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/Makefile            |   2 +-
 drivers/char/tpm/tpm_eventlog.c      | 116 +---------------------------
 drivers/char/tpm/tpm_eventlog.h      |   3 +
 drivers/char/tpm/tpm_eventlog_init.c | 143 +++++++++++++++++++++++++++++++++++
 4 files changed, 149 insertions(+), 115 deletions(-)
 create mode 100644 drivers/char/tpm/tpm_eventlog_init.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index e8c7b4d..200b957 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
+	tpm_eventlog.o tpm_eventlog_init.o
 
 tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
 tpm-$(CONFIG_OF) += tpm_of.o
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index f84ce71..3f1aba5 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -258,12 +258,6 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
 
 }
 
-static int tpm_bios_measurements_release(struct inode *inode,
-					 struct file *file)
-{
-	return seq_release(inode, file);
-}
-
 static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
 {
 	int len = 0;
@@ -297,122 +291,16 @@ static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
 	return 0;
 }
 
-static const struct seq_operations tpm_ascii_b_measurments_seqops = {
+const struct seq_operations tpm_ascii_b_measurments_seqops = {
 	.start = tpm_bios_measurements_start,
 	.next = tpm_bios_measurements_next,
 	.stop = tpm_bios_measurements_stop,
 	.show = tpm_ascii_bios_measurements_show,
 };
 
-static const struct seq_operations tpm_binary_b_measurments_seqops = {
+const struct seq_operations tpm_binary_b_measurments_seqops = {
 	.start = tpm_bios_measurements_start,
 	.next = tpm_bios_measurements_next,
 	.stop = tpm_bios_measurements_stop,
 	.show = tpm_binary_bios_measurements_show,
 };
-
-static int tpm_bios_measurements_open(struct inode *inode,
-					    struct file *file)
-{
-	int err;
-	struct seq_file *seq;
-	struct tpm_chip *chip;
-	const struct seq_operations *seqops =
-	(const struct seq_operations *)inode->i_private;
-
-	chip = (struct tpm_chip
-	*)file->f_path.dentry->d_parent->d_inode->i_private;
-
-	/* now register seq file */
-	err = seq_open(file, seqops);
-	if (!err) {
-		seq = file->private_data;
-		seq->private = &chip->log;
-	}
-
-	return err;
-}
-
-static const struct file_operations tpm_bios_measurements_ops = {
-	.open = tpm_bios_measurements_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = tpm_bios_measurements_release,
-};
-
-static int is_bad(void *p)
-{
-	if (!p)
-		return 1;
-	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
-		return 1;
-	return 0;
-}
-
-int read_log(struct tpm_chip *chip)
-{
-	int rc;
-
-	if (chip->log.bios_event_log != NULL) {
-		dev_dbg(&chip->dev, "%s:ERROR - Eventlog already initialized\n",
-			__func__);
-		return -EFAULT;
-	}
-
-	rc = read_log_acpi(chip);
-	if (rc == 0)
-		return rc;
-	rc = read_log_of(chip);
-	return rc;
-}
-
-void tpm_bios_log_setup(struct tpm_chip *chip)
-{
-	const char *name = dev_name(&chip->dev);
-	int rc = 0;
-
-	rc = read_log(chip);
-	if (rc < 0)
-		return;
-
-	chip->bios_dir_count = 0;
-	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
-	NULL);
-	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
-		goto err;
-	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip;
-	chip->bios_dir_count++;
-
-	chip->bios_dir[chip->bios_dir_count] =
-	    securityfs_create_file("binary_bios_measurements",
-				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&tpm_binary_b_measurments_seqops,
-				   &tpm_bios_measurements_ops);
-	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
-		goto err;
-	chip->bios_dir_count++;
-
-	chip->bios_dir[chip->bios_dir_count] =
-	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&tpm_ascii_b_measurments_seqops,
-				   &tpm_bios_measurements_ops);
-	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
-		goto err;
-	chip->bios_dir_count++;
-
-	return;
-
-err:
-	tpm_bios_log_teardown(chip);
-}
-
-void tpm_bios_log_teardown(struct tpm_chip *chip)
-{
-	int i;
-
-	for (i = chip->bios_dir_count; i > 0; --i)
-		securityfs_remove(chip->bios_dir[i-1]);
-
-	kfree(chip->log.bios_event_log);
-}
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 0e599ab..6a36a9d 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -12,6 +12,9 @@
 #define do_endian_conversion(x) x
 #endif
 
+extern const struct seq_operations tpm_ascii_b_measurments_seqops;
+extern const struct seq_operations tpm_binary_b_measurments_seqops;
+
 enum bios_platform_class {
 	BIOS_CLIENT = 0x00,
 	BIOS_SERVER = 0x01,
diff --git a/drivers/char/tpm/tpm_eventlog_init.c b/drivers/char/tpm/tpm_eventlog_init.c
new file mode 100644
index 0000000..038771a
--- /dev/null
+++ b/drivers/char/tpm/tpm_eventlog_init.c
@@ -0,0 +1,143 @@
+/*
+ * Copyright (C) 2005, 2012 IBM Corporation
+ *
+ * Authors:
+ *	Kent Yoder <key-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
+ *	Seiji Munetoh <munetoh-JE5g2YyFxFHQT0dZR+AlfA@public.gmane.org>
+ *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
+ *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
+ *      Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
+ *
+ * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
+ *
+ * Defines TPM1.2 and TPM2.0 common initialization functions
+ * to access firmware eventlog.
+ *
+ * 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 tpm_bios_measurements_release(struct inode *inode,
+					struct file *file)
+{
+	return seq_release(inode, file);
+}
+
+static int tpm_bios_measurements_open(struct inode *inode,
+					    struct file *file)
+{
+	int err;
+	struct seq_file *seq;
+	struct tpm_chip *chip;
+	const struct seq_operations *seqops =
+	(const struct seq_operations *)inode->i_private;
+
+	chip = (struct tpm_chip
+	*)file->f_path.dentry->d_parent->d_inode->i_private;
+
+	/* now register seq file */
+	err = seq_open(file, seqops);
+	if (!err) {
+		seq = file->private_data;
+		seq->private = &chip->log;
+	}
+
+	return err;
+}
+
+static const struct file_operations tpm_bios_measurements_ops = {
+	.open = tpm_bios_measurements_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = tpm_bios_measurements_release,
+};
+
+static int is_bad(void *p)
+{
+	if (!p)
+		return 1;
+	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
+		return 1;
+	return 0;
+}
+
+int read_log(struct tpm_chip *chip)
+{
+	int rc;
+
+	if (chip->log.bios_event_log != NULL) {
+		dev_dbg(&chip->dev, "%s:ERROR - Eventlog already initialized\n",
+			__func__);
+		return -EFAULT;
+	}
+
+	rc = read_log_acpi(chip);
+	if (rc == 0)
+		return rc;
+	rc = read_log_of(chip);
+	return rc;
+}
+
+void tpm_bios_log_setup(struct tpm_chip *chip)
+{
+	const char *name = dev_name(&chip->dev);
+	int rc = 0;
+
+	rc = read_log(chip);
+	if (rc < 0)
+		return;
+
+	chip->bios_dir_count = 0;
+	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
+	NULL);
+	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+		goto err;
+	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip;
+	chip->bios_dir_count++;
+
+	chip->bios_dir[chip->bios_dir_count] =
+	    securityfs_create_file("binary_bios_measurements",
+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
+				   (void *)&tpm_binary_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
+	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+		goto err;
+	chip->bios_dir_count++;
+
+	chip->bios_dir[chip->bios_dir_count] =
+	    securityfs_create_file("ascii_bios_measurements",
+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
+				   (void *)&tpm_ascii_b_measurments_seqops,
+				   &tpm_bios_measurements_ops);
+	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+		goto err;
+	chip->bios_dir_count++;
+
+	return;
+
+err:
+	tpm_bios_log_teardown(chip);
+}
+
+void tpm_bios_log_teardown(struct tpm_chip *chip)
+{
+	int i;
+
+	for (i = chip->bios_dir_count; i > 0; --i)
+		securityfs_remove(chip->bios_dir[i-1]);
+
+	kfree(chip->log.bios_event_log);
+}
-- 
2.5.0


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

[PATCH v3 7/7] tpm: Adds securityfs support for TPM2.0 eventlog

[Nayna Jain]

Adds securityfs support for TPM2.0.
This patch add supports only for binary_bios_measurements.

Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/Makefile            |   2 +-
 drivers/char/tpm/tpm2.h              |  85 +++++++++++++
 drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_eventlog_init.c |  28 +++--
 drivers/char/tpm/tpm_of.c            |  42 ++++---
 5 files changed, 357 insertions(+), 24 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2.h
 create mode 100644 drivers/char/tpm/tpm2_eventlog.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 200b957..b10316d 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 tpm_eventlog_init.o
+	tpm_eventlog.o tpm_eventlog_init.o tpm2_eventlog.o
 
 tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
 tpm-$(CONFIG_OF) += tpm_of.o
diff --git a/drivers/char/tpm/tpm2.h b/drivers/char/tpm/tpm2.h
new file mode 100644
index 0000000..38a8073
--- /dev/null
+++ b/drivers/char/tpm/tpm2.h
@@ -0,0 +1,85 @@
+#ifndef __TPM2_H__
+#define __TPM2_H__
+
+#define TPM_ALG_SHA1_DIGEST_SIZE	20
+#define TPM_ALG_SHA256_DIGEST_SIZE	32
+#define TPM_ALG_SHA384_DIGEST_SIZE	48
+
+#define HASH_COUNT	3
+#define MAX_TPM_LOG_MSG	128
+
+/**
+ * All the structures related to Event Log are taken from TCG EFI 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 TPM2.0 are defined in document
+ * Trusted Platform Module Library Part 2: Structures, Family "2.0".
+ */
+
+/* Event log header algorithm spec. */
+struct tcg_efispecideventalgorithmsize {
+	u16	algorithm_id;
+	u16	digest_size;
+} __packed;
+
+/* Event log header data. */
+struct tcg_efispecideventstruct {
+	u8					signature[16];
+	u32					platform_class;
+	u8					spec_version_minor;
+	u8					spec_version_major;
+	u8					spec_errata;
+	u8					uintnsize;
+	u32					num_algs;
+	struct tcg_efispecideventalgorithmsize	digest_sizes[HASH_COUNT];
+	u8					vendor_info_size;
+	u8					vendor_info[0];
+} __packed;
+
+/* Header entry for eventlog. */
+struct tcg_pcr_event {
+	u32	pcr_index;
+	u32	event_type;
+	u8	digest[20];
+	u32	event_size;
+	u8	event[MAX_TPM_LOG_MSG];
+} __packed;
+
+/* Digest union for crypto agility. */
+union tpmu_ha {
+	u8	 sha1[TPM_ALG_SHA1_DIGEST_SIZE];
+	u8	 sha256[TPM_ALG_SHA256_DIGEST_SIZE];
+	u8	 sha384[TPM_ALG_SHA384_DIGEST_SIZE];
+} __packed;
+
+/* Crypto Agile algorithm and respective digest. */
+struct tpmt_ha {
+	u16		algorithm_id;
+	union tpmu_ha	digest;
+} __packed;
+
+/* Crypto agile digests list. */
+struct tpml_digest_values {
+	u32		count;
+	struct tpmt_ha	digests[HASH_COUNT];
+} __packed;
+
+/* Event field structure. */
+struct tcg_event_field {
+	u32	event_size;
+	u8      event[MAX_TPM_LOG_MSG];
+} __packed;
+
+/* Crypto agile log entry format for TPM 2.0. */
+struct tcg_pcr_event2 {
+	u32				pcr_index;
+	u32				event_type;
+	struct tpml_digest_values	digests;
+	struct tcg_event_field		event;
+} __packed;
+
+extern const struct seq_operations tpm2_binary_b_measurments_seqops;
+
+#endif
diff --git a/drivers/char/tpm/tpm2_eventlog.c b/drivers/char/tpm/tpm2_eventlog.c
new file mode 100644
index 0000000..f1e8c4a
--- /dev/null
+++ b/drivers/char/tpm/tpm2_eventlog.c
@@ -0,0 +1,224 @@
+/*
+ * Copyright (C) 2016 IBM Corporation
+ *
+ * Authors:
+ *      Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
+
+ * Access to TPM2.0 event log as written by Firmware.
+ * It assumes that writer of event log has followed TCG Spec 2.0
+ * has 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 "tpm2.h"
+#include "tpm_eventlog.h"
+
+
+static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
+		struct tcg_pcr_event *event_header)
+{
+	struct tcg_efispecideventstruct *efispecid;
+	struct tcg_event_field *event_field;
+	void *marker, *marker_start;
+	int j;
+	size_t size = 0;
+
+	/*
+	 * NOTE: TPM2.0 allows support for extend to multiple PCR Banks.
+	 * This implies that eventlog also has multiple digest values
+	 * one for each PCR Bank. This is called Crypto Agile Log Entry
+	 * Format. Current support implementation is for SHA1 and SHA256.
+	 * Number of digest values are identified by parsing first
+	 * structure stored in event log also called event header.
+	 * Further, the eventlog is written in packed form so to calculate
+	 * offset it is important to know the type of algorithm used.
+	 * Eg. 1:
+	 * digest_values.count = 1;
+	 * digest_values.digest[0].algorithm_id = sha1;
+	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
+	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 20
+	 *
+	 * Eg. 2:
+	 * digest_values.count = 1;
+	 * digest_values.digest[0].algorithm_id = sha256;
+	 * digest_values.digest[0].digest.sha1 = {32 bytes raw data};
+	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 32
+
+	 * Eg. 3:
+	 * digest_values.count = 2;
+	 * digest_values.digest[0].algorithm_id = sha1;
+	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
+	 * digest_values.digest[1].algorithm_id = sha256;
+	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
+	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 20
+	 *			+ sizeof(algorithm_id) + 32;
+	 *
+	 * So, it implies that offset of event_size can vary based on digest
+	 * values as defined by vendor. And so we have to calculate the
+	 * offset by parsing through number and type of digests being used.
+	 * And this is the purpose of using *marker to traverse the structure
+	 * and calculate the offset of event_size. This function uses *marker
+	 * to parse and calculate the dynamic size of the whole event structure.
+	 */
+
+	marker = event;
+	marker_start = marker;
+	marker = marker + sizeof(event->pcr_index) + sizeof(event->event_type)
+		+ sizeof(event->digests.count);
+
+	efispecid = (struct tcg_efispecideventstruct *) event_header->event;
+
+	for (j = 0; (j < efispecid->num_algs) && (j < HASH_COUNT); j++) {
+		marker = marker
+			+ sizeof(efispecid->digest_sizes[j].algorithm_id);
+		marker = marker + efispecid->digest_sizes[j].digest_size;
+	}
+
+	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_bios_log *log = m->private;
+	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_bios_log *log = m->private;
+	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_bios_log *log = m->private;
+	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_measurments_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_init.c b/drivers/char/tpm/tpm_eventlog_init.c
index 038771a..9afeb3d 100644
--- a/drivers/char/tpm/tpm_eventlog_init.c
+++ b/drivers/char/tpm/tpm_eventlog_init.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 
 #include "tpm.h"
+#include "tpm2.h"
 #include "tpm_eventlog.h"
 
 static int tpm_bios_measurements_release(struct inode *inode,
@@ -94,6 +95,7 @@ int read_log(struct tpm_chip *chip)
 void tpm_bios_log_setup(struct tpm_chip *chip)
 {
 	const char *name = dev_name(&chip->dev);
+	void *seq_ops;
 	int rc = 0;
 
 	rc = read_log(chip);
@@ -108,23 +110,31 @@ void tpm_bios_log_setup(struct tpm_chip *chip)
 	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip;
 	chip->bios_dir_count++;
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		seq_ops = (void *)&tpm2_binary_b_measurments_seqops;
+	else
+		seq_ops = (void *)&tpm_binary_b_measurments_seqops;
+
+
 	chip->bios_dir[chip->bios_dir_count] =
 	    securityfs_create_file("binary_bios_measurements",
 				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&tpm_binary_b_measurments_seqops,
+				   seq_ops,
 				   &tpm_bios_measurements_ops);
 	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
 		goto err;
 	chip->bios_dir_count++;
 
-	chip->bios_dir[chip->bios_dir_count] =
-	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&tpm_ascii_b_measurments_seqops,
-				   &tpm_bios_measurements_ops);
-	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
-		goto err;
-	chip->bios_dir_count++;
+	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
+		chip->bios_dir[chip->bios_dir_count] =
+			securityfs_create_file("ascii_bios_measurements",
+					S_IRUSR | S_IRGRP, chip->bios_dir[0],
+					(void *)&tpm_ascii_b_measurments_seqops,
+					&tpm_bios_measurements_ops);
+		if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+			goto err;
+		chip->bios_dir_count++;
+	}
 
 	return;
 
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 4e4eed7..2fc58fe 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -15,12 +15,10 @@
  *
  */
 
-#include <linux/seq_file.h>
-#include <linux/fs.h>
-#include <linux/security.h>
-#include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/of_device.h>
 #include <linux/of.h>
+#include <linux/string.h>
 
 #include "tpm.h"
 #include "tpm_eventlog.h"
@@ -30,44 +28,60 @@ int read_log_of(struct tpm_chip *chip)
 	struct device_node *np;
 	const u32 *sizep;
 	const u64 *basep;
+	const struct of_device_id *of_id;
+	const char *compat;
+	u32 log_size;
 
 	if (chip->dev.of_node)
 		np = chip->dev.of_node;
 	if (!np) {
-		dev_dbg(&chip->dev, "%s: ERROR - IBMVTPM not supported\n",
+		dev_dbg(&chip->dev, "%s: ERROR - TPM not supported\n",
 			__func__);
 		return -ENODEV;
 	}
 
+	compat = of_get_property(np, "compatible", NULL);
+	if (compat == NULL) {
+		dev_dbg(&chip->dev, "%s: ERROR - Compatible device not found",
+			__func__);
+		return -EIO;
+	}
+
 	sizep = of_get_property(np, "linux,sml-size", NULL);
 	if (sizep == NULL) {
 		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
 			__func__);
-		goto cleanup_eio;
+		return -EIO;
 	}
 	if (*sizep == 0) {
 		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
 			__func__);
-		goto cleanup_eio;
+		return -EIO;
 	}
 
+	if (!strcasecmp(compat, "IBM,vtpm"))
+		log_size = *sizep;
+	else
+		log_size = be32_to_cpup(sizep);
+
 	basep = of_get_property(np, "linux,sml-base", NULL);
 	if (basep == NULL) {
 		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n", __func__);
-		goto cleanup_eio;
+		return -EIO;
 	}
 
-	chip->log.bios_event_log = kmalloc(*sizep, GFP_KERNEL);
+	chip->log.bios_event_log = kmalloc(log_size, GFP_KERNEL);
 	if (!chip->log.bios_event_log) {
 		return -ENOMEM;
 	}
 
-	chip->log.bios_event_log_end = chip->log.bios_event_log + *sizep;
+	chip->log.bios_event_log_end = chip->log.bios_event_log + log_size;
 
-	memcpy(chip->log.bios_event_log, __va(*basep), *sizep);
+	if (!strcasecmp(compat, "IBM,vtpm"))
+		memcpy(chip->log.bios_event_log, __va(*basep), log_size);
+	else
+		memcpy(chip->log.bios_event_log, __va(be64_to_cpup(basep)),
+				log_size);
 
 	return 0;
-
-cleanup_eio:
-	return -EIO;
 }
-- 
2.5.0


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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Jarkko Sakkinen]

On Tue, Aug 30, 2016 at 12:50:12AM -0400, Nayna Jain wrote:
> Existing TPM2.0 support lacks the support for eventlog securityfs file.
> This patch adds the binary_bios_measurements to TPM2.0 eventlog
> securityfs file.
> 
> Additionally, it also includes the review feedbacks as suggested by
> Jason.
> 
> Further, commit msg subject line is prefixed with tpm as was suggested
> by Jarkko.

Please start using get_maintainers.pl...

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

Where is the changlog for v2?

/Jarkko

> 
> Nayna Jain (7):
>   tpm: Define a generic open() method for ascii & bios measurements.
>   tpm: Replace the dynamically allocated bios_dir as struct dentry
>     array.
>   tpm: Validate the eventlog access before tpm_bios_log_setup
>   tpm: Redefine the read_log method to check for ACPI/OF properties
>     sequentially
>   tpm: Replace the of_find_node_by_name() with dev of_node property
>   tpm: Moves the eventlog init functions to tpm_eventlog_init.c
>   tpm: Adds securityfs support for TPM2.0 eventlog
> 
>  drivers/char/tpm/Makefile            |  13 +-
>  drivers/char/tpm/tpm-chip.c          |  21 +---
>  drivers/char/tpm/tpm.h               |   7 +-
>  drivers/char/tpm/tpm2.h              |  85 +++++++++++++
>  drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_acpi.c          |  19 +--
>  drivers/char/tpm/tpm_eventlog.c      | 154 +-----------------------
>  drivers/char/tpm/tpm_eventlog.h      |  26 ++--
>  drivers/char/tpm/tpm_eventlog_init.c | 153 ++++++++++++++++++++++++
>  drivers/char/tpm/tpm_of.c            |  65 ++++++----
>  10 files changed, 543 insertions(+), 224 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm2.h
>  create mode 100644 drivers/char/tpm/tpm2_eventlog.c
>  create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
> 
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

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

Re: [PATCH v3 1/7] tpm: Define a generic open() method for ascii & bios measurements.

[Jarkko Sakkinen]

On Tue, Aug 30, 2016 at 12:50:13AM -0400, Nayna Jain wrote:
> Open methods for eventlog ascii and binary bios measurements file
> operations are very similar. This patch refactors the code into
> single open() call by passing seq_operations as i_node->private data.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/tpm_eventlog.c | 59 +++++++++--------------------------------
>  1 file changed, 13 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index e722886..b0a4d02 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -7,6 +7,7 @@
>   *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>   *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
>   *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> + *	Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>   *
>   * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>   *
> @@ -318,12 +319,14 @@ static const struct seq_operations tpm_binary_b_measurments_seqops = {
>  	.show = tpm_binary_bios_measurements_show,
>  };
>  
> -static int tpm_ascii_bios_measurements_open(struct inode *inode,
> +static int tpm_bios_measurements_open(struct inode *inode,
>  					    struct file *file)
>  {
>  	int err;
>  	struct tpm_bios_log *log;
>  	struct seq_file *seq;
> +	const struct seq_operations *seqops =
> +	(const struct seq_operations *)inode->i_private;

Indentation missing.

>  
>  	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
>  	if (!log)
> @@ -333,7 +336,7 @@ static int tpm_ascii_bios_measurements_open(struct inode *inode,
>  		goto out_free;
>  
>  	/* now register seq file */
> -	err = seq_open(file, &tpm_ascii_b_measurments_seqops);
> +	err = seq_open(file, seqops);
>  	if (!err) {
>  		seq = file->private_data;
>  		seq->private = log;
> @@ -349,46 +352,8 @@ out_free:
>  	goto out;
>  }
>  
> -static const struct file_operations tpm_ascii_bios_measurements_ops = {
> -	.open = tpm_ascii_bios_measurements_open,
> -	.read = seq_read,
> -	.llseek = seq_lseek,
> -	.release = tpm_bios_measurements_release,
> -};
> -
> -static int tpm_binary_bios_measurements_open(struct inode *inode,
> -					     struct file *file)
> -{
> -	int err;
> -	struct tpm_bios_log *log;
> -	struct seq_file *seq;
> -
> -	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
> -	if (!log)
> -		return -ENOMEM;
> -
> -	if ((err = read_log(log)))
> -		goto out_free;
> -
> -	/* now register seq file */
> -	err = seq_open(file, &tpm_binary_b_measurments_seqops);
> -	if (!err) {
> -		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> -	}
> -
> -out:
> -	return err;
> -out_free:
> -	kfree(log->bios_event_log);
> -	kfree(log);
> -	goto out;
> -}
> -
> -static const struct file_operations tpm_binary_bios_measurements_ops = {
> -	.open = tpm_binary_bios_measurements_open,
> +static const struct file_operations tpm_bios_measurements_ops = {
> +	.open = tpm_bios_measurements_open,
>  	.read = seq_read,
>  	.llseek = seq_lseek,
>  	.release = tpm_bios_measurements_release,
> @@ -413,15 +378,17 @@ struct dentry **tpm_bios_log_setup(const char *name)
>  
>  	bin_file =
>  	    securityfs_create_file("binary_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
> -				   &tpm_binary_bios_measurements_ops);
> +				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   (void *)&tpm_binary_b_measurments_seqops,
> +				   &tpm_bios_measurements_ops);
>  	if (is_bad(bin_file))
>  		goto out_tpm;
>  
>  	ascii_file =
>  	    securityfs_create_file("ascii_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir, NULL,
> -				   &tpm_ascii_bios_measurements_ops);
> +				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   (void *)&tpm_ascii_b_measurments_seqops,
> +				   &tpm_bios_measurements_ops);
>  	if (is_bad(ascii_file))
>  		goto out_bin;

Otherwise fine.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

>  
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

/Jarkko

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

Re: [PATCH v3 2/7] tpm: Replace the dynamically allocated bios_dir as struct dentry array.

[Jarkko Sakkinen]

On Tue, Aug 30, 2016 at 12:50:14AM -0400, Nayna Jain wrote:
> bios_dir is defined as struct dentry **bios_dir, which results in
> dynamic allocation and possible memory leak. This patch replaces
> it with struct dentry array i.e. struct dentry *bios_dir[3]
> similar to what is done for sysfs groups.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/tpm-chip.c     |  5 ++--
>  drivers/char/tpm/tpm.h          |  3 ++-
>  drivers/char/tpm/tpm_eventlog.c | 60 ++++++++++++++++++-----------------------
>  drivers/char/tpm/tpm_eventlog.h | 10 +++----
>  4 files changed, 35 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e595013..1cd1238 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -283,7 +283,7 @@ static int tpm1_chip_register(struct tpm_chip *chip)
>  
>  	tpm_sysfs_add_device(chip);
>  
> -	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
> +	tpm_bios_log_setup(chip);
>  
>  	return 0;
>  }
> @@ -293,8 +293,7 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return;
>  
> -	if (chip->bios_dir)
> -		tpm_bios_log_teardown(chip->bios_dir);
> +	tpm_bios_log_teardown(chip);
>  }
>  
>  static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 6e002c4..603a661 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -171,7 +171,8 @@ struct tpm_chip {
>  	unsigned long duration[3]; /* jiffies */
>  	bool duration_adjusted;
>  
> -	struct dentry **bios_dir;
> +	struct dentry *bios_dir[3];
> +	unsigned int bios_dir_count;


Do this without bios_dir_count with a four element array. Last idex is
the NULL terminator.

>  	const struct attribute_group *groups[3];
>  	unsigned int groups_cnt;
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index b0a4d02..9dd69a7 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -332,7 +332,8 @@ static int tpm_bios_measurements_open(struct inode *inode,
>  	if (!log)
>  		return -ENOMEM;
>  
> -	if ((err = read_log(log)))
> +	err = read_log(log);
> +	if (err)
>  		goto out_free;
>  
>  	/* now register seq file */
> @@ -368,54 +369,45 @@ static int is_bad(void *p)
>  	return 0;
>  }
>  
> -struct dentry **tpm_bios_log_setup(const char *name)
> +void tpm_bios_log_setup(struct tpm_chip *chip)
>  {
> -	struct dentry **ret = NULL, *tpm_dir, *bin_file, *ascii_file;
> +	const char *name = dev_name(&chip->dev);
>  
> -	tpm_dir = securityfs_create_dir(name, NULL);
> -	if (is_bad(tpm_dir))
> -		goto out;
> +	chip->bios_dir_count = 0;
> +	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
> +	NULL);
> +	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> +		goto err;
> +	chip->bios_dir_count++;
>  
> -	bin_file =
> +	chip->bios_dir[chip->bios_dir_count] =
>  	    securityfs_create_file("binary_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
>  				   (void *)&tpm_binary_b_measurments_seqops,
>  				   &tpm_bios_measurements_ops);
> -	if (is_bad(bin_file))
> -		goto out_tpm;
> +	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> +		goto err;
> +	chip->bios_dir_count++;
>  
> -	ascii_file =
> +	chip->bios_dir[chip->bios_dir_count] =
>  	    securityfs_create_file("ascii_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
>  				   (void *)&tpm_ascii_b_measurments_seqops,
>  				   &tpm_bios_measurements_ops);
> -	if (is_bad(ascii_file))
> -		goto out_bin;
> +	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> +		goto err;
> +	chip->bios_dir_count++;
>  
> -	ret = kmalloc(3 * sizeof(struct dentry *), GFP_KERNEL);
> -	if (!ret)
> -		goto out_ascii;
> +	return;
>  
> -	ret[0] = ascii_file;
> -	ret[1] = bin_file;
> -	ret[2] = tpm_dir;
> -
> -	return ret;
> -
> -out_ascii:
> -	securityfs_remove(ascii_file);
> -out_bin:
> -	securityfs_remove(bin_file);
> -out_tpm:
> -	securityfs_remove(tpm_dir);
> -out:
> -	return NULL;
> +err:
> +	tpm_bios_log_teardown(chip);
>  }
>  
> -void tpm_bios_log_teardown(struct dentry **lst)
> +void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  	int i;
>  
> -	for (i = 0; i < 3; i++)
> -		securityfs_remove(lst[i]);
> +	for (i = chip->bios_dir_count; i > 0; --i)
> +		securityfs_remove(chip->bios_dir[i-1]);

Are you doing this to somehow "optimize"? If the answer is yes, please
don't do it. Just loop from zero.

Anyway with NULL terminated array:

for (i = 0; chip->bios_dir[i] != NULL; i++)

>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index 8de62b0..67621c9 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -77,14 +77,14 @@ int read_log(struct tpm_bios_log *log);
>  
>  #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 **);
> +extern void tpm_bios_log_setup(struct tpm_chip *chip);
> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>  #else
> -static inline struct dentry **tpm_bios_log_setup(const char *name)
> +static inline void tpm_bios_log_setup(struct tpm_chip *chip)
>  {
> -	return NULL;
> +	chip->bios_dir_count = 0;
>  }
> -static inline void tpm_bios_log_teardown(struct dentry **dir)
> +static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  }
>  #endif
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

/Jarkko

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

Re: [PATCH v3 3/7] tpm: Validate the eventlog access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Tue, Aug 30, 2016 at 12:50:15AM -0400, Nayna Jain wrote:
> Currently, securityfs files for eventlog is created irrespective of
> logs properties exist or not i.e. event log base address and
> size.
> 
> This patch will create ascii and bios measurements file
> only if readlog() is successful.

Doesn't this also uncoditionally enable event log for TPM2?

> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/Makefile       | 10 ++++++----
>  drivers/char/tpm/tpm-chip.c     | 18 ++++--------------
>  drivers/char/tpm/tpm.h          |  4 ++++
>  drivers/char/tpm/tpm_acpi.c     | 14 +++++++-------
>  drivers/char/tpm/tpm_eventlog.c | 36 ++++++++++++------------------------
>  drivers/char/tpm/tpm_eventlog.h | 17 +++--------------
>  drivers/char/tpm/tpm_of.c       | 12 ++++++------
>  7 files changed, 42 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a385fb8..00e48e4 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -2,14 +2,16 @@
>  # Makefile for the kernel tpm device drivers.
>  #
>  obj-$(CONFIG_TCG_TPM) += tpm.o
> -tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o
> +tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
> +	tpm_eventlog.o
> +
>  tpm-$(CONFIG_ACPI) += tpm_ppi.o
>  
>  ifdef CONFIG_ACPI
> -	tpm-y += tpm_eventlog.o tpm_acpi.o
> +	tpm-y += tpm_acpi.o
>  else
> -ifdef CONFIG_TCG_IBMVTPM
> -	tpm-y += tpm_eventlog.o tpm_of.o

Why?

> +ifdef CONFIG_OF
> +	tpm-y += tpm_of.o
>  endif
>  endif
>  obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 1cd1238..307130e 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -283,19 +283,9 @@ static int tpm1_chip_register(struct tpm_chip *chip)
>  
>  	tpm_sysfs_add_device(chip);
>  
> -	tpm_bios_log_setup(chip);
> -
>  	return 0;
>  }
>  
> -static void tpm1_chip_unregister(struct tpm_chip *chip)
> -{
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> -		return;
> -
> -	tpm_bios_log_teardown(chip);
> -}
> -
>  static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
>  {
>  	struct attribute **i;
> @@ -369,10 +359,8 @@ int tpm_chip_register(struct tpm_chip *chip)
>  	tpm_add_ppi(chip);
>  
>  	rc = tpm_add_char_device(chip);
> -	if (rc) {
> -		tpm1_chip_unregister(chip);
> +	if (rc)
>  		return rc;
> -	}
>  
>  	chip->flags |= TPM_CHIP_FLAG_REGISTERED;
>  
> @@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip)
>  		return rc;
>  	}
>  
> +	tpm_bios_log_setup(chip);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_register);
> @@ -406,7 +396,7 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  
>  	tpm_del_legacy_sysfs(chip);
>  
> -	tpm1_chip_unregister(chip);
> +	tpm_bios_log_teardown(chip);
>  	tpm_del_char_device(chip);
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_unregister);
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 603a661..032eb7d 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -35,6 +35,8 @@
>  #include <linux/cdev.h>
>  #include <linux/highmem.h>
>  
> +#include "tpm_eventlog.h"
> +
>  enum tpm_const {
>  	TPM_MINOR = 224,	/* officially assigned */
>  	TPM_BUFSIZE = 4096,
> @@ -156,6 +158,8 @@ struct tpm_chip {
>  	struct rw_semaphore ops_sem;
>  	const struct tpm_class_ops *ops;
>  
> +	struct tpm_bios_log log;
> +
>  	unsigned int flags;
>  
>  	int dev_num;		/* /dev/tpm# */
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 565a947..05b4e8a 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -45,14 +45,14 @@ struct acpi_tcpa {
>  };
>  
>  /* read binary bios log */
> -int read_log(struct tpm_bios_log *log)
> +int read_log(struct tpm_chip *chip)
>  {
>  	struct acpi_tcpa *buff;
>  	acpi_status status;
>  	void __iomem *virt;
>  	u64 len, start;
>  
> -	if (log->bios_event_log != NULL) {
> +	if (chip->log.bios_event_log != NULL) {
>  		printk(KERN_ERR
>  		       "%s: ERROR - Eventlog already initialized\n",
>  		       __func__);
> @@ -86,23 +86,23 @@ int read_log(struct tpm_bios_log *log)
>  	}
>  
>  	/* malloc EventLog space */
> -	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> +	chip->log.bios_event_log = kmalloc(len, GFP_KERNEL);
> +	if (!chip->log.bios_event_log) {
>  		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
>  			__func__);
>  		return -ENOMEM;
>  	}
>  
> -	log->bios_event_log_end = log->bios_event_log + len;
> +	chip->log.bios_event_log_end = chip->log.bios_event_log + len;
>  
>  	virt = acpi_os_map_iomem(start, len);
>  	if (!virt) {
> -		kfree(log->bios_event_log);
> +		kfree(chip->log.bios_event_log);
>  		printk("%s: ERROR - Unable to map memory\n", __func__);
>  		return -EIO;
>  	}
>  
> -	memcpy_fromio(log->bios_event_log, virt, len);
> +	memcpy_fromio(chip->log.bios_event_log, virt, len);
>  
>  	acpi_os_unmap_iomem(virt, len);
>  	return 0;
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index 9dd69a7..d6f2477 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -261,14 +261,6 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
>  static int tpm_bios_measurements_release(struct inode *inode,
>  					 struct file *file)
>  {
> -	struct seq_file *seq = file->private_data;
> -	struct tpm_bios_log *log = seq->private;
> -
> -	if (log) {
> -		kfree(log->bios_event_log);
> -		kfree(log);
> -	}
> -
>  	return seq_release(inode, file);
>  }
>  
> @@ -323,34 +315,22 @@ static int tpm_bios_measurements_open(struct inode *inode,
>  					    struct file *file)
>  {
>  	int err;
> -	struct tpm_bios_log *log;
>  	struct seq_file *seq;
> +	struct tpm_chip *chip;
>  	const struct seq_operations *seqops =
>  	(const struct seq_operations *)inode->i_private;
>  
> -	log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL);
> -	if (!log)
> -		return -ENOMEM;
> -
> -	err = read_log(log);
> -	if (err)
> -		goto out_free;
> +	chip = (struct tpm_chip
> +	*)file->f_path.dentry->d_parent->d_inode->i_private;
>  
>  	/* now register seq file */
>  	err = seq_open(file, seqops);
>  	if (!err) {
>  		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> +		seq->private = &chip->log;
>  	}
>  
> -out:
>  	return err;
> -out_free:
> -	kfree(log->bios_event_log);
> -	kfree(log);
> -	goto out;
>  }
>  
>  static const struct file_operations tpm_bios_measurements_ops = {
> @@ -372,12 +352,18 @@ static int is_bad(void *p)
>  void tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
> +	int rc = 0;
> +
> +	rc = read_log(chip);
> +	if (rc < 0)
> +		return;
>  
>  	chip->bios_dir_count = 0;
>  	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
>  	NULL);
>  	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>  		goto err;
> +	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip;
>  	chip->bios_dir_count++;
>  
>  	chip->bios_dir[chip->bios_dir_count] =
> @@ -410,4 +396,6 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
>  
>  	for (i = chip->bios_dir_count; i > 0; --i)
>  		securityfs_remove(chip->bios_dir[i-1]);
> +
> +	kfree(chip->log.bios_event_log);
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index 67621c9..6a01d43 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -73,20 +73,9 @@ enum tcpa_pc_event_ids {
>  	HOST_TABLE_OF_DEVICES,
>  };
>  
> -int read_log(struct tpm_bios_log *log);
> +int read_log(struct tpm_chip *chip);
>  
> -#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
> -	defined(CONFIG_ACPI)
> -extern void tpm_bios_log_setup(struct tpm_chip *chip);
> -extern void tpm_bios_log_teardown(struct tpm_chip *chip);
> -#else
> -static inline void tpm_bios_log_setup(struct tpm_chip *chip)
> -{
> -	chip->bios_dir_count = 0;
> -}
> -static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
> -{
> -}
> -#endif
> +void tpm_bios_log_setup(struct tpm_chip *chip);
> +void tpm_bios_log_teardown(struct tpm_chip *chip);
>  
>  #endif
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 570f30c..8e77976 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -20,13 +20,13 @@
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
>  
> -int read_log(struct tpm_bios_log *log)
> +int read_log(struct tpm_chip *chip)
>  {
>  	struct device_node *np;
>  	const u32 *sizep;
>  	const u64 *basep;
>  
> -	if (log->bios_event_log != NULL) {
> +	if (chip->log.bios_event_log != NULL) {
>  		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
>  		return -EFAULT;
>  	}
> @@ -53,17 +53,17 @@ int read_log(struct tpm_bios_log *log)
>  		goto cleanup_eio;
>  	}
>  
> -	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> +	chip->log.bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> +	if (!chip->log.bios_event_log) {
>  		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
>  		       __func__);
>  		of_node_put(np);
>  		return -ENOMEM;
>  	}
>  
> -	log->bios_event_log_end = log->bios_event_log + *sizep;
> +	chip->log.bios_event_log_end = chip->log.bios_event_log + *sizep;
>  
> -	memcpy(log->bios_event_log, __va(*basep), *sizep);
> +	memcpy(chip->log.bios_event_log, __va(*basep), *sizep);
>  	of_node_put(np);
>  
>  	return 0;
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

/Jarkko

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

Re: [PATCH v3 6/7] tpm: Moves the eventlog init functions to tpm_eventlog_init.c

[Jarkko Sakkinen]

On Tue, Aug 30, 2016 at 12:50:18AM -0400, Nayna Jain wrote:
> Eventlog initialization functions are common for TPM1.2 and TPM2.0
> Currently, they are defined in tpm_eventlog.c which does parsing of
> TPM1.2 specific eventlog.
> 
> Since initialization functions are common for TPM2.0 also, have
> moved the init functions to tpm_eventlog_init.c.

No reason to use bad formatting. Please write "TPM 2.0" and "TPM 1.2".

> 
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/Makefile            |   2 +-
>  drivers/char/tpm/tpm_eventlog.c      | 116 +---------------------------
>  drivers/char/tpm/tpm_eventlog.h      |   3 +
>  drivers/char/tpm/tpm_eventlog_init.c | 143 +++++++++++++++++++++++++++++++++++
>  4 files changed, 149 insertions(+), 115 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index e8c7b4d..200b957 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
> +	tpm_eventlog.o tpm_eventlog_init.o
>  
>  tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
>  tpm-$(CONFIG_OF) += tpm_of.o
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index f84ce71..3f1aba5 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -258,12 +258,6 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
>  
>  }
>  
> -static int tpm_bios_measurements_release(struct inode *inode,
> -					 struct file *file)
> -{
> -	return seq_release(inode, file);
> -}
> -
>  static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
>  {
>  	int len = 0;
> @@ -297,122 +291,16 @@ static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
>  	return 0;
>  }
>  
> -static const struct seq_operations tpm_ascii_b_measurments_seqops = {
> +const struct seq_operations tpm_ascii_b_measurments_seqops = {
>  	.start = tpm_bios_measurements_start,
>  	.next = tpm_bios_measurements_next,
>  	.stop = tpm_bios_measurements_stop,
>  	.show = tpm_ascii_bios_measurements_show,
>  };
>  
> -static const struct seq_operations tpm_binary_b_measurments_seqops = {
> +const struct seq_operations tpm_binary_b_measurments_seqops = {
>  	.start = tpm_bios_measurements_start,
>  	.next = tpm_bios_measurements_next,
>  	.stop = tpm_bios_measurements_stop,
>  	.show = tpm_binary_bios_measurements_show,
>  };
> -
> -static int tpm_bios_measurements_open(struct inode *inode,
> -					    struct file *file)
> -{
> -	int err;
> -	struct seq_file *seq;
> -	struct tpm_chip *chip;
> -	const struct seq_operations *seqops =
> -	(const struct seq_operations *)inode->i_private;
> -
> -	chip = (struct tpm_chip
> -	*)file->f_path.dentry->d_parent->d_inode->i_private;
> -
> -	/* now register seq file */
> -	err = seq_open(file, seqops);
> -	if (!err) {
> -		seq = file->private_data;
> -		seq->private = &chip->log;
> -	}
> -
> -	return err;
> -}
> -
> -static const struct file_operations tpm_bios_measurements_ops = {
> -	.open = tpm_bios_measurements_open,
> -	.read = seq_read,
> -	.llseek = seq_lseek,
> -	.release = tpm_bios_measurements_release,
> -};
> -
> -static int is_bad(void *p)
> -{
> -	if (!p)
> -		return 1;
> -	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
> -		return 1;
> -	return 0;
> -}
> -
> -int read_log(struct tpm_chip *chip)
> -{
> -	int rc;
> -
> -	if (chip->log.bios_event_log != NULL) {
> -		dev_dbg(&chip->dev, "%s:ERROR - Eventlog already initialized\n",
> -			__func__);
> -		return -EFAULT;
> -	}
> -
> -	rc = read_log_acpi(chip);
> -	if (rc == 0)
> -		return rc;
> -	rc = read_log_of(chip);
> -	return rc;
> -}
> -
> -void tpm_bios_log_setup(struct tpm_chip *chip)
> -{
> -	const char *name = dev_name(&chip->dev);
> -	int rc = 0;
> -
> -	rc = read_log(chip);
> -	if (rc < 0)
> -		return;
> -
> -	chip->bios_dir_count = 0;
> -	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
> -	NULL);
> -	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> -		goto err;
> -	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip;
> -	chip->bios_dir_count++;
> -
> -	chip->bios_dir[chip->bios_dir_count] =
> -	    securityfs_create_file("binary_bios_measurements",
> -				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_binary_b_measurments_seqops,
> -				   &tpm_bios_measurements_ops);
> -	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> -		goto err;
> -	chip->bios_dir_count++;
> -
> -	chip->bios_dir[chip->bios_dir_count] =
> -	    securityfs_create_file("ascii_bios_measurements",
> -				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_ascii_b_measurments_seqops,
> -				   &tpm_bios_measurements_ops);
> -	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> -		goto err;
> -	chip->bios_dir_count++;
> -
> -	return;
> -
> -err:
> -	tpm_bios_log_teardown(chip);
> -}
> -
> -void tpm_bios_log_teardown(struct tpm_chip *chip)
> -{
> -	int i;
> -
> -	for (i = chip->bios_dir_count; i > 0; --i)
> -		securityfs_remove(chip->bios_dir[i-1]);
> -
> -	kfree(chip->log.bios_event_log);
> -}
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index 0e599ab..6a36a9d 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -12,6 +12,9 @@
>  #define do_endian_conversion(x) x
>  #endif
>  
> +extern const struct seq_operations tpm_ascii_b_measurments_seqops;
> +extern const struct seq_operations tpm_binary_b_measurments_seqops;
> +
>  enum bios_platform_class {
>  	BIOS_CLIENT = 0x00,
>  	BIOS_SERVER = 0x01,
> diff --git a/drivers/char/tpm/tpm_eventlog_init.c b/drivers/char/tpm/tpm_eventlog_init.c
> new file mode 100644
> index 0000000..038771a
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_eventlog_init.c
> @@ -0,0 +1,143 @@
> +/*
> + * Copyright (C) 2005, 2012 IBM Corporation
> + *
> + * Authors:
> + *	Kent Yoder <key-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> + *	Seiji Munetoh <munetoh-JE5g2YyFxFHQT0dZR+AlfA@public.gmane.org>
> + *	Stefan Berger <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> + *	Reiner Sailer <sailer-aZOuKsOsJu3MbYB6QlFGEg@public.gmane.org>
> + *	Kylene Hall <kjhall-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> + *      Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> + *
> + * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> + *
> + * Defines TPM1.2 and TPM2.0 common initialization functions
> + * to access firmware eventlog.
> + *
> + * 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 tpm_bios_measurements_release(struct inode *inode,
> +					struct file *file)
> +{
> +	return seq_release(inode, file);
> +}
> +
> +static int tpm_bios_measurements_open(struct inode *inode,
> +					    struct file *file)
> +{
> +	int err;
> +	struct seq_file *seq;
> +	struct tpm_chip *chip;
> +	const struct seq_operations *seqops =
> +	(const struct seq_operations *)inode->i_private;
> +
> +	chip = (struct tpm_chip
> +	*)file->f_path.dentry->d_parent->d_inode->i_private;
> +
> +	/* now register seq file */
> +	err = seq_open(file, seqops);
> +	if (!err) {
> +		seq = file->private_data;
> +		seq->private = &chip->log;
> +	}
> +
> +	return err;
> +}
> +
> +static const struct file_operations tpm_bios_measurements_ops = {
> +	.open = tpm_bios_measurements_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = tpm_bios_measurements_release,
> +};
> +
> +static int is_bad(void *p)
> +{
> +	if (!p)
> +		return 1;
> +	if (IS_ERR(p) && (PTR_ERR(p) != -ENODEV))
> +		return 1;
> +	return 0;
> +}
> +
> +int read_log(struct tpm_chip *chip)
> +{
> +	int rc;
> +
> +	if (chip->log.bios_event_log != NULL) {
> +		dev_dbg(&chip->dev, "%s:ERROR - Eventlog already initialized\n",
> +			__func__);
> +		return -EFAULT;
> +	}
> +
> +	rc = read_log_acpi(chip);
> +	if (rc == 0)
> +		return rc;
> +	rc = read_log_of(chip);
> +	return rc;
> +}
> +
> +void tpm_bios_log_setup(struct tpm_chip *chip)
> +{
> +	const char *name = dev_name(&chip->dev);
> +	int rc = 0;
> +
> +	rc = read_log(chip);
> +	if (rc < 0)
> +		return;
> +
> +	chip->bios_dir_count = 0;
> +	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
> +	NULL);
> +	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> +		goto err;
> +	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip;
> +	chip->bios_dir_count++;
> +
> +	chip->bios_dir[chip->bios_dir_count] =
> +	    securityfs_create_file("binary_bios_measurements",
> +				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> +				   (void *)&tpm_binary_b_measurments_seqops,
> +				   &tpm_bios_measurements_ops);
> +	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> +		goto err;
> +	chip->bios_dir_count++;
> +
> +	chip->bios_dir[chip->bios_dir_count] =
> +	    securityfs_create_file("ascii_bios_measurements",
> +				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> +				   (void *)&tpm_ascii_b_measurments_seqops,
> +				   &tpm_bios_measurements_ops);
> +	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> +		goto err;
> +	chip->bios_dir_count++;
> +
> +	return;
> +
> +err:
> +	tpm_bios_log_teardown(chip);
> +}
> +
> +void tpm_bios_log_teardown(struct tpm_chip *chip)
> +{
> +	int i;
> +
> +	for (i = chip->bios_dir_count; i > 0; --i)
> +		securityfs_remove(chip->bios_dir[i-1]);
> +
> +	kfree(chip->log.bios_event_log);
> +}
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

/Jarkko

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

Re: [PATCH v3 7/7] tpm: Adds securityfs support for TPM2.0 eventlog

[Jarkko Sakkinen]

On Tue, Aug 30, 2016 at 12:50:19AM -0400, Nayna Jain wrote:
> Adds securityfs support for TPM2.0.
> This patch add supports only for binary_bios_measurements.

You should use more time to your commit messages. Descriptions like
this are unacceptable.

> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/Makefile            |   2 +-
>  drivers/char/tpm/tpm2.h              |  85 +++++++++++++
>  drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_eventlog_init.c |  28 +++--
>  drivers/char/tpm/tpm_of.c            |  42 ++++---
>  5 files changed, 357 insertions(+), 24 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm2.h
>  create mode 100644 drivers/char/tpm/tpm2_eventlog.c
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 200b957..b10316d 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 tpm_eventlog_init.o
> +	tpm_eventlog.o tpm_eventlog_init.o tpm2_eventlog.o
>  
>  tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
>  tpm-$(CONFIG_OF) += tpm_of.o
> diff --git a/drivers/char/tpm/tpm2.h b/drivers/char/tpm/tpm2.h
> new file mode 100644
> index 0000000..38a8073
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2.h
> @@ -0,0 +1,85 @@
> +#ifndef __TPM2_H__
> +#define __TPM2_H__
> +
> +#define TPM_ALG_SHA1_DIGEST_SIZE	20
> +#define TPM_ALG_SHA256_DIGEST_SIZE	32
> +#define TPM_ALG_SHA384_DIGEST_SIZE	48
> +
> +#define HASH_COUNT	3
> +#define MAX_TPM_LOG_MSG	128
> +
> +/**
> + * All the structures related to Event Log are taken from TCG EFI 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 TPM2.0 are defined in document
> + * Trusted Platform Module Library Part 2: Structures, Family "2.0".
> + */
> +
> +/* Event log header algorithm spec. */
> +struct tcg_efispecideventalgorithmsize {
> +	u16	algorithm_id;
> +	u16	digest_size;
> +} __packed;
> +
> +/* Event log header data. */
> +struct tcg_efispecideventstruct {
> +	u8					signature[16];
> +	u32					platform_class;
> +	u8					spec_version_minor;
> +	u8					spec_version_major;
> +	u8					spec_errata;
> +	u8					uintnsize;
> +	u32					num_algs;
> +	struct tcg_efispecideventalgorithmsize	digest_sizes[HASH_COUNT];
> +	u8					vendor_info_size;
> +	u8					vendor_info[0];
> +} __packed;
> +
> +/* Header entry for eventlog. */
> +struct tcg_pcr_event {
> +	u32	pcr_index;
> +	u32	event_type;
> +	u8	digest[20];
> +	u32	event_size;
> +	u8	event[MAX_TPM_LOG_MSG];
> +} __packed;
> +
> +/* Digest union for crypto agility. */
> +union tpmu_ha {
> +	u8	 sha1[TPM_ALG_SHA1_DIGEST_SIZE];
> +	u8	 sha256[TPM_ALG_SHA256_DIGEST_SIZE];
> +	u8	 sha384[TPM_ALG_SHA384_DIGEST_SIZE];
> +} __packed;
> +
> +/* Crypto Agile algorithm and respective digest. */
> +struct tpmt_ha {
> +	u16		algorithm_id;
> +	union tpmu_ha	digest;
> +} __packed;
> +
> +/* Crypto agile digests list. */
> +struct tpml_digest_values {
> +	u32		count;
> +	struct tpmt_ha	digests[HASH_COUNT];
> +} __packed;
> +
> +/* Event field structure. */
> +struct tcg_event_field {
> +	u32	event_size;
> +	u8      event[MAX_TPM_LOG_MSG];
> +} __packed;
> +
> +/* Crypto agile log entry format for TPM 2.0. */
> +struct tcg_pcr_event2 {
> +	u32				pcr_index;
> +	u32				event_type;
> +	struct tpml_digest_values	digests;
> +	struct tcg_event_field		event;
> +} __packed;
> +
> +extern const struct seq_operations tpm2_binary_b_measurments_seqops;
> +
> +#endif
> diff --git a/drivers/char/tpm/tpm2_eventlog.c b/drivers/char/tpm/tpm2_eventlog.c
> new file mode 100644
> index 0000000..f1e8c4a
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2_eventlog.c
> @@ -0,0 +1,224 @@
> +/*
> + * Copyright (C) 2016 IBM Corporation
> + *
> + * Authors:
> + *      Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> +
> + * Access to TPM2.0 event log as written by Firmware.
> + * It assumes that writer of event log has followed TCG Spec 2.0
> + * has 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 "tpm2.h"
> +#include "tpm_eventlog.h"
> +
> +
> +static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
> +		struct tcg_pcr_event *event_header)
> +{
> +	struct tcg_efispecideventstruct *efispecid;
> +	struct tcg_event_field *event_field;
> +	void *marker, *marker_start;
> +	int j;
> +	size_t size = 0;
> +
> +	/*
> +	 * NOTE: TPM2.0 allows support for extend to multiple PCR Banks.
> +	 * This implies that eventlog also has multiple digest values
> +	 * one for each PCR Bank. This is called Crypto Agile Log Entry
> +	 * Format. Current support implementation is for SHA1 and SHA256.
> +	 * Number of digest values are identified by parsing first
> +	 * structure stored in event log also called event header.
> +	 * Further, the eventlog is written in packed form so to calculate
> +	 * offset it is important to know the type of algorithm used.
> +	 * Eg. 1:
> +	 * digest_values.count = 1;
> +	 * digest_values.digest[0].algorithm_id = sha1;
> +	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
> +	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 20
> +	 *
> +	 * Eg. 2:
> +	 * digest_values.count = 1;
> +	 * digest_values.digest[0].algorithm_id = sha256;
> +	 * digest_values.digest[0].digest.sha1 = {32 bytes raw data};
> +	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 32
> +
> +	 * Eg. 3:
> +	 * digest_values.count = 2;
> +	 * digest_values.digest[0].algorithm_id = sha1;
> +	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
> +	 * digest_values.digest[1].algorithm_id = sha256;
> +	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
> +	 * Offset of eventsize is sizeof(count) + sizeof(algorithm_id) + 20
> +	 *			+ sizeof(algorithm_id) + 32;
> +	 *
> +	 * So, it implies that offset of event_size can vary based on digest
> +	 * values as defined by vendor. And so we have to calculate the
> +	 * offset by parsing through number and type of digests being used.
> +	 * And this is the purpose of using *marker to traverse the structure
> +	 * and calculate the offset of event_size. This function uses *marker
> +	 * to parse and calculate the dynamic size of the whole event structure.
> +	 */
> +
> +	marker = event;
> +	marker_start = marker;
> +	marker = marker + sizeof(event->pcr_index) + sizeof(event->event_type)
> +		+ sizeof(event->digests.count);
> +
> +	efispecid = (struct tcg_efispecideventstruct *) event_header->event;
> +
> +	for (j = 0; (j < efispecid->num_algs) && (j < HASH_COUNT); j++) {
> +		marker = marker
> +			+ sizeof(efispecid->digest_sizes[j].algorithm_id);
> +		marker = marker + efispecid->digest_sizes[j].digest_size;
> +	}
> +
> +	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_bios_log *log = m->private;
> +	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_bios_log *log = m->private;
> +	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_bios_log *log = m->private;
> +	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_measurments_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_init.c b/drivers/char/tpm/tpm_eventlog_init.c
> index 038771a..9afeb3d 100644
> --- a/drivers/char/tpm/tpm_eventlog_init.c
> +++ b/drivers/char/tpm/tpm_eventlog_init.c
> @@ -28,6 +28,7 @@
>  #include <linux/slab.h>
>  
>  #include "tpm.h"
> +#include "tpm2.h"
>  #include "tpm_eventlog.h"
>  
>  static int tpm_bios_measurements_release(struct inode *inode,
> @@ -94,6 +95,7 @@ int read_log(struct tpm_chip *chip)
>  void tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
> +	void *seq_ops;
>  	int rc = 0;
>  
>  	rc = read_log(chip);
> @@ -108,23 +110,31 @@ void tpm_bios_log_setup(struct tpm_chip *chip)
>  	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip;
>  	chip->bios_dir_count++;
>  
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		seq_ops = (void *)&tpm2_binary_b_measurments_seqops;
> +	else
> +		seq_ops = (void *)&tpm_binary_b_measurments_seqops;
> +
> +
>  	chip->bios_dir[chip->bios_dir_count] =
>  	    securityfs_create_file("binary_bios_measurements",
>  				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_binary_b_measurments_seqops,
> +				   seq_ops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>  		goto err;
>  	chip->bios_dir_count++;
>  
> -	chip->bios_dir[chip->bios_dir_count] =
> -	    securityfs_create_file("ascii_bios_measurements",
> -				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_ascii_b_measurments_seqops,
> -				   &tpm_bios_measurements_ops);
> -	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> -		goto err;
> -	chip->bios_dir_count++;
> +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> +		chip->bios_dir[chip->bios_dir_count] =
> +			securityfs_create_file("ascii_bios_measurements",
> +					S_IRUSR | S_IRGRP, chip->bios_dir[0],
> +					(void *)&tpm_ascii_b_measurments_seqops,
> +					&tpm_bios_measurements_ops);
> +		if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> +			goto err;
> +		chip->bios_dir_count++;
> +	}
>  
>  	return;
>  
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 4e4eed7..2fc58fe 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -15,12 +15,10 @@
>   *
>   */
>  
> -#include <linux/seq_file.h>
> -#include <linux/fs.h>
> -#include <linux/security.h>
> -#include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/of_device.h>
>  #include <linux/of.h>
> +#include <linux/string.h>
>  
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
> @@ -30,44 +28,60 @@ int read_log_of(struct tpm_chip *chip)
>  	struct device_node *np;
>  	const u32 *sizep;
>  	const u64 *basep;
> +	const struct of_device_id *of_id;
> +	const char *compat;
> +	u32 log_size;
>  
>  	if (chip->dev.of_node)
>  		np = chip->dev.of_node;
>  	if (!np) {
> -		dev_dbg(&chip->dev, "%s: ERROR - IBMVTPM not supported\n",
> +		dev_dbg(&chip->dev, "%s: ERROR - TPM not supported\n",
>  			__func__);
>  		return -ENODEV;
>  	}
>  
> +	compat = of_get_property(np, "compatible", NULL);
> +	if (compat == NULL) {
> +		dev_dbg(&chip->dev, "%s: ERROR - Compatible device not found",
> +			__func__);
> +		return -EIO;
> +	}
> +
>  	sizep = of_get_property(np, "linux,sml-size", NULL);
>  	if (sizep == NULL) {
>  		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
>  			__func__);
> -		goto cleanup_eio;
> +		return -EIO;
>  	}
>  	if (*sizep == 0) {
>  		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
>  			__func__);
> -		goto cleanup_eio;
> +		return -EIO;
>  	}
>  
> +	if (!strcasecmp(compat, "IBM,vtpm"))
> +		log_size = *sizep;
> +	else
> +		log_size = be32_to_cpup(sizep);
> +
>  	basep = of_get_property(np, "linux,sml-base", NULL);
>  	if (basep == NULL) {
>  		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n", __func__);
> -		goto cleanup_eio;
> +		return -EIO;
>  	}
>  
> -	chip->log.bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> +	chip->log.bios_event_log = kmalloc(log_size, GFP_KERNEL);
>  	if (!chip->log.bios_event_log) {
>  		return -ENOMEM;
>  	}
>  
> -	chip->log.bios_event_log_end = chip->log.bios_event_log + *sizep;
> +	chip->log.bios_event_log_end = chip->log.bios_event_log + log_size;
>  
> -	memcpy(chip->log.bios_event_log, __va(*basep), *sizep);
> +	if (!strcasecmp(compat, "IBM,vtpm"))
> +		memcpy(chip->log.bios_event_log, __va(*basep), log_size);
> +	else
> +		memcpy(chip->log.bios_event_log, __va(be64_to_cpup(basep)),
> +				log_size);
>  
>  	return 0;
> -
> -cleanup_eio:
> -	return -EIO;
>  }
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

/Jarkko

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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Jarkko Sakkinen]

On Tue, Aug 30, 2016 at 12:50:12AM -0400, Nayna Jain wrote:
> Existing TPM2.0 support lacks the support for eventlog securityfs file.
> This patch adds the binary_bios_measurements to TPM2.0 eventlog
> securityfs file.

This is kind of patch set that would require very elaborate description
how the problem was solved. I cannot really mirror the patches to
anything (especially as the commit messages in commits are also very low
quality).

If you write bad commit messages, it leaves me worried that the quality
is low by other measure.

This is just an example but I do not know how this scales with algorithmic 
agility. 

You also fail to explain how this should work with ACPI even though
we know that there does not exist any kind for event log through ACPI
with TPM 2.0 hardware. I.e. just by reading the commits I can obviously
see that you are doing major untested code path changes.

This will need a lot of rework...

> Additionally, it also includes the review feedbacks as suggested by
> Jason.
> 
> Further, commit msg subject line is prefixed with tpm as was suggested
> by Jarkko.
> 
> Changelog 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

BTW, what is the logic in this indentation.

> 
> Nayna Jain (7):
>   tpm: Define a generic open() method for ascii & bios measurements.
>   tpm: Replace the dynamically allocated bios_dir as struct dentry
>     array.
>   tpm: Validate the eventlog access before tpm_bios_log_setup
>   tpm: Redefine the read_log method to check for ACPI/OF properties
>     sequentially
>   tpm: Replace the of_find_node_by_name() with dev of_node property
>   tpm: Moves the eventlog init functions to tpm_eventlog_init.c
>   tpm: Adds securityfs support for TPM2.0 eventlog
> 
>  drivers/char/tpm/Makefile            |  13 +-
>  drivers/char/tpm/tpm-chip.c          |  21 +---
>  drivers/char/tpm/tpm.h               |   7 +-
>  drivers/char/tpm/tpm2.h              |  85 +++++++++++++
>  drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_acpi.c          |  19 +--
>  drivers/char/tpm/tpm_eventlog.c      | 154 +-----------------------
>  drivers/char/tpm/tpm_eventlog.h      |  26 ++--
>  drivers/char/tpm/tpm_eventlog_init.c | 153 ++++++++++++++++++++++++
>  drivers/char/tpm/tpm_of.c            |  65 ++++++----
>  10 files changed, 543 insertions(+), 224 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm2.h
>  create mode 100644 drivers/char/tpm/tpm2_eventlog.c
>  create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
> 
> -- 
> 2.5.0
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

/Jarkko

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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Jarkko Sakkinen]

On Tue, Aug 30, 2016 at 01:16:11PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 30, 2016 at 12:50:12AM -0400, Nayna Jain wrote:
> > Existing TPM2.0 support lacks the support for eventlog securityfs file.
> > This patch adds the binary_bios_measurements to TPM2.0 eventlog
> > securityfs file.
> 
> This is kind of patch set that would require very elaborate description
> how the problem was solved. I cannot really mirror the patches to
> anything (especially as the commit messages in commits are also very low
> quality).
> 
> If you write bad commit messages, it leaves me worried that the quality
> is low by other measure.
> 
> This is just an example but I do not know how this scales with algorithmic 
> agility. 
> 
> You also fail to explain how this should work with ACPI even though
> we know that there does not exist any kind for event log through ACPI
> with TPM 2.0 hardware. I.e. just by reading the commits I can obviously
> see that you are doing major untested code path changes.
> 
> This will need a lot of rework...

I can create a topic branch for this when this patch set starts to
converge more or less acceptable shape because that will help other
to work on the grub-kernel event log hand over. This will take a while
because I first want to test the code and I'm just started to acquire
hardware for that.

The dots need to be connected with this, event log handover and IMA for
the kexec.

/Jarkko

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

Re: [PATCH v3 1/7] tpm: Define a generic open() method for ascii & bios measurements.

[Jason Gunthorpe]

On Tue, Aug 30, 2016 at 12:50:13AM -0400, Nayna Jain wrote:
> Open methods for eventlog ascii and binary bios measurements file
> operations are very similar. This patch refactors the code into
> single open() call by passing seq_operations as i_node->private data.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Looks basically fine, with Jarkko's comment addressed. I recommend
using clang-format when working with the kernel, it makes everything
easy.

Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

Jason

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

Re: [PATCH v3 2/7] tpm: Replace the dynamically allocated bios_dir as struct dentry array.

[Jason Gunthorpe]

On Tue, Aug 30, 2016 at 12:50:14AM -0400, Nayna Jain wrote:
> bios_dir is defined as struct dentry **bios_dir, which results in
> dynamic allocation and possible memory leak. This patch replaces
> it with struct dentry array i.e. struct dentry *bios_dir[3]
> similar to what is done for sysfs groups.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Yep, looks sane too.

Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

I wonder if it needs a Fixes line?

> -	struct dentry **bios_dir;
> +	struct dentry *bios_dir[3];
> +	unsigned int bios_dir_count;

Regarding Jarkko's comment - I don't care either way. If you want to
use null or the count. We use a count for the sysfs scheme, but it is
also more dynamic.

> +	chip->bios_dir_count = 0;
> +	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
> +	NULL);

Indenting again.

Are you running your patches through scripts/checkpatch.pl ?

> -	bin_file =
> +	chip->bios_dir[chip->bios_dir_count] =
>  	    securityfs_create_file("binary_bios_measurements",
> -				   S_IRUSR | S_IRGRP, tpm_dir,
> +				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
>  				   (void *)&tpm_binary_b_measurments_seqops,
>  				   &tpm_bios_measurements_ops);
> -	if (is_bad(bin_file))
> -		goto out_tpm;
> +	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> +		goto err;
> +	chip->bios_dir_count++;

This idiom is why the count works better, since we are storing
non-null in the array before doing the error check.

> -void tpm_bios_log_teardown(struct dentry **lst)
> +void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  	int i;
>  
> -	for (i = 0; i < 3; i++)
> -		securityfs_remove(lst[i]);
> +	for (i = chip->bios_dir_count; i > 0; --i)
> +		securityfs_remove(chip->bios_dir[i-1]);

Regarding Jarkko's comment..

I think you need to keep it like this. There is clearly an ordering
requirement with security_remove, so reverse iterating is the right
thing to do.

> +static inline void tpm_bios_log_setup(struct tpm_chip *chip)
>  {
> -	return NULL;
> +	chip->bios_dir_count = 0;

This assignment is probably not necessary since the teardown is
stubbed too.

Jason

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

Re: [PATCH v3 3/7] tpm: Validate the eventlog access before tpm_bios_log_setup

[Jason Gunthorpe]

On Tue, Aug 30, 2016 at 12:50:15AM -0400, Nayna Jain wrote:
> @@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip)
>  		return rc;
>  	}
>  
> +	tpm_bios_log_setup(chip);

Surely this can fail, right? At least if the security fs setup fails
this should propogate that error.

That is a mistake in an earlier patch now that I think about it..
>  
>  	/* malloc EventLog space */
> -	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> +	chip->log.bios_event_log = kmalloc(len, GFP_KERNEL);
> +	if (!chip->log.bios_event_log) {
>  		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
>  			__func__);

Please delete all prints on kmalloc failure, maybe as another patch.

>  		return -ENOMEM;
>  	}
>  
> -	log->bios_event_log_end = log->bios_event_log + len;
> +	chip->log.bios_event_log_end = chip->log.bios_event_log + len;
>  
>  	virt = acpi_os_map_iomem(start, len);
>  	if (!virt) {
> -		kfree(log->bios_event_log);
> +		kfree(chip->log.bios_event_log);

It would also be nice to see this written in the standard
goto-unwind idiom.

>  static const struct file_operations tpm_bios_measurements_ops = {
> @@ -372,12 +352,18 @@ static int is_bad(void *p)
>  void tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
> +	int rc = 0;
> +
> +	rc = read_log(chip);
> +	if (rc < 0)
> +		return;
>  
>  	chip->bios_dir_count = 0;
>  	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
>  	NULL);
>  	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>  		goto err;
> +	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private =
>  	chip;

Hum.

So I don't know if this is right. You should get someone more familiar
with securityfs to double check it. I see apparmorfs.c doing a similar
approach, so that would be a good starting place to copy. Notice how
it uses aa_get_(x)

Still, I wonder if that is even right, is securityfs_remove() really a
strong fence against open? I guess the inode locking is doing that?

This also means that the file can remain held open in userspace
*after* securityfs_remove returns, so the filp must hold a kref on the
chip as well.

At a minimum you need to do something like this:

Create:

chip->sfs_data_bin.chip = chip;
chip->sfs_data_bin.ops = &tpm_binary_b_measurments_seqops;
securityfs_create_file(...,&chip->sfs_data_bin)

It must be done like that to be atomic with open, create two new
members of chip to hold a struct to pass through as the private
data. Do not use the dentry private.

Open:
chip = (struct tpm_chip *)inode->i_private;
dev_get(&chip->dev);
seq_open(..)
seq->private = chip;

Release:
dev_put(&((struct tpm_chip *)seq->private)->dev);

Teardown
 the kfree needs to move to the chip release function.

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

This is too early in the patch series. This change needs to go into
'Redefine the read_log method to check for ACPI/OF properties
sequentially'

> -#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
> -     defined(CONFIG_ACPI)

Ditto

Regarding Jarkko's comment,

Yes, move the check for TPM2 into both of the read_log() - do not
allow TPM2 to read the log until you patch the OF stuff to support the
TPM2 log format.

Jason

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

Re: [PATCH v3 4/7] tpm: Redefine the read_log method to check for ACPI/OF properties sequentially

[Jason Gunthorpe]

On Tue, Aug 30, 2016 at 12:50:16AM -0400, Nayna Jain wrote:
> Currently, the difference in read_log method for ACPI/OF based platforms
> is handled by defining respective read_log method and handing
> them using CONFIG based #ifdef condition in Makefile which is not
> the recommended approach.
> 
> This patch cleans up the ifdef condition in Makefile by defining
> single read_log method which checks for ACPI/OF event log memory in
> sequence.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

Yep, this is what I was looking to see..

> +#if defined(CONFIG_ACPI)
> +int read_log_acpi(struct tpm_chip *chip);
> +#else
> +static inline int read_log_acpi(struct tpm_chip *chip)
> +{
> +	return -1;
> +}
> +#endif
> +
> +#if defined(CONFIG_OF)
> +int read_log_of(struct tpm_chip *chip);
> +#else
> +static inline int read_log_of(struct tpm_chip *chip)
> +{
> +	return -1;
> +}
> +#endif

Though shouldn't these two be ERRNOs of some kind? -ENODEV?

Jason

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

Re: [PATCH v3 5/7] tpm: Replace the of_find_node_by_name() with dev of_node property

[Jason Gunthorpe]

On Tue, Aug 30, 2016 at 12:50:17AM -0400, Nayna Jain wrote:
> Using device of_node property is better way to refer to device node
> rather than of_find_node_by_name().
> 
> Additionally, this patch replaces all currently used pr_err()  with
> recommended dev_dbg().
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>  drivers/char/tpm/tpm-chip.c |  2 ++
>  drivers/char/tpm/tpm_of.c   | 20 ++++++++++----------
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 307130e..a040080 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -171,6 +171,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>  	chip->dev.release = tpm_dev_release;
>  	chip->dev.parent = dev;
>  	chip->dev.groups = chip->groups;
> +	if (dev->of_node)
> +		chip->dev.of_node = chip->dev.parent->of_node;

No, that could cause all manner of problems.

> +	if (chip->dev.of_node)
> +		np = chip->dev.of_node;

Just use chip->dev.parent->of_node here

Jason

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

Re: [PATCH v3 7/7] tpm: Adds securityfs support for TPM2.0 eventlog

[Jason Gunthorpe]

On Tue, Aug 30, 2016 at 12:50:19AM -0400, Nayna Jain wrote:
>  	if (chip->dev.of_node)
>  		np = chip->dev.of_node;
>  	if (!np) {
> -		dev_dbg(&chip->dev, "%s: ERROR - IBMVTPM not supported\n",
> +		dev_dbg(&chip->dev, "%s: ERROR - TPM not supported\n",
>  			__func__);

ERm, this is a mistake in the earlier patch that adds OF and ACPI
together.

Do not print anything if OF is present but no event log is
defined. Ditto for ACPI.

Do not print anything if there is no OF node either.

> +	if (!strcasecmp(compat, "IBM,vtpm"))

No, this is the wrong way to work with compatible lists. I'm pretty
sure there is an OF helper to do this.

> +		log_size = *sizep;

???

All DT values should be big endian. Please include a comment
explaining why this is would not be the case.

Are you certain this isn't just a pre-existing bug - the code assumed
host endian because it was always running on big endian PPC?

Jason

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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Nayna]

Thanks Jarkko for the review. I will address all your comments in my 
next version of patches.

Thanks & Regards,
   - Nayna

On 08/30/2016 12:40 PM, Jarkko Sakkinen wrote:
> On Tue, Aug 30, 2016 at 12:50:12AM -0400, Nayna Jain wrote:
>> Existing TPM2.0 support lacks the support for eventlog securityfs file.
>> This patch adds the binary_bios_measurements to TPM2.0 eventlog
>> securityfs file.
>>
>> Additionally, it also includes the review feedbacks as suggested by
>> Jason.
>>
>> Further, commit msg subject line is prefixed with tpm as was suggested
>> by Jarkko.
>
> Please start using get_maintainers.pl...
>
>> Changelog 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
>
> Where is the changlog for v2?
>
> /Jarkko
>
>>
>> Nayna Jain (7):
>>    tpm: Define a generic open() method for ascii & bios measurements.
>>    tpm: Replace the dynamically allocated bios_dir as struct dentry
>>      array.
>>    tpm: Validate the eventlog access before tpm_bios_log_setup
>>    tpm: Redefine the read_log method to check for ACPI/OF properties
>>      sequentially
>>    tpm: Replace the of_find_node_by_name() with dev of_node property
>>    tpm: Moves the eventlog init functions to tpm_eventlog_init.c
>>    tpm: Adds securityfs support for TPM2.0 eventlog
>>
>>   drivers/char/tpm/Makefile            |  13 +-
>>   drivers/char/tpm/tpm-chip.c          |  21 +---
>>   drivers/char/tpm/tpm.h               |   7 +-
>>   drivers/char/tpm/tpm2.h              |  85 +++++++++++++
>>   drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
>>   drivers/char/tpm/tpm_acpi.c          |  19 +--
>>   drivers/char/tpm/tpm_eventlog.c      | 154 +-----------------------
>>   drivers/char/tpm/tpm_eventlog.h      |  26 ++--
>>   drivers/char/tpm/tpm_eventlog_init.c | 153 ++++++++++++++++++++++++
>>   drivers/char/tpm/tpm_of.c            |  65 ++++++----
>>   10 files changed, 543 insertions(+), 224 deletions(-)
>>   create mode 100644 drivers/char/tpm/tpm2.h
>>   create mode 100644 drivers/char/tpm/tpm2_eventlog.c
>>   create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
>>
>> --
>> 2.5.0
>>
>>
>> ------------------------------------------------------------------------------
>> _______________________________________________
>> tpmdd-devel mailing list
>> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>


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

Re: [PATCH v3 1/7] tpm: Define a generic open() method for ascii & bios measurements.

[Nayna]

Thanks Jason for review. I will address your comments in my next version 
of patches.

I also have some thoughts on one of your comment.. Responding inline in 
respective patch.

Thanks & Regards,
   - Nayna

On 08/30/2016 10:33 PM, Jason Gunthorpe wrote:
> On Tue, Aug 30, 2016 at 12:50:13AM -0400, Nayna Jain wrote:
>> Open methods for eventlog ascii and binary bios measurements file
>> operations are very similar. This patch refactors the code into
>> single open() call by passing seq_operations as i_node->private data.
>>
>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>
> Looks basically fine, with Jarkko's comment addressed. I recommend
> using clang-format when working with the kernel, it makes everything
> easy.
>
> Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>
> Jason
>


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

Re: [PATCH v3 4/7] tpm: Redefine the read_log method to check for ACPI/OF properties sequentially

[Nayna]

On 08/30/2016 11:24 PM, Jason Gunthorpe wrote:
> On Tue, Aug 30, 2016 at 12:50:16AM -0400, Nayna Jain wrote:
>> Currently, the difference in read_log method for ACPI/OF based platforms
>> is handled by defining respective read_log method and handing
>> them using CONFIG based #ifdef condition in Makefile which is not
>> the recommended approach.
>>
>> This patch cleans up the ifdef condition in Makefile by defining
>> single read_log method which checks for ACPI/OF event log memory in
>> sequence.
>>
>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>
> Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>
> Yep, this is what I was looking to see..
>
>> +#if defined(CONFIG_ACPI)
>> +int read_log_acpi(struct tpm_chip *chip);
>> +#else
>> +static inline int read_log_acpi(struct tpm_chip *chip)
>> +{
>> +	return -1;
>> +}
>> +#endif
>> +
>> +#if defined(CONFIG_OF)
>> +int read_log_of(struct tpm_chip *chip);
>> +#else
>> +static inline int read_log_of(struct tpm_chip *chip)
>> +{
>> +	return -1;
>> +}
>> +#endif
>
> Though shouldn't these two be ERRNOs of some kind? -ENODEV?

Sure..
Was just trying to see possible errno. And here are some thoughts.


#define EPERM            1      /* Operation not permitted */
#define ENODEV          19      /* No such device */

Was thinking that since tpm device will still be present and its either 
ACPI or OF way of accessing its properties, and one of them will return 
this errno. So, assuming it is ACPI, that means no OF functions 
permitted. So, how about using EPERM ?

Please let me know if it doesn't sound correct.

Thanks & Regards,
Nayna

>
> Jason
>


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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Jarkko Sakkinen]

On Wed, Aug 31, 2016 at 11:26:24PM +0530, Nayna wrote:
> Thanks Jarkko for the review. I will address all your comments in my next
> version of patches.

OK maybe the point I'm trying to make if you forget all the whining is
that with this particular feature we have to be extremely careful
because of the number of stakeholders that depend on it.

This is not something I would put into 4.10 and no matter who is doing
it, it might take a few more iterations to get right. So take your time.
There's no rush (or more like there can't be rush).

In the meanwhile I'm still in progress on getting the suitable hardware
so that I could test at least the DT stuff.

PS. It's better to keep the attribute names the same since the code is
already depending those names even if I don't like the naming :)

/Jarkko

> Thanks & Regards,
>   - Nayna
> 
> On 08/30/2016 12:40 PM, Jarkko Sakkinen wrote:
> >On Tue, Aug 30, 2016 at 12:50:12AM -0400, Nayna Jain wrote:
> >>Existing TPM2.0 support lacks the support for eventlog securityfs file.
> >>This patch adds the binary_bios_measurements to TPM2.0 eventlog
> >>securityfs file.
> >>
> >>Additionally, it also includes the review feedbacks as suggested by
> >>Jason.
> >>
> >>Further, commit msg subject line is prefixed with tpm as was suggested
> >>by Jarkko.
> >
> >Please start using get_maintainers.pl...
> >
> >>Changelog 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
> >
> >Where is the changlog for v2?
> >
> >/Jarkko
> >
> >>
> >>Nayna Jain (7):
> >>   tpm: Define a generic open() method for ascii & bios measurements.
> >>   tpm: Replace the dynamically allocated bios_dir as struct dentry
> >>     array.
> >>   tpm: Validate the eventlog access before tpm_bios_log_setup
> >>   tpm: Redefine the read_log method to check for ACPI/OF properties
> >>     sequentially
> >>   tpm: Replace the of_find_node_by_name() with dev of_node property
> >>   tpm: Moves the eventlog init functions to tpm_eventlog_init.c
> >>   tpm: Adds securityfs support for TPM2.0 eventlog
> >>
> >>  drivers/char/tpm/Makefile            |  13 +-
> >>  drivers/char/tpm/tpm-chip.c          |  21 +---
> >>  drivers/char/tpm/tpm.h               |   7 +-
> >>  drivers/char/tpm/tpm2.h              |  85 +++++++++++++
> >>  drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
> >>  drivers/char/tpm/tpm_acpi.c          |  19 +--
> >>  drivers/char/tpm/tpm_eventlog.c      | 154 +-----------------------
> >>  drivers/char/tpm/tpm_eventlog.h      |  26 ++--
> >>  drivers/char/tpm/tpm_eventlog_init.c | 153 ++++++++++++++++++++++++
> >>  drivers/char/tpm/tpm_of.c            |  65 ++++++----
> >>  10 files changed, 543 insertions(+), 224 deletions(-)
> >>  create mode 100644 drivers/char/tpm/tpm2.h
> >>  create mode 100644 drivers/char/tpm/tpm2_eventlog.c
> >>  create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
> >>
> >>--
> >>2.5.0
> >>
> >>
> >>------------------------------------------------------------------------------
> >>_______________________________________________
> >>tpmdd-devel mailing list
> >>tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> >>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> >
> 

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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Jarkko Sakkinen]

On Thu, Sep 01, 2016 at 04:45:01PM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 31, 2016 at 11:26:24PM +0530, Nayna wrote:
> > Thanks Jarkko for the review. I will address all your comments in my next
> > version of patches.
> 
> OK maybe the point I'm trying to make if you forget all the whining is
> that with this particular feature we have to be extremely careful
> because of the number of stakeholders that depend on it.
> 
> This is not something I would put into 4.10 and no matter who is doing
> it, it might take a few more iterations to get right. So take your time.
> There's no rush (or more like there can't be rush).

Sorry, a typo. I meant the 4.9 release :) 4.10 release is an open
question. This should be in production quality by 4.9-rc4/5 in order
to make that happen.

The good timeline for topic branch would be end of Oct before LPC so
that I could carry a setup involving Minnowboard and a discrete TPM
module and demo this. I already have a topic branch called 'tabrm'
in place for the same conference.

/Jarkko

> In the meanwhile I'm still in progress on getting the suitable hardware
> so that I could test at least the DT stuff.
> 
> PS. It's better to keep the attribute names the same since the code is
> already depending those names even if I don't like the naming :)
> 
> /Jarkko
> 
> > Thanks & Regards,
> >   - Nayna
> > 
> > On 08/30/2016 12:40 PM, Jarkko Sakkinen wrote:
> > >On Tue, Aug 30, 2016 at 12:50:12AM -0400, Nayna Jain wrote:
> > >>Existing TPM2.0 support lacks the support for eventlog securityfs file.
> > >>This patch adds the binary_bios_measurements to TPM2.0 eventlog
> > >>securityfs file.
> > >>
> > >>Additionally, it also includes the review feedbacks as suggested by
> > >>Jason.
> > >>
> > >>Further, commit msg subject line is prefixed with tpm as was suggested
> > >>by Jarkko.
> > >
> > >Please start using get_maintainers.pl...
> > >
> > >>Changelog 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
> > >
> > >Where is the changlog for v2?
> > >
> > >/Jarkko
> > >
> > >>
> > >>Nayna Jain (7):
> > >>   tpm: Define a generic open() method for ascii & bios measurements.
> > >>   tpm: Replace the dynamically allocated bios_dir as struct dentry
> > >>     array.
> > >>   tpm: Validate the eventlog access before tpm_bios_log_setup
> > >>   tpm: Redefine the read_log method to check for ACPI/OF properties
> > >>     sequentially
> > >>   tpm: Replace the of_find_node_by_name() with dev of_node property
> > >>   tpm: Moves the eventlog init functions to tpm_eventlog_init.c
> > >>   tpm: Adds securityfs support for TPM2.0 eventlog
> > >>
> > >>  drivers/char/tpm/Makefile            |  13 +-
> > >>  drivers/char/tpm/tpm-chip.c          |  21 +---
> > >>  drivers/char/tpm/tpm.h               |   7 +-
> > >>  drivers/char/tpm/tpm2.h              |  85 +++++++++++++
> > >>  drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
> > >>  drivers/char/tpm/tpm_acpi.c          |  19 +--
> > >>  drivers/char/tpm/tpm_eventlog.c      | 154 +-----------------------
> > >>  drivers/char/tpm/tpm_eventlog.h      |  26 ++--
> > >>  drivers/char/tpm/tpm_eventlog_init.c | 153 ++++++++++++++++++++++++
> > >>  drivers/char/tpm/tpm_of.c            |  65 ++++++----
> > >>  10 files changed, 543 insertions(+), 224 deletions(-)
> > >>  create mode 100644 drivers/char/tpm/tpm2.h
> > >>  create mode 100644 drivers/char/tpm/tpm2_eventlog.c
> > >>  create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
> > >>
> > >>--
> > >>2.5.0
> > >>
> > >>
> > >>------------------------------------------------------------------------------
> > >>_______________________________________________
> > >>tpmdd-devel mailing list
> > >>tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> > >>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> > >
> > 

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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Jason Gunthorpe]

On Thu, Sep 01, 2016 at 04:45:01PM +0300, Jarkko Sakkinen wrote:

> PS. It's better to keep the attribute names the same since the code is
> already depending those names even if I don't like the naming :)

Yes, we cannot change the dt attribute names that IBM used in the
original PPC patches, and Nayna, you should clarify that situation
when talking to the Rob Herring, etc as the DT maintainers.

It is not often they deal with patches from a company like IBM that
has been using DT before the linux process started.

Jason

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

Re: [PATCH v3 4/7] tpm: Redefine the read_log method to check for ACPI/OF properties sequentially

[Jason Gunthorpe]

On Thu, Sep 01, 2016 at 12:39:46AM +0530, Nayna wrote:
> >>+int read_log_of(struct tpm_chip *chip);
> >>+#else
> >>+static inline int read_log_of(struct tpm_chip *chip)
> >>+{
> >>+	return -1;
> >>+}
> >>+#endif
> >
> >Though shouldn't these two be ERRNOs of some kind? -ENODEV?
> 
> Sure..
> Was just trying to see possible errno. And here are some thoughts.
> 
> 
> #define EPERM            1      /* Operation not permitted */
> #define ENODEV          19      /* No such device */

> Was thinking that since tpm device will still be present and its either ACPI
> or OF way of accessing its properties, and one of them will return this
> errno. So, assuming it is ACPI, that means no OF functions permitted. So,
> how about using EPERM ?

I'd choose ENODEV over EPERM, that is the usual way in the kernel to
signal 'probe failed'

Remember, which ever it is, it should not cause any messages to be printed.

Jason

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

Re: [PATCH v3 4/7] tpm: Redefine the read_log method to check for ACPI/OF properties sequentially

[Peter Huewe]

Am 6. September 2016 12:47:37 GMT-07:00, schrieb Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>:
>On Thu, Sep 01, 2016 at 12:39:46AM +0530, Nayna wrote:
>> >>+int read_log_of(struct tpm_chip *chip);
>> >>+#else
>> >>+static inline int read_log_of(struct tpm_chip *chip)
>> >>+{
>> >>+	return -1;
>> >>+}
>> >>+#endif
>> >
>> >Though shouldn't these two be ERRNOs of some kind? -ENODEV?
>> 
>> Sure..
>> Was just trying to see possible errno. And here are some thoughts.
>> 
>> 
>> #define EPERM            1      /* Operation not permitted */
>> #define ENODEV          19      /* No such device */
>
>> Was thinking that since tpm device will still be present and its
>either ACPI
>> or OF way of accessing its properties, and one of them will return
>this
>> errno. So, assuming it is ACPI, that means no OF functions permitted.
>So,
>> how about using EPERM ?
>
>I'd choose ENODEV over EPERM, that is the usual way in the kernel to
>signal 'probe failed'

Me too, EPERM sounds more like the caller is lacking priviledge to do so.

>
>Remember, which ever it is, it should not cause any messages to be
>printed.
>
>Jason
>
>------------------------------------------------------------------------------
>_______________________________________________
>tpmdd-devel mailing list
>tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

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

Re: [PATCH v3 3/7] tpm: Validate the eventlog access before tpm_bios_log_setup

[Nayna]

On 08/30/2016 11:22 PM, Jason Gunthorpe wrote:
> On Tue, Aug 30, 2016 at 12:50:15AM -0400, Nayna Jain wrote:
>> @@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip)
>>   		return rc;
>>   	}
>>
>> +	tpm_bios_log_setup(chip);
>
> Surely this can fail, right? At least if the security fs setup fails
> this should propogate that error.

What action we want to take if it fails to do bios_log_setup ?
I have done all other fixes, just am not sure that if we propogate this 
error, then will it mean that tpm_chip_register (where this function is 
called) should fail ? or it is just an error logging on failure of 
bios_log_setup.

>
> That is a mistake in an earlier patch now that I think about it..

>>
>>   	/* malloc EventLog space */
>> -	log->bios_event_log = kmalloc(len, GFP_KERNEL);
>> -	if (!log->bios_event_log) {
>> +	chip->log.bios_event_log = kmalloc(len, GFP_KERNEL);
>> +	if (!chip->log.bios_event_log) {
>>   		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
>>   			__func__);
>
> Please delete all prints on kmalloc failure, maybe as another patch.
>
>>   		return -ENOMEM;
>>   	}
>>
>> -	log->bios_event_log_end = log->bios_event_log + len;
>> +	chip->log.bios_event_log_end = chip->log.bios_event_log + len;
>>
>>   	virt = acpi_os_map_iomem(start, len);
>>   	if (!virt) {
>> -		kfree(log->bios_event_log);
>> +		kfree(chip->log.bios_event_log);
>
> It would also be nice to see this written in the standard
> goto-unwind idiom.
>
>>   static const struct file_operations tpm_bios_measurements_ops = {
>> @@ -372,12 +352,18 @@ static int is_bad(void *p)
>>   void tpm_bios_log_setup(struct tpm_chip *chip)
>>   {
>>   	const char *name = dev_name(&chip->dev);
>> +	int rc = 0;
>> +
>> +	rc = read_log(chip);
>> +	if (rc < 0)
>> +		return;
>>
>>   	chip->bios_dir_count = 0;
>>   	chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name,
>>   	NULL);
>>   	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>>   		goto err;
>> +	chip->bios_dir[chip->bios_dir_count]->d_inode->i_private =
>>   	chip;
>
> Hum.
>
> So I don't know if this is right. You should get someone more familiar
> with securityfs to double check it. I see apparmorfs.c doing a similar
> approach, so that would be a good starting place to copy. Notice how
> it uses aa_get_(x)
>
> Still, I wonder if that is even right, is securityfs_remove() really a
> strong fence against open? I guess the inode locking is doing that?
>
> This also means that the file can remain held open in userspace
> *after* securityfs_remove returns, so the filp must hold a kref on the
> chip as well.
>
> At a minimum you need to do something like this:
>
> Create:
>
> chip->sfs_data_bin.chip = chip;
> chip->sfs_data_bin.ops = &tpm_binary_b_measurments_seqops;
> securityfs_create_file(...,&chip->sfs_data_bin)
>
> It must be done like that to be atomic with open, create two new
> members of chip to hold a struct to pass through as the private
> data. Do not use the dentry private.
>
> Open:
> chip = (struct tpm_chip *)inode->i_private;
> dev_get(&chip->dev);
> seq_open(..)
> seq->private = chip;
>
> Release:
> dev_put(&((struct tpm_chip *)seq->private)->dev);
>
> Teardown
>   the kfree needs to move to the chip release function.
>
>>   ifdef CONFIG_ACPI
>> -     tpm-y += tpm_eventlog.o tpm_acpi.o
>> +     tpm-y += tpm_acpi.o
>>   else
>> -ifdef CONFIG_TCG_IBMVTPM
>> -     tpm-y += tpm_eventlog.o tpm_of.o
>> +ifdef CONFIG_OF
>> +     tpm-y += tpm_of.o
>>   endif
>
> This is too early in the patch series. This change needs to go into
> 'Redefine the read_log method to check for ACPI/OF properties
> sequentially'
>
>> -#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
>> -     defined(CONFIG_ACPI)
>
> Ditto
>
> Regarding Jarkko's comment,
>
> Yes, move the check for TPM2 into both of the read_log() - do not
> allow TPM2 to read the log until you patch the OF stuff to support the
> TPM2 log format.
>
> Jason
>


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

Re: [PATCH v3 3/7] tpm: Validate the eventlog access before tpm_bios_log_setup

[Jason Gunthorpe]

On Fri, Sep 09, 2016 at 10:54:25PM +0530, Nayna wrote:
> 
> 
> On 08/30/2016 11:22 PM, Jason Gunthorpe wrote:
> >On Tue, Aug 30, 2016 at 12:50:15AM -0400, Nayna Jain wrote:
> >>@@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip)
> >>  		return rc;
> >>  	}
> >>
> >>+	tpm_bios_log_setup(chip);
> >
> >Surely this can fail, right? At least if the security fs setup fails
> >this should propogate that error.
> 
> What action we want to take if it fails to do bios_log_setup ?
> I have done all other fixes, just am not sure that if we propogate this
> error, then will it mean that tpm_chip_register (where this function is
> called) should fail ? or it is just an error logging on failure of
> bios_log_setup.

Typically we'd want to fail probe, so keep flowing it up.

I'm expecting this should only happen in fatal cases like security_fs
functions failing.

Failures like no optional ACPI/DT description should not log or return
an error up to probe..

Jason

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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 4527 bytes --]

Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote on 08/30/2016 
06:16:11 AM:

> 
> On Tue, Aug 30, 2016 at 12:50:12AM -0400, Nayna Jain wrote:
> > Existing TPM2.0 support lacks the support for eventlog securityfs 
file.
> > This patch adds the binary_bios_measurements to TPM2.0 eventlog
> > securityfs file.
> 
> This is kind of patch set that would require very elaborate description
> how the problem was solved. I cannot really mirror the patches to
> anything (especially as the commit messages in commits are also very low
> quality).
> 
> If you write bad commit messages, it leaves me worried that the quality
> is low by other measure.
> 
> This is just an example but I do not know how this scales with 
algorithmic 
> agility. 
> 
> You also fail to explain how this should work with ACPI even though
> we know that there does not exist any kind for event log through ACPI
> with TPM 2.0 hardware. I.e. just by reading the commits I can obviously
> see that you are doing major untested code path changes.

That's true there there's not spec for a BIOS at the moment and I would 
expect that TCG will likely not write one. Likely all vendors have moved 
on to (U)EFI. We realized this also while implementing TPM 2 support for 
SeaBIOS and I ended up reusing the ACPI TCPA table but adopted the EFI 
specified log format with that special first entry. Can we accomodate that 
?

   Stefan


> 
> This will need a lot of rework...
> 
> > Additionally, it also includes the review feedbacks as suggested by
> > Jason.
> > 
> > Further, commit msg subject line is prefixed with tpm as was suggested
> > by Jarkko.
> > 
> > Changelog 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
> 
> BTW, what is the logic in this indentation.
> 
> > 
> > Nayna Jain (7):
> >   tpm: Define a generic open() method for ascii & bios measurements.
> >   tpm: Replace the dynamically allocated bios_dir as struct dentry
> >     array.
> >   tpm: Validate the eventlog access before tpm_bios_log_setup
> >   tpm: Redefine the read_log method to check for ACPI/OF properties
> >     sequentially
> >   tpm: Replace the of_find_node_by_name() with dev of_node property
> >   tpm: Moves the eventlog init functions to tpm_eventlog_init.c
> >   tpm: Adds securityfs support for TPM2.0 eventlog
> > 
> >  drivers/char/tpm/Makefile            |  13 +-
> >  drivers/char/tpm/tpm-chip.c          |  21 +---
> >  drivers/char/tpm/tpm.h               |   7 +-
> >  drivers/char/tpm/tpm2.h              |  85 +++++++++++++
> >  drivers/char/tpm/tpm2_eventlog.c     | 224 ++++++++++++++++++++++
> +++++++++++++
> >  drivers/char/tpm/tpm_acpi.c          |  19 +--
> >  drivers/char/tpm/tpm_eventlog.c      | 154 +-----------------------
> >  drivers/char/tpm/tpm_eventlog.h      |  26 ++--
> >  drivers/char/tpm/tpm_eventlog_init.c | 153 ++++++++++++++++++++++++
> >  drivers/char/tpm/tpm_of.c            |  65 ++++++----
> >  10 files changed, 543 insertions(+), 224 deletions(-)
> >  create mode 100644 drivers/char/tpm/tpm2.h
> >  create mode 100644 drivers/char/tpm/tpm2_eventlog.c
> >  create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
> > 
> > -- 
> > 2.5.0
> > 
> > 
> > 
> 
------------------------------------------------------------------------------
> > _______________________________________________
> > tpmdd-devel mailing list
> > tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> 
> /Jarkko
> 
> 
------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> 



[-- Attachment #1.2: Type: text/html, Size: 6153 bytes --]

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Jarkko Sakkinen]

On Mon, Sep 19, 2016 at 10:50:15AM -0400, Stefan Berger wrote:
>    > You also fail to explain how this should work with ACPI even though
>    > we know that there does not exist any kind for event log through ACPI
>    > with TPM 2.0 hardware. I.e. just by reading the commits I can obviously
>    > see that you are doing major untested code path changes.
> 
>    That's true there there's not spec for a BIOS at the moment and I would
>    expect that TCG will likely not write one. Likely all vendors have moved
>    on to (U)EFI. We realized this also while implementing TPM 2 support for
>    SeaBIOS and I ended up reusing the ACPI TCPA table but adopted the EFI
>    specified log format with that special first entry. Can we accomodate that
>    ?

Does that match to "SHA1 Event Log Entry Format" defined in [1]? In
addition "Crypto Agile Log Entry Format" must be supported.

Philip: what was the UEFI handover procedure that was discussed in
TPM BoF at LSS 2016?

>       Stefan

[1] http://www.trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf

/Jarkko

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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 2032 bytes --]

Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote on 09/20/2016 
06:04:23 AM:


> 
> On Mon, Sep 19, 2016 at 10:50:15AM -0400, Stefan Berger wrote:
> >    > You also fail to explain how this should work with ACPI even 
though
> >    > we know that there does not exist any kind for event log through 
ACPI
> >    > with TPM 2.0 hardware. I.e. just by reading the commits I 
canobviously
> >    > see that you are doing major untested code path changes.
> > 
> >    That's true there there's not spec for a BIOS at the moment and I 
would
> >    expect that TCG will likely not write one. Likely all vendors have 
moved
> >    on to (U)EFI. We realized this also while implementing TPM 2 
support for
> >    SeaBIOS and I ended up reusing the ACPI TCPA table but adopted the 
EFI
> >    specified log format with that special first entry. Can we 
> accomodate that
> >    ?
> 
> Does that match to "SHA1 Event Log Entry Format" defined in [1]? In
> addition "Crypto Agile Log Entry Format" must be supported.

SeaBIOS supports the SHA1 Event Log Entry Format [5.1 in that spec]. It 
uses it for TPM 1.2.

https://code.coreboot.org/p/seabios/source/tree/master/src/std/tcg.h#L521

In case of TPM 2 it will write the first log entry in the format of the 
Event Log Header [5.3].

https://code.coreboot.org/p/seabios/source/tree/master/src/std/tcg.h#L521

All subsequent entries in the log will be written in Crypto Agile Log 
Entry Format [5.2].

Again: 
https://code.coreboot.org/p/seabios/source/tree/master/src/std/tcg.h#L521

UEFI may write into some special buffer that the OS can get to via an API 
call. In case of SeaBIOS this buffer is just in the TCPA ACPI table, as in 
TPM 1.2.


> 
> Philip: what was the UEFI handover procedure that was discussed in
> TPM BoF at LSS 2016?
> 
> >       Stefan
> 
> [1] http://www.trustedcomputinggroup.org/wp-content/uploads/EFI-
> Protocol-Specification-rev13-160330final.pdf
> 
> /Jarkko
> 



[-- Attachment #1.2: Type: text/html, Size: 3130 bytes --]

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Nayna]

On 09/01/2016 08:22 PM, Jarkko Sakkinen wrote:
> On Thu, Sep 01, 2016 at 04:45:01PM +0300, Jarkko Sakkinen wrote:
>> On Wed, Aug 31, 2016 at 11:26:24PM +0530, Nayna wrote:
>>> Thanks Jarkko for the review. I will address all your comments in my next
>>> version of patches.
>>
>> OK maybe the point I'm trying to make if you forget all the whining is
>> that with this particular feature we have to be extremely careful
>> because of the number of stakeholders that depend on it.
>>
>> This is not something I would put into 4.10 and no matter who is doing
>> it, it might take a few more iterations to get right. So take your time.
>> There's no rush (or more like there can't be rush).
>
> Sorry, a typo. I meant the 4.9 release :) 4.10 release is an open
> question. This should be in production quality by 4.9-rc4/5 in order
> to make that happen.
>
> The good timeline for topic branch would be end of Oct before LPC so
> that I could carry a setup involving Minnowboard and a discrete TPM
> module and demo this. I already have a topic branch called 'tabrm'
> in place for the same conference.

Sure Jarkko. Thanks for this !! I have included the feedbacks and have 
posted V4 version of the patches just now.

Thanks & Regards,
    - Nayna

>
> /Jarkko
>
>> In the meanwhile I'm still in progress on getting the suitable hardware
>> so that I could test at least the DT stuff.
>>
>> PS. It's better to keep the attribute names the same since the code is
>> already depending those names even if I don't like the naming :)
>>
>> /Jarkko
>>
>>> Thanks & Regards,
>>>    - Nayna
>>>
>>> On 08/30/2016 12:40 PM, Jarkko Sakkinen wrote:
>>>> On Tue, Aug 30, 2016 at 12:50:12AM -0400, Nayna Jain wrote:
>>>>> Existing TPM2.0 support lacks the support for eventlog securityfs file.
>>>>> This patch adds the binary_bios_measurements to TPM2.0 eventlog
>>>>> securityfs file.
>>>>>
>>>>> Additionally, it also includes the review feedbacks as suggested by
>>>>> Jason.
>>>>>
>>>>> Further, commit msg subject line is prefixed with tpm as was suggested
>>>>> by Jarkko.
>>>>
>>>> Please start using get_maintainers.pl...
>>>>
>>>>> Changelog 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
>>>>
>>>> Where is the changlog for v2?
>>>>
>>>> /Jarkko
>>>>
>>>>>
>>>>> Nayna Jain (7):
>>>>>    tpm: Define a generic open() method for ascii & bios measurements.
>>>>>    tpm: Replace the dynamically allocated bios_dir as struct dentry
>>>>>      array.
>>>>>    tpm: Validate the eventlog access before tpm_bios_log_setup
>>>>>    tpm: Redefine the read_log method to check for ACPI/OF properties
>>>>>      sequentially
>>>>>    tpm: Replace the of_find_node_by_name() with dev of_node property
>>>>>    tpm: Moves the eventlog init functions to tpm_eventlog_init.c
>>>>>    tpm: Adds securityfs support for TPM2.0 eventlog
>>>>>
>>>>>   drivers/char/tpm/Makefile            |  13 +-
>>>>>   drivers/char/tpm/tpm-chip.c          |  21 +---
>>>>>   drivers/char/tpm/tpm.h               |   7 +-
>>>>>   drivers/char/tpm/tpm2.h              |  85 +++++++++++++
>>>>>   drivers/char/tpm/tpm2_eventlog.c     | 224 +++++++++++++++++++++++++++++++++++
>>>>>   drivers/char/tpm/tpm_acpi.c          |  19 +--
>>>>>   drivers/char/tpm/tpm_eventlog.c      | 154 +-----------------------
>>>>>   drivers/char/tpm/tpm_eventlog.h      |  26 ++--
>>>>>   drivers/char/tpm/tpm_eventlog_init.c | 153 ++++++++++++++++++++++++
>>>>>   drivers/char/tpm/tpm_of.c            |  65 ++++++----
>>>>>   10 files changed, 543 insertions(+), 224 deletions(-)
>>>>>   create mode 100644 drivers/char/tpm/tpm2.h
>>>>>   create mode 100644 drivers/char/tpm/tpm2_eventlog.c
>>>>>   create mode 100644 drivers/char/tpm/tpm_eventlog_init.c
>>>>>
>>>>> --
>>>>> 2.5.0
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>> _______________________________________________
>>>>> tpmdd-devel mailing list
>>>>> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>>>> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>>>>
>>>
>


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

Re: [PATCH v3 0/7] tpm: TPM2.0 eventlog securityfs support

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 02:19:41PM +0530, Nayna wrote:
> 
> 
> On 09/01/2016 08:22 PM, Jarkko Sakkinen wrote:
> >On Thu, Sep 01, 2016 at 04:45:01PM +0300, Jarkko Sakkinen wrote:
> >>On Wed, Aug 31, 2016 at 11:26:24PM +0530, Nayna wrote:
> >>>Thanks Jarkko for the review. I will address all your comments in my next
> >>>version of patches.
> >>
> >>OK maybe the point I'm trying to make if you forget all the whining is
> >>that with this particular feature we have to be extremely careful
> >>because of the number of stakeholders that depend on it.
> >>
> >>This is not something I would put into 4.10 and no matter who is doing
> >>it, it might take a few more iterations to get right. So take your time.
> >>There's no rush (or more like there can't be rush).
> >
> >Sorry, a typo. I meant the 4.9 release :) 4.10 release is an open
> >question. This should be in production quality by 4.9-rc4/5 in order
> >to make that happen.
> >
> >The good timeline for topic branch would be end of Oct before LPC so
> >that I could carry a setup involving Minnowboard and a discrete TPM
> >module and demo this. I already have a topic branch called 'tabrm'
> >in place for the same conference.
> 
> Sure Jarkko. Thanks for this !! I have included the feedbacks and have
> posted V4 version of the patches just now.

This is still on my backlog but I'm still waiting to get some usable
hardware.

I really would think that clean up stuff should be digested first.
That I can test with TPM 1.2 hardware. If I create now a topic branch
with all of the eight commits it's just too much overhead to maintain.

If the topic branch would only have TPM 2.0 specific stuff, then it
wouldn't be such big of a deal.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot