[tpmdd-devel,v4,3/8] tpm: validate event log access before tpm_bios_log_setup

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@obsidianresearch.com>
Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 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(-)

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@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>

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

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@obsidianresearch.com>
> > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> 
> 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

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@obsidianresearch.com>
> > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > 
> > 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

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

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

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@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> ---
>  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

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@obsidianresearch.com>
> > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > ---
> >  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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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@obsidianresearch.com>
>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>> ---
>>   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

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

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@obsidianresearch.com>
>>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>>> ---
>>>    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@lists.sourceforge.net
> 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