diff mbox

[v5,4/7] tpm: fix the race condition between event log access and chip getting unregistered

Message ID 1476838185-24007-5-git-send-email-nayna@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nayna Oct. 19, 2016, 12:49 a.m. UTC
Currently, the event log file operations are not serialized with
tpm_chip_unregister(), which can possibly cause a race condition.

This patch fixes the race condition by:
 - moving read_log() from fops to chip register.
 - disallowing event log file operations when chip unregister is in
   progress.
 - guarding event log memory using chip krefs.

Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm-chip.c     |  1 +
 drivers/char/tpm/tpm.h          | 11 ++++++
 drivers/char/tpm/tpm_acpi.c     | 12 +++++--
 drivers/char/tpm/tpm_eventlog.c | 80 ++++++++++++++++++++++++++---------------
 drivers/char/tpm/tpm_eventlog.h |  2 +-
 drivers/char/tpm/tpm_of.c       |  4 ++-
 6 files changed, 76 insertions(+), 34 deletions(-)

Comments

Jarkko Sakkinen Oct. 19, 2016, 4:13 p.m. UTC | #1
On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> Currently, the event log file operations are not serialized with
> tpm_chip_unregister(), which can possibly cause a race condition.
> 
> This patch fixes the race condition by:
>  - moving read_log() from fops to chip register.
>  - disallowing event log file operations when chip unregister is in
>    progress.
>  - guarding event log memory using chip krefs.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>

I cannot promise yet that I have time to properly review this before
LPC but I'll try to find time to test this and apply to the eventlog
branch.

/Jarkko

> ---
>  drivers/char/tpm/tpm-chip.c     |  1 +
>  drivers/char/tpm/tpm.h          | 11 ++++++
>  drivers/char/tpm/tpm_acpi.c     | 12 +++++--
>  drivers/char/tpm/tpm_eventlog.c | 80 ++++++++++++++++++++++++++---------------
>  drivers/char/tpm/tpm_eventlog.h |  2 +-
>  drivers/char/tpm/tpm_of.c       |  4 ++-
>  6 files changed, 76 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index eac1f10..813e0d7 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev)
>  	idr_remove(&dev_nums_idr, chip->dev_num);
>  	mutex_unlock(&idr_lock);
>  
> +	kfree(chip->log.bios_event_log);
>  	kfree(chip);
>  }
>  
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 4c118a4..bfe93e6 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,
> @@ -146,6 +148,11 @@ enum tpm_chip_flags {
>  	TPM_CHIP_FLAG_VIRTUAL		= BIT(3),
>  };
>  
> +struct tpm_chip_seqops {
> +	struct tpm_chip *chip;
> +	const struct seq_operations *seqops;
> +};
> +
>  struct tpm_chip {
>  	struct device dev;
>  	struct cdev cdev;
> @@ -157,6 +164,10 @@ struct tpm_chip {
>  	struct rw_semaphore ops_sem;
>  	const struct tpm_class_ops *ops;
>  
> +	struct tpm_bios_log log;
> +	struct tpm_chip_seqops bin_log_seqops;
> +	struct tpm_chip_seqops ascii_log_seqops;
> +
>  	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 753e92d..bb142f2 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -73,7 +73,8 @@ static const char* tcpa_pc_event_id_strings[] = {
>  static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos)
>  {
>  	loff_t i;
> -	struct tpm_bios_log *log = m->private;
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
>  	void *addr = log->bios_event_log;
>  	void *limit = log->bios_event_log_end;
>  	struct tcpa_event *event;
> @@ -120,7 +121,8 @@ static void *tpm_bios_measurements_next(struct seq_file *m, void *v,
>  					loff_t *pos)
>  {
>  	struct tcpa_event *event = v;
> -	struct tpm_bios_log *log = m->private;
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
>  	void *limit = log->bios_event_log_end;
>  	u32 converted_event_size;
>  	u32 converted_event_type;
> @@ -261,13 +263,10 @@ 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;
> +	struct seq_file *seq = (struct seq_file *)file->private_data;
> +	struct tpm_chip *chip = (struct tpm_chip *)seq->private;
>  
> -	if (log) {
> -		kfree(log->bios_event_log);
> -		kfree(log);
> -	}
> +	put_device(&chip->dev);
>  
>  	return seq_release(inode, file);
>  }
> @@ -323,36 +322,34 @@ 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;
> -
> -	if ((err = read_log(log)))
> -		goto out_free;
> +	struct tpm_chip_seqops *chip_seqops;
> +	const struct seq_operations *seqops;
> +	struct tpm_chip *chip;
> +
> +	inode_lock(inode);
> +	if (!inode->i_private) {
> +		inode_unlock(inode);
> +		return -ENODEV;
> +	}
> +	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
> +	seqops = chip_seqops->seqops;
> +	chip = chip_seqops->chip;
> +	get_device(&chip->dev);
>  
>  	/* now register seq file */
>  	err = seq_open(file, seqops);
>  	if (!err) {
>  		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> +		seq->private = chip;
>  	}
>  
> -out:
> +	inode_unlock(inode);
>  	return err;
> -out_free:
> -	kfree(log->bios_event_log);
> -	kfree(log);
> -	goto out;
>  }
>  
>  static const struct file_operations tpm_bios_measurements_ops = {
> +	.owner = THIS_MODULE,
>  	.open = tpm_bios_measurements_open,
>  	.read = seq_read,
>  	.llseek = seq_lseek,
> @@ -372,10 +369,22 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
>  	unsigned int cnt;
> +	int rc = 0;
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return 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;
> +	}
> +
>  	cnt = 0;
>  	chip->bios_dir[cnt] =
>  		securityfs_create_dir(name, NULL);
> @@ -383,19 +392,25 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  		goto err;
>  	cnt++;
>  
> +	chip->bin_log_seqops.chip = chip;
> +	chip->bin_log_seqops.seqops = &tpm_binary_b_measurments_seqops;
> +
>  	chip->bios_dir[cnt] =
>  	    securityfs_create_file("binary_bios_measurements",
>  				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_binary_b_measurments_seqops,
> +				   (void *)&chip->bin_log_seqops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[cnt]))
>  		goto err;
>  	cnt++;
>  
> +	chip->ascii_log_seqops.chip = chip;
> +	chip->ascii_log_seqops.seqops = &tpm_ascii_b_measurments_seqops;
> +
>  	chip->bios_dir[cnt] =
>  	    securityfs_create_file("ascii_bios_measurements",
>  				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_ascii_b_measurments_seqops,
> +				   (void *)&chip->ascii_log_seqops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[cnt]))
>  		goto err;
> @@ -412,7 +427,14 @@ err:
>  void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  	int i;
> +	struct inode *inode;
>  
> -	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
> +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> +		inode = d_inode(chip->bios_dir[i]);
> +		inode_lock(inode);
> +		inode->i_private = NULL;
> +		inode_unlock(inode);
>  		securityfs_remove(chip->bios_dir[i]);
> +	}
> +
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index fd3357e..6df2f8e 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -73,7 +73,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
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jason Gunthorpe Oct. 20, 2016, 8:28 p.m. UTC | #2
On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> +	struct tpm_chip_seqops *chip_seqops;
> +	const struct seq_operations *seqops;
> +	struct tpm_chip *chip;
> +
> +	inode_lock(inode);
> +	if (!inode->i_private) {
> +		inode_unlock(inode);
> +		return -ENODEV;
> +	}
> +	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
> +	seqops = chip_seqops->seqops;
> +	chip = chip_seqops->chip;
> +	get_device(&chip->dev);

The inode_unlock should be here, not further below - reduces the risk of
deadlocking.

> +	rc = read_log(chip);
> +	/*
> +	 * read_log failure means event log is not supported except for ENOMEM
> +	 */
> +	if (rc < 0) {
> +		if (rc == -ENOMEM)

It would be more consistent to use -ENODEV as indicating no support,
and everything else is pass as an error

Rest looks right 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
Jarkko Sakkinen Nov. 4, 2016, 5:14 a.m. UTC | #3
On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> Currently, the event log file operations are not serialized with
> tpm_chip_unregister(), which can possibly cause a race condition.
> 
> This patch fixes the race condition by:
>  - moving read_log() from fops to chip register.

What is "chip register"? Please use exact names.

>  - disallowing event log file operations when chip unregister is in
>    progress.

Could you elaborate this sentence?

>  - guarding event log memory using chip krefs.

Could you elaborate this sentence?

Please describe how the race condition could happen and provide the
"Fixes:" line for the commit ID that caused it. Otherwise, your commit
message won't make any sense. I cannot apply this commit with this
commit message.

The commit message does not make much sense...

> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm-chip.c     |  1 +
>  drivers/char/tpm/tpm.h          | 11 ++++++
>  drivers/char/tpm/tpm_acpi.c     | 12 +++++--
>  drivers/char/tpm/tpm_eventlog.c | 80 ++++++++++++++++++++++++++---------------
>  drivers/char/tpm/tpm_eventlog.h |  2 +-
>  drivers/char/tpm/tpm_of.c       |  4 ++-
>  6 files changed, 76 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index eac1f10..813e0d7 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev)
>  	idr_remove(&dev_nums_idr, chip->dev_num);
>  	mutex_unlock(&idr_lock);
>  
> +	kfree(chip->log.bios_event_log);
>  	kfree(chip);
>  }
>  
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 4c118a4..bfe93e6 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,
> @@ -146,6 +148,11 @@ enum tpm_chip_flags {
>  	TPM_CHIP_FLAG_VIRTUAL		= BIT(3),
>  };
>  
> +struct tpm_chip_seqops {
> +	struct tpm_chip *chip;
> +	const struct seq_operations *seqops;
> +};
> +
>  struct tpm_chip {
>  	struct device dev;
>  	struct cdev cdev;
> @@ -157,6 +164,10 @@ struct tpm_chip {
>  	struct rw_semaphore ops_sem;
>  	const struct tpm_class_ops *ops;
>  
> +	struct tpm_bios_log log;
> +	struct tpm_chip_seqops bin_log_seqops;
> +	struct tpm_chip_seqops ascii_log_seqops;
> +
>  	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);

Is this kfree() necessary?

> +	return -EIO;
> +
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index 753e92d..bb142f2 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -73,7 +73,8 @@ static const char* tcpa_pc_event_id_strings[] = {
>  static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos)
>  {
>  	loff_t i;
> -	struct tpm_bios_log *log = m->private;
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
>  	void *addr = log->bios_event_log;
>  	void *limit = log->bios_event_log_end;
>  	struct tcpa_event *event;
> @@ -120,7 +121,8 @@ static void *tpm_bios_measurements_next(struct seq_file *m, void *v,
>  					loff_t *pos)
>  {
>  	struct tcpa_event *event = v;
> -	struct tpm_bios_log *log = m->private;
> +	struct tpm_chip *chip = m->private;
> +	struct tpm_bios_log *log = &chip->log;
>  	void *limit = log->bios_event_log_end;
>  	u32 converted_event_size;
>  	u32 converted_event_type;
> @@ -261,13 +263,10 @@ 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;
> +	struct seq_file *seq = (struct seq_file *)file->private_data;
> +	struct tpm_chip *chip = (struct tpm_chip *)seq->private;
>  
> -	if (log) {
> -		kfree(log->bios_event_log);
> -		kfree(log);
> -	}
> +	put_device(&chip->dev);
>  
>  	return seq_release(inode, file);
>  }
> @@ -323,36 +322,34 @@ 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;
> -
> -	if ((err = read_log(log)))
> -		goto out_free;
> +	struct tpm_chip_seqops *chip_seqops;
> +	const struct seq_operations *seqops;
> +	struct tpm_chip *chip;
> +
> +	inode_lock(inode);
> +	if (!inode->i_private) {
> +		inode_unlock(inode);
> +		return -ENODEV;
> +	}

Why is this is done (inode_lock + setting to NULL), and if so, why
it wasn't done in 1/7?

> +	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
> +	seqops = chip_seqops->seqops;
> +	chip = chip_seqops->chip;
> +	get_device(&chip->dev);

If I recall the last time I looked at this patch (which was few weeks
ago) this was needed because securityfs did not have proper fencing.

tpm_bios_log_teardown() is called before tpm_del_char_device() but this
won't help because callbacks could be in action after
securityfs_remove().

Please put a comment here explaining why this get_device() is done so
that it is easy to backtrack when changing this part of the driver later
on.

>  	/* now register seq file */
>  	err = seq_open(file, seqops);
>  	if (!err) {
>  		seq = file->private_data;
> -		seq->private = log;
> -	} else {
> -		goto out_free;
> +		seq->private = chip;
>  	}
>  
> -out:
> +	inode_unlock(inode);
>  	return err;
> -out_free:
> -	kfree(log->bios_event_log);
> -	kfree(log);
> -	goto out;
>  }
>  
>  static const struct file_operations tpm_bios_measurements_ops = {
> +	.owner = THIS_MODULE,

Should be a separate commit event though it is a one line change.
Please split the commit into two.

>  	.open = tpm_bios_measurements_open,
>  	.read = seq_read,
>  	.llseek = seq_lseek,
> @@ -372,10 +369,22 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  {
>  	const char *name = dev_name(&chip->dev);
>  	unsigned int cnt;
> +	int rc = 0;
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return 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;
> +	}
> +
>  	cnt = 0;
>  	chip->bios_dir[cnt] =
>  		securityfs_create_dir(name, NULL);
> @@ -383,19 +392,25 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
>  		goto err;
>  	cnt++;
>  
> +	chip->bin_log_seqops.chip = chip;
> +	chip->bin_log_seqops.seqops = &tpm_binary_b_measurments_seqops;
> +
>  	chip->bios_dir[cnt] =
>  	    securityfs_create_file("binary_bios_measurements",
>  				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_binary_b_measurments_seqops,
> +				   (void *)&chip->bin_log_seqops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[cnt]))
>  		goto err;
>  	cnt++;
>  
> +	chip->ascii_log_seqops.chip = chip;
> +	chip->ascii_log_seqops.seqops = &tpm_ascii_b_measurments_seqops;
> +
>  	chip->bios_dir[cnt] =
>  	    securityfs_create_file("ascii_bios_measurements",
>  				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
> -				   (void *)&tpm_ascii_b_measurments_seqops,
> +				   (void *)&chip->ascii_log_seqops,
>  				   &tpm_bios_measurements_ops);
>  	if (is_bad(chip->bios_dir[cnt]))
>  		goto err;
> @@ -412,7 +427,14 @@ err:
>  void tpm_bios_log_teardown(struct tpm_chip *chip)
>  {
>  	int i;
> +	struct inode *inode;
>  
> -	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
> +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> +		inode = d_inode(chip->bios_dir[i]);
> +		inode_lock(inode);
> +		inode->i_private = NULL;
> +		inode_unlock(inode);

Why is this is done (inode_lock + setting to NULL), and if so, why
it wasn't done in 1/7?

>  		securityfs_remove(chip->bios_dir[i]);
> +	}
> +
>  }
> diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
> index fd3357e..6df2f8e 100644
> --- a/drivers/char/tpm/tpm_eventlog.h
> +++ b/drivers/char/tpm/tpm_eventlog.h
> @@ -73,7 +73,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

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
Jason Gunthorpe Nov. 7, 2016, 11:48 p.m. UTC | #4
On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > Currently, the event log file operations are not serialized with
> > tpm_chip_unregister(), which can possibly cause a race condition.
> > 
> > This patch fixes the race condition by:
> >  - moving read_log() from fops to chip register.
> 
> What is "chip register"? Please use exact names.
> 
> >  - disallowing event log file operations when chip unregister is in
> >    progress.
> 
> Could you elaborate this sentence?
> 
> >  - guarding event log memory using chip krefs.
> 
> Could you elaborate this sentence?
> 
> Please describe how the race condition could happen and provide the
> "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> message won't make any sense. I cannot apply this commit with this
> commit message.
> 
> The commit message does not make much sense...

Lets get this moving along, it is hard to keep everything straight
over months..

Nayna: This commit message should work:

tpm: Have eventlog use the tpm_chip

Move the backing memory for the eventlog into tpm_chip and push
the tpm_chip into read_log. This optimizes read_log processing by
only doing it once and prepares things for the next patches in the
series which require the tpm_chip to locate the event log via
ACPI and OF handles instead of searching.

This is straightfoward except for the issue of passing a kref through
i_private with securityfs. Since securityfs_remove does not have any
removal fencing like sysfs we use the inode lock to safely get a
kref on the tpm_chip.


> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index eac1f10..813e0d7 100644
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev)
> >  	idr_remove(&dev_nums_idr, chip->dev_num);
> >  	mutex_unlock(&idr_lock);
> >  
> > +	kfree(chip->log.bios_event_log);
> >  	kfree(chip);
> >  }
[..]
> >  /* read binary bios log */
> > -int read_log(struct tpm_bios_log *log)
> > +int read_log(struct tpm_chip *chip)
[..]
> > +err:
> > +	kfree(log->bios_event_log);
> 
> Is this kfree() necessary?

Yes, if the log is not loaded then bios_event_log log should be null.

Nayna - there is a bug here, you have to null bios_event_log after
kfree'ing it so that tpm_dev_release does not attempt to kfree a
free'd pointer.

> > +	inode_lock(inode);
> > +	if (!inode->i_private) {
> > +		inode_unlock(inode);
> > +		return -ENODEV;
> > +	}
> 
> Why is this is done (inode_lock + setting to NULL), and if so, why
> it wasn't done in 1/7?

Patch 1 does not use memory backed by tpm_chip, this is the first
patch in the series that introduces that concept, so it is the first
place that needs to do this.

> > -	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
> > +	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> > +		inode = d_inode(chip->bios_dir[i]);
> > +		inode_lock(inode);
> > +		inode->i_private = NULL;
> > +		inode_unlock(inode);
> 
> Why is this is done (inode_lock + setting to NULL), and if so, why
> it wasn't done in 1/7?

I'm sure I've explained this before, but again, the inode lock is
a work around for lack of removal fencing in securityfs. We null the
i_private here during remove and test for null during open under this
lock.

This design ensures that open either safely gets a kref or fails.

Holding the kref as long as the securityfs file is open by userspace
ensures there are no use-after-frees in this code.

A comment to that effect would be good.

Jason

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
Jarkko Sakkinen Nov. 8, 2016, 12:26 a.m. UTC | #5
On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote:
> On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > > Currently, the event log file operations are not serialized with
> > > tpm_chip_unregister(), which can possibly cause a race condition.
> > > 
> > > This patch fixes the race condition by:
> > >  - moving read_log() from fops to chip register.
> > 
> > What is "chip register"? Please use exact names.
> > 
> > >  - disallowing event log file operations when chip unregister is in
> > >    progress.
> > 
> > Could you elaborate this sentence?
> > 
> > >  - guarding event log memory using chip krefs.
> > 
> > Could you elaborate this sentence?
> > 
> > Please describe how the race condition could happen and provide the
> > "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> > message won't make any sense. I cannot apply this commit with this
> > commit message.
> > 
> > The commit message does not make much sense...
> 
> Lets get this moving along, it is hard to keep everything straight
> over months..
> 
> Nayna: This commit message should work:
> 
> tpm: Have eventlog use the tpm_chip
> 
> Move the backing memory for the eventlog into tpm_chip and push
> the tpm_chip into read_log. This optimizes read_log processing by
> only doing it once and prepares things for the next patches in the
> series which require the tpm_chip to locate the event log via
> ACPI and OF handles instead of searching.
> 
> This is straightfoward except for the issue of passing a kref through
> i_private with securityfs. Since securityfs_remove does not have any
> removal fencing like sysfs we use the inode lock to safely get a
> kref on the tpm_chip.

Perfect. Thank you.

With this

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

No need for resend.

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
Jason Gunthorpe Nov. 8, 2016, 12:29 a.m. UTC | #6
On Mon, Nov 07, 2016 at 04:26:43PM -0800, Jarkko Sakkinen wrote:
> On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote:
> > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > > > Currently, the event log file operations are not serialized with
> > > > tpm_chip_unregister(), which can possibly cause a race condition.
> > > > 
> > > > This patch fixes the race condition by:
> > > >  - moving read_log() from fops to chip register.
> > > 
> > > What is "chip register"? Please use exact names.
> > > 
> > > >  - disallowing event log file operations when chip unregister is in
> > > >    progress.
> > > 
> > > Could you elaborate this sentence?
> > > 
> > > >  - guarding event log memory using chip krefs.
> > > 
> > > Could you elaborate this sentence?
> > > 
> > > Please describe how the race condition could happen and provide the
> > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> > > message won't make any sense. I cannot apply this commit with this
> > > commit message.
> > > 
> > > The commit message does not make much sense...
> > 
> > Lets get this moving along, it is hard to keep everything straight
> > over months..
> > 
> > Nayna: This commit message should work:
> > 
> > tpm: Have eventlog use the tpm_chip
> > 
> > Move the backing memory for the eventlog into tpm_chip and push
> > the tpm_chip into read_log. This optimizes read_log processing by
> > only doing it once and prepares things for the next patches in the
> > series which require the tpm_chip to locate the event log via
> > ACPI and OF handles instead of searching.
> > 
> > This is straightfoward except for the issue of passing a kref through
> > i_private with securityfs. Since securityfs_remove does not have any
> > removal fencing like sysfs we use the inode lock to safely get a
> > kref on the tpm_chip.
> 
> Perfect. Thank you.
> 
> With this
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> No need for resend.

The missing null still needs to be fixed, and the inode lock needs to
be held only over get_device in open()

Jason

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
Jarkko Sakkinen Nov. 8, 2016, 12:34 a.m. UTC | #7
On Mon, Nov 07, 2016 at 04:26:42PM -0800, Jarkko Sakkinen wrote:
> On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote:
> > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > > > Currently, the event log file operations are not serialized with
> > > > tpm_chip_unregister(), which can possibly cause a race condition.
> > > > 
> > > > This patch fixes the race condition by:
> > > >  - moving read_log() from fops to chip register.
> > > 
> > > What is "chip register"? Please use exact names.
> > > 
> > > >  - disallowing event log file operations when chip unregister is in
> > > >    progress.
> > > 
> > > Could you elaborate this sentence?
> > > 
> > > >  - guarding event log memory using chip krefs.
> > > 
> > > Could you elaborate this sentence?
> > > 
> > > Please describe how the race condition could happen and provide the
> > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> > > message won't make any sense. I cannot apply this commit with this
> > > commit message.
> > > 
> > > The commit message does not make much sense...
> > 
> > Lets get this moving along, it is hard to keep everything straight
> > over months..
> > 
> > Nayna: This commit message should work:
> > 
> > tpm: Have eventlog use the tpm_chip
> > 
> > Move the backing memory for the eventlog into tpm_chip and push
> > the tpm_chip into read_log. This optimizes read_log processing by
> > only doing it once and prepares things for the next patches in the
> > series which require the tpm_chip to locate the event log via
> > ACPI and OF handles instead of searching.
> > 
> > This is straightfoward except for the issue of passing a kref through
> > i_private with securityfs. Since securityfs_remove does not have any
> > removal fencing like sysfs we use the inode lock to safely get a
> > kref on the tpm_chip.
> 
> Perfect. Thank you.
> 
> With this
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> No need for resend.

There actually *is* need for one more resend because the patces were not
CC'd to linux-security-module. I discussed about this at LPC with James
Morris and his take was that *major* changes should be CC'd to
linux-security-module. This is such a major set of changes.

If this series is sent one more time with CC to linux-kernel and
linux-security-module and the commit message is changed for this
commit from my part it is ready for to be applied. You can add my
Reviewed-by to this patch.

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
Jarkko Sakkinen Nov. 8, 2016, 12:48 a.m. UTC | #8
On Mon, Nov 07, 2016 at 05:29:43PM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 07, 2016 at 04:26:43PM -0800, Jarkko Sakkinen wrote:
> > On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote:
> > > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> > > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > > > > Currently, the event log file operations are not serialized with
> > > > > tpm_chip_unregister(), which can possibly cause a race condition.
> > > > > 
> > > > > This patch fixes the race condition by:
> > > > >  - moving read_log() from fops to chip register.
> > > > 
> > > > What is "chip register"? Please use exact names.
> > > > 
> > > > >  - disallowing event log file operations when chip unregister is in
> > > > >    progress.
> > > > 
> > > > Could you elaborate this sentence?
> > > > 
> > > > >  - guarding event log memory using chip krefs.
> > > > 
> > > > Could you elaborate this sentence?
> > > > 
> > > > Please describe how the race condition could happen and provide the
> > > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> > > > message won't make any sense. I cannot apply this commit with this
> > > > commit message.
> > > > 
> > > > The commit message does not make much sense...
> > > 
> > > Lets get this moving along, it is hard to keep everything straight
> > > over months..
> > > 
> > > Nayna: This commit message should work:
> > > 
> > > tpm: Have eventlog use the tpm_chip
> > > 
> > > Move the backing memory for the eventlog into tpm_chip and push
> > > the tpm_chip into read_log. This optimizes read_log processing by
> > > only doing it once and prepares things for the next patches in the
> > > series which require the tpm_chip to locate the event log via
> > > ACPI and OF handles instead of searching.
> > > 
> > > This is straightfoward except for the issue of passing a kref through
> > > i_private with securityfs. Since securityfs_remove does not have any
> > > removal fencing like sysfs we use the inode lock to safely get a
> > > kref on the tpm_chip.
> > 
> > Perfect. Thank you.
> > 
> > With this
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > No need for resend.
> 
> The missing null still needs to be fixed, and the inode lock needs to
> be held only over get_device in open()

Right, I'm sorry I forgot that... Another reason for one more resend.

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index eac1f10..813e0d7 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -127,6 +127,7 @@  static void tpm_dev_release(struct device *dev)
 	idr_remove(&dev_nums_idr, chip->dev_num);
 	mutex_unlock(&idr_lock);
 
+	kfree(chip->log.bios_event_log);
 	kfree(chip);
 }
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 4c118a4..bfe93e6 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,
@@ -146,6 +148,11 @@  enum tpm_chip_flags {
 	TPM_CHIP_FLAG_VIRTUAL		= BIT(3),
 };
 
+struct tpm_chip_seqops {
+	struct tpm_chip *chip;
+	const struct seq_operations *seqops;
+};
+
 struct tpm_chip {
 	struct device dev;
 	struct cdev cdev;
@@ -157,6 +164,10 @@  struct tpm_chip {
 	struct rw_semaphore ops_sem;
 	const struct tpm_class_ops *ops;
 
+	struct tpm_bios_log log;
+	struct tpm_chip_seqops bin_log_seqops;
+	struct tpm_chip_seqops ascii_log_seqops;
+
 	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 753e92d..bb142f2 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -73,7 +73,8 @@  static const char* tcpa_pc_event_id_strings[] = {
 static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos)
 {
 	loff_t i;
-	struct tpm_bios_log *log = m->private;
+	struct tpm_chip *chip = m->private;
+	struct tpm_bios_log *log = &chip->log;
 	void *addr = log->bios_event_log;
 	void *limit = log->bios_event_log_end;
 	struct tcpa_event *event;
@@ -120,7 +121,8 @@  static void *tpm_bios_measurements_next(struct seq_file *m, void *v,
 					loff_t *pos)
 {
 	struct tcpa_event *event = v;
-	struct tpm_bios_log *log = m->private;
+	struct tpm_chip *chip = m->private;
+	struct tpm_bios_log *log = &chip->log;
 	void *limit = log->bios_event_log_end;
 	u32 converted_event_size;
 	u32 converted_event_type;
@@ -261,13 +263,10 @@  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;
+	struct seq_file *seq = (struct seq_file *)file->private_data;
+	struct tpm_chip *chip = (struct tpm_chip *)seq->private;
 
-	if (log) {
-		kfree(log->bios_event_log);
-		kfree(log);
-	}
+	put_device(&chip->dev);
 
 	return seq_release(inode, file);
 }
@@ -323,36 +322,34 @@  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;
-
-	if ((err = read_log(log)))
-		goto out_free;
+	struct tpm_chip_seqops *chip_seqops;
+	const struct seq_operations *seqops;
+	struct tpm_chip *chip;
+
+	inode_lock(inode);
+	if (!inode->i_private) {
+		inode_unlock(inode);
+		return -ENODEV;
+	}
+	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
+	seqops = chip_seqops->seqops;
+	chip = chip_seqops->chip;
+	get_device(&chip->dev);
 
 	/* now register seq file */
 	err = seq_open(file, seqops);
 	if (!err) {
 		seq = file->private_data;
-		seq->private = log;
-	} else {
-		goto out_free;
+		seq->private = chip;
 	}
 
-out:
+	inode_unlock(inode);
 	return err;
-out_free:
-	kfree(log->bios_event_log);
-	kfree(log);
-	goto out;
 }
 
 static const struct file_operations tpm_bios_measurements_ops = {
+	.owner = THIS_MODULE,
 	.open = tpm_bios_measurements_open,
 	.read = seq_read,
 	.llseek = seq_lseek,
@@ -372,10 +369,22 @@  int tpm_bios_log_setup(struct tpm_chip *chip)
 {
 	const char *name = dev_name(&chip->dev);
 	unsigned int cnt;
+	int rc = 0;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return 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;
+	}
+
 	cnt = 0;
 	chip->bios_dir[cnt] =
 		securityfs_create_dir(name, NULL);
@@ -383,19 +392,25 @@  int tpm_bios_log_setup(struct tpm_chip *chip)
 		goto err;
 	cnt++;
 
+	chip->bin_log_seqops.chip = chip;
+	chip->bin_log_seqops.seqops = &tpm_binary_b_measurments_seqops;
+
 	chip->bios_dir[cnt] =
 	    securityfs_create_file("binary_bios_measurements",
 				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&tpm_binary_b_measurments_seqops,
+				   (void *)&chip->bin_log_seqops,
 				   &tpm_bios_measurements_ops);
 	if (is_bad(chip->bios_dir[cnt]))
 		goto err;
 	cnt++;
 
+	chip->ascii_log_seqops.chip = chip;
+	chip->ascii_log_seqops.seqops = &tpm_ascii_b_measurments_seqops;
+
 	chip->bios_dir[cnt] =
 	    securityfs_create_file("ascii_bios_measurements",
 				   S_IRUSR | S_IRGRP, chip->bios_dir[0],
-				   (void *)&tpm_ascii_b_measurments_seqops,
+				   (void *)&chip->ascii_log_seqops,
 				   &tpm_bios_measurements_ops);
 	if (is_bad(chip->bios_dir[cnt]))
 		goto err;
@@ -412,7 +427,14 @@  err:
 void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
 	int i;
+	struct inode *inode;
 
-	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
+	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
+		inode = d_inode(chip->bios_dir[i]);
+		inode_lock(inode);
+		inode->i_private = NULL;
+		inode_unlock(inode);
 		securityfs_remove(chip->bios_dir[i]);
+	}
+
 }
diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h
index fd3357e..6df2f8e 100644
--- a/drivers/char/tpm/tpm_eventlog.h
+++ b/drivers/char/tpm/tpm_eventlog.h
@@ -73,7 +73,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;