[PATCH v4 0/8] tpm: add the securityfs pseudo files support for TPM 2.0 firmware event log

[PATCH v4 0/8] tpm: add the securityfs pseudo files support for TPM 2.0 firmware event log

[Nayna Jain]

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

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

- Patches 1 - 6 clean up the code and fix pre-existing issues.
- Patch 7 moves the common TPM 1.2 and 2.0 event log initialization
functions to a new file named tpm_eventlog_init.c.
- Patch 8 adds the support for creating securityfs files and for
displaying the TPM 2.0 crypto agile event log in binary format.

With that, this patch set splits the event log functionality into
initialization functions, TPM 1.2 event log parsing and TPM 2.0 event
log parsing.

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

Changelog History:

v4:

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

v3:

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

v2:

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

Nayna Jain (8):
  tpm: define a generic open() method for ascii & bios measurements
  tpm: replace dynamically allocated bios_dir with dentry array
  tpm: validate event log access before tpm_bios_log_setup
  tpm: redefine read_log() to handle ACPI/OF at runtime
  tpm: replace of_find_node_by_name() with dev of_node property
  tpm: remove printk error messages
  tpm: move event log init functions to tpm_eventlog_init.c
  tpm: add securityfs support for TPM 2.0 firmware event log

 drivers/char/tpm/Makefile            |  14 +--
 drivers/char/tpm/tpm-chip.c          |  24 ++--
 drivers/char/tpm/tpm.h               |   9 +-
 drivers/char/tpm/tpm2.h              |  79 +++++++++++++
 drivers/char/tpm/tpm2_eventlog.c     | 216 +++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_acpi.c          |  36 +++---
 drivers/char/tpm/tpm_eventlog.c      | 157 +------------------------
 drivers/char/tpm/tpm_eventlog.h      |  30 +++--
 drivers/char/tpm/tpm_eventlog_init.c | 163 ++++++++++++++++++++++++++
 drivers/char/tpm/tpm_of.c            |  68 ++++++-----
 10 files changed, 559 insertions(+), 237 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 v4 1/8] tpm: define a generic open() method for ascii & bios measurements

[Nayna Jain]

open() method for event log ascii and binary bios measurements file
operations are very similar. This patch refactors the code into a
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>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@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..75e6644 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 v4 2/8] tpm: replace dynamically allocated bios_dir with dentry array

[Nayna Jain]

bios_dir is defined as struct dentry **bios_dir, which results in
dynamic allocation and therefore possibly a memory leak. This patch
replaces it with struct dentry array(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>
Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c     |  8 +++---
 drivers/char/tpm/tpm.h          |  3 +-
 drivers/char/tpm/tpm_eventlog.c | 63 +++++++++++++++++++----------------------
 drivers/char/tpm/tpm_eventlog.h | 10 +++----
 4 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e595013..826609d 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -278,14 +278,15 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 
 static int tpm1_chip_register(struct tpm_chip *chip)
 {
+	int rc;
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return 0;
 
 	tpm_sysfs_add_device(chip);
 
-	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
+	rc = tpm_bios_log_setup(chip);
 
-	return 0;
+	return rc;
 }
 
 static void tpm1_chip_unregister(struct tpm_chip *chip)
@@ -293,8 +294,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 3e952fb..b5866bb 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 75e6644..f1df782 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,48 @@ static int is_bad(void *p)
 	return 0;
 }
 
-struct dentry **tpm_bios_log_setup(const char *name)
+int 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;
-
-	ret[0] = ascii_file;
-	ret[1] = bin_file;
-	ret[2] = tpm_dir;
-
-	return ret;
+	return 0;
 
-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);
+	return -EIO;
 }
 
-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]);
+	chip->bios_dir_count = i;
+
 }
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 8de62b0..fd3357e 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 int 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 int tpm_bios_log_setup(struct tpm_chip *chip)
 {
-	return NULL;
+	return 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


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

Re: [PATCH v4 2/8] tpm: replace dynamically allocated bios_dir with dentry array

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 04:34:36AM -0400, Nayna Jain wrote:
> bios_dir is defined as struct dentry **bios_dir, which results in
> dynamic allocation and therefore possibly a memory leak. This patch
> replaces it with struct dentry array(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>
> Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
>  drivers/char/tpm/tpm-chip.c     |  8 +++---
>  drivers/char/tpm/tpm.h          |  3 +-
>  drivers/char/tpm/tpm_eventlog.c | 63 +++++++++++++++++++----------------------
>  drivers/char/tpm/tpm_eventlog.h | 10 +++----
>  4 files changed, 40 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e595013..826609d 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -278,14 +278,15 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>  
>  static int tpm1_chip_register(struct tpm_chip *chip)
>  {
> +	int rc;
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return 0;
>  
>  	tpm_sysfs_add_device(chip);
>  
> -	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
> +	rc = tpm_bios_log_setup(chip);
>  
> -	return 0;
> +	return rc;
>  }
>  
>  static void tpm1_chip_unregister(struct tpm_chip *chip)
> @@ -293,8 +294,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 3e952fb..b5866bb 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;

You should rather have a four entry array and use a 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 75e6644..f1df782 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,48 @@ static int is_bad(void *p)
>  	return 0;
>  }
>  
> -struct dentry **tpm_bios_log_setup(const char *name)
> +int 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;
> -
> -	ret[0] = ascii_file;
> -	ret[1] = bin_file;
> -	ret[2] = tpm_dir;
> -
> -	return ret;
> +	return 0;
>  
> -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);
> +	return -EIO;
>  }
>  
> -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]);
> +	chip->bios_dir_count = i;
> +
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index 8de62b0..fd3357e 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 int 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 int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
> -	return NULL;
> +	return 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
> 

/Jarkko

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

Re: [PATCH v4 2/8] tpm: replace dynamically allocated bios_dir with dentry array

[Jason Gunthorpe]

On Fri, Sep 30, 2016 at 09:27:03PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 28, 2016 at 04:34:36AM -0400, Nayna Jain wrote:

> > -	struct dentry **bios_dir;
> > +	struct dentry *bios_dir[3];
> > +	unsigned int bios_dir_count;
> 
> You should rather have a four entry array and use a NULL terminator.

Why? Then everything has to compute the length before doing
anything. The basic ops are 'append to back' and 'pop off back' which
require length.

Jason

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

Re: [PATCH v4 2/8] tpm: replace dynamically allocated bios_dir with dentry array

[Jarkko Sakkinen]

On Fri, Sep 30, 2016 at 12:30:26PM -0600, Jason Gunthorpe wrote:
> On Fri, Sep 30, 2016 at 09:27:03PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Sep 28, 2016 at 04:34:36AM -0400, Nayna Jain wrote:
> 
> > > -	struct dentry **bios_dir;
> > > +	struct dentry *bios_dir[3];
> > > +	unsigned int bios_dir_count;
> > 
> > You should rather have a four entry array and use a NULL terminator.
> 
> Why? Then everything has to compute the length before doing
> anything. The basic ops are 'append to back' and 'pop off back' which
> require length.

One variable less to struct tpm_chip. The loop that removes the files
simply can continue until NULL is encountered.

Where do you need the length?

> Jason

/Jarkko

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

Re: [PATCH v4 2/8] tpm: replace dynamically allocated bios_dir with dentry array

[Jason Gunthorpe]

On Fri, Sep 30, 2016 at 10:48:25PM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 30, 2016 at 12:30:26PM -0600, Jason Gunthorpe wrote:
> > On Fri, Sep 30, 2016 at 09:27:03PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Sep 28, 2016 at 04:34:36AM -0400, Nayna Jain wrote:
> > 
> > > > -	struct dentry **bios_dir;
> > > > +	struct dentry *bios_dir[3];
> > > > +	unsigned int bios_dir_count;
> > > 
> > > You should rather have a four entry array and use a NULL terminator.
> > 
> > Why? Then everything has to compute the length before doing
> > anything. The basic ops are 'append to back' and 'pop off back' which
> > require length.
> 
> One variable less to struct tpm_chip. The loop that removes the files
> simply can continue until NULL is encountered.

We've been over this, the loop has to remove them in reverse over.

Why are you so concerned about size of tpm_chip??

> Where do you need the length?

Every place that touches the array

Jason

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

Re: [PATCH v4 2/8] tpm: replace dynamically allocated bios_dir with dentry array

[Jarkko Sakkinen]

On Fri, Sep 30, 2016 at 02:31:47PM -0600, Jason Gunthorpe wrote:
> On Fri, Sep 30, 2016 at 10:48:25PM +0300, Jarkko Sakkinen wrote:
> > On Fri, Sep 30, 2016 at 12:30:26PM -0600, Jason Gunthorpe wrote:
> > > On Fri, Sep 30, 2016 at 09:27:03PM +0300, Jarkko Sakkinen wrote:
> > > > On Wed, Sep 28, 2016 at 04:34:36AM -0400, Nayna Jain wrote:
> > > 
> > > > > -	struct dentry **bios_dir;
> > > > > +	struct dentry *bios_dir[3];
> > > > > +	unsigned int bios_dir_count;
> > > > 
> > > > You should rather have a four entry array and use a NULL terminator.
> > > 
> > > Why? Then everything has to compute the length before doing
> > > anything. The basic ops are 'append to back' and 'pop off back' which
> > > require length.
> > 
> > One variable less to struct tpm_chip. The loop that removes the files
> > simply can continue until NULL is encountered.
> 
> We've been over this, the loop has to remove them in reverse over.
> 
> Why are you so concerned about size of tpm_chip??

It's not the size. It's the added complexity.

I'll look into revising this commit a bit in order in order to move
things forward...

/Jarkko

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

Re: [PATCH v4 2/8] tpm: replace dynamically allocated bios_dir with dentry array

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 04:34:36AM -0400, Nayna Jain wrote:
> bios_dir is defined as struct dentry **bios_dir, which results in
> dynamic allocation and therefore possibly a memory leak. This patch
> replaces it with struct dentry array(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>
> Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
>  drivers/char/tpm/tpm-chip.c     |  8 +++---
>  drivers/char/tpm/tpm.h          |  3 +-
>  drivers/char/tpm/tpm_eventlog.c | 63 +++++++++++++++++++----------------------
>  drivers/char/tpm/tpm_eventlog.h | 10 +++----
>  4 files changed, 40 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e595013..826609d 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -278,14 +278,15 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>  
>  static int tpm1_chip_register(struct tpm_chip *chip)
>  {
> +	int rc;
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return 0;
>  
>  	tpm_sysfs_add_device(chip);
>  
> -	chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev));
> +	rc = tpm_bios_log_setup(chip);
>  
> -	return 0;
> +	return rc;
>  }
>  
>  static void tpm1_chip_unregister(struct tpm_chip *chip)
> @@ -293,8 +294,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 3e952fb..b5866bb 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 75e6644..f1df782 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,48 @@ static int is_bad(void *p)
>  	return 0;
>  }
>  
> -struct dentry **tpm_bios_log_setup(const char *name)
> +int 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);

The following securityfs_create_file calls overwrite the same field,
which leaks memory, as you pass chip->biod_dir[0] to these calls.

I guess securityfs_remove() returns with NULL input (haven't checked).

/Jarkko

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

[PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna Jain]

Currently, the securityfs pseudo files for obtaining the firmware
event log are created whether the event log properties exist or not.
This patch creates ascii and bios measurements pseudo files
only if read_log() 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/tpm.h          |  6 +++++
 drivers/char/tpm/tpm_acpi.c     | 12 +++++++---
 drivers/char/tpm/tpm_eventlog.c | 53 +++++++++++++++++++----------------------
 drivers/char/tpm/tpm_eventlog.h |  7 +++++-
 drivers/char/tpm/tpm_of.c       |  4 +++-
 5 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b5866bb..68630cd 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,10 @@ struct tpm_chip {
 	struct rw_semaphore ops_sem;
 	const struct tpm_class_ops *ops;
 
+	struct tpm_bios_log log;
+	struct tpm_securityfs_data bin_sfs_data;
+	struct tpm_securityfs_data ascii_sfs_data;
+
 	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..4d6c2d7 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -45,13 +45,15 @@ 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;
+	struct tpm_bios_log *log;
 
+	log = &chip->log;
 	if (log->bios_event_log != NULL) {
 		printk(KERN_ERR
 		       "%s: ERROR - Eventlog already initialized\n",
@@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log)
 
 	virt = acpi_os_map_iomem(start, len);
 	if (!virt) {
-		kfree(log->bios_event_log);
 		printk("%s: ERROR - Unable to map memory\n", __func__);
-		return -EIO;
+		goto err;
 	}
 
 	memcpy_fromio(log->bios_event_log, virt, len);
 
 	acpi_os_unmap_iomem(virt, len);
 	return 0;
+
+err:
+	kfree(log->bios_event_log);
+	return -EIO;
+
 }
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index f1df782..a8cd4a1 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,19 @@ 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)
-		return -ENOMEM;
-
-	err = read_log(log);
-	if (err)
-		goto out_free;
+	const struct tpm_securityfs_data *sfs_data =
+		(const struct tpm_securityfs_data *)inode->i_private;
+	const struct seq_operations *seqops = sfs_data->seqops;
 
 	/* now register seq file */
 	err = seq_open(file, seqops);
 	if (!err) {
 		seq = file->private_data;
-		seq->private = log;
-	} else {
-		goto out_free;
+		seq->private = sfs_data->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,6 +349,18 @@ static int is_bad(void *p)
 int tpm_bios_log_setup(struct tpm_chip *chip)
 {
 	const char *name = dev_name(&chip->dev);
+	int rc = 0;
+
+	rc = read_log(chip);
+	/*
+	 * read_log failure means event log is not supported except for ENOMEM
+	 */
+	if (rc < 0) {
+		if (rc == -ENOMEM)
+			return rc;
+		else
+			return 0;
+	}
 
 	chip->bios_dir_count = 0;
 	chip->bios_dir[chip->bios_dir_count] =
@@ -380,19 +369,24 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 		goto err;
 	chip->bios_dir_count++;
 
+	chip->bin_sfs_data.log = &chip->log;
+	chip->bin_sfs_data.seqops = &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,
+				   (void *)&chip->bin_sfs_data,
 				   &tpm_bios_measurements_ops);
 	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
 		goto err;
 	chip->bios_dir_count++;
 
+	chip->ascii_sfs_data.log = &chip->log;
+	chip->ascii_sfs_data.seqops =  &tpm_ascii_b_measurments_seqops;
 	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,
+				   (void *)&chip->ascii_sfs_data,
 				   &tpm_bios_measurements_ops);
 	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
 		goto err;
@@ -413,4 +407,5 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
 		securityfs_remove(chip->bios_dir[i-1]);
 	chip->bios_dir_count = i;
 
+	kfree(chip->log.bios_event_log);
 }
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index fd3357e..7ea066c 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -22,6 +22,11 @@ struct tpm_bios_log {
 	void *bios_event_log_end;
 };
 
+struct tpm_securityfs_data {
+	struct tpm_bios_log *log;
+	const struct seq_operations *seqops;
+};
+
 struct tcpa_event {
 	u32 pcr_index;
 	u32 event_type;
@@ -73,7 +78,7 @@ 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)
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 570f30c..68d891a 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -20,12 +20,14 @@
 #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;
+	struct tpm_bios_log *log;
 
+	log = &chip->log;
 	if (log->bios_event_log != NULL) {
 		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
 		return -EFAULT;
-- 
2.5.0


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote:
> Currently, the securityfs pseudo files for obtaining the firmware
> event log are created whether the event log properties exist or not.
> This patch creates ascii and bios measurements pseudo files
> only if read_log() is successful.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

I'm not going to accept this commit as this increases permanent memory
consumption of the subsystem.

You don't need to read the log in order check if it is there.

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Fri, Sep 30, 2016 at 09:57:43PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote:
> > Currently, the securityfs pseudo files for obtaining the firmware
> > event log are created whether the event log properties exist or not.
> > This patch creates ascii and bios measurements pseudo files
> > only if read_log() is successful.
> > 
> > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> I'm not going to accept this commit as this increases permanent memory
> consumption of the subsystem.

How much memory is this?

The patch set is supposed to change things so the permanent log is
used for all the accessor functions which avoids re-parsing every time
the log file sysfs is opened. Since the log never changes this is an
overall saner approach to handling the sysfs files. IIRC this was also
a simple way to solve some ref counting bugs in the current code.

Since this is just referencing reserved system memory, could the
memcpy and allocation just be eliminated? Or is there too much
transformation?

> You don't need to read the log in order check if it is there.

I disagree, a full parse is necessary, so it does need to be read.

We could throw it away and read it again and again at every use..

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Fri, Sep 30, 2016 at 01:11:12PM -0600, Jason Gunthorpe wrote:
> On Fri, Sep 30, 2016 at 09:57:43PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote:
> > > Currently, the securityfs pseudo files for obtaining the firmware
> > > event log are created whether the event log properties exist or not.
> > > This patch creates ascii and bios measurements pseudo files
> > > only if read_log() is successful.
> > > 
> > > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > 
> > I'm not going to accept this commit as this increases permanent memory
> > consumption of the subsystem.
> 
> How much memory is this?
> 
> The patch set is supposed to change things so the permanent log is
> used for all the accessor functions which avoids re-parsing every time
> the log file sysfs is opened. Since the log never changes this is an
> overall saner approach to handling the sysfs files. IIRC this was also
> a simple way to solve some ref counting bugs in the current code.

Ok, this is interesting. What kind of refcounting bugs are related
to existing approach?

> Since this is just referencing reserved system memory, could the
> memcpy and allocation just be eliminated? Or is there too much
> transformation?

Yeah, maybe the bigger reason is that I'm quite resistant to add
stuff to struct tpm_chip without very good reasons.

If there are good reasons, then why not.

If you read the commit message, it basically says that this is done
because of validation that the logs exist. As a simple minded person
I then think of simplest thing that could work that sorts that out
(in ACPI case check for existence of TCPA).

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Fri, Sep 30, 2016 at 10:45:38PM +0300, Jarkko Sakkinen wrote:

> Ok, this is interesting. What kind of refcounting bugs are related
> to existing approach?

IIRC it was because the log was being processed in an fops open()
callback, which itself was not properly serialized against chip
unregister. Avoiding doing any work with the pdev from under the
fops stuff makes the entire problem trivialy go away.

> > Since this is just referencing reserved system memory, could the
> > memcpy and allocation just be eliminated? Or is there too much
> > transformation?
> 
> Yeah, maybe the bigger reason is that I'm quite resistant to add
> stuff to struct tpm_chip without very good reasons.

Why? There is only 1 tpm event log and a few kb of memory means
nothing in a modern system.

> If you read the commit message, it basically says that this is done
> because of validation that the logs exist. As a simple minded person
> I then think of simplest thing that could work that sorts that out
> (in ACPI case check for existence of TCPA).

This is part of a larger theme to fix the event log processing stuff -
it is the last bit that hasn't been touched by the modernizing
efforts. It makes very little sense to reparse the log on every open
from user space and it makes zero sense to create the event log if the
log cannot parsed, plus the various issues with lifetime.

I personally am happy to burn small amounts of RAM if it makes the
code simpler and more obviously correct. Don't overoptimize this
stuff, it isn't worth it.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Fri, Sep 30, 2016 at 08:42:13PM -0600, Jason Gunthorpe wrote:
> On Fri, Sep 30, 2016 at 10:45:38PM +0300, Jarkko Sakkinen wrote:
> 
> > Ok, this is interesting. What kind of refcounting bugs are related
> > to existing approach?
> 
> IIRC it was because the log was being processed in an fops open()
> callback, which itself was not properly serialized against chip
> unregister. Avoiding doing any work with the pdev from under the
> fops stuff makes the entire problem trivialy go away.

Right. Got you. OK, this a good reason alone to refactor this.

> > > Since this is just referencing reserved system memory, could the
> > > memcpy and allocation just be eliminated? Or is there too much
> > > transformation?
> > 
> > Yeah, maybe the bigger reason is that I'm quite resistant to add
> > stuff to struct tpm_chip without very good reasons.
> 
> Why? There is only 1 tpm event log and a few kb of memory means
> nothing in a modern system.
> 
> > If you read the commit message, it basically says that this is done
> > because of validation that the logs exist. As a simple minded person
> > I then think of simplest thing that could work that sorts that out
> > (in ACPI case check for existence of TCPA).
> 
> This is part of a larger theme to fix the event log processing stuff -
> it is the last bit that hasn't been touched by the modernizing
> efforts. It makes very little sense to reparse the log on every open
> from user space and it makes zero sense to create the event log if the
> log cannot parsed, plus the various issues with lifetime.
> 
> I personally am happy to burn small amounts of RAM if it makes the
> code simpler and more obviously correct. Don't overoptimize this
> stuff, it isn't worth it.

I think I got you on this. Just didn't understand the reasoning in
the commit message because it only spoke about existence check.

> Jason

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote:
> Currently, the securityfs pseudo files for obtaining the firmware
> event log are created whether the event log properties exist or not.
> This patch creates ascii and bios measurements pseudo files
> only if read_log() is successful.

Re-reviewing this. The commit message should mention about preventing
a race condition.

I think Jason was right. It makes code much more manageable with a
small price of memory consumption.

> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/tpm.h          |  6 +++++
>  drivers/char/tpm/tpm_acpi.c     | 12 +++++++---
>  drivers/char/tpm/tpm_eventlog.c | 53 +++++++++++++++++++----------------------
>  drivers/char/tpm/tpm_eventlog.h |  7 +++++-
>  drivers/char/tpm/tpm_of.c       |  4 +++-
>  5 files changed, 48 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index b5866bb..68630cd 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,10 @@ struct tpm_chip {
>  	struct rw_semaphore ops_sem;
>  	const struct tpm_class_ops *ops;
>  
> +	struct tpm_bios_log log;

struct tpm_bios_log should be renamed as struct tpm_event_log in some
commit of this patch set as tpm_bios_log is a misleading name.

> +	struct tpm_securityfs_data bin_sfs_data;
> +	struct tpm_securityfs_data ascii_sfs_data;

I think this is otherwise right but the struct name is very clunky.
First of all it doesn't own the data and IMHO now it kind of implies
of owning.

Maybe something like tpm_event_log_fd would a better name. It's a
description of the event log file essentially.

> +
>  	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..4d6c2d7 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -45,13 +45,15 @@ 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;
> +	struct tpm_bios_log *log;
>  
> +	log = &chip->log;
>  	if (log->bios_event_log != NULL) {
>  		printk(KERN_ERR
>  		       "%s: ERROR - Eventlog already initialized\n",
> @@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log)
>  
>  	virt = acpi_os_map_iomem(start, len);
>  	if (!virt) {
> -		kfree(log->bios_event_log);
>  		printk("%s: ERROR - Unable to map memory\n", __func__);
> -		return -EIO;
> +		goto err;
>  	}
>  
>  	memcpy_fromio(log->bios_event_log, virt, len);
>  
>  	acpi_os_unmap_iomem(virt, len);
>  	return 0;
> +
> +err:
> +	kfree(log->bios_event_log);
> +	return -EIO;
> +
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index f1df782..a8cd4a1 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,19 @@ 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)
> -		return -ENOMEM;
> -
> -	err = read_log(log);
> -	if (err)
> -		goto out_free;
> +	const struct tpm_securityfs_data *sfs_data =
> +		(const struct tpm_securityfs_data *)inode->i_private;
> +	const struct seq_operations *seqops = sfs_data->seqops;
>  
>  	/* now register seq file */
>  	err = seq_open(file, seqops);
>  	if (!err) {
>  		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> +		seq->private = sfs_data->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,6 +349,18 @@ static int is_bad(void *p)
>  int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
> +	int rc = 0;
> +
> +	rc = read_log(chip);
> +	/*
> +	 * read_log failure means event log is not supported except for ENOMEM
> +	 */
> +	if (rc < 0) {
> +		if (rc == -ENOMEM)
> +			return rc;
> +		else
> +			return 0;
> +	}
>  
>  	chip->bios_dir_count = 0;
>  	chip->bios_dir[chip->bios_dir_count] =
> @@ -380,19 +369,24 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  		goto err;
>  	chip->bios_dir_count++;
>  
> +	chip->bin_sfs_data.log = &chip->log;
> +	chip->bin_sfs_data.seqops = &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,
> +				   (void *)&chip->bin_sfs_data,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>  		goto err;
>  	chip->bios_dir_count++;
>  
> +	chip->ascii_sfs_data.log = &chip->log;
> +	chip->ascii_sfs_data.seqops =  &tpm_ascii_b_measurments_seqops;
>  	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,
> +				   (void *)&chip->ascii_sfs_data,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>  		goto err;
> @@ -413,4 +407,5 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
>  		securityfs_remove(chip->bios_dir[i-1]);
>  	chip->bios_dir_count = i;
>  
> +	kfree(chip->log.bios_event_log);
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index fd3357e..7ea066c 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -22,6 +22,11 @@ struct tpm_bios_log {
>  	void *bios_event_log_end;
>  };
>  
> +struct tpm_securityfs_data {
> +	struct tpm_bios_log *log;
> +	const struct seq_operations *seqops;
> +};
> +
>  struct tcpa_event {
>  	u32 pcr_index;
>  	u32 event_type;
> @@ -73,7 +78,7 @@ 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)
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 570f30c..68d891a 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -20,12 +20,14 @@
>  #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;
> +	struct tpm_bios_log *log;
>  
> +	log = &chip->log;
>  	if (log->bios_event_log != NULL) {
>  		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
>  		return -EFAULT;
> -- 
> 2.5.0
> 

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Sat, Oct 01, 2016 at 03:01:25PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote:
> > Currently, the securityfs pseudo files for obtaining the firmware
> > event log are created whether the event log properties exist or not.
> > This patch creates ascii and bios measurements pseudo files
> > only if read_log() is successful.
> 
> Re-reviewing this. The commit message should mention about preventing
> a race condition.
> 
> I think Jason was right. It makes code much more manageable with a
> small price of memory consumption.
> 
> > Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/char/tpm/tpm.h          |  6 +++++
> >  drivers/char/tpm/tpm_acpi.c     | 12 +++++++---
> >  drivers/char/tpm/tpm_eventlog.c | 53 +++++++++++++++++++----------------------
> >  drivers/char/tpm/tpm_eventlog.h |  7 +++++-
> >  drivers/char/tpm/tpm_of.c       |  4 +++-
> >  5 files changed, 48 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index b5866bb..68630cd 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,10 @@ struct tpm_chip {
> >  	struct rw_semaphore ops_sem;
> >  	const struct tpm_class_ops *ops;
> >  
> > +	struct tpm_bios_log log;
> 
> struct tpm_bios_log should be renamed as struct tpm_event_log in some
> commit of this patch set as tpm_bios_log is a misleading name.
> 
> > +	struct tpm_securityfs_data bin_sfs_data;
> > +	struct tpm_securityfs_data ascii_sfs_data;
> 
> I think this is otherwise right but the struct name is very clunky.
> First of all it doesn't own the data and IMHO now it kind of implies
> of owning.
> 
> Maybe something like tpm_event_log_fd would a better name. It's a
> description of the event log file essentially.

That's not a good name either because who knows if we have
new files there at some point. I would propose to use simply
struct tpmfs_fd for this data type.

Then the declariots would be simply:

struct tpmfs_fd binary_measurements_fd;
struct tpmfs_fd ascii_measurements_fd;

I think here the long descriptive names would be good use because
these fields are not heavily used in the soure code.

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Sat, Oct 01, 2016 at 03:01:25PM +0300, Jarkko Sakkinen wrote:

> > +	struct tpm_securityfs_data bin_sfs_data;
> > +	struct tpm_securityfs_data ascii_sfs_data;
> 
> I think this is otherwise right but the struct name is very clunky.
> First of all it doesn't own the data and IMHO now it kind of implies
> of owning.

These are passed in here:

> >  	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,
> > +				   (void *)&chip->ascii_sfs_data,
> >  				   &tpm_bios_measurements_ops);

And the argument to securityfs_create_file is called 'data'..

The key question with these patches is if all the locking is done
right and we have the correct lifetime model now.

Eg how much does securityfs_remove serialize and is the kref on the
chip held for the duration of any fops. Can open() start after the
kref is dropped, etc.

Otherwise this scheme isn't good enough either :/

I haven't looked in detail at that topic yet.. Maybe Nayna can explain
what is expected here. Would be excellend to get someone from security to
review this lifetime model.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Sat, Oct 01, 2016 at 10:54:36AM -0600, Jason Gunthorpe wrote:
> On Sat, Oct 01, 2016 at 03:01:25PM +0300, Jarkko Sakkinen wrote:
> 
> > > +	struct tpm_securityfs_data bin_sfs_data;
> > > +	struct tpm_securityfs_data ascii_sfs_data;
> > 
> > I think this is otherwise right but the struct name is very clunky.
> > First of all it doesn't own the data and IMHO now it kind of implies
> > of owning.

Ok, I'm not going to make this an issue. If you think these are good
names, I'll live with that :)

> These are passed in here:
> 
> > >  	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,
> > > +				   (void *)&chip->ascii_sfs_data,
> > >  				   &tpm_bios_measurements_ops);
> 
> And the argument to securityfs_create_file is called 'data'..
> 
> The key question with these patches is if all the locking is done
> right and we have the correct lifetime model now.
> 
> Eg how much does securityfs_remove serialize and is the kref on the
> chip held for the duration of any fops. Can open() start after the
> kref is dropped, etc.

Why not make tpm_securityfs_data refcounted in order to remove
binding to the chip?

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Sat, Oct 01, 2016 at 10:32:39PM +0300, Jarkko Sakkinen wrote:
> On Sat, Oct 01, 2016 at 10:54:36AM -0600, Jason Gunthorpe wrote:
> > On Sat, Oct 01, 2016 at 03:01:25PM +0300, Jarkko Sakkinen wrote:
> > 
> > > > +	struct tpmfs_data bin_sfs_data;
> > > > +	struct tpmfs_data ascii_sfs_data;
> > > 
> > > I think this is otherwise right but the struct name is very clunky.
> > > First of all it doesn't own the data and IMHO now it kind of implies
> > > of owning.
> 
> Ok, I'm not going to make this an issue. If you think these are good
> names, I'll live with that :)
> 
> > These are passed in here:
> > 
> > > >  	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,
> > > > +				   (void *)&chip->ascii_sfs_data,
> > > >  				   &tpm_bios_measurements_ops);
> > 
> > And the argument to securityfs_create_file is called 'data'..
> > 
> > The key question with these patches is if all the locking is done
> > right and we have the correct lifetime model now.
> > 
> > Eg how much does securityfs_remove serialize and is the kref on the
> > chip held for the duration of any fops. Can open() start after the
> > kref is dropped, etc.
> 
> Why not make tpmfs_data refcounted in order to remove
> binding to the chip?

Data type could be something like

struct tpmfs_data {
	struct tpm_bios_log log;
	const struct seq_operations *seqops;
	struct kref refcount;
};


void tpmfs_data_release(struct kref *ref)
{
	struct tpmfs_data *data =
		container_of(ref, struct tpmfs_data, refcount);

	kfree(data->bios_event_log);	
	kfree(data);
}

In tpm_bios_log_setup:

chip->tpmfs_data = kzalloc(sizeof(*chip->tpmfs_data));
kref_init(&chip->tpmfs_data->refcount);

Then use kref_get() in open and kref_put() in close and finally
kref_put() in tpm_bios_log_teardown.

If the chip is destroyed while the file is still open the event log data
would be still alive.

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Sat, Oct 01, 2016 at 10:32:39PM +0300, Jarkko Sakkinen wrote:
> > chip held for the duration of any fops. Can open() start after the
> > kref is dropped, etc.
> 
> Why not make tpm_securityfs_data refcounted in order to remove
> binding to the chip?

The chip is already kref'd. How does swapping one kref for another
solve anything?

The possible issue is that the krefs are not covering
the right code.

The scheme you suggested is also way off the mark for how fops works,
fops->close has no relation to the needed duration for 'data', the
duration is related to securityfs_remove.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Sun, Oct 02, 2016 at 03:25:51PM -0600, Jason Gunthorpe wrote:
> On Sat, Oct 01, 2016 at 10:32:39PM +0300, Jarkko Sakkinen wrote:
> > > chip held for the duration of any fops. Can open() start after the
> > > kref is dropped, etc.
> > 
> > Why not make tpm_securityfs_data refcounted in order to remove
> > binding to the chip?
> 
> The chip is already kref'd. How does swapping one kref for another
> solve anything?
> 
> The possible issue is that the krefs are not covering
> the right code.
> 
> The scheme you suggested is also way off the mark for how fops works,
> fops->close has no relation to the needed duration for 'data', the
> duration is related to securityfs_remove.

Right, the above would not work because it's not linked to
securityfs_remove by any means.

Are you trying to say that after securityfs_remove() there might be a
"grace period" when user space could still see the files visible and
open them?

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Mon, Oct 03, 2016 at 03:20:13PM +0300, Jarkko Sakkinen wrote:
> On Sun, Oct 02, 2016 at 03:25:51PM -0600, Jason Gunthorpe wrote:
> > On Sat, Oct 01, 2016 at 10:32:39PM +0300, Jarkko Sakkinen wrote:
> > > > chip held for the duration of any fops. Can open() start after the
> > > > kref is dropped, etc.
> > > 
> > > Why not make tpm_securityfs_data refcounted in order to remove
> > > binding to the chip?
> > 
> > The chip is already kref'd. How does swapping one kref for another
> > solve anything?
> > 
> > The possible issue is that the krefs are not covering
> > the right code.
> > 
> > The scheme you suggested is also way off the mark for how fops works,
> > fops->close has no relation to the needed duration for 'data', the
> > duration is related to securityfs_remove.
> 
> Right, the above would not work because it's not linked to
> securityfs_remove by any means.
> 
> Are you trying to say that after securityfs_remove() there might be a
> "grace period" when user space could still see the files visible and
> open them?

You have to provide something factors more concrete. Otherwise,
I'm inclined to accept the approach in Naynas patch. It's an
improvement.

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Mon, Oct 03, 2016 at 03:35:23PM +0300, Jarkko Sakkinen wrote:

> > > The scheme you suggested is also way off the mark for how fops works,
> > > fops->close has no relation to the needed duration for 'data', the
> > > duration is related to securityfs_remove.
> > 
> > Right, the above would not work because it's not linked to
> > securityfs_remove by any means.
> > 
 
> You have to provide something factors more concrete. Otherwise,
> I'm inclined to accept the approach in Naynas patch. It's an
> improvement.

I said I haven't checked the patch yet to see if the lifetime model
for 'data' with securityfs is correct. Only that it matches the only
other user of this feature..

I looked more carefully, and I still can't find the right sort of
locking in securityfs_remove..

> > Are you trying to say that after securityfs_remove() there might be a
> > "grace period" when user space could still see the files visible and
> > open them?

Sort of, the typical race is broadly

    CPU0                           CPU1

fops->open()
                                securityfs_remove()
				kref_put(chip)
				kfree(chip)
kref_get(data->chip.kref)

This race should always be analyzed when working with user files.

We deal with this situation in the other user interface:
- cdev uses 'chip->cdev.kobj.parent = &chip->dev.kobj;' and the cdev
  core handles get/put of the chip at the proper time
- sysfs uses kernfs_drain which guarentees nothing is running in any
  callback before returning

I suspect securityfs_remove is defective in this regard. Eg debugfs is
built on the same libfs scheme as securityfs and it incorporates the
mechanism around 'debugfs_use_file_start/etc' to provide sensible
removal fencing.

I don't know if there is a simple fix, so mabye the best thing is to
just leave it be with a comment saying it securityfs_remove probably
races with fops->open(), and that should be fixed inside securityfs
not tpm.

The file operations are also missing '.owner = THIS_MODULE' which is
bad as well.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Mon, Oct 03, 2016 at 10:35:16AM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 03, 2016 at 03:35:23PM +0300, Jarkko Sakkinen wrote:
> 
> > > > The scheme you suggested is also way off the mark for how fops works,
> > > > fops->close has no relation to the needed duration for 'data', the
> > > > duration is related to securityfs_remove.
> > > 
> > > Right, the above would not work because it's not linked to
> > > securityfs_remove by any means.
> > > 
>  
> > You have to provide something factors more concrete. Otherwise,
> > I'm inclined to accept the approach in Naynas patch. It's an
> > improvement.
> 
> I said I haven't checked the patch yet to see if the lifetime model
> for 'data' with securityfs is correct. Only that it matches the only
> other user of this feature..
> 
> I looked more carefully, and I still can't find the right sort of
> locking in securityfs_remove..
> 
> > > Are you trying to say that after securityfs_remove() there might be a
> > > "grace period" when user space could still see the files visible and
> > > open them?
> 
> Sort of, the typical race is broadly
> 
>     CPU0                           CPU1
> 
> fops->open()
>                                 securityfs_remove()
> 				kref_put(chip)
> 				kfree(chip)
> kref_get(data->chip.kref)

I see. So could this be reproduced by:

1. Open binary_measurements.
2. rmmod tpm_tis
3. Read contents of binary_measurements.

> This race should always be analyzed when working with user files.
> 
> We deal with this situation in the other user interface:
> - cdev uses 'chip->cdev.kobj.parent = &chip->dev.kobj;' and the cdev
>   core handles get/put of the chip at the proper time
> - sysfs uses kernfs_drain which guarentees nothing is running in any
>   callback before returning

Yeah, I get it. These securityfs files are nasty in a way compared to
sysfs attributes that they are not connected to the device hierachy.

Their life-cyce management will always be side-channel stuff, which is
not that nice to maintain.

Rather than finding a perfect solution in the code I think a better
angle would be find ways to test and reproduce possible races, which
you already started in your response.

Right now we basically don't have any good acceptance criteria to make
any changes to securityfs stuff. Yes, you can do the analysis (and
should) but human mind is weak sometimes :)

/Jarkko

> I suspect securityfs_remove is defective in this regard. Eg debugfs is
> built on the same libfs scheme as securityfs and it incorporates the
> mechanism around 'debugfs_use_file_start/etc' to provide sensible
> removal fencing.
> 
> I don't know if there is a simple fix, so mabye the best thing is to
> just leave it be with a comment saying it securityfs_remove probably
> races with fops->open(), and that should be fixed inside securityfs
> not tpm.
> 
> The file operations are also missing '.owner = THIS_MODULE' which is
> bad as well.
> 
> Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote:

> > Sort of, the typical race is broadly
> > 
> >     CPU0                           CPU1
> > 
> > fops->open()
> >                                 securityfs_remove()
> > 				kref_put(chip)
> > 				kfree(chip)
> > kref_get(data->chip.kref)
> 
> I see. So could this be reproduced by:
> 
> 1. Open binary_measurements.
> 2. rmmod tpm_tis
> 3. Read contents of binary_measurements.

No, but that method shows the bug I pointed out in my email to Nayna
where the fops stuff is not getting a kref on the chip.

You need to actually race open and securityfs_remove to see the
kref_get() loose its race and then use-after-free.

> Yeah, I get it. These securityfs files are nasty in a way compared to
> sysfs attributes that they are not connected to the device hierachy.

Well, it is not so bad, it is just missing the fence on removal that
sysfs has, or the kref tracking that cdev has. Sadly this is a typical
error within the fops stuff, I've seen it in many places.

> Rather than finding a perfect solution in the code I think a better
> angle would be find ways to test and reproduce possible races, which
> you already started in your response.

That would be very hard to do, racing two calls like that is
quite difficult in any sort of automatic way, AFAIK.

> Right now we basically don't have any good acceptance criteria to make
> any changes to securityfs stuff. Yes, you can do the analysis (and
> should) but human mind is weak sometimes :)

Since it is unlikely and not caused by our subsystem I'm inclined to
just leave a comment (that we expect securityfs_remove to fence) and
you can send a note to James to see what they think.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Mon, Oct 03, 2016 at 03:11:29PM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote:
> 
> > > Sort of, the typical race is broadly
> > > 
> > >     CPU0                           CPU1
> > > 
> > > fops->open()
> > >                                 securityfs_remove()
> > > 				kref_put(chip)
> > > 				kfree(chip)
> > > kref_get(data->chip.kref)
> > 
> > I see. So could this be reproduced by:
> > 
> > 1. Open binary_measurements.
> > 2. rmmod tpm_tis
> > 3. Read contents of binary_measurements.
> 
> No, but that method shows the bug I pointed out in my email to Nayna
> where the fops stuff is not getting a kref on the chip.
> 
> You need to actually race open and securityfs_remove to see the
> kref_get() loose its race and then use-after-free.

So you are worried that get_device() might come when the chip is already
gone?

> > Yeah, I get it. These securityfs files are nasty in a way compared to
> > sysfs attributes that they are not connected to the device hierachy.
> 
> Well, it is not so bad, it is just missing the fence on removal that
> sysfs has, or the kref tracking that cdev has. Sadly this is a typical
> error within the fops stuff, I've seen it in many places.

Do you think that this should be fixed above the driver i.e. add fencing
to the securityfs code itself?

> > Rather than finding a perfect solution in the code I think a better
> > angle would be find ways to test and reproduce possible races, which
> > you already started in your response.
> 
> That would be very hard to do, racing two calls like that is
> quite difficult in any sort of automatic way, AFAIK.

I wonder if SPI has similar file to 'remove' that PCI devices have
(checking from the documentation later on).

> > Right now we basically don't have any good acceptance criteria to make
> > any changes to securityfs stuff. Yes, you can do the analysis (and
> > should) but human mind is weak sometimes :)
> 
> Since it is unlikely and not caused by our subsystem I'm inclined to
> just leave a comment (that we expect securityfs_remove to fence) and
> you can send a note to James to see what they think.

Agreed.

> Jason

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Tue, Oct 04, 2016 at 08:26:51AM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 03, 2016 at 03:11:29PM -0600, Jason Gunthorpe wrote:
> > On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote:
> > 
> > > > Sort of, the typical race is broadly
> > > > 
> > > >     CPU0                           CPU1
> > > > 
> > > > fops->open()
> > > >                                 securityfs_remove()
> > > > 				kref_put(chip)
> > > > 				kfree(chip)
> > > > kref_get(data->chip.kref)

> > You need to actually race open and securityfs_remove to see the
> > kref_get() loose its race and then use-after-free.
> 
> So you are worried that get_device() might come when the chip is already
> gone?

Yes, I'm worried that securityfs_remove doesn not guarentee that
all threads running open() have completed and that no new threads can
start an open(). If that is guarenteed then we are fine once the
get_device is added.

There might be some tricky thing guaranteeing that but I haven't found
it..

> > Well, it is not so bad, it is just missing the fence on removal that
> > sysfs has, or the kref tracking that cdev has. Sadly this is a typical
> > error within the fops stuff, I've seen it in many places.
> 
> Do you think that this should be fixed above the driver i.e. add fencing
> to the securityfs code itself?

It appears debugfs choose to do that, so yes.

I'm not sure what a driver is supposed to do. The problem is managing
the lifetime of the 'data' (aka i_private) memory. We are using a kref
for 'data' so we need to know when it is s that open is fenced

synchronize with 

> > That would be very hard to do, racing two calls like that is
> > quite difficult in any sort of automatic way, AFAIK.
> 
> I wonder if SPI has similar file to 'remove' that PCI devices have
> (checking from the documentation later on).

IIRC belive remove comes from the device core, so it should be present
for spi also...

This solution might work.. Assuming the inode_lock is safe to get
within open.

tpm_bios_measurements_open(struct inode *inode)
{
	struct tpm_chip *chip;

	inode_lock(inode);
	if (!inode->i_private) {
		inode_unlock(inode);
		return -ENODEV;
	}
	chip = inode->i_private;
	get_device(&chip->dev);
	inode_unlock(inode);
}

tpm_bios_log_teardown()
{
	for (dentry *I ...) {
	    struct inode *inode = d_inode(I);
	    // securityfs_remove does not fence open, do it ourselves
	    inode_lock(inode);
	    inode->i_private = NULL;
	    inode_unlock(inode);
	    securityfs_remove(I);
	}
}

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jarkko Sakkinen]

On Tue, Oct 04, 2016 at 11:12:31AM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 04, 2016 at 08:26:51AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 03, 2016 at 03:11:29PM -0600, Jason Gunthorpe wrote:
> > > On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote:
> > > 
> > > > > Sort of, the typical race is broadly
> > > > > 
> > > > >     CPU0                           CPU1
> > > > > 
> > > > > fops->open()
> > > > >                                 securityfs_remove()
> > > > > 				kref_put(chip)
> > > > > 				kfree(chip)
> > > > > kref_get(data->chip.kref)
> 
> > > You need to actually race open and securityfs_remove to see the
> > > kref_get() loose its race and then use-after-free.
> > 
> > So you are worried that get_device() might come when the chip is already
> > gone?
> 
> Yes, I'm worried that securityfs_remove doesn not guarentee that
> all threads running open() have completed and that no new threads can
> start an open(). If that is guarenteed then we are fine once the
> get_device is added.
> 
> There might be some tricky thing guaranteeing that but I haven't found
> it..

Great, thanks for time and patience explaining. This will help me a lot
to properly review the next revisions of this series.

/Jarkko

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/04/2016 10:42 PM, Jason Gunthorpe wrote:
> On Tue, Oct 04, 2016 at 08:26:51AM +0300, Jarkko Sakkinen wrote:
>> On Mon, Oct 03, 2016 at 03:11:29PM -0600, Jason Gunthorpe wrote:
>>> On Mon, Oct 03, 2016 at 11:22:30PM +0300, Jarkko Sakkinen wrote:
>>>
>>>>> Sort of, the typical race is broadly
>>>>>
>>>>>      CPU0                           CPU1
>>>>>
>>>>> fops->open()
>>>>>                                  securityfs_remove()
>>>>> 				kref_put(chip)
>>>>> 				kfree(chip)
>>>>> kref_get(data->chip.kref)
>
>>> You need to actually race open and securityfs_remove to see the
>>> kref_get() loose its race and then use-after-free.
>>
>> So you are worried that get_device() might come when the chip is already
>> gone?
>
> Yes, I'm worried that securityfs_remove doesn not guarentee that
> all threads running open() have completed and that no new threads can
> start an open(). If that is guarenteed then we are fine once the
> get_device is added.
>
> There might be some tricky thing guaranteeing that but I haven't found
> it..
>
>>> Well, it is not so bad, it is just missing the fence on removal that
>>> sysfs has, or the kref tracking that cdev has. Sadly this is a typical
>>> error within the fops stuff, I've seen it in many places.
>>
>> Do you think that this should be fixed above the driver i.e. add fencing
>> to the securityfs code itself?
>
> It appears debugfs choose to do that, so yes.
>
> I'm not sure what a driver is supposed to do. The problem is managing
> the lifetime of the 'data' (aka i_private) memory. We are using a kref
> for 'data' so we need to know when it is s that open is fenced
>
> synchronize with
>
>>> That would be very hard to do, racing two calls like that is
>>> quite difficult in any sort of automatic way, AFAIK.
>>
>> I wonder if SPI has similar file to 'remove' that PCI devices have
>> (checking from the documentation later on).
>
> IIRC belive remove comes from the device core, so it should be present
> for spi also...
>
> This solution might work.. Assuming the inode_lock is safe to get
> within open.
>
> tpm_bios_measurements_open(struct inode *inode)
> {
> 	struct tpm_chip *chip;
>
> 	inode_lock(inode);
> 	if (!inode->i_private) {
> 		inode_unlock(inode);
> 		return -ENODEV;
> 	}
> 	chip = inode->i_private;
> 	get_device(&chip->dev);
> 	inode_unlock(inode);
> }

>
> tpm_bios_log_teardown()
> {
> 	for (dentry *I ...) {
> 	    struct inode *inode = d_inode(I);
> 	    // securityfs_remove does not fence open, do it ourselves
> 	    inode_lock(inode);
> 	    inode->i_private = NULL;
> 	    inode_unlock(inode);

Are we trying to say that, once the teardown() is started, no more 
opening of files are allowed, even if they are visible ?

But if open() has happened first, and then teardown(), in that case 
private data is already passed to seq_open(). That means here the 
behaviour will still be same as existing.. that opened files continue to 
access the data, and teardown will not be waiting for opened files to be 
closed() ? Is this correct understanding of what we are trying to do ?

Thanks & Regards,
    - Nayna

> 	    securityfs_remove(I);
> 	}
> }
>
> Jason
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Fri, Oct 07, 2016 at 01:41:29AM +0530, Nayna wrote:
> Are we trying to say that, once the teardown() is started, no more opening
> of files are allowed, even if they are visible ?

Yes.

> But if open() has happened first, and then teardown(), in that case private
> data is already passed to seq_open().

Yes.

> That means here the behaviour will still be same as existing..

No, the crucial difference is that the 'get_device' is now safe. It
either acquires a kref or it returns ENODEV. Since we have safely done
get_device, and hold a kref, we know the log cannot become freed since you
will move the log free to the tpm_dev_release function.

tpm_dev_release is called when the chip kref count goes to 0.

> opened files continue to access the data, and teardown will not be
> waiting for opened files to be closed() ? Is this correct
> understanding of what we are trying to do ?

Right, no waiting. Instead we defer kfree(chip) until all users are
done, including open user files. This is the same approach we use in
other parts of the subsystem.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/03/2016 10:05 PM, Jason Gunthorpe wrote:
> On Mon, Oct 03, 2016 at 03:35:23PM +0300, Jarkko Sakkinen wrote:
>
>>>> The scheme you suggested is also way off the mark for how fops works,
>>>> fops->close has no relation to the needed duration for 'data', the
>>>> duration is related to securityfs_remove.
>>>
>>> Right, the above would not work because it's not linked to
>>> securityfs_remove by any means.
>>>
>
>> You have to provide something factors more concrete. Otherwise,
>> I'm inclined to accept the approach in Naynas patch. It's an
>> improvement.
>
> I said I haven't checked the patch yet to see if the lifetime model
> for 'data' with securityfs is correct. Only that it matches the only
> other user of this feature..
>
> I looked more carefully, and I still can't find the right sort of
> locking in securityfs_remove..
>
>>> Are you trying to say that after securityfs_remove() there might be a
>>> "grace period" when user space could still see the files visible and
>>> open them?
>
> Sort of, the typical race is broadly
>
>      CPU0                           CPU1
>
> fops->open()
>                                  securityfs_remove()
> 				kref_put(chip)
> 				kfree(chip)
> kref_get(data->chip.kref)

I didn't understand which kref_get() are we referring here. I mean is it 
expected to happen somewhere during eventlog parsing, or exactly which 
code path ?

>
> This race should always be analyzed when working with user files.
>
> We deal with this situation in the other user interface:
> - cdev uses 'chip->cdev.kobj.parent = &chip->dev.kobj;' and the cdev
>    core handles get/put of the chip at the proper time
> - sysfs uses kernfs_drain which guarentees nothing is running in any
>    callback before returning
>
> I suspect securityfs_remove is defective in this regard. Eg debugfs is
> built on the same libfs scheme as securityfs and it incorporates the
> mechanism around 'debugfs_use_file_start/etc' to provide sensible
> removal fencing.
>
> I don't know if there is a simple fix, so mabye the best thing is to
> just leave it be with a comment saying it securityfs_remove probably
> races with fops->open(), and that should be fixed inside securityfs
> not tpm.
>
> The file operations are also missing '.owner = THIS_MODULE' which is
> bad as well.

Yeah, this I will fix.
>
> Jason
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Fri, Oct 07, 2016 at 01:28:47AM +0530, Nayna wrote:
> >fops->open()
> >                                 securityfs_remove()
> >				kref_put(chip)
> >				kfree(chip)
> >kref_get(data->chip.kref)
> 
> I didn't understand which kref_get() are we referring here. I mean is it
> expected to happen somewhere during eventlog parsing, or exactly which code
> path ?

This is the missing get_device() I pointed out. Without a kref there
is nothing stopping the chip, data and log from being kfree'd.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/01/2016 10:24 PM, Jason Gunthorpe wrote:
> On Sat, Oct 01, 2016 at 03:01:25PM +0300, Jarkko Sakkinen wrote:
>
>>> +	struct tpm_securityfs_data bin_sfs_data;
>>> +	struct tpm_securityfs_data ascii_sfs_data;
>>
>> I think this is otherwise right but the struct name is very clunky.
>> First of all it doesn't own the data and IMHO now it kind of implies
>> of owning.
>
> These are passed in here:
>
>>>   	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,
>>> +				   (void *)&chip->ascii_sfs_data,
>>>   				   &tpm_bios_measurements_ops);
>
> And the argument to securityfs_create_file is called 'data'..
>
> The key question with these patches is if all the locking is done
> right and we have the correct lifetime model now.
>
> Eg how much does securityfs_remove serialize and is the kref on the
> chip held for the duration of any fops. Can open() start after the
> kref is dropped, etc.

This is my understanding here:

        tpm_chip_register() --->bios log setup

fops->open()-->private data as "log"
seq->open()--->private data as "log"

        tpm_chip_unregister() ---> bios log teardown
                                   securityfs_remove()

Few things if I understood correctly:

- there is no kref increment during eventlog fops or seq_ops operations.
- fops and seq ops are parsing over memory buffer. fops->open() returns 
after giving the memory buffer(log) to seq->open(). And, seq ops on 
reading of log memory are not bound to any locks or krefs.
- once securityfs_remove() is done, there are no more files accessible 
to user to do open(). Which implies, there can't be any new open() after 
chip unregister, but existing open() might continue to work(this is I 
expecting for now).

However, I do see one issue as I am freeing log.bios_event_log during 
teardown(). which implies seq_ops might fail if there is no proper null 
checks for log.bios_event_log.

Also, now log is also part of tpm_chip, so once chip is deregistered and 
tpm_chip is free, log might also be freed, but then that implies that 
private "data" in fops->open() is no more valid anyway.

I do see there are issues with serializing, however, I am trying to 
understand that what type of solution are we looking for:

#1. Do we want securityfs_remove() to wait till all the opened eventlog 
files are closed().

OR

#2 Are we ok with securityfs_remove() being done and files are removed, 
but existing seq ops, eventlog parsing should continue to work till 
closed by user. That implies, we should not unknowingly nullify log 
pointers which are used by parser.

I took sometime to understand how is kref getting accessed for tpm_chip. 
And I tried tracing krefs. I have been looking at chip->dev.kobj.kref.. 
Please let me know if there is any other kref also.
And I found that currently eventlog is unassociated with tpm_chip as 
what everyone discussed. During fops-open(), there is no kref increment. 
But, do we want to make tpm_chip wait on eventlog files ? or are we fine 
with opened files accessible, but once closed they are not as files 
would be removed.

Please let me know if I am missing any internals.

Thanks & Regards,
    - Nayna

>
> Otherwise this scheme isn't good enough either :/
>
> I haven't looked in detail at that topic yet.. Maybe Nayna can explain
> what is expected here. Would be excellend to get someone from security to
> review this lifetime model.



>
> Jason
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Fri, Oct 07, 2016 at 01:26:45AM +0530, Nayna wrote:

> - there is no kref increment during eventlog fops or seq_ops operations.
> - fops and seq ops are parsing over memory buffer. fops->open() returns
> after giving the memory buffer(log) to seq->open(). And, seq ops on reading
> of log memory are not bound to any locks or krefs.

I sent a email about this, you are missing a get_device(chip) in open.
(see entry Jason Gunthorpe - Oct. 3, 2016, 5:14 p.m.
 https://patchwork.kernel.org/patch/9353259/ )

> - once securityfs_remove() is done, there are no more files accessible to
> user to do open(). Which implies, there can't be any new open() after chip
> unregister, but existing open() might continue to work(this is I expecting
> for now).

Right..

> However, I do see one issue as I am freeing log.bios_event_log during
> teardown().

Correct, I thought I pointed that out last round, that kfree must be
moved to tpm_dev_release

> which implies seq_ops might fail if there is no proper null
> checks for log.bios_event_log.

No, hold the chip mutex between fops->open() -> release() which will
ensure that the log memory continues exists.

> #1. Do we want securityfs_remove() to wait till all the opened eventlog
> files are closed().

That is not simple, or necessary..

> #2 Are we ok with securityfs_remove() being done and files are removed, but
> existing seq ops, eventlog parsing should continue to work till closed by
> user. That implies, we should not unknowingly nullify log pointers which are
> used by parser.

This is simpler, moving the kfree to tpm_dev_release and holding the
chip kref for the lifetime of the filp is easy and safe.

> I took sometime to understand how is kref getting accessed for tpm_chip. And
> I tried tracing krefs. I have been looking at
> chip->dev.kobj.kref.. Please

Right, it used with get_device/put_device

The model to go for is that open() acquires a get_device() kref on
the chip and that kref is held for the duration of the lifetime of the
filp.

The log and data members of chip must remain allocated and unchanged until
tpm_dev_release.

Try something like the algorithm I gave in 'Jason Gunthorpe - Oct. 4,
2016, 5:12 p.m.' to solve the remove/open race for now with a big fat
comment.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/07/2016 01:40 AM, Jason Gunthorpe wrote:
> On Fri, Oct 07, 2016 at 01:26:45AM +0530, Nayna wrote:
>
>> - there is no kref increment during eventlog fops or seq_ops operations.
>> - fops and seq ops are parsing over memory buffer. fops->open() returns
>> after giving the memory buffer(log) to seq->open(). And, seq ops on reading
>> of log memory are not bound to any locks or krefs.
>
> I sent a email about this, you are missing a get_device(chip) in open.
> (see entry Jason Gunthorpe - Oct. 3, 2016, 5:14 p.m.
>   https://patchwork.kernel.org/patch/9353259/ )
>
>> - once securityfs_remove() is done, there are no more files accessible to
>> user to do open(). Which implies, there can't be any new open() after chip
>> unregister, but existing open() might continue to work(this is I expecting
>> for now).
>
> Right..
>
>> However, I do see one issue as I am freeing log.bios_event_log during
>> teardown().
>
> Correct, I thought I pointed that out last round, that kfree must be
> moved to tpm_dev_release
>
>> which implies seq_ops might fail if there is no proper null
>> checks for log.bios_event_log.
>
> No, hold the chip mutex between fops->open() -> release() which will
> ensure that the log memory continues exists.
>
>> #1. Do we want securityfs_remove() to wait till all the opened eventlog
>> files are closed().
>
> That is not simple, or necessary..
>
>> #2 Are we ok with securityfs_remove() being done and files are removed, but
>> existing seq ops, eventlog parsing should continue to work till closed by
>> user. That implies, we should not unknowingly nullify log pointers which are
>> used by parser.
>
> This is simpler, moving the kfree to tpm_dev_release and holding the
> chip kref for the lifetime of the filp is easy and safe.
>
>> I took sometime to understand how is kref getting accessed for tpm_chip. And
>> I tried tracing krefs. I have been looking at
>> chip->dev.kobj.kref.. Please
>
> Right, it used with get_device/put_device
>
> The model to go for is that open() acquires a get_device() kref on
> the chip and that kref is held for the duration of the lifetime of the
> filp.
>
> The log and data members of chip must remain allocated and unchanged until
> tpm_dev_release.
>
> Try something like the algorithm I gave in 'Jason Gunthorpe - Oct. 4,
> 2016, 5:12 p.m.' to solve the remove/open race for now with a big fat
> comment.

Sure, will try this and will also include all other feedbacks.

Thanks & Regards,
    - Nayna

>
> Jason
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/01/2016 05:31 PM, Jarkko Sakkinen wrote:
> On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote:
>> Currently, the securityfs pseudo files for obtaining the firmware
>> event log are created whether the event log properties exist or not.
>> This patch creates ascii and bios measurements pseudo files
>> only if read_log() is successful.
>
> Re-reviewing this. The commit message should mention about preventing
> a race condition.
>
> I think Jason was right. It makes code much more manageable with a
> small price of memory consumption.
>
>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> ---
>>   drivers/char/tpm/tpm.h          |  6 +++++
>>   drivers/char/tpm/tpm_acpi.c     | 12 +++++++---
>>   drivers/char/tpm/tpm_eventlog.c | 53 +++++++++++++++++++----------------------
>>   drivers/char/tpm/tpm_eventlog.h |  7 +++++-
>>   drivers/char/tpm/tpm_of.c       |  4 +++-
>>   5 files changed, 48 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index b5866bb..68630cd 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,10 @@ struct tpm_chip {
>>   	struct rw_semaphore ops_sem;
>>   	const struct tpm_class_ops *ops;
>>
>> +	struct tpm_bios_log log;
>
> struct tpm_bios_log should be renamed as struct tpm_event_log in some
> commit of this patch set as tpm_bios_log is a misleading name.

My understanding is that other event log functions are also named in 
consistent with tpm_bios_log naming.. for eg.. 
tpm_bios_log_setup(/teardown), tpm_bios_measurements_open,etc. So, 
wanted to understand if idea is only to change the struct name to 
tpm_event_log ?

Thanks & Regards,
   - Nayna

>
>> +	struct tpm_securityfs_data bin_sfs_data;
>> +	struct tpm_securityfs_data ascii_sfs_data;
>
> I think this is otherwise right but the struct name is very clunky.
> First of all it doesn't own the data and IMHO now it kind of implies
> of owning.
>
> Maybe something like tpm_event_log_fd would a better name. It's a
> description of the event log file essentially.
>
>> +
>>   	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..4d6c2d7 100644
>> --- a/drivers/char/tpm/tpm_acpi.c
>> +++ b/drivers/char/tpm/tpm_acpi.c
>> @@ -45,13 +45,15 @@ 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;
>> +	struct tpm_bios_log *log;
>>
>> +	log = &chip->log;
>>   	if (log->bios_event_log != NULL) {
>>   		printk(KERN_ERR
>>   		       "%s: ERROR - Eventlog already initialized\n",
>> @@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log)
>>
>>   	virt = acpi_os_map_iomem(start, len);
>>   	if (!virt) {
>> -		kfree(log->bios_event_log);
>>   		printk("%s: ERROR - Unable to map memory\n", __func__);
>> -		return -EIO;
>> +		goto err;
>>   	}
>>
>>   	memcpy_fromio(log->bios_event_log, virt, len);
>>
>>   	acpi_os_unmap_iomem(virt, len);
>>   	return 0;
>> +
>> +err:
>> +	kfree(log->bios_event_log);
>> +	return -EIO;
>> +
>>   }
>> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
>> index f1df782..a8cd4a1 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,19 @@ 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)
>> -		return -ENOMEM;
>> -
>> -	err = read_log(log);
>> -	if (err)
>> -		goto out_free;
>> +	const struct tpm_securityfs_data *sfs_data =
>> +		(const struct tpm_securityfs_data *)inode->i_private;
>> +	const struct seq_operations *seqops = sfs_data->seqops;
>>
>>   	/* now register seq file */
>>   	err = seq_open(file, seqops);
>>   	if (!err) {
>>   		seq = file->private_data;
>> -		seq->private = log;
>> -	} else {
>> -		goto out_free;
>> +		seq->private = sfs_data->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,6 +349,18 @@ static int is_bad(void *p)
>>   int tpm_bios_log_setup(struct tpm_chip *chip)
>>   {
>>   	const char *name = dev_name(&chip->dev);
>> +	int rc = 0;
>> +
>> +	rc = read_log(chip);
>> +	/*
>> +	 * read_log failure means event log is not supported except for ENOMEM
>> +	 */
>> +	if (rc < 0) {
>> +		if (rc == -ENOMEM)
>> +			return rc;
>> +		else
>> +			return 0;
>> +	}
>>
>>   	chip->bios_dir_count = 0;
>>   	chip->bios_dir[chip->bios_dir_count] =
>> @@ -380,19 +369,24 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>>   		goto err;
>>   	chip->bios_dir_count++;
>>
>> +	chip->bin_sfs_data.log = &chip->log;
>> +	chip->bin_sfs_data.seqops = &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,
>> +				   (void *)&chip->bin_sfs_data,
>>   				   &tpm_bios_measurements_ops);
>>   	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>>   		goto err;
>>   	chip->bios_dir_count++;
>>
>> +	chip->ascii_sfs_data.log = &chip->log;
>> +	chip->ascii_sfs_data.seqops =  &tpm_ascii_b_measurments_seqops;
>>   	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,
>> +				   (void *)&chip->ascii_sfs_data,
>>   				   &tpm_bios_measurements_ops);
>>   	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>>   		goto err;
>> @@ -413,4 +407,5 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
>>   		securityfs_remove(chip->bios_dir[i-1]);
>>   	chip->bios_dir_count = i;
>>
>> +	kfree(chip->log.bios_event_log);
>>   }
>> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
>> index fd3357e..7ea066c 100644
>> --- a/drivers/char/tpm/tpm_eventlog.h
>> +++ b/drivers/char/tpm/tpm_eventlog.h
>> @@ -22,6 +22,11 @@ struct tpm_bios_log {
>>   	void *bios_event_log_end;
>>   };
>>
>> +struct tpm_securityfs_data {
>> +	struct tpm_bios_log *log;
>> +	const struct seq_operations *seqops;
>> +};
>> +
>>   struct tcpa_event {
>>   	u32 pcr_index;
>>   	u32 event_type;
>> @@ -73,7 +78,7 @@ 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)
>> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
>> index 570f30c..68d891a 100644
>> --- a/drivers/char/tpm/tpm_of.c
>> +++ b/drivers/char/tpm/tpm_of.c
>> @@ -20,12 +20,14 @@
>>   #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;
>> +	struct tpm_bios_log *log;
>>
>> +	log = &chip->log;
>>   	if (log->bios_event_log != NULL) {
>>   		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
>>   		return -EFAULT;
>> --
>> 2.5.0
>>
>
> /Jarkko
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/14/2016 12:21 AM, Nayna wrote:
>
>
> On 10/01/2016 05:31 PM, Jarkko Sakkinen wrote:
>> On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote:
>>> Currently, the securityfs pseudo files for obtaining the firmware
>>> event log are created whether the event log properties exist or not.
>>> This patch creates ascii and bios measurements pseudo files
>>> only if read_log() is successful.
>>
>> Re-reviewing this. The commit message should mention about preventing
>> a race condition.
>>
>> I think Jason was right. It makes code much more manageable with a
>> small price of memory consumption.
>>
>>> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>>> ---
>>>    drivers/char/tpm/tpm.h          |  6 +++++
>>>    drivers/char/tpm/tpm_acpi.c     | 12 +++++++---
>>>    drivers/char/tpm/tpm_eventlog.c | 53 +++++++++++++++++++----------------------
>>>    drivers/char/tpm/tpm_eventlog.h |  7 +++++-
>>>    drivers/char/tpm/tpm_of.c       |  4 +++-
>>>    5 files changed, 48 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>> index b5866bb..68630cd 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,10 @@ struct tpm_chip {
>>>    	struct rw_semaphore ops_sem;
>>>    	const struct tpm_class_ops *ops;
>>>
>>> +	struct tpm_bios_log log;
>>
>> struct tpm_bios_log should be renamed as struct tpm_event_log in some
>> commit of this patch set as tpm_bios_log is a misleading name.
>
> My understanding is that other event log functions are also named in
> consistent with tpm_bios_log naming.. for eg..
> tpm_bios_log_setup(/teardown), tpm_bios_measurements_open,etc. So,
> wanted to understand if idea is only to change the struct name to
> tpm_event_log ?
>
> Thanks & Regards,
>     - Nayna

I have not modified the tpm_bios_log naming in my new patch set for the 
above reason. But if we think that it is appropriate to change the data 
type (and functions ?) naming, I will post it as separate single patch.

Thanks & Regards,
    - Nayna

>
>>
>>> +	struct tpm_securityfs_data bin_sfs_data;
>>> +	struct tpm_securityfs_data ascii_sfs_data;
>>
>> I think this is otherwise right but the struct name is very clunky.
>> First of all it doesn't own the data and IMHO now it kind of implies
>> of owning.
>>
>> Maybe something like tpm_event_log_fd would a better name. It's a
>> description of the event log file essentially.
>>
>>> +
>>>    	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..4d6c2d7 100644
>>> --- a/drivers/char/tpm/tpm_acpi.c
>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>> @@ -45,13 +45,15 @@ 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;
>>> +	struct tpm_bios_log *log;
>>>
>>> +	log = &chip->log;
>>>    	if (log->bios_event_log != NULL) {
>>>    		printk(KERN_ERR
>>>    		       "%s: ERROR - Eventlog already initialized\n",
>>> @@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log)
>>>
>>>    	virt = acpi_os_map_iomem(start, len);
>>>    	if (!virt) {
>>> -		kfree(log->bios_event_log);
>>>    		printk("%s: ERROR - Unable to map memory\n", __func__);
>>> -		return -EIO;
>>> +		goto err;
>>>    	}
>>>
>>>    	memcpy_fromio(log->bios_event_log, virt, len);
>>>
>>>    	acpi_os_unmap_iomem(virt, len);
>>>    	return 0;
>>> +
>>> +err:
>>> +	kfree(log->bios_event_log);
>>> +	return -EIO;
>>> +
>>>    }
>>> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
>>> index f1df782..a8cd4a1 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,19 @@ 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)
>>> -		return -ENOMEM;
>>> -
>>> -	err = read_log(log);
>>> -	if (err)
>>> -		goto out_free;
>>> +	const struct tpm_securityfs_data *sfs_data =
>>> +		(const struct tpm_securityfs_data *)inode->i_private;
>>> +	const struct seq_operations *seqops = sfs_data->seqops;
>>>
>>>    	/* now register seq file */
>>>    	err = seq_open(file, seqops);
>>>    	if (!err) {
>>>    		seq = file->private_data;
>>> -		seq->private = log;
>>> -	} else {
>>> -		goto out_free;
>>> +		seq->private = sfs_data->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,6 +349,18 @@ static int is_bad(void *p)
>>>    int tpm_bios_log_setup(struct tpm_chip *chip)
>>>    {
>>>    	const char *name = dev_name(&chip->dev);
>>> +	int rc = 0;
>>> +
>>> +	rc = read_log(chip);
>>> +	/*
>>> +	 * read_log failure means event log is not supported except for ENOMEM
>>> +	 */
>>> +	if (rc < 0) {
>>> +		if (rc == -ENOMEM)
>>> +			return rc;
>>> +		else
>>> +			return 0;
>>> +	}
>>>
>>>    	chip->bios_dir_count = 0;
>>>    	chip->bios_dir[chip->bios_dir_count] =
>>> @@ -380,19 +369,24 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>>>    		goto err;
>>>    	chip->bios_dir_count++;
>>>
>>> +	chip->bin_sfs_data.log = &chip->log;
>>> +	chip->bin_sfs_data.seqops = &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,
>>> +				   (void *)&chip->bin_sfs_data,
>>>    				   &tpm_bios_measurements_ops);
>>>    	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>>>    		goto err;
>>>    	chip->bios_dir_count++;
>>>
>>> +	chip->ascii_sfs_data.log = &chip->log;
>>> +	chip->ascii_sfs_data.seqops =  &tpm_ascii_b_measurments_seqops;
>>>    	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,
>>> +				   (void *)&chip->ascii_sfs_data,
>>>    				   &tpm_bios_measurements_ops);
>>>    	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>>>    		goto err;
>>> @@ -413,4 +407,5 @@ void tpm_bios_log_teardown(struct tpm_chip *chip)
>>>    		securityfs_remove(chip->bios_dir[i-1]);
>>>    	chip->bios_dir_count = i;
>>>
>>> +	kfree(chip->log.bios_event_log);
>>>    }
>>> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
>>> index fd3357e..7ea066c 100644
>>> --- a/drivers/char/tpm/tpm_eventlog.h
>>> +++ b/drivers/char/tpm/tpm_eventlog.h
>>> @@ -22,6 +22,11 @@ struct tpm_bios_log {
>>>    	void *bios_event_log_end;
>>>    };
>>>
>>> +struct tpm_securityfs_data {
>>> +	struct tpm_bios_log *log;
>>> +	const struct seq_operations *seqops;
>>> +};
>>> +
>>>    struct tcpa_event {
>>>    	u32 pcr_index;
>>>    	u32 event_type;
>>> @@ -73,7 +78,7 @@ 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)
>>> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
>>> index 570f30c..68d891a 100644
>>> --- a/drivers/char/tpm/tpm_of.c
>>> +++ b/drivers/char/tpm/tpm_of.c
>>> @@ -20,12 +20,14 @@
>>>    #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;
>>> +	struct tpm_bios_log *log;
>>>
>>> +	log = &chip->log;
>>>    	if (log->bios_event_log != NULL) {
>>>    		pr_err("%s: ERROR - Eventlog already initialized\n", __func__);
>>>    		return -EFAULT;
>>> --
>>> 2.5.0
>>>
>>
>> /Jarkko
>>
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote:
> @@ -323,34 +315,19 @@ 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)
> -		return -ENOMEM;
> -
> -	err = read_log(log);
> -	if (err)
> -		goto out_free;
> +	const struct tpm_securityfs_data *sfs_data =
> +		(const struct tpm_securityfs_data *)inode->i_private;
> +	const struct seq_operations *seqops = sfs_data->seqops;

You need a get_device(&chip->dev) here, and the matching put_device in fops->release().

> +		seq->private = sfs_data->log;

So store the chip here

> +	chip->bin_sfs_data.log = &chip->log;

And pass the chip in here

And other related changes.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/03/2016 10:44 PM, Jason Gunthorpe wrote:
> On Wed, Sep 28, 2016 at 04:34:37AM -0400, Nayna Jain wrote:
>> @@ -323,34 +315,19 @@ 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)
>> -		return -ENOMEM;
>> -
>> -	err = read_log(log);
>> -	if (err)
>> -		goto out_free;
>> +	const struct tpm_securityfs_data *sfs_data =
>> +		(const struct tpm_securityfs_data *)inode->i_private;
>> +	const struct seq_operations *seqops = sfs_data->seqops;
>
> You need a get_device(&chip->dev) here, and the matching put_device in fops->release().
>
>> +		seq->private = sfs_data->log;
>
> So store the chip here
Sorry, I think I didn't understand the purpose of storing chip here.

I thought we can do it as:
struct tpm_chip *chip = sfs_data->chip;
seq->private = &chip->log;

Whatever is parsed in seq_private is used by seq parsing functions 
(start, next) to retrieve the log.

And currently, it is retrieved as:
struct tpm_bios_log *log = m->private;

And by storing chip, we will be doing it as:
struct tpm_chip *chip = m->private;
struct tpm_bios_log *log = &chip->log;

Key data structure which is used by parsing function is log. So, I 
didn't understand how parsing the chip here helps.

Thanks & Regards,
   - Nayna

>
>> +	chip->bin_sfs_data.log = &chip->log;
>
> And pass the chip in here
>
> And other related changes.
>
> Jason
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Sun, Oct 09, 2016 at 09:47:08AM +0530, Nayna wrote:

> >>+	const struct tpm_securityfs_data *sfs_data =
> >>+		(const struct tpm_securityfs_data *)inode->i_private;
> >>+	const struct seq_operations *seqops = sfs_data->seqops;
> >
> >You need a get_device(&chip->dev) here, and the matching put_device in fops->release().
> >
> >>+		seq->private = sfs_data->log;
> >
> >So store the chip here

> Sorry, I think I didn't understand the purpose of storing chip here.

Since we need to do get_device in open() you need to do put_device
in release()

How will you reliably do put_device if you do not store chip in the
seq_private?

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/10/2016 04:55 AM, Jason Gunthorpe wrote:
> On Sun, Oct 09, 2016 at 09:47:08AM +0530, Nayna wrote:
>
>>>> +	const struct tpm_securityfs_data *sfs_data =
>>>> +		(const struct tpm_securityfs_data *)inode->i_private;
>>>> +	const struct seq_operations *seqops = sfs_data->seqops;
>>>
>>> You need a get_device(&chip->dev) here, and the matching put_device in fops->release().
>>>
>>>> +		seq->private = sfs_data->log;
>>>
>>> So store the chip here
>
>> Sorry, I think I didn't understand the purpose of storing chip here.
>
> Since we need to do get_device in open() you need to do put_device
> in release()
>
> How will you reliably do put_device if you do not store chip in the
> seq_private?

We are storing tpm_securityfs_data in inode->private.
Currently, tpm_securityfs_data is

struct tpm_securityfs_data {
         struct tpm_bios_log *log;
         const struct seq_operations *seqops;
};

This, I am changing in new version to

struct tpm_securityfs_data {
         struct tpm_bios_log *chip;
         const struct seq_operations *seqops;
};

And we pass this as private data to i_node in tpm_bios_log_setup.

So, we are referring chip as i_node->i_private->chip.

And both open() and release() gets i_node as input parameter.

Thanks & Regards,
   - Nayna

>
> Jason
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Mon, Oct 10, 2016 at 07:23:33AM +0530, Nayna wrote:

> And we pass this as private data to i_node in tpm_bios_log_setup.
 
> So, we are referring chip as i_node->i_private->chip.

That probably works, but you can't use the i_private = NULL scheme I
outlined with that.

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/10/2016 08:51 AM, Jason Gunthorpe wrote:
> On Mon, Oct 10, 2016 at 07:23:33AM +0530, Nayna wrote:
>
>> And we pass this as private data to i_node in tpm_bios_log_setup.
>
>> So, we are referring chip as i_node->i_private->chip.
>
> That probably works, but you can't use the i_private = NULL scheme I
> outlined with that.

Why ? we are doing i_private = NULL during teardown to imply that chip 
unregister is in progress. and no more securityfs operations should be 
done. So, whether chip is NULL or securityfs_data is NULL, either should 
be ok. Isn't it ?

Below is the open() function with NULL check and get_device(). 
put_device() I will do in release()

static int tpm_bios_measurements_open(struct inode *inode,
                                             struct file *file)
{
         int err;
         struct seq_file *seq;
         struct tpm_securityfs_data *sfs_data;
         const struct seq_operations *seqops;
         struct tpm_chip *chip;

         inode_lock(inode);
         if (!inode->i_private) {  ---> This would be made NULL by teardown
                 inode_unlock(inode);
                 return -ENODEV;
         }
         sfs_data = (const struct tpm_securityfs_data *)inode->i_private;
         seqops = sfs_data->seqops;
         chip = sfs_data->chip;
         get_device(&chip->dev);
         /* now register seq file */
         err = seq_open(file, seqops);
         if (!err) {
                 seq = file->private_data;
                 seq->private = &chip->log;
         }
         inode_unlock(inode);

         return err;
}


Let me know if I am missing something basic.

Thanks & Regards,
   - Nayna


>
> Jason
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Mon, Oct 10, 2016 at 09:43:05AM +0530, Nayna wrote:
> 
> 
> On 10/10/2016 08:51 AM, Jason Gunthorpe wrote:
> >On Mon, Oct 10, 2016 at 07:23:33AM +0530, Nayna wrote:
> >
> >>And we pass this as private data to i_node in tpm_bios_log_setup.
> >
> >>So, we are referring chip as i_node->i_private->chip.
> >
> >That probably works, but you can't use the i_private = NULL scheme I
> >outlined with that.
> 
> Why ? we are doing i_private = NULL during teardown to imply that chip
> unregister is in progress. and no more securityfs operations should be done.
> So, whether chip is NULL or securityfs_data is NULL, either should be ok.
> Isn't it ?

How does release() work if you have to do:

  put_device(&((const struct tpm_securityfs_data *)inode->i_private)->chip.dev)

i_private could be null

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/11/2016 10:21 PM, Jason Gunthorpe wrote:
> On Mon, Oct 10, 2016 at 09:43:05AM +0530, Nayna wrote:
>>
>>
>> On 10/10/2016 08:51 AM, Jason Gunthorpe wrote:
>>> On Mon, Oct 10, 2016 at 07:23:33AM +0530, Nayna wrote:
>>>
>>>> And we pass this as private data to i_node in tpm_bios_log_setup.
>>>
>>>> So, we are referring chip as i_node->i_private->chip.
>>>
>>> That probably works, but you can't use the i_private = NULL scheme I
>>> outlined with that.
>>
>> Why ? we are doing i_private = NULL during teardown to imply that chip
>> unregister is in progress. and no more securityfs operations should be done.
>> So, whether chip is NULL or securityfs_data is NULL, either should be ok.
>> Isn't it ?
>
> How does release() work if you have to do:
>
>    put_device(&((const struct tpm_securityfs_data *)inode->i_private)->chip.dev)
>
> i_private could be null

Yeah, I actually tried this today.
And on call of securityfs_remove(), release() gets called for the opened 
file. And if i_private is NULL, the process opening the file gets killed 
with some random outputted characters.

There are actually two private data:
inode->private
seq->private

I understand inode->private is where we pass sfs_data has both chip and 
seqops. This is the one being used in open(), release() and defined as 
NULL in teardown().

But seq->private is used by seq_ops. And I am still not sure how passing 
seq->private as chip can help.

I might be missing something basic, so can you please help me to 
understand that.

Thanks & Regards,
   - Nayna

>
> Jason
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Jason Gunthorpe]

On Wed, Oct 12, 2016 at 12:41:05AM +0530, Nayna wrote:

> Yeah, I actually tried this today.
> And on call of securityfs_remove(), release() gets called for the
> opened

Are you saying securityfs_remove somehow causes a synchronous call to
release? How does that come about?

> There are actually two private data:
> inode->private
> seq->private
> 
> I understand inode->private is where we pass sfs_data has both chip and
> seqops. This is the one being used in open(), release() and defined as NULL
> in teardown().

> But seq->private is used by seq_ops. And I am still not sure how passing
> seq->private as chip can help.

> I might be missing something basic, so can you please help me to understand
> that.

open does:

 struct tpm_chip *chip = inode->i_private
 get_device(&chip->dev);
 seq = file->private_data;
 seq->private = chip;

release does:

 struct seq_file *seq = file->private_data;
 struct tpm_chip *chip = seq->private;
 put_device(&chip->dev);

seqops like tpm_bios_measurements_start do:

 struct tpm_chip *chip = m->private;
 struct tpm_bios_log *log = &chip->log;

[locking, error handling, and other stuff elided]

open is the only thing that ever looks a inode->i_private.

open krefs's chip and stores it in seq->private

seqop accessors use seq->private->log to access the log, the memory of
which is guared by the kref.

release drops the kref on chip and does not use inode->i_private

Jason

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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/12/2016 01:45 AM, Jason Gunthorpe wrote:
> On Wed, Oct 12, 2016 at 12:41:05AM +0530, Nayna wrote:
>
>> Yeah, I actually tried this today.
>> And on call of securityfs_remove(), release() gets called for the
>> opened
>
> Are you saying securityfs_remove somehow causes a synchronous call to
> release? How does that come about?
>
>> There are actually two private data:
>> inode->private
>> seq->private
>>
>> I understand inode->private is where we pass sfs_data has both chip and
>> seqops. This is the one being used in open(), release() and defined as NULL
>> in teardown().
>
>> But seq->private is used by seq_ops. And I am still not sure how passing
>> seq->private as chip can help.
>
>> I might be missing something basic, so can you please help me to understand
>> that.
>
> open does:
>
>   struct tpm_chip *chip = inode->i_private
>   get_device(&chip->dev);
>   seq = file->private_data;
>   seq->private = chip;

Yeah, I realized later that I overlooked file->private_data.
In total, there are three private actually.

>
> release does:
>
>   struct seq_file *seq = file->private_data;
>   struct tpm_chip *chip = seq->private;
>   put_device(&chip->dev);
>
> seqops like tpm_bios_measurements_start do:
>
>   struct tpm_chip *chip = m->private;
>   struct tpm_bios_log *log = &chip->log;
>
> [locking, error handling, and other stuff elided]
>
> open is the only thing that ever looks a inode->i_private.
>
> open krefs's chip and stores it in seq->private
>
> seqop accessors use seq->private->log to access the log, the memory of
> which is guared by the kref.
>
> release drops the kref on chip and does not use inode->i_private

Thanks for the detailed explanation.

Thanks & Regards,
    - Nayna


>
> Jason
>


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

Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna]

On 10/12/2016 01:45 AM, Jason Gunthorpe wrote:
> On Wed, Oct 12, 2016 at 12:41:05AM +0530, Nayna wrote:
>
>> Yeah, I actually tried this today.
>> And on call of securityfs_remove(), release() gets called for the
>> opened
>
> Are you saying securityfs_remove somehow causes a synchronous call to
> release? How does that come about?

Sorry, I realized , that I missed to answer this in previous response.
I think I misinterpreted the sequence and there is no release() call on 
securityfs_remove().. Sorry for the confusion.

Thanks & Regards,
    - Nayna

>
>> There are actually two private data:
>> inode->private
>> seq->private
>>
>> I understand inode->private is where we pass sfs_data has both chip and
>> seqops. This is the one being used in open(), release() and defined as NULL
>> in teardown().
>
>> But seq->private is used by seq_ops. And I am still not sure how passing
>> seq->private as chip can help.
>
>> I might be missing something basic, so can you please help me to understand
>> that.
>
> open does:
>
>   struct tpm_chip *chip = inode->i_private
>   get_device(&chip->dev);
>   seq = file->private_data;
>   seq->private = chip;
>
> release does:
>
>   struct seq_file *seq = file->private_data;
>   struct tpm_chip *chip = seq->private;
>   put_device(&chip->dev);
>
> seqops like tpm_bios_measurements_start do:
>
>   struct tpm_chip *chip = m->private;
>   struct tpm_bios_log *log = &chip->log;
>
> [locking, error handling, and other stuff elided]
>
> open is the only thing that ever looks a inode->i_private.
>
> open krefs's chip and stores it in seq->private
>
> seqop accessors use seq->private->log to access the log, the memory of
> which is guared by the kref.
>
> release drops the kref on chip and does not use inode->i_private
>
> Jason
>


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

[PATCH v4 4/8] tpm: redefine read_log() to handle ACPI/OF at runtime

[Nayna Jain]

Currently, read_log() has two implementations: one for ACPI platforms
and the other for OF platforms. The proper one is selected at compile
time using Kconfig and #ifdef in the Makefile, which is not the
recommended approach.

This patch removes the #ifdef in the Makefile by defining a single
read_log() method, which checks for ACPI/OF event log properties at
runtime.

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>
---
 drivers/char/tpm/Makefile       | 14 ++++----------
 drivers/char/tpm/tpm_acpi.c     |  9 ++-------
 drivers/char/tpm/tpm_eventlog.c | 18 ++++++++++++++++++
 drivers/char/tpm/tpm_eventlog.h | 22 +++++++++++++---------
 drivers/char/tpm/tpm_of.c       |  8 ++------
 5 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a385fb8..a05b1eb 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -2,16 +2,10 @@
 # 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-$(CONFIG_ACPI) += tpm_ppi.o
-
-ifdef CONFIG_ACPI
-	tpm-y += tpm_eventlog.o tpm_acpi.o
-else
-ifdef CONFIG_TCG_IBMVTPM
-	tpm-y += tpm_eventlog.o tpm_of.o
-endif
-endif
+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 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 4d6c2d7..859bdba 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -6,6 +6,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>
  *
@@ -45,7 +46,7 @@ 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;
@@ -54,12 +55,6 @@ int read_log(struct tpm_chip *chip)
 	struct tpm_bios_log *log;
 
 	log = &chip->log;
-	if (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,
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index a8cd4a1..c327089 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -346,6 +346,24 @@ 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) || (rc == -ENOMEM))
+		return rc;
+	rc = read_log_of(chip);
+	return rc;
+
+}
+
 int 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 7ea066c..9e95b7e 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -78,20 +78,24 @@ enum tcpa_pc_event_ids {
 	HOST_TABLE_OF_DEVICES,
 };
 
-int read_log(struct tpm_chip *chip);
-
-#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
-	defined(CONFIG_ACPI)
-extern int tpm_bios_log_setup(struct tpm_chip *chip);
-extern void tpm_bios_log_teardown(struct tpm_chip *chip);
+#if defined(CONFIG_ACPI)
+int read_log_acpi(struct tpm_chip *chip);
 #else
-static inline int tpm_bios_log_setup(struct tpm_chip *chip)
+static inline int read_log_acpi(struct tpm_chip *chip)
 {
-	return 0;
+	return -ENODEV;
 }
-static inline void tpm_bios_log_teardown(struct tpm_chip *chip)
+#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 -ENODEV;
 }
 #endif
 
+int 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 68d891a..7c30752 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>
  *
@@ -20,7 +21,7 @@
 #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;
@@ -28,11 +29,6 @@ int read_log(struct tpm_chip *chip)
 	struct tpm_bios_log *log;
 
 	log = &chip->log;
-	if (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


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

Re: [PATCH v4 4/8] tpm: redefine read_log() to handle ACPI/OF at runtime

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 04:34:38AM -0400, Nayna Jain wrote:
> Currently, read_log() has two implementations: one for ACPI platforms
> and the other for OF platforms. The proper one is selected at compile
> time using Kconfig and #ifdef in the Makefile, which is not the
> recommended approach.
> 
> This patch removes the #ifdef in the Makefile by defining a single
> read_log() method, which checks for ACPI/OF event log properties at
> runtime.
> 
> 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>
> ---
>  drivers/char/tpm/Makefile       | 14 ++++----------
>  drivers/char/tpm/tpm_acpi.c     |  9 ++-------
>  drivers/char/tpm/tpm_eventlog.c | 18 ++++++++++++++++++
>  drivers/char/tpm/tpm_eventlog.h | 22 +++++++++++++---------
>  drivers/char/tpm/tpm_of.c       |  8 ++------
>  5 files changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index a385fb8..a05b1eb 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -2,16 +2,10 @@
>  # 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-$(CONFIG_ACPI) += tpm_ppi.o
> -
> -ifdef CONFIG_ACPI
> -	tpm-y += tpm_eventlog.o tpm_acpi.o
> -else
> -ifdef CONFIG_TCG_IBMVTPM
> -	tpm-y += tpm_eventlog.o tpm_of.o
> -endif
> -endif
> +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 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 4d6c2d7..859bdba 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -6,6 +6,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>
>   *
> @@ -45,7 +46,7 @@ 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;
> @@ -54,12 +55,6 @@ int read_log(struct tpm_chip *chip)
>  	struct tpm_bios_log *log;
>  
>  	log = &chip->log;
> -	if (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,
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index a8cd4a1..c327089 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -346,6 +346,24 @@ 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) || (rc == -ENOMEM))
> +		return rc;
> +	rc = read_log_of(chip);
> +	return rc;
> +
> +}

I'm wondering if it is a better idea to leverage tpm_class_ops? This
would be kind of cool idea to implement this because then the decision
to support event log could be leveraged to the driver level.

If the event_log pointer is NULL, then event log is not supported.

/Jarkko

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

Re: [PATCH v4 4/8] tpm: redefine read_log() to handle ACPI/OF at runtime

[Nayna]

On 10/01/2016 12:35 AM, Jarkko Sakkinen wrote:
> On Wed, Sep 28, 2016 at 04:34:38AM -0400, Nayna Jain wrote:
>> Currently, read_log() has two implementations: one for ACPI platforms
>> and the other for OF platforms. The proper one is selected at compile
>> time using Kconfig and #ifdef in the Makefile, which is not the
>> recommended approach.
>>
>> This patch removes the #ifdef in the Makefile by defining a single
>> read_log() method, which checks for ACPI/OF event log properties at
>> runtime.
>>
>> 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>
>> ---
>>   drivers/char/tpm/Makefile       | 14 ++++----------
>>   drivers/char/tpm/tpm_acpi.c     |  9 ++-------
>>   drivers/char/tpm/tpm_eventlog.c | 18 ++++++++++++++++++
>>   drivers/char/tpm/tpm_eventlog.h | 22 +++++++++++++---------
>>   drivers/char/tpm/tpm_of.c       |  8 ++------
>>   5 files changed, 39 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
>> index a385fb8..a05b1eb 100644
>> --- a/drivers/char/tpm/Makefile
>> +++ b/drivers/char/tpm/Makefile
>> @@ -2,16 +2,10 @@
>>   # 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-$(CONFIG_ACPI) += tpm_ppi.o
>> -
>> -ifdef CONFIG_ACPI
>> -	tpm-y += tpm_eventlog.o tpm_acpi.o
>> -else
>> -ifdef CONFIG_TCG_IBMVTPM
>> -	tpm-y += tpm_eventlog.o tpm_of.o
>> -endif
>> -endif
>> +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 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 4d6c2d7..859bdba 100644
>> --- a/drivers/char/tpm/tpm_acpi.c
>> +++ b/drivers/char/tpm/tpm_acpi.c
>> @@ -6,6 +6,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>
>>    *
>> @@ -45,7 +46,7 @@ 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;
>> @@ -54,12 +55,6 @@ int read_log(struct tpm_chip *chip)
>>   	struct tpm_bios_log *log;
>>
>>   	log = &chip->log;
>> -	if (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,
>> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
>> index a8cd4a1..c327089 100644
>> --- a/drivers/char/tpm/tpm_eventlog.c
>> +++ b/drivers/char/tpm/tpm_eventlog.c
>> @@ -346,6 +346,24 @@ 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) || (rc == -ENOMEM))
>> +		return rc;
>> +	rc = read_log_of(chip);
>> +	return rc;
>> +
>> +}
>
> I'm wondering if it is a better idea to leverage tpm_class_ops? This
> would be kind of cool idea to implement this because then the decision
> to support event log could be leveraged to the driver level.

Can you please explain me this bit more ?

For eg, when we say driver level.. does that mean to the level of tis, 
nuvoton ?

And what type of decision ? I mean like decision to create securityfs 
setup for userspace to read eventlog ? or there are other things ?

>
> If the event_log pointer is NULL, then event log is not supported.

Yeah, sure will fix this.

Thanks & Regards,
     - Nayna
>
> /Jarkko
>


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

Re: [PATCH v4 4/8] tpm: redefine read_log() to handle ACPI/OF at runtime

[Nayna]

On 10/07/2016 02:08 AM, Nayna wrote:
>
>
> On 10/01/2016 12:35 AM, Jarkko Sakkinen wrote:
>> On Wed, Sep 28, 2016 at 04:34:38AM -0400, Nayna Jain wrote:
>>> Currently, read_log() has two implementations: one for ACPI platforms
>>> and the other for OF platforms. The proper one is selected at compile
>>> time using Kconfig and #ifdef in the Makefile, which is not the
>>> recommended approach.
>>>
>>> This patch removes the #ifdef in the Makefile by defining a single
>>> read_log() method, which checks for ACPI/OF event log properties at
>>> runtime.
>>>
>>> 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>
>>> ---
>>>    drivers/char/tpm/Makefile       | 14 ++++----------
>>>    drivers/char/tpm/tpm_acpi.c     |  9 ++-------
>>>    drivers/char/tpm/tpm_eventlog.c | 18 ++++++++++++++++++
>>>    drivers/char/tpm/tpm_eventlog.h | 22 +++++++++++++---------
>>>    drivers/char/tpm/tpm_of.c       |  8 ++------
>>>    5 files changed, 39 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
>>> index a385fb8..a05b1eb 100644
>>> --- a/drivers/char/tpm/Makefile
>>> +++ b/drivers/char/tpm/Makefile
>>> @@ -2,16 +2,10 @@
>>>    # 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-$(CONFIG_ACPI) += tpm_ppi.o
>>> -
>>> -ifdef CONFIG_ACPI
>>> -	tpm-y += tpm_eventlog.o tpm_acpi.o
>>> -else
>>> -ifdef CONFIG_TCG_IBMVTPM
>>> -	tpm-y += tpm_eventlog.o tpm_of.o
>>> -endif
>>> -endif
>>> +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 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 4d6c2d7..859bdba 100644
>>> --- a/drivers/char/tpm/tpm_acpi.c
>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>> @@ -6,6 +6,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>
>>>     *
>>> @@ -45,7 +46,7 @@ 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;
>>> @@ -54,12 +55,6 @@ int read_log(struct tpm_chip *chip)
>>>    	struct tpm_bios_log *log;
>>>
>>>    	log = &chip->log;
>>> -	if (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,
>>> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
>>> index a8cd4a1..c327089 100644
>>> --- a/drivers/char/tpm/tpm_eventlog.c
>>> +++ b/drivers/char/tpm/tpm_eventlog.c
>>> @@ -346,6 +346,24 @@ 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) || (rc == -ENOMEM))
>>> +		return rc;
>>> +	rc = read_log_of(chip);
>>> +	return rc;
>>> +
>>> +}
>>
>> I'm wondering if it is a better idea to leverage tpm_class_ops? This
>> would be kind of cool idea to implement this because then the decision
>> to support event log could be leveraged to the driver level.
>
> Can you please explain me this bit more ?
>
> For eg, when we say driver level.. does that mean to the level of tis,
> nuvoton ?
>
> And what type of decision ? I mean like decision to create securityfs
> setup for userspace to read eventlog ? or there are other things ?
>

Jarkko, can you please also help me to understand here more ?

Thanks & Regards,
   - Nayna

>>
>> If the event_log pointer is NULL, then event log is not supported.
>
> Yeah, sure will fix this.
>
> Thanks & Regards,
>       - Nayna
>>
>> /Jarkko
>>
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>


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

Re: [PATCH v4 4/8] tpm: redefine read_log() to handle ACPI/OF at runtime

[Jarkko Sakkinen]

On Sun, Oct 09, 2016 at 04:59:15PM +0530, Nayna wrote:
> 
> 
> On 10/07/2016 02:08 AM, Nayna wrote:
> >
> >
> >On 10/01/2016 12:35 AM, Jarkko Sakkinen wrote:
> >>On Wed, Sep 28, 2016 at 04:34:38AM -0400, Nayna Jain wrote:
> >>>Currently, read_log() has two implementations: one for ACPI platforms
> >>>and the other for OF platforms. The proper one is selected at compile
> >>>time using Kconfig and #ifdef in the Makefile, which is not the
> >>>recommended approach.
> >>>
> >>>This patch removes the #ifdef in the Makefile by defining a single
> >>>read_log() method, which checks for ACPI/OF event log properties at
> >>>runtime.
> >>>
> >>>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>
> >>>---
> >>>   drivers/char/tpm/Makefile       | 14 ++++----------
> >>>   drivers/char/tpm/tpm_acpi.c     |  9 ++-------
> >>>   drivers/char/tpm/tpm_eventlog.c | 18 ++++++++++++++++++
> >>>   drivers/char/tpm/tpm_eventlog.h | 22 +++++++++++++---------
> >>>   drivers/char/tpm/tpm_of.c       |  8 ++------
> >>>   5 files changed, 39 insertions(+), 32 deletions(-)
> >>>
> >>>diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> >>>index a385fb8..a05b1eb 100644
> >>>--- a/drivers/char/tpm/Makefile
> >>>+++ b/drivers/char/tpm/Makefile
> >>>@@ -2,16 +2,10 @@
> >>>   # 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-$(CONFIG_ACPI) += tpm_ppi.o
> >>>-
> >>>-ifdef CONFIG_ACPI
> >>>-	tpm-y += tpm_eventlog.o tpm_acpi.o
> >>>-else
> >>>-ifdef CONFIG_TCG_IBMVTPM
> >>>-	tpm-y += tpm_eventlog.o tpm_of.o
> >>>-endif
> >>>-endif
> >>>+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 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 4d6c2d7..859bdba 100644
> >>>--- a/drivers/char/tpm/tpm_acpi.c
> >>>+++ b/drivers/char/tpm/tpm_acpi.c
> >>>@@ -6,6 +6,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>
> >>>    *
> >>>@@ -45,7 +46,7 @@ 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;
> >>>@@ -54,12 +55,6 @@ int read_log(struct tpm_chip *chip)
> >>>   	struct tpm_bios_log *log;
> >>>
> >>>   	log = &chip->log;
> >>>-	if (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,
> >>>diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> >>>index a8cd4a1..c327089 100644
> >>>--- a/drivers/char/tpm/tpm_eventlog.c
> >>>+++ b/drivers/char/tpm/tpm_eventlog.c
> >>>@@ -346,6 +346,24 @@ 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) || (rc == -ENOMEM))
> >>>+		return rc;
> >>>+	rc = read_log_of(chip);
> >>>+	return rc;
> >>>+
> >>>+}
> >>
> >>I'm wondering if it is a better idea to leverage tpm_class_ops? This
> >>would be kind of cool idea to implement this because then the decision
> >>to support event log could be leveraged to the driver level.
> >
> >Can you please explain me this bit more ?
> >
> >For eg, when we say driver level.. does that mean to the level of tis,
> >nuvoton ?
> >
> >And what type of decision ? I mean like decision to create securityfs
> >setup for userspace to read eventlog ? or there are other things ?
> >
> 
> Jarkko, can you please also help me to understand here more ?

Sorry, I missed your original response.

Umh, it was more like an open question and I'm not sure if it even
makes sense. Maybe you can just ignore it until the next revision.

/Jarkko

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

Re: [PATCH v4 4/8] tpm: redefine read_log() to handle ACPI/OF at runtime

[Jason Gunthorpe]

On Sun, Oct 09, 2016 at 03:05:53PM +0300, Jarkko Sakkinen wrote:

> > Jarkko, can you please also help me to understand here more ?
> 
> Sorry, I missed your original response.
> 
> Umh, it was more like an open question and I'm not sure if it even
> makes sense. Maybe you can just ignore it until the next revision.

For now we two standard ways to find the log that do not require
intervention of the low level driver, so I'd leave it as is until
something changes.

Jason

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

[PATCH v4 5/8] tpm: replace of_find_node_by_name() with dev of_node property

[Nayna Jain]

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

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

diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 7c30752..22b8f81 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -29,7 +29,8 @@ int read_log_of(struct tpm_chip *chip)
 	struct tpm_bios_log *log;
 
 	log = &chip->log;
-	np = of_find_node_by_name(NULL, "vtpm");
+	if (chip->dev.parent->of_node)
+		np = chip->dev.parent->of_node;
 	if (!np) {
 		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
 		return -ENODEV;
@@ -55,18 +56,15 @@ int read_log_of(struct tpm_chip *chip)
 	if (!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;
 
 	memcpy(log->bios_event_log, __va(*basep), *sizep);
-	of_node_put(np);
 
 	return 0;
 
 cleanup_eio:
-	of_node_put(np);
 	return -EIO;
 }
-- 
2.5.0


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

Re: [PATCH v4 5/8] tpm: replace of_find_node_by_name() with dev of_node property

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 04:34:39AM -0400, Nayna Jain wrote:
> Using the device of_node property is a better way to refer to the
> device tree node rather than of_find_node_by_name().
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

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

/Jarkko

> ---
>  drivers/char/tpm/tpm_of.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 7c30752..22b8f81 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -29,7 +29,8 @@ int read_log_of(struct tpm_chip *chip)
>  	struct tpm_bios_log *log;
>  
>  	log = &chip->log;
> -	np = of_find_node_by_name(NULL, "vtpm");
> +	if (chip->dev.parent->of_node)
> +		np = chip->dev.parent->of_node;
>  	if (!np) {
>  		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
>  		return -ENODEV;
> @@ -55,18 +56,15 @@ int read_log_of(struct tpm_chip *chip)
>  	if (!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;
>  
>  	memcpy(log->bios_event_log, __va(*basep), *sizep);
> -	of_node_put(np);
>  
>  	return 0;
>  
>  cleanup_eio:
> -	of_node_put(np);
>  	return -EIO;
>  }
> -- 
> 2.5.0
> 

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

[PATCH v4 6/8] tpm: remove printk error messages

[Nayna Jain]

This patch removes the unnecessary messages for failure to allocate
memory. It also replaces pr_err/printk with 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_acpi.c | 17 +++++------------
 drivers/char/tpm/tpm_of.c   | 26 ++++++++++----------------
 2 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 859bdba..22e42da 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
 	status = acpi_get_table(ACPI_SIG_TCPA, 1,
 				(struct acpi_table_header **)&buff);
 
-	if (ACPI_FAILURE(status)) {
-		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
-		       __func__);
+	if (ACPI_FAILURE(status))
 		return -EIO;
-	}
 
 	switch(buff->platform_class) {
 	case BIOS_SERVER:
@@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
 		break;
 	}
 	if (!len) {
-		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
+			__func__);
 		return -EIO;
 	}
 
 	/* malloc EventLog space */
 	log->bios_event_log = kmalloc(len, GFP_KERNEL);
-	if (!log->bios_event_log) {
-		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
-			__func__);
+	if (!log->bios_event_log)
 		return -ENOMEM;
-	}
 
 	log->bios_event_log_end = log->bios_event_log + len;
 
 	virt = acpi_os_map_iomem(start, len);
-	if (!virt) {
-		printk("%s: ERROR - Unable to map memory\n", __func__);
+	if (!virt)
 		goto err;
-	}
 
 	memcpy_fromio(log->bios_event_log, virt, len);
 
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 22b8f81..1464cae 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -31,40 +31,34 @@ int read_log_of(struct tpm_chip *chip)
 	log = &chip->log;
 	if (chip->dev.parent->of_node)
 		np = chip->dev.parent->of_node;
-	if (!np) {
-		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
+	if (!np)
 		return -ENODEV;
-	}
 
 	sizep = of_get_property(np, "linux,sml-size", NULL);
 	if (sizep == NULL) {
-		pr_err("%s: ERROR - SML size not found\n", __func__);
-		goto cleanup_eio;
+		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
+			__func__);
+		return -EIO;
 	}
 	if (*sizep == 0) {
-		pr_err("%s: ERROR - event log area empty\n", __func__);
-		goto cleanup_eio;
+		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
+			__func__);
+		return -EIO;
 	}
 
 	basep = of_get_property(np, "linux,sml-base", NULL);
 	if (basep == NULL) {
-		pr_err("%s: ERROR - SML not found\n", __func__);
-		goto cleanup_eio;
+		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n", __func__);
+		return -EIO;
 	}
 
 	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
-	if (!log->bios_event_log) {
-		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
-		       __func__);
+	if (!log->bios_event_log)
 		return -ENOMEM;
-	}
 
 	log->bios_event_log_end = log->bios_event_log + *sizep;
 
 	memcpy(log->bios_event_log, __va(*basep), *sizep);
 
 	return 0;
-
-cleanup_eio:
-	return -EIO;
 }
-- 
2.5.0


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

Re: [PATCH v4 6/8] tpm: remove printk error messages

[Nayna]

Hi,

Does this patch (v4 6/8) and next patch (v4 7/8) looks fine ?

Thanks & Regards,
    - Nayna

On 09/28/2016 02:04 PM, Nayna Jain wrote:
> This patch removes the unnecessary messages for failure to allocate
> memory. It also replaces pr_err/printk with 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_acpi.c | 17 +++++------------
>   drivers/char/tpm/tpm_of.c   | 26 ++++++++++----------------
>   2 files changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 859bdba..22e42da 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>   	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>   				(struct acpi_table_header **)&buff);
>
> -	if (ACPI_FAILURE(status)) {
> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> -		       __func__);
> +	if (ACPI_FAILURE(status))
>   		return -EIO;
> -	}
>
>   	switch(buff->platform_class) {
>   	case BIOS_SERVER:
> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>   		break;
>   	}
>   	if (!len) {
> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> +			__func__);
>   		return -EIO;
>   	}
>
>   	/* malloc EventLog space */
>   	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
> -			__func__);
> +	if (!log->bios_event_log)
>   		return -ENOMEM;
> -	}
>
>   	log->bios_event_log_end = log->bios_event_log + len;
>
>   	virt = acpi_os_map_iomem(start, len);
> -	if (!virt) {
> -		printk("%s: ERROR - Unable to map memory\n", __func__);
> +	if (!virt)
>   		goto err;
> -	}
>
>   	memcpy_fromio(log->bios_event_log, virt, len);
>
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 22b8f81..1464cae 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -31,40 +31,34 @@ int read_log_of(struct tpm_chip *chip)
>   	log = &chip->log;
>   	if (chip->dev.parent->of_node)
>   		np = chip->dev.parent->of_node;
> -	if (!np) {
> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> +	if (!np)
>   		return -ENODEV;
> -	}
>
>   	sizep = of_get_property(np, "linux,sml-size", NULL);
>   	if (sizep == NULL) {
> -		pr_err("%s: ERROR - SML size not found\n", __func__);
> -		goto cleanup_eio;
> +		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
> +			__func__);
> +		return -EIO;
>   	}
>   	if (*sizep == 0) {
> -		pr_err("%s: ERROR - event log area empty\n", __func__);
> -		goto cleanup_eio;
> +		dev_dbg(&chip->dev, "%s: ERROR - event log area empty\n",
> +			__func__);
> +		return -EIO;
>   	}
>
>   	basep = of_get_property(np, "linux,sml-base", NULL);
>   	if (basep == NULL) {
> -		pr_err("%s: ERROR - SML not found\n", __func__);
> -		goto cleanup_eio;
> +		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n", __func__);
> +		return -EIO;
>   	}
>
>   	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
> -		       __func__);
> +	if (!log->bios_event_log)
>   		return -ENOMEM;
> -	}
>
>   	log->bios_event_log_end = log->bios_event_log + *sizep;
>
>   	memcpy(log->bios_event_log, __va(*basep), *sizep);
>
>   	return 0;
> -
> -cleanup_eio:
> -	return -EIO;
>   }
>


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

Re: [PATCH v4 6/8] tpm: remove printk error messages

[Jason Gunthorpe]

On Sun, Oct 09, 2016 at 07:25:30AM +0530, Nayna wrote:

> >diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> >index 22b8f81..1464cae 100644
> >+++ b/drivers/char/tpm/tpm_of.c
> >@@ -31,40 +31,34 @@ int read_log_of(struct tpm_chip *chip)
> >  	log = &chip->log;
> >  	if (chip->dev.parent->of_node)
> >  		np = chip->dev.parent->of_node;
> >-	if (!np) {
> >-		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> >+	if (!np)
> >  		return -ENODEV;
> >-	}


> >  	sizep = of_get_property(np, "linux,sml-size", NULL);
> >  	if (sizep == NULL) {
> >-		pr_err("%s: ERROR - SML size not found\n", __func__);
> >-		goto cleanup_eio;
> >+		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
> >+			__func__);
> >+		return -EIO;
> >  	}

The properties are optional (eg my DT bound TPMs on ARM do not have
them) so I'm not sure the debug is appropriate either...

Everything else looks OK to me.

Jason

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

Re: [PATCH v4 6/8] tpm: remove printk error messages

[Nayna]

On 10/10/2016 04:52 AM, Jason Gunthorpe wrote:
> On Sun, Oct 09, 2016 at 07:25:30AM +0530, Nayna wrote:
>
>>> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
>>> index 22b8f81..1464cae 100644
>>> +++ b/drivers/char/tpm/tpm_of.c
>>> @@ -31,40 +31,34 @@ int read_log_of(struct tpm_chip *chip)
>>>   	log = &chip->log;
>>>   	if (chip->dev.parent->of_node)
>>>   		np = chip->dev.parent->of_node;
>>> -	if (!np) {
>>> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
>>> +	if (!np)
>>>   		return -ENODEV;
>>> -	}
>
>
>>>   	sizep = of_get_property(np, "linux,sml-size", NULL);
>>>   	if (sizep == NULL) {
>>> -		pr_err("%s: ERROR - SML size not found\n", __func__);
>>> -		goto cleanup_eio;
>>> +		dev_dbg(&chip->dev, "%s: ERROR - SML size not found\n",
>>> +			__func__);
>>> +		return -EIO;
>>>   	}
>
> The properties are optional (eg my DT bound TPMs on ARM do not have
> them) so I'm not sure the debug is appropriate either...

Hmm.. does that imply that do we even need a msg ?. or probably if 
dev_info(..) looks appropriate, this can give the indication that the 
platform does not support eventlog, and that is ok.

Thanks & Regards,
   - Nayna

>
> Everything else looks OK to me.
>
> Jason
>


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

[PATCH v4 7/8] tpm: move event log init functions to tpm_eventlog_init.c

[Nayna Jain]

The device driver code for the event log has the init functions and
TPM 1.2 parsing logic both defined in same file(tpm_eventlog.c).

Since the initialization functions are common with the TPM 2.0 event
log support, this patch moves 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      | 130 +----------------------------
 drivers/char/tpm/tpm_eventlog.h      |   3 +
 drivers/char/tpm/tpm_eventlog_init.c | 155 +++++++++++++++++++++++++++++++++++
 4 files changed, 163 insertions(+), 127 deletions(-)
 create mode 100644 drivers/char/tpm/tpm_eventlog_init.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a05b1eb..1dc2671 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
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
index c327089..2492134 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -11,7 +11,8 @@
  *
  * Maintained by: <tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
  *
- * Access to the eventlog created by a system's firmware / BIOS
+ * Access to the TPM 1.2 event log created by a system's
+ * firmware / BIOS
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -258,12 +259,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,133 +292,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;
-	const struct tpm_securityfs_data *sfs_data =
-		(const struct tpm_securityfs_data *)inode->i_private;
-	const struct seq_operations *seqops = sfs_data->seqops;
-
-	/* now register seq file */
-	err = seq_open(file, seqops);
-	if (!err) {
-		seq = file->private_data;
-		seq->private = sfs_data->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) || (rc == -ENOMEM))
-		return rc;
-	rc = read_log_of(chip);
-	return rc;
-
-}
-
-int tpm_bios_log_setup(struct tpm_chip *chip)
-{
-	const char *name = dev_name(&chip->dev);
-	int rc = 0;
-
-	rc = read_log(chip);
-	/*
-	 * read_log failure means event log is not supported except for ENOMEM
-	 */
-	if (rc < 0) {
-		if (rc == -ENOMEM)
-			return rc;
-		else
-			return 0;
-	}
-
-	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++;
-
-	chip->bin_sfs_data.log = &chip->log;
-	chip->bin_sfs_data.seqops = &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 *)&chip->bin_sfs_data,
-				   &tpm_bios_measurements_ops);
-	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
-		goto err;
-	chip->bios_dir_count++;
-
-	chip->ascii_sfs_data.log = &chip->log;
-	chip->ascii_sfs_data.seqops =  &tpm_ascii_b_measurments_seqops;
-	chip->bios_dir[chip->bios_dir_count] =
-	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&chip->ascii_sfs_data,
-				   &tpm_bios_measurements_ops);
-	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
-		goto err;
-	chip->bios_dir_count++;
-
-	return 0;
-
-err:
-	tpm_bios_log_teardown(chip);
-	return -EIO;
-}
-
-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]);
-	chip->bios_dir_count = i;
-
-	kfree(chip->log.bios_event_log);
-}
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index 9e95b7e..ed6ab93 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..c4ac42630
--- /dev/null
+++ b/drivers/char/tpm/tpm_eventlog_init.c
@@ -0,0 +1,155 @@
+/*
+ * 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 common initialization functions to access
+ * firmware event log for TPM 1.2 and TPM 2.0
+ *
+ * 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;
+	const struct tpm_securityfs_data *sfs_data =
+		(const struct tpm_securityfs_data *)inode->i_private;
+	const struct seq_operations *seqops = sfs_data->seqops;
+
+	/* now register seq file */
+	err = seq_open(file, seqops);
+	if (!err) {
+		seq = file->private_data;
+		seq->private = sfs_data->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) || (rc == -ENOMEM))
+		return rc;
+	rc = read_log_of(chip);
+	return rc;
+
+}
+
+int tpm_bios_log_setup(struct tpm_chip *chip)
+{
+	const char *name = dev_name(&chip->dev);
+	int rc = 0;
+
+	rc = read_log(chip);
+	/*
+	 * read_log failure means event log is not supported except for ENOMEM
+	 */
+	if (rc < 0) {
+		if (rc == -ENOMEM)
+			return rc;
+		else
+			return 0;
+	}
+
+	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++;
+
+	chip->bin_sfs_data.log = &chip->log;
+	chip->bin_sfs_data.seqops = &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 *)&chip->bin_sfs_data,
+				   &tpm_bios_measurements_ops);
+	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+		goto err;
+	chip->bios_dir_count++;
+
+	chip->ascii_sfs_data.log = &chip->log;
+	chip->ascii_sfs_data.seqops =  &tpm_ascii_b_measurments_seqops;
+	chip->bios_dir[chip->bios_dir_count] =
+	    securityfs_create_file("ascii_bios_measurements",
+				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
+				   (void *)&chip->ascii_sfs_data,
+				   &tpm_bios_measurements_ops);
+	if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+		goto err;
+	chip->bios_dir_count++;
+
+	return 0;
+
+err:
+	tpm_bios_log_teardown(chip);
+	return -EIO;
+}
+
+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]);
+	chip->bios_dir_count = i;
+
+	kfree(chip->log.bios_event_log);
+}
-- 
2.5.0


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

[PATCH v4 8/8] tpm: add securityfs support for TPM 2.0 firmware event log

[Nayna Jain]

Unlike the device driver support for TPM 1.2, the TPM 2.0 support
does not create the securityfs pseudo files for displaying the
firmware event log.

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

Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 drivers/char/tpm/Makefile            |   2 +-
 drivers/char/tpm/tpm-chip.c          |  26 ++---
 drivers/char/tpm/tpm2.h              |  79 +++++++++++++
 drivers/char/tpm/tpm2_eventlog.c     | 216 +++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_eventlog_init.c |  30 +++--
 drivers/char/tpm/tpm_of.c            |  26 ++++-
 6 files changed, 348 insertions(+), 31 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 1dc2671..f185a6a5 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
 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 826609d..72715fa 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -278,23 +278,12 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 
 static int tpm1_chip_register(struct tpm_chip *chip)
 {
-	int rc;
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return 0;
 
 	tpm_sysfs_add_device(chip);
 
-	rc = tpm_bios_log_setup(chip);
-
-	return rc;
-}
-
-static void tpm1_chip_unregister(struct tpm_chip *chip)
-{
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		return;
-
-	tpm_bios_log_teardown(chip);
+	return 0;
 }
 
 static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
@@ -370,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;
 
@@ -383,6 +370,12 @@ int tpm_chip_register(struct tpm_chip *chip)
 		return rc;
 	}
 
+	rc = tpm_bios_log_setup(chip);
+	if (rc) {
+		tpm_chip_unregister(chip);
+		return rc;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_chip_register);
@@ -405,9 +398,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
 		return;
 
+	tpm_bios_log_teardown(chip);
+
 	tpm_del_legacy_sysfs(chip);
 
-	tpm1_chip_unregister(chip);
 	tpm_del_char_device(chip);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_unregister);
diff --git a/drivers/char/tpm/tpm2.h b/drivers/char/tpm/tpm2.h
new file mode 100644
index 0000000..399c15c
--- /dev/null
+++ b/drivers/char/tpm/tpm2.h
@@ -0,0 +1,79 @@
+#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
+#define MAX_DIGEST_SIZE	64
+
+/**
+ * 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 TPM 2.0 are defined in document
+ * Trusted Platform Module Library Part 2: Structures, Family "2.0".
+ */
+
+/* Event log header algorithm spec. */
+struct tcg_efispecideventalgorithmsize {
+	u16	alg_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_idx;
+	u32	event_type;
+	u8	digest[20];
+	u32	event_size;
+	u8	event[MAX_TPM_LOG_MSG];
+} __packed;
+
+/* Crypto Agile algorithm and respective digest. */
+struct tpmt_ha {
+	u16		alg_id;
+	u8		digest[MAX_DIGEST_SIZE];
+} __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_idx;
+	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..32c7d5c
--- /dev/null
+++ b/drivers/char/tpm/tpm2_eventlog.c
@@ -0,0 +1,216 @@
+/*
+ * Copyright (C) 2016 IBM Corporation
+ *
+ * Authors:
+ *      Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
+ *
+ * Access to TPM 2.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 i, j;
+	u16 halg;
+	u32 halg_size;
+	size_t size = 0;
+
+	/*
+	 * NOTE: TPM 2.0 supports extend to multiple PCR Banks. This implies
+	 * event log also has multiple digest values, one for each PCR Bank.
+	 * This is called Crypto Agile Log Entry Format.
+	 * TCG EFI Protocol Specification defines the procedure to parse
+	 * the event log. Below code implements this procedure to parse
+	 * correctly the Crypto agile log entry format.
+	 * Example of Crypto Agile Log Digests Format :
+	 * digest_values.count = 2;
+	 * digest_values.digest[0].alg_id = sha1;
+	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
+	 * digest_values.digest[1].alg_id = sha256;
+	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
+	 * Offset of eventsize is sizeof(count) + sizeof(alg_id) + 20
+	 *			+ sizeof(alg_id) + 32;
+	 *
+	 * Since, offset of event_size can vary based on digests count, offset
+	 * has to be calculated at run time. void *marker is used to traverse
+	 * the dynamic structure and calculate the offset of event_size.
+	 */
+
+	marker = event;
+	marker_start = marker;
+	marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
+		+ sizeof(event->digests.count);
+
+	efispecid = (struct tcg_efispecideventstruct *) event_header->event;
+
+	for (i = 0; (i < event->digests.count) && (i < HASH_COUNT); i++) {
+		halg_size = sizeof(event->digests.digests[i].alg_id);
+		memcpy(&halg, marker, halg_size);
+		marker = marker + halg_size;
+		for (j = 0; (j < efispecid->num_algs); j++) {
+			if (halg == efispecid->digest_sizes[j].alg_id) {
+				marker = marker +
+					efispecid->digest_sizes[j].digest_size;
+				break;
+			}
+		}
+	}
+
+	event_field = (struct tcg_event_field *) marker;
+	marker = marker + sizeof(event_field->event_size)
+		+ event_field->event_size;
+	size = marker - marker_start;
+
+	if ((event->event_type == 0) && (event_field->event_size == 0))
+		return 0;
+
+	return size;
+}
+
+static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
+{
+	struct tpm_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 c4ac42630..aaac0e9 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,
@@ -114,7 +115,11 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 	chip->bios_dir_count++;
 
 	chip->bin_sfs_data.log = &chip->log;
-	chip->bin_sfs_data.seqops = &tpm_binary_b_measurments_seqops;
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		chip->bin_sfs_data.seqops = &tpm2_binary_b_measurments_seqops;
+	else
+		chip->bin_sfs_data.seqops = &tpm_binary_b_measurments_seqops;
+
 
 	chip->bios_dir[chip->bios_dir_count] =
 	    securityfs_create_file("binary_bios_measurements",
@@ -125,16 +130,19 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
 		goto err;
 	chip->bios_dir_count++;
 
-	chip->ascii_sfs_data.log = &chip->log;
-	chip->ascii_sfs_data.seqops =  &tpm_ascii_b_measurments_seqops;
-	chip->bios_dir[chip->bios_dir_count] =
-	    securityfs_create_file("ascii_bios_measurements",
-				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&chip->ascii_sfs_data,
-				   &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->ascii_sfs_data.log = &chip->log;
+		chip->ascii_sfs_data.seqops =
+			&tpm_ascii_b_measurments_seqops;
+		chip->bios_dir[chip->bios_dir_count] =
+			securityfs_create_file("ascii_bios_measurements",
+				       S_IRUSR | S_IRGRP, chip->bios_dir[0],
+				       (void *)&chip->ascii_sfs_data,
+				       &tpm_bios_measurements_ops);
+		if (is_bad(chip->bios_dir[chip->bios_dir_count]))
+			goto err;
+		chip->bios_dir_count++;
+	}
 
 	return 0;
 
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 1464cae..d4151b5 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -17,6 +17,7 @@
 
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/string.h>
 
 #include "tpm.h"
 #include "tpm_eventlog.h"
@@ -27,6 +28,7 @@ int read_log_of(struct tpm_chip *chip)
 	const u32 *sizep;
 	const u64 *basep;
 	struct tpm_bios_log *log;
+	u32 log_size;
 
 	log = &chip->log;
 	if (chip->dev.parent->of_node)
@@ -46,19 +48,37 @@ int read_log_of(struct tpm_chip *chip)
 		return -EIO;
 	}
 
+	/*
+	 * For both vtpm/tpm, firmware has log addr and log size in big
+	 * endian format. But in case of vtpm, there is a method called
+	 * sml-handover which is run during kernel init even before
+	 * device tree is setup. This sml-handover function takes care
+	 * of endianness and writes to sml-base and sml-size in little
+	 * endian format. For this reason, vtpm doesn't need conversion
+	 * but physical tpm needs the conversion.
+	 */
+	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0)
+		log_size = be32_to_cpup(sizep);
+	else
+		log_size = *sizep;
+
 	basep = of_get_property(np, "linux,sml-base", NULL);
 	if (basep == NULL) {
 		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n", __func__);
 		return -EIO;
 	}
 
-	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
+	log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
 	if (!log->bios_event_log)
 		return -ENOMEM;
 
-	log->bios_event_log_end = log->bios_event_log + *sizep;
+	log->bios_event_log_end = log->bios_event_log + log_size;
 
-	memcpy(log->bios_event_log, __va(*basep), *sizep);
+	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0)
+		memcpy(chip->log.bios_event_log, __va(be64_to_cpup(basep)),
+		       log_size);
+	else
+		memcpy(chip->log.bios_event_log, __va(*basep), log_size);
 
 	return 0;
 }
-- 
2.5.0


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

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

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 04:34:42AM -0400, Nayna Jain wrote:
> Unlike the device driver support for TPM 1.2, the TPM 2.0 support
> does not create the securityfs pseudo files for displaying the
> firmware event log.
> 
> This patch enables support for providing the TPM 2.0 event log in
> binary form. TPM 2.0 event log supports a crypto agile format that
> records multiple digests, which is different from TPM 1.2. This
> patch adds the TPM 2.0 event log parser to understand the crypto
> agile format.
> 
> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/Makefile            |   2 +-
>  drivers/char/tpm/tpm-chip.c          |  26 ++---
>  drivers/char/tpm/tpm2.h              |  79 +++++++++++++
>  drivers/char/tpm/tpm2_eventlog.c     | 216 +++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_eventlog_init.c |  30 +++--
>  drivers/char/tpm/tpm_of.c            |  26 ++++-
>  6 files changed, 348 insertions(+), 31 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm2.h
>  create mode 100644 drivers/char/tpm/tpm2_eventlog.c

This commit is too large. It would have to be split into 'of' part
and generic part at minimum.

The bigger problem with this patch set is that it is for majority of
people untestable at this point. There is zero Tested-by's so far.

I think the clean up work would deserve its own patch set. That we
can digest to mainline with quicker schedule than TPM 2.0 part.

/Jarkko

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

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

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 04:34:42AM -0400, Nayna Jain wrote:
> Unlike the device driver support for TPM 1.2, the TPM 2.0 support
> does not create the securityfs pseudo files for displaying the
> firmware event log.
> 
> This patch enables support for providing the TPM 2.0 event log in
> binary form. TPM 2.0 event log supports a crypto agile format that
> records multiple digests, which is different from TPM 1.2. This
> patch adds the TPM 2.0 event log parser to understand the crypto
> agile format.

I'll got through the patch (the split comment is valid)

> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/char/tpm/Makefile            |   2 +-
>  drivers/char/tpm/tpm-chip.c          |  26 ++---
>  drivers/char/tpm/tpm2.h              |  79 +++++++++++++
>  drivers/char/tpm/tpm2_eventlog.c     | 216 +++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_eventlog_init.c |  30 +++--
>  drivers/char/tpm/tpm_of.c            |  26 ++++-
>  6 files changed, 348 insertions(+), 31 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 1dc2671..f185a6a5 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
>  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 826609d..72715fa 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -278,23 +278,12 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>  
>  static int tpm1_chip_register(struct tpm_chip *chip)
>  {
> -	int rc;
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return 0;
>  
>  	tpm_sysfs_add_device(chip);
>  
> -	rc = tpm_bios_log_setup(chip);
> -
> -	return rc;
> -}
> -
> -static void tpm1_chip_unregister(struct tpm_chip *chip)
> -{
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> -		return;
> -
> -	tpm_bios_log_teardown(chip);
> +	return 0;
>  }
>  
>  static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
> @@ -370,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;
>  
> @@ -383,6 +370,12 @@ int tpm_chip_register(struct tpm_chip *chip)
>  		return rc;
>  	}
>  
> +	rc = tpm_bios_log_setup(chip);
> +	if (rc) {
> +		tpm_chip_unregister(chip);
> +		return rc;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_register);
> @@ -405,9 +398,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
>  		return;
>  
> +	tpm_bios_log_teardown(chip);
> +
>  	tpm_del_legacy_sysfs(chip);
>  
> -	tpm1_chip_unregister(chip);
>  	tpm_del_char_device(chip);
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_unregister);

This is good place for split. Do a separate patch that moves the BIOS
log setup to tpm_chip_register(). In the preceding commit, just return
if the chip is TPM2

> diff --git a/drivers/char/tpm/tpm2.h b/drivers/char/tpm/tpm2.h
> new file mode 100644
> index 0000000..399c15c
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2.h
> @@ -0,0 +1,79 @@
> +#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
> +#define MAX_DIGEST_SIZE	64
> +
> +/**
> + * 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 TPM 2.0 are defined in document
> + * Trusted Platform Module Library Part 2: Structures, Family "2.0".
> + */
> +
> +/* Event log header algorithm spec. */
> +struct tcg_efispecideventalgorithmsize {
> +	u16	alg_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;

I think it would be good practice for the subsystem that struct fields
the fields are not aligned like this because it can easily break down
(not it in this particular case but some times you need to add nested
unions to structures).

For enums, it's easy to stay consistent and it improves readability.
There pros override cons (in a rare occasion you might need to realign
the fields).

I had discussion about this before with Jason. If I understood his point
right he was not aligning both for structs and enums. I agree with that
for structs.

I know that in tpm2-cmd.c I used aligning for structs but I think it
was not a good idea afterall.

> +
> +/* Header entry for eventlog. */
> +struct tcg_pcr_event {
> +	u32	pcr_idx;
> +	u32	event_type;
> +	u8	digest[20];
> +	u32	event_size;
> +	u8	event[MAX_TPM_LOG_MSG];
> +} __packed;
> +
> +/* Crypto Agile algorithm and respective digest. */
> +struct tpmt_ha {
> +	u16		alg_id;
> +	u8		digest[MAX_DIGEST_SIZE];
> +} __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_idx;
> +	u32				event_type;
> +	struct tpml_digest_values	digests;
> +	struct tcg_event_field		event;
> +} __packed;
> +
> +extern const struct seq_operations tpm2_binary_b_measurments_seqops;

There's a typo here. Also I'm wondering what this '_b_' stands for?

> +
> +#endif
> diff --git a/drivers/char/tpm/tpm2_eventlog.c b/drivers/char/tpm/tpm2_eventlog.c
> new file mode 100644
> index 0000000..32c7d5c
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2_eventlog.c
> @@ -0,0 +1,216 @@
> +/*
> + * Copyright (C) 2016 IBM Corporation
> + *
> + * Authors:
> + *      Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> + *
> + * Access to TPM 2.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 i, j;
> +	u16 halg;
> +	u32 halg_size;
> +	size_t size = 0;
> +
> +	/*
> +	 * NOTE: TPM 2.0 supports extend to multiple PCR Banks. This implies
> +	 * event log also has multiple digest values, one for each PCR Bank.
> +	 * This is called Crypto Agile Log Entry Format.
> +	 * TCG EFI Protocol Specification defines the procedure to parse
> +	 * the event log. Below code implements this procedure to parse
> +	 * correctly the Crypto agile log entry format.
> +	 * Example of Crypto Agile Log Digests Format :
> +	 * digest_values.count = 2;
> +	 * digest_values.digest[0].alg_id = sha1;
> +	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
> +	 * digest_values.digest[1].alg_id = sha256;
> +	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
> +	 * Offset of eventsize is sizeof(count) + sizeof(alg_id) + 20
> +	 *			+ sizeof(alg_id) + 32;
> +	 *
> +	 * Since, offset of event_size can vary based on digests count, offset
> +	 * has to be calculated at run time. void *marker is used to traverse
> +	 * the dynamic structure and calculate the offset of event_size.
> +	 */
> +
> +	marker = event;
> +	marker_start = marker;
> +	marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
> +		+ sizeof(event->digests.count);
> +
> +	efispecid = (struct tcg_efispecideventstruct *) event_header->event;
> +
> +	for (i = 0; (i < event->digests.count) && (i < HASH_COUNT); i++) {
> +		halg_size = sizeof(event->digests.digests[i].alg_id);
> +		memcpy(&halg, marker, halg_size);
> +		marker = marker + halg_size;
> +		for (j = 0; (j < efispecid->num_algs); j++) {
> +			if (halg == efispecid->digest_sizes[j].alg_id) {
> +				marker = marker +
> +					efispecid->digest_sizes[j].digest_size;
> +				break;
> +			}
> +		}
> +	}
> +
> +	event_field = (struct tcg_event_field *) marker;
> +	marker = marker + sizeof(event_field->event_size)
> +		+ event_field->event_size;
> +	size = marker - marker_start;
> +
> +	if ((event->event_type == 0) && (event_field->event_size == 0))
> +		return 0;
> +
> +	return size;
> +}
> +
> +static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
> +{
> +	struct tpm_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) {
> +

Extra new line.

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

Extra new line.

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

I don't want to go too much into reviewing these because I don't have
facilities to run this code. Overrally it looks good to me (not same
as reviewed-by).

> diff --git a/drivers/char/tpm/tpm_eventlog_init.c b/drivers/char/tpm/tpm_eventlog_init.c
> index c4ac42630..aaac0e9 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,
> @@ -114,7 +115,11 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  	chip->bios_dir_count++;
>  
>  	chip->bin_sfs_data.log = &chip->log;
> -	chip->bin_sfs_data.seqops = &tpm_binary_b_measurments_seqops;
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		chip->bin_sfs_data.seqops = &tpm2_binary_b_measurments_seqops;
> +	else
> +		chip->bin_sfs_data.seqops = &tpm_binary_b_measurments_seqops;
> +
>  
>  	chip->bios_dir[chip->bios_dir_count] =
>  	    securityfs_create_file("binary_bios_measurements",
> @@ -125,16 +130,19 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  		goto err;
>  	chip->bios_dir_count++;
>  
> -	chip->ascii_sfs_data.log = &chip->log;
> -	chip->ascii_sfs_data.seqops =  &tpm_ascii_b_measurments_seqops;
> -	chip->bios_dir[chip->bios_dir_count] =
> -	    securityfs_create_file("ascii_bios_measurements",
> -				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&chip->ascii_sfs_data,
> -				   &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->ascii_sfs_data.log = &chip->log;
> +		chip->ascii_sfs_data.seqops =
> +			&tpm_ascii_b_measurments_seqops;
> +		chip->bios_dir[chip->bios_dir_count] =
> +			securityfs_create_file("ascii_bios_measurements",
> +				       S_IRUSR | S_IRGRP, chip->bios_dir[0],
> +				       (void *)&chip->ascii_sfs_data,
> +				       &tpm_bios_measurements_ops);
> +		if (is_bad(chip->bios_dir[chip->bios_dir_count]))
> +			goto err;
> +		chip->bios_dir_count++;
> +	}
>  
>  	return 0;
>  
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 1464cae..d4151b5 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -17,6 +17,7 @@
>  
>  #include <linux/slab.h>
>  #include <linux/of.h>
> +#include <linux/string.h>
>  
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
> @@ -27,6 +28,7 @@ int read_log_of(struct tpm_chip *chip)
>  	const u32 *sizep;
>  	const u64 *basep;
>  	struct tpm_bios_log *log;
> +	u32 log_size;
>  
>  	log = &chip->log;
>  	if (chip->dev.parent->of_node)
> @@ -46,19 +48,37 @@ int read_log_of(struct tpm_chip *chip)
>  		return -EIO;
>  	}
>  
> +	/*
> +	 * For both vtpm/tpm, firmware has log addr and log size in big
> +	 * endian format. But in case of vtpm, there is a method called
> +	 * sml-handover which is run during kernel init even before
> +	 * device tree is setup. This sml-handover function takes care
> +	 * of endianness and writes to sml-base and sml-size in little
> +	 * endian format. For this reason, vtpm doesn't need conversion
> +	 * but physical tpm needs the conversion.
> +	 */
> +	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0)
> +		log_size = be32_to_cpup(sizep);
> +	else
> +		log_size = *sizep;
> +
>  	basep = of_get_property(np, "linux,sml-base", NULL);
>  	if (basep == NULL) {
>  		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n", __func__);
>  		return -EIO;
>  	}
>  
> -	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> +	log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
>  	if (!log->bios_event_log)
>  		return -ENOMEM;
>  
> -	log->bios_event_log_end = log->bios_event_log + *sizep;
> +	log->bios_event_log_end = log->bios_event_log + log_size;
>  
> -	memcpy(log->bios_event_log, __va(*basep), *sizep);
> +	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0)
> +		memcpy(chip->log.bios_event_log, __va(be64_to_cpup(basep)),
> +		       log_size);
> +	else
> +		memcpy(chip->log.bios_event_log, __va(*basep), log_size);
>  
>  	return 0;
>  }
> -- 
> 2.5.0
> 

/Jarkko

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

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

[Nayna]

On 10/01/2016 05:21 PM, Jarkko Sakkinen wrote:
> On Wed, Sep 28, 2016 at 04:34:42AM -0400, Nayna Jain wrote:
>> Unlike the device driver support for TPM 1.2, the TPM 2.0 support
>> does not create the securityfs pseudo files for displaying the
>> firmware event log.
>>
>> This patch enables support for providing the TPM 2.0 event log in
>> binary form. TPM 2.0 event log supports a crypto agile format that
>> records multiple digests, which is different from TPM 1.2. This
>> patch adds the TPM 2.0 event log parser to understand the crypto
>> agile format.
>
> I'll got through the patch (the split comment is valid)

Sure, will do it.

>
>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> ---
>>   drivers/char/tpm/Makefile            |   2 +-
>>   drivers/char/tpm/tpm-chip.c          |  26 ++---
>>   drivers/char/tpm/tpm2.h              |  79 +++++++++++++
>>   drivers/char/tpm/tpm2_eventlog.c     | 216 +++++++++++++++++++++++++++++++++++
>>   drivers/char/tpm/tpm_eventlog_init.c |  30 +++--
>>   drivers/char/tpm/tpm_of.c            |  26 ++++-
>>   6 files changed, 348 insertions(+), 31 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 1dc2671..f185a6a5 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
>>   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 826609d..72715fa 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -278,23 +278,12 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>>
>>   static int tpm1_chip_register(struct tpm_chip *chip)
>>   {
>> -	int rc;
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>>   		return 0;
>>
>>   	tpm_sysfs_add_device(chip);
>>
>> -	rc = tpm_bios_log_setup(chip);
>> -
>> -	return rc;
>> -}
>> -
>> -static void tpm1_chip_unregister(struct tpm_chip *chip)
>> -{
>> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>> -		return;
>> -
>> -	tpm_bios_log_teardown(chip);
>> +	return 0;
>>   }
>>
>>   static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
>> @@ -370,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;
>>
>> @@ -383,6 +370,12 @@ int tpm_chip_register(struct tpm_chip *chip)
>>   		return rc;
>>   	}
>>
>> +	rc = tpm_bios_log_setup(chip);
>> +	if (rc) {
>> +		tpm_chip_unregister(chip);
>> +		return rc;
>> +	}
>> +
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(tpm_chip_register);
>> @@ -405,9 +398,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>>   	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
>>   		return;
>>
>> +	tpm_bios_log_teardown(chip);
>> +
>>   	tpm_del_legacy_sysfs(chip);
>>
>> -	tpm1_chip_unregister(chip);
>>   	tpm_del_char_device(chip);
>>   }
>>   EXPORT_SYMBOL_GPL(tpm_chip_unregister);
>
> This is good place for split. Do a separate patch that moves the BIOS
> log setup to tpm_chip_register(). In the preceding commit, just return
> if the chip is TPM2

Ok. Sure.

>> diff --git a/drivers/char/tpm/tpm2.h b/drivers/char/tpm/tpm2.h
>> new file mode 100644
>> index 0000000..399c15c
>> --- /dev/null
>> +++ b/drivers/char/tpm/tpm2.h
>> @@ -0,0 +1,79 @@
>> +#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
>> +#define MAX_DIGEST_SIZE	64
>> +
>> +/**
>> + * 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 TPM 2.0 are defined in document
>> + * Trusted Platform Module Library Part 2: Structures, Family "2.0".
>> + */
>> +
>> +/* Event log header algorithm spec. */
>> +struct tcg_efispecideventalgorithmsize {
>> +	u16	alg_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;
>
> I think it would be good practice for the subsystem that struct fields
> the fields are not aligned like this because it can easily break down
> (not it in this particular case but some times you need to add nested
> unions to structures).
>
> For enums, it's easy to stay consistent and it improves readability.
> There pros override cons (in a rare occasion you might need to realign
> the fields).
>
> I had discussion about this before with Jason. If I understood his point
> right he was not aligning both for structs and enums. I agree with that
> for structs.
>
> I know that in tpm2-cmd.c I used aligning for structs but I think it
> was not a good idea afterall.

Sure, will change this.

>
>> +
>> +/* Header entry for eventlog. */
>> +struct tcg_pcr_event {
>> +	u32	pcr_idx;
>> +	u32	event_type;
>> +	u8	digest[20];
>> +	u32	event_size;
>> +	u8	event[MAX_TPM_LOG_MSG];
>> +} __packed;
>> +
>> +/* Crypto Agile algorithm and respective digest. */
>> +struct tpmt_ha {
>> +	u16		alg_id;
>> +	u8		digest[MAX_DIGEST_SIZE];
>> +} __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_idx;
>> +	u32				event_type;
>> +	struct tpml_digest_values	digests;
>> +	struct tcg_event_field		event;
>> +} __packed;
>> +
>> +extern const struct seq_operations tpm2_binary_b_measurments_seqops;
>
> There's a typo here. Also I'm wondering what this '_b_' stands for?

I guess you mean the typo in measurments spelling.

Naming convention is taken from existing naming for TPM 1.2 eventlog.
And if I understood correctly, _b_ implied bios.. so 
binary_bios_measurements.

>
>> +
>> +#endif
>> diff --git a/drivers/char/tpm/tpm2_eventlog.c b/drivers/char/tpm/tpm2_eventlog.c
>> new file mode 100644
>> index 0000000..32c7d5c
>> --- /dev/null
>> +++ b/drivers/char/tpm/tpm2_eventlog.c
>> @@ -0,0 +1,216 @@
>> +/*
>> + * Copyright (C) 2016 IBM Corporation
>> + *
>> + * Authors:
>> + *      Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> + *
>> + * Access to TPM 2.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 i, j;
>> +	u16 halg;
>> +	u32 halg_size;
>> +	size_t size = 0;
>> +
>> +	/*
>> +	 * NOTE: TPM 2.0 supports extend to multiple PCR Banks. This implies
>> +	 * event log also has multiple digest values, one for each PCR Bank.
>> +	 * This is called Crypto Agile Log Entry Format.
>> +	 * TCG EFI Protocol Specification defines the procedure to parse
>> +	 * the event log. Below code implements this procedure to parse
>> +	 * correctly the Crypto agile log entry format.
>> +	 * Example of Crypto Agile Log Digests Format :
>> +	 * digest_values.count = 2;
>> +	 * digest_values.digest[0].alg_id = sha1;
>> +	 * digest_values.digest[0].digest.sha1 = {20 bytes raw data};
>> +	 * digest_values.digest[1].alg_id = sha256;
>> +	 * digest_values.digest[1].digest.sha256 = {32 bytes raw data};
>> +	 * Offset of eventsize is sizeof(count) + sizeof(alg_id) + 20
>> +	 *			+ sizeof(alg_id) + 32;
>> +	 *
>> +	 * Since, offset of event_size can vary based on digests count, offset
>> +	 * has to be calculated at run time. void *marker is used to traverse
>> +	 * the dynamic structure and calculate the offset of event_size.
>> +	 */
>> +
>> +	marker = event;
>> +	marker_start = marker;
>> +	marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
>> +		+ sizeof(event->digests.count);
>> +
>> +	efispecid = (struct tcg_efispecideventstruct *) event_header->event;
>> +
>> +	for (i = 0; (i < event->digests.count) && (i < HASH_COUNT); i++) {
>> +		halg_size = sizeof(event->digests.digests[i].alg_id);
>> +		memcpy(&halg, marker, halg_size);
>> +		marker = marker + halg_size;
>> +		for (j = 0; (j < efispecid->num_algs); j++) {
>> +			if (halg == efispecid->digest_sizes[j].alg_id) {
>> +				marker = marker +
>> +					efispecid->digest_sizes[j].digest_size;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	event_field = (struct tcg_event_field *) marker;
>> +	marker = marker + sizeof(event_field->event_size)
>> +		+ event_field->event_size;
>> +	size = marker - marker_start;
>> +
>> +	if ((event->event_type == 0) && (event_field->event_size == 0))
>> +		return 0;
>> +
>> +	return size;
>> +}
>> +
>> +static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
>> +{
>> +	struct tpm_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) {
>> +
>
> Extra new line.
Will fix.

>
>> +		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 {
>> +
>
> Extra new line.
Will fix.
>
>> +		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,
>> +};
>
> I don't want to go too much into reviewing these because I don't have
> facilities to run this code. Overrally it looks good to me (not same
> as reviewed-by).

Sure. Will include the feedbacks in my next version of patches.

Thanks & Regards,
   - Nayna

>
>> diff --git a/drivers/char/tpm/tpm_eventlog_init.c b/drivers/char/tpm/tpm_eventlog_init.c
>> index c4ac42630..aaac0e9 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,
>> @@ -114,7 +115,11 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>>   	chip->bios_dir_count++;
>>
>>   	chip->bin_sfs_data.log = &chip->log;
>> -	chip->bin_sfs_data.seqops = &tpm_binary_b_measurments_seqops;
>> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>> +		chip->bin_sfs_data.seqops = &tpm2_binary_b_measurments_seqops;
>> +	else
>> +		chip->bin_sfs_data.seqops = &tpm_binary_b_measurments_seqops;
>> +
>>
>>   	chip->bios_dir[chip->bios_dir_count] =
>>   	    securityfs_create_file("binary_bios_measurements",
>> @@ -125,16 +130,19 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>>   		goto err;
>>   	chip->bios_dir_count++;
>>
>> -	chip->ascii_sfs_data.log = &chip->log;
>> -	chip->ascii_sfs_data.seqops =  &tpm_ascii_b_measurments_seqops;
>> -	chip->bios_dir[chip->bios_dir_count] =
>> -	    securityfs_create_file("ascii_bios_measurements",
>> -				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
>> -				   (void *)&chip->ascii_sfs_data,
>> -				   &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->ascii_sfs_data.log = &chip->log;
>> +		chip->ascii_sfs_data.seqops =
>> +			&tpm_ascii_b_measurments_seqops;
>> +		chip->bios_dir[chip->bios_dir_count] =
>> +			securityfs_create_file("ascii_bios_measurements",
>> +				       S_IRUSR | S_IRGRP, chip->bios_dir[0],
>> +				       (void *)&chip->ascii_sfs_data,
>> +				       &tpm_bios_measurements_ops);
>> +		if (is_bad(chip->bios_dir[chip->bios_dir_count]))
>> +			goto err;
>> +		chip->bios_dir_count++;
>> +	}
>>
>>   	return 0;
>>
>> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
>> index 1464cae..d4151b5 100644
>> --- a/drivers/char/tpm/tpm_of.c
>> +++ b/drivers/char/tpm/tpm_of.c
>> @@ -17,6 +17,7 @@
>>
>>   #include <linux/slab.h>
>>   #include <linux/of.h>
>> +#include <linux/string.h>
>>
>>   #include "tpm.h"
>>   #include "tpm_eventlog.h"
>> @@ -27,6 +28,7 @@ int read_log_of(struct tpm_chip *chip)
>>   	const u32 *sizep;
>>   	const u64 *basep;
>>   	struct tpm_bios_log *log;
>> +	u32 log_size;
>>
>>   	log = &chip->log;
>>   	if (chip->dev.parent->of_node)
>> @@ -46,19 +48,37 @@ int read_log_of(struct tpm_chip *chip)
>>   		return -EIO;
>>   	}
>>
>> +	/*
>> +	 * For both vtpm/tpm, firmware has log addr and log size in big
>> +	 * endian format. But in case of vtpm, there is a method called
>> +	 * sml-handover which is run during kernel init even before
>> +	 * device tree is setup. This sml-handover function takes care
>> +	 * of endianness and writes to sml-base and sml-size in little
>> +	 * endian format. For this reason, vtpm doesn't need conversion
>> +	 * but physical tpm needs the conversion.
>> +	 */
>> +	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0)
>> +		log_size = be32_to_cpup(sizep);
>> +	else
>> +		log_size = *sizep;
>> +
>>   	basep = of_get_property(np, "linux,sml-base", NULL);
>>   	if (basep == NULL) {
>>   		dev_dbg(&chip->dev, "%s: ERROR - SML not found\n", __func__);
>>   		return -EIO;
>>   	}
>>
>> -	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
>> +	log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
>>   	if (!log->bios_event_log)
>>   		return -ENOMEM;
>>
>> -	log->bios_event_log_end = log->bios_event_log + *sizep;
>> +	log->bios_event_log_end = log->bios_event_log + log_size;
>>
>> -	memcpy(log->bios_event_log, __va(*basep), *sizep);
>> +	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0)
>> +		memcpy(chip->log.bios_event_log, __va(be64_to_cpup(basep)),
>> +		       log_size);
>> +	else
>> +		memcpy(chip->log.bios_event_log, __va(*basep), log_size);
>>
>>   	return 0;
>>   }
>> --
>> 2.5.0
>>
>
> /Jarkko
>


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

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

[Jarkko Sakkinen]

HI

On Sun, Oct 09, 2016 at 07:32:20AM +0530, Nayna wrote:
> 
> 
> On 10/01/2016 05:21 PM, Jarkko Sakkinen wrote:
> >On Wed, Sep 28, 2016 at 04:34:42AM -0400, Nayna Jain wrote:
> >>Unlike the device driver support for TPM 1.2, the TPM 2.0 support
> >>does not create the securityfs pseudo files for displaying the
> >>firmware event log.
> >>
> >>This patch enables support for providing the TPM 2.0 event log in
> >>binary form. TPM 2.0 event log supports a crypto agile format that
> >>records multiple digests, which is different from TPM 1.2. This
> >>patch adds the TPM 2.0 event log parser to understand the crypto
> >>agile format.
> >
> >I'll got through the patch (the split comment is valid)
> 
> Sure, will do it.

Please go through this series and see if they look good to you:

  https://lkml.org/lkml/2016/10/1/121

You should have them in your email archive (posted a week ago).

/Jarkko

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

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

[Nayna]

On 10/09/2016 02:44 PM, Jarkko Sakkinen wrote:
> HI
>
> On Sun, Oct 09, 2016 at 07:32:20AM +0530, Nayna wrote:
>>
>>
>> On 10/01/2016 05:21 PM, Jarkko Sakkinen wrote:
>>> On Wed, Sep 28, 2016 at 04:34:42AM -0400, Nayna Jain wrote:
>>>> Unlike the device driver support for TPM 1.2, the TPM 2.0 support
>>>> does not create the securityfs pseudo files for displaying the
>>>> firmware event log.
>>>>
>>>> This patch enables support for providing the TPM 2.0 event log in
>>>> binary form. TPM 2.0 event log supports a crypto agile format that
>>>> records multiple digests, which is different from TPM 1.2. This
>>>> patch adds the TPM 2.0 event log parser to understand the crypto
>>>> agile format.
>>>
>>> I'll got through the patch (the split comment is valid)
>>
>> Sure, will do it.
>
> Please go through this series and see if they look good to you:
>
>    https://lkml.org/lkml/2016/10/1/121
>
> You should have them in your email archive (posted a week ago).

I went through this series, looks good to me, just a minor comment on 
Patch 2/3.

Thanks & Regards,
    - Nayna

>
> /Jarkko
>


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

Re: [PATCH v4 0/8] tpm: add the securityfs pseudo files support for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Wed, Sep 28, 2016 at 04:34:34AM -0400, Nayna Jain wrote:
> The TPM device driver defines ascii and binary methods for
> displaying the TPM 1.2 event log via securityfs files, which are
> needed for validating a TPM quote. The device driver for TPM 2.0
> does not have similar support for displaying the TPM 2.0
> event log. This patch set adds the support for displaying
> TPM 2.0 event log in binary format.
> 
> The parsing mechanism to display the TPM 2.0 event log in binary
> format is implemented as defined in the TPM 2.0 TCG specification[1].
> If the firmware event log support exists and is successfully read,
> the securityfs file is created to provide the event log in binary
> format for both the OF device tree and ACPI.
> 
> - Patches 1 - 6 clean up the code and fix pre-existing issues.
> - Patch 7 moves the common TPM 1.2 and 2.0 event log initialization
> functions to a new file named tpm_eventlog_init.c.
> - Patch 8 adds the support for creating securityfs files and for
> displaying the TPM 2.0 crypto agile event log in binary format.
> 
> With that, this patch set splits the event log functionality into
> initialization functions, TPM 1.2 event log parsing and TPM 2.0 event
> log parsing.
> 
> [1] TCG EFI Protocol Specification, Family "2.0" - Section 5 "Event
> Log Structure"

Thanks, I'm sorry but I'll have to most probably postpone reviewing
for next week.

/Jarkko

> 
> Changelog History:
> 
> v4:
> 
> - Includes feedbacks from Jarkko and Jason.
> - Patch "tpm: define a generic open() method for ascii & bios
> measurements".
>   - Fix indentation issue.
> - Patch "tpm: replace the dynamically allocated bios_dir as
> struct dentry array".
>   - Continue to use bios_dir_count variable to use is_bad() checks and
>   to maintain correct order for securityfs_remove() during teardown.
>   - Reset chip->bios_dir_count in teardown() function.
> - Patch "tpm: validate the eventlog access before tpm_bios_log_setup".
>   - Retain TPM2 check which was removed in previous patch.
>   - Add tpm_bios_log_setup failure handling.
>   - Remove use of private data from v3 version of patch. Add a new 
>   member to struct tpm_chip to achieve the same purpose.
> - Patch "tpm: redefine the read_log method to check for ACPI/OF 
> properties sequentially".
>   - Move replacement of CONFIG_TCG_IBMVTPM with CONFIG_OF to this
>   patch from patch 3.
>   - Replace -1 error code with -ENODEV.
>   - Patch "tpm: replace the of_find_node_by_name() with dev of_node 
>   property".
>   - Uses chip->dev.parent->of_node.
>   - Created separate patch for cleanup of pr_err messages.
> - Patch "tpm: remove printk error messages".
>   - New Patch.
> - Patch "tpm: add the securityfs file support for TPM 2.0 eventlog".
>   - Parses event digests using event alg_id rather than event log header 
>   alg_id.
>   - Uses of_property_match_string to differentiate tpm/vtpm compatible 
>   property.
>   - Adds the comment for difference in tpm/vtpm endianness.
> 
> v3:
> 
> - Includes the review feedbacks as suggested by Jason.
> - Split of patches into one patch per idea.
> - Generic open() method for ascii/bios measurements.
> - Replacement of of **bios_dir with *bios_dir[3].
> - Verifying readlog() is successful before creating securityfs entries.
> - Generic readlog() to check for ACPI/OF in sequence.
> - read_log_of() method now uses of_node propertry rather than
> calling find_device_by_name.
> - read_log differentiates vtpm/tpm using its compatible property.
> - Cleans pr_err with dev_dbg.
> - Commit msgs subject line prefixed with tpm.
> 
> v2:
> 
> - Fixes issues as given in feedback by Jason.
> - Adds documentation for device tree.
> 
> Nayna Jain (8):
>   tpm: define a generic open() method for ascii & bios measurements
>   tpm: replace dynamically allocated bios_dir with dentry array
>   tpm: validate event log access before tpm_bios_log_setup
>   tpm: redefine read_log() to handle ACPI/OF at runtime
>   tpm: replace of_find_node_by_name() with dev of_node property
>   tpm: remove printk error messages
>   tpm: move event log init functions to tpm_eventlog_init.c
>   tpm: add securityfs support for TPM 2.0 firmware event log
> 
>  drivers/char/tpm/Makefile            |  14 +--
>  drivers/char/tpm/tpm-chip.c          |  24 ++--
>  drivers/char/tpm/tpm.h               |   9 +-
>  drivers/char/tpm/tpm2.h              |  79 +++++++++++++
>  drivers/char/tpm/tpm2_eventlog.c     | 216 +++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_acpi.c          |  36 +++---
>  drivers/char/tpm/tpm_eventlog.c      | 157 +------------------------
>  drivers/char/tpm/tpm_eventlog.h      |  30 +++--
>  drivers/char/tpm/tpm_eventlog_init.c | 163 ++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_of.c            |  68 ++++++-----
>  10 files changed, 559 insertions(+), 237 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
> 

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