| Message ID | 1476838185-24007-5-git-send-email-nayna@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote: > Currently, the event log file operations are not serialized with > tpm_chip_unregister(), which can possibly cause a race condition. > > This patch fixes the race condition by: > - moving read_log() from fops to chip register. > - disallowing event log file operations when chip unregister is in > progress. > - guarding event log memory using chip krefs. > > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> I cannot promise yet that I have time to properly review this before LPC but I'll try to find time to test this and apply to the eventlog branch. /Jarkko > --- > drivers/char/tpm/tpm-chip.c | 1 + > drivers/char/tpm/tpm.h | 11 ++++++ > drivers/char/tpm/tpm_acpi.c | 12 +++++-- > drivers/char/tpm/tpm_eventlog.c | 80 ++++++++++++++++++++++++++--------------- > drivers/char/tpm/tpm_eventlog.h | 2 +- > drivers/char/tpm/tpm_of.c | 4 ++- > 6 files changed, 76 insertions(+), 34 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index eac1f10..813e0d7 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev) > idr_remove(&dev_nums_idr, chip->dev_num); > mutex_unlock(&idr_lock); > > + kfree(chip->log.bios_event_log); > kfree(chip); > } > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 4c118a4..bfe93e6 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -35,6 +35,8 @@ > #include <linux/cdev.h> > #include <linux/highmem.h> > > +#include "tpm_eventlog.h" > + > enum tpm_const { > TPM_MINOR = 224, /* officially assigned */ > TPM_BUFSIZE = 4096, > @@ -146,6 +148,11 @@ enum tpm_chip_flags { > TPM_CHIP_FLAG_VIRTUAL = BIT(3), > }; > > +struct tpm_chip_seqops { > + struct tpm_chip *chip; > + const struct seq_operations *seqops; > +}; > + > struct tpm_chip { > struct device dev; > struct cdev cdev; > @@ -157,6 +164,10 @@ struct tpm_chip { > struct rw_semaphore ops_sem; > const struct tpm_class_ops *ops; > > + struct tpm_bios_log log; > + struct tpm_chip_seqops bin_log_seqops; > + struct tpm_chip_seqops ascii_log_seqops; > + > unsigned int flags; > > int dev_num; /* /dev/tpm# */ > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c > index 565a947..4d6c2d7 100644 > --- a/drivers/char/tpm/tpm_acpi.c > +++ b/drivers/char/tpm/tpm_acpi.c > @@ -45,13 +45,15 @@ struct acpi_tcpa { > }; > > /* read binary bios log */ > -int read_log(struct tpm_bios_log *log) > +int read_log(struct tpm_chip *chip) > { > struct acpi_tcpa *buff; > acpi_status status; > void __iomem *virt; > u64 len, start; > + struct tpm_bios_log *log; > > + log = &chip->log; > if (log->bios_event_log != NULL) { > printk(KERN_ERR > "%s: ERROR - Eventlog already initialized\n", > @@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log) > > virt = acpi_os_map_iomem(start, len); > if (!virt) { > - kfree(log->bios_event_log); > printk("%s: ERROR - Unable to map memory\n", __func__); > - return -EIO; > + goto err; > } > > memcpy_fromio(log->bios_event_log, virt, len); > > acpi_os_unmap_iomem(virt, len); > return 0; > + > +err: > + kfree(log->bios_event_log); > + return -EIO; > + > } > diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c > index 753e92d..bb142f2 100644 > --- a/drivers/char/tpm/tpm_eventlog.c > +++ b/drivers/char/tpm/tpm_eventlog.c > @@ -73,7 +73,8 @@ static const char* tcpa_pc_event_id_strings[] = { > static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos) > { > loff_t i; > - struct tpm_bios_log *log = m->private; > + struct tpm_chip *chip = m->private; > + struct tpm_bios_log *log = &chip->log; > void *addr = log->bios_event_log; > void *limit = log->bios_event_log_end; > struct tcpa_event *event; > @@ -120,7 +121,8 @@ static void *tpm_bios_measurements_next(struct seq_file *m, void *v, > loff_t *pos) > { > struct tcpa_event *event = v; > - struct tpm_bios_log *log = m->private; > + struct tpm_chip *chip = m->private; > + struct tpm_bios_log *log = &chip->log; > void *limit = log->bios_event_log_end; > u32 converted_event_size; > u32 converted_event_type; > @@ -261,13 +263,10 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v) > static int tpm_bios_measurements_release(struct inode *inode, > struct file *file) > { > - struct seq_file *seq = file->private_data; > - struct tpm_bios_log *log = seq->private; > + struct seq_file *seq = (struct seq_file *)file->private_data; > + struct tpm_chip *chip = (struct tpm_chip *)seq->private; > > - if (log) { > - kfree(log->bios_event_log); > - kfree(log); > - } > + put_device(&chip->dev); > > return seq_release(inode, file); > } > @@ -323,36 +322,34 @@ static int tpm_bios_measurements_open(struct inode *inode, > struct file *file) > { > int err; > - struct tpm_bios_log *log; > struct seq_file *seq; > - const struct seq_operations *seqops = > - (const struct seq_operations *)inode->i_private; > - > - log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL); > - if (!log) > - return -ENOMEM; > - > - if ((err = read_log(log))) > - goto out_free; > + struct tpm_chip_seqops *chip_seqops; > + const struct seq_operations *seqops; > + struct tpm_chip *chip; > + > + inode_lock(inode); > + if (!inode->i_private) { > + inode_unlock(inode); > + return -ENODEV; > + } > + chip_seqops = (struct tpm_chip_seqops *)inode->i_private; > + seqops = chip_seqops->seqops; > + chip = chip_seqops->chip; > + get_device(&chip->dev); > > /* now register seq file */ > err = seq_open(file, seqops); > if (!err) { > seq = file->private_data; > - seq->private = log; > - } else { > - goto out_free; > + seq->private = chip; > } > > -out: > + inode_unlock(inode); > return err; > -out_free: > - kfree(log->bios_event_log); > - kfree(log); > - goto out; > } > > static const struct file_operations tpm_bios_measurements_ops = { > + .owner = THIS_MODULE, > .open = tpm_bios_measurements_open, > .read = seq_read, > .llseek = seq_lseek, > @@ -372,10 +369,22 @@ int tpm_bios_log_setup(struct tpm_chip *chip) > { > const char *name = dev_name(&chip->dev); > unsigned int cnt; > + int rc = 0; > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > return 0; > > + rc = read_log(chip); > + /* > + * read_log failure means event log is not supported except for ENOMEM > + */ > + if (rc < 0) { > + if (rc == -ENOMEM) > + return rc; > + else > + return 0; > + } > + > cnt = 0; > chip->bios_dir[cnt] = > securityfs_create_dir(name, NULL); > @@ -383,19 +392,25 @@ int tpm_bios_log_setup(struct tpm_chip *chip) > goto err; > cnt++; > > + chip->bin_log_seqops.chip = chip; > + chip->bin_log_seqops.seqops = &tpm_binary_b_measurments_seqops; > + > chip->bios_dir[cnt] = > securityfs_create_file("binary_bios_measurements", > S_IRUSR | S_IRGRP, chip->bios_dir[0], > - (void *)&tpm_binary_b_measurments_seqops, > + (void *)&chip->bin_log_seqops, > &tpm_bios_measurements_ops); > if (is_bad(chip->bios_dir[cnt])) > goto err; > cnt++; > > + chip->ascii_log_seqops.chip = chip; > + chip->ascii_log_seqops.seqops = &tpm_ascii_b_measurments_seqops; > + > chip->bios_dir[cnt] = > securityfs_create_file("ascii_bios_measurements", > S_IRUSR | S_IRGRP, chip->bios_dir[0], > - (void *)&tpm_ascii_b_measurments_seqops, > + (void *)&chip->ascii_log_seqops, > &tpm_bios_measurements_ops); > if (is_bad(chip->bios_dir[cnt])) > goto err; > @@ -412,7 +427,14 @@ err: > void tpm_bios_log_teardown(struct tpm_chip *chip) > { > int i; > + struct inode *inode; > > - for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) > + for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > + inode = d_inode(chip->bios_dir[i]); > + inode_lock(inode); > + inode->i_private = NULL; > + inode_unlock(inode); > securityfs_remove(chip->bios_dir[i]); > + } > + > } > diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h > index fd3357e..6df2f8e 100644 > --- a/drivers/char/tpm/tpm_eventlog.h > +++ b/drivers/char/tpm/tpm_eventlog.h > @@ -73,7 +73,7 @@ enum tcpa_pc_event_ids { > HOST_TABLE_OF_DEVICES, > }; > > -int read_log(struct tpm_bios_log *log); > +int read_log(struct tpm_chip *chip); > > #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \ > defined(CONFIG_ACPI) > diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c > index 570f30c..68d891a 100644 > --- a/drivers/char/tpm/tpm_of.c > +++ b/drivers/char/tpm/tpm_of.c > @@ -20,12 +20,14 @@ > #include "tpm.h" > #include "tpm_eventlog.h" > > -int read_log(struct tpm_bios_log *log) > +int read_log(struct tpm_chip *chip) > { > struct device_node *np; > const u32 *sizep; > const u64 *basep; > + struct tpm_bios_log *log; > > + log = &chip->log; > if (log->bios_event_log != NULL) { > pr_err("%s: ERROR - Eventlog already initialized\n", __func__); > return -EFAULT; > -- > 2.5.0 > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote: > + struct tpm_chip_seqops *chip_seqops; > + const struct seq_operations *seqops; > + struct tpm_chip *chip; > + > + inode_lock(inode); > + if (!inode->i_private) { > + inode_unlock(inode); > + return -ENODEV; > + } > + chip_seqops = (struct tpm_chip_seqops *)inode->i_private; > + seqops = chip_seqops->seqops; > + chip = chip_seqops->chip; > + get_device(&chip->dev); The inode_unlock should be here, not further below - reduces the risk of deadlocking. > + rc = read_log(chip); > + /* > + * read_log failure means event log is not supported except for ENOMEM > + */ > + if (rc < 0) { > + if (rc == -ENOMEM) It would be more consistent to use -ENODEV as indicating no support, and everything else is pass as an error Rest looks right to me Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote: > Currently, the event log file operations are not serialized with > tpm_chip_unregister(), which can possibly cause a race condition. > > This patch fixes the race condition by: > - moving read_log() from fops to chip register. What is "chip register"? Please use exact names. > - disallowing event log file operations when chip unregister is in > progress. Could you elaborate this sentence? > - guarding event log memory using chip krefs. Could you elaborate this sentence? Please describe how the race condition could happen and provide the "Fixes:" line for the commit ID that caused it. Otherwise, your commit message won't make any sense. I cannot apply this commit with this commit message. The commit message does not make much sense... > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> > --- > drivers/char/tpm/tpm-chip.c | 1 + > drivers/char/tpm/tpm.h | 11 ++++++ > drivers/char/tpm/tpm_acpi.c | 12 +++++-- > drivers/char/tpm/tpm_eventlog.c | 80 ++++++++++++++++++++++++++--------------- > drivers/char/tpm/tpm_eventlog.h | 2 +- > drivers/char/tpm/tpm_of.c | 4 ++- > 6 files changed, 76 insertions(+), 34 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index eac1f10..813e0d7 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev) > idr_remove(&dev_nums_idr, chip->dev_num); > mutex_unlock(&idr_lock); > > + kfree(chip->log.bios_event_log); > kfree(chip); > } > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 4c118a4..bfe93e6 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -35,6 +35,8 @@ > #include <linux/cdev.h> > #include <linux/highmem.h> > > +#include "tpm_eventlog.h" > + > enum tpm_const { > TPM_MINOR = 224, /* officially assigned */ > TPM_BUFSIZE = 4096, > @@ -146,6 +148,11 @@ enum tpm_chip_flags { > TPM_CHIP_FLAG_VIRTUAL = BIT(3), > }; > > +struct tpm_chip_seqops { > + struct tpm_chip *chip; > + const struct seq_operations *seqops; > +}; > + > struct tpm_chip { > struct device dev; > struct cdev cdev; > @@ -157,6 +164,10 @@ struct tpm_chip { > struct rw_semaphore ops_sem; > const struct tpm_class_ops *ops; > > + struct tpm_bios_log log; > + struct tpm_chip_seqops bin_log_seqops; > + struct tpm_chip_seqops ascii_log_seqops; > + > unsigned int flags; > > int dev_num; /* /dev/tpm# */ > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c > index 565a947..4d6c2d7 100644 > --- a/drivers/char/tpm/tpm_acpi.c > +++ b/drivers/char/tpm/tpm_acpi.c > @@ -45,13 +45,15 @@ struct acpi_tcpa { > }; > > /* read binary bios log */ > -int read_log(struct tpm_bios_log *log) > +int read_log(struct tpm_chip *chip) > { > struct acpi_tcpa *buff; > acpi_status status; > void __iomem *virt; > u64 len, start; > + struct tpm_bios_log *log; > > + log = &chip->log; > if (log->bios_event_log != NULL) { > printk(KERN_ERR > "%s: ERROR - Eventlog already initialized\n", > @@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log) > > virt = acpi_os_map_iomem(start, len); > if (!virt) { > - kfree(log->bios_event_log); > printk("%s: ERROR - Unable to map memory\n", __func__); > - return -EIO; > + goto err; > } > > memcpy_fromio(log->bios_event_log, virt, len); > > acpi_os_unmap_iomem(virt, len); > return 0; > + > +err: > + kfree(log->bios_event_log); Is this kfree() necessary? > + return -EIO; > + > } > diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c > index 753e92d..bb142f2 100644 > --- a/drivers/char/tpm/tpm_eventlog.c > +++ b/drivers/char/tpm/tpm_eventlog.c > @@ -73,7 +73,8 @@ static const char* tcpa_pc_event_id_strings[] = { > static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos) > { > loff_t i; > - struct tpm_bios_log *log = m->private; > + struct tpm_chip *chip = m->private; > + struct tpm_bios_log *log = &chip->log; > void *addr = log->bios_event_log; > void *limit = log->bios_event_log_end; > struct tcpa_event *event; > @@ -120,7 +121,8 @@ static void *tpm_bios_measurements_next(struct seq_file *m, void *v, > loff_t *pos) > { > struct tcpa_event *event = v; > - struct tpm_bios_log *log = m->private; > + struct tpm_chip *chip = m->private; > + struct tpm_bios_log *log = &chip->log; > void *limit = log->bios_event_log_end; > u32 converted_event_size; > u32 converted_event_type; > @@ -261,13 +263,10 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v) > static int tpm_bios_measurements_release(struct inode *inode, > struct file *file) > { > - struct seq_file *seq = file->private_data; > - struct tpm_bios_log *log = seq->private; > + struct seq_file *seq = (struct seq_file *)file->private_data; > + struct tpm_chip *chip = (struct tpm_chip *)seq->private; > > - if (log) { > - kfree(log->bios_event_log); > - kfree(log); > - } > + put_device(&chip->dev); > > return seq_release(inode, file); > } > @@ -323,36 +322,34 @@ static int tpm_bios_measurements_open(struct inode *inode, > struct file *file) > { > int err; > - struct tpm_bios_log *log; > struct seq_file *seq; > - const struct seq_operations *seqops = > - (const struct seq_operations *)inode->i_private; > - > - log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL); > - if (!log) > - return -ENOMEM; > - > - if ((err = read_log(log))) > - goto out_free; > + struct tpm_chip_seqops *chip_seqops; > + const struct seq_operations *seqops; > + struct tpm_chip *chip; > + > + inode_lock(inode); > + if (!inode->i_private) { > + inode_unlock(inode); > + return -ENODEV; > + } Why is this is done (inode_lock + setting to NULL), and if so, why it wasn't done in 1/7? > + chip_seqops = (struct tpm_chip_seqops *)inode->i_private; > + seqops = chip_seqops->seqops; > + chip = chip_seqops->chip; > + get_device(&chip->dev); If I recall the last time I looked at this patch (which was few weeks ago) this was needed because securityfs did not have proper fencing. tpm_bios_log_teardown() is called before tpm_del_char_device() but this won't help because callbacks could be in action after securityfs_remove(). Please put a comment here explaining why this get_device() is done so that it is easy to backtrack when changing this part of the driver later on. > /* now register seq file */ > err = seq_open(file, seqops); > if (!err) { > seq = file->private_data; > - seq->private = log; > - } else { > - goto out_free; > + seq->private = chip; > } > > -out: > + inode_unlock(inode); > return err; > -out_free: > - kfree(log->bios_event_log); > - kfree(log); > - goto out; > } > > static const struct file_operations tpm_bios_measurements_ops = { > + .owner = THIS_MODULE, Should be a separate commit event though it is a one line change. Please split the commit into two. > .open = tpm_bios_measurements_open, > .read = seq_read, > .llseek = seq_lseek, > @@ -372,10 +369,22 @@ int tpm_bios_log_setup(struct tpm_chip *chip) > { > const char *name = dev_name(&chip->dev); > unsigned int cnt; > + int rc = 0; > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > return 0; > > + rc = read_log(chip); > + /* > + * read_log failure means event log is not supported except for ENOMEM > + */ > + if (rc < 0) { > + if (rc == -ENOMEM) > + return rc; > + else > + return 0; > + } > + > cnt = 0; > chip->bios_dir[cnt] = > securityfs_create_dir(name, NULL); > @@ -383,19 +392,25 @@ int tpm_bios_log_setup(struct tpm_chip *chip) > goto err; > cnt++; > > + chip->bin_log_seqops.chip = chip; > + chip->bin_log_seqops.seqops = &tpm_binary_b_measurments_seqops; > + > chip->bios_dir[cnt] = > securityfs_create_file("binary_bios_measurements", > S_IRUSR | S_IRGRP, chip->bios_dir[0], > - (void *)&tpm_binary_b_measurments_seqops, > + (void *)&chip->bin_log_seqops, > &tpm_bios_measurements_ops); > if (is_bad(chip->bios_dir[cnt])) > goto err; > cnt++; > > + chip->ascii_log_seqops.chip = chip; > + chip->ascii_log_seqops.seqops = &tpm_ascii_b_measurments_seqops; > + > chip->bios_dir[cnt] = > securityfs_create_file("ascii_bios_measurements", > S_IRUSR | S_IRGRP, chip->bios_dir[0], > - (void *)&tpm_ascii_b_measurments_seqops, > + (void *)&chip->ascii_log_seqops, > &tpm_bios_measurements_ops); > if (is_bad(chip->bios_dir[cnt])) > goto err; > @@ -412,7 +427,14 @@ err: > void tpm_bios_log_teardown(struct tpm_chip *chip) > { > int i; > + struct inode *inode; > > - for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) > + for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > + inode = d_inode(chip->bios_dir[i]); > + inode_lock(inode); > + inode->i_private = NULL; > + inode_unlock(inode); Why is this is done (inode_lock + setting to NULL), and if so, why it wasn't done in 1/7? > securityfs_remove(chip->bios_dir[i]); > + } > + > } > diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h > index fd3357e..6df2f8e 100644 > --- a/drivers/char/tpm/tpm_eventlog.h > +++ b/drivers/char/tpm/tpm_eventlog.h > @@ -73,7 +73,7 @@ enum tcpa_pc_event_ids { > HOST_TABLE_OF_DEVICES, > }; > > -int read_log(struct tpm_bios_log *log); > +int read_log(struct tpm_chip *chip); > > #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \ > defined(CONFIG_ACPI) > diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c > index 570f30c..68d891a 100644 > --- a/drivers/char/tpm/tpm_of.c > +++ b/drivers/char/tpm/tpm_of.c > @@ -20,12 +20,14 @@ > #include "tpm.h" > #include "tpm_eventlog.h" > > -int read_log(struct tpm_bios_log *log) > +int read_log(struct tpm_chip *chip) > { > struct device_node *np; > const u32 *sizep; > const u64 *basep; > + struct tpm_bios_log *log; > > + log = &chip->log; > if (log->bios_event_log != NULL) { > pr_err("%s: ERROR - Eventlog already initialized\n", __func__); > return -EFAULT; > -- > 2.5.0 > /Jarkko ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote: > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote: > > Currently, the event log file operations are not serialized with > > tpm_chip_unregister(), which can possibly cause a race condition. > > > > This patch fixes the race condition by: > > - moving read_log() from fops to chip register. > > What is "chip register"? Please use exact names. > > > - disallowing event log file operations when chip unregister is in > > progress. > > Could you elaborate this sentence? > > > - guarding event log memory using chip krefs. > > Could you elaborate this sentence? > > Please describe how the race condition could happen and provide the > "Fixes:" line for the commit ID that caused it. Otherwise, your commit > message won't make any sense. I cannot apply this commit with this > commit message. > > The commit message does not make much sense... Lets get this moving along, it is hard to keep everything straight over months.. Nayna: This commit message should work: tpm: Have eventlog use the tpm_chip Move the backing memory for the eventlog into tpm_chip and push the tpm_chip into read_log. This optimizes read_log processing by only doing it once and prepares things for the next patches in the series which require the tpm_chip to locate the event log via ACPI and OF handles instead of searching. This is straightfoward except for the issue of passing a kref through i_private with securityfs. Since securityfs_remove does not have any removal fencing like sysfs we use the inode lock to safely get a kref on the tpm_chip. > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > > index eac1f10..813e0d7 100644 > > +++ b/drivers/char/tpm/tpm-chip.c > > @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev) > > idr_remove(&dev_nums_idr, chip->dev_num); > > mutex_unlock(&idr_lock); > > > > + kfree(chip->log.bios_event_log); > > kfree(chip); > > } [..] > > /* read binary bios log */ > > -int read_log(struct tpm_bios_log *log) > > +int read_log(struct tpm_chip *chip) [..] > > +err: > > + kfree(log->bios_event_log); > > Is this kfree() necessary? Yes, if the log is not loaded then bios_event_log log should be null. Nayna - there is a bug here, you have to null bios_event_log after kfree'ing it so that tpm_dev_release does not attempt to kfree a free'd pointer. > > + inode_lock(inode); > > + if (!inode->i_private) { > > + inode_unlock(inode); > > + return -ENODEV; > > + } > > Why is this is done (inode_lock + setting to NULL), and if so, why > it wasn't done in 1/7? Patch 1 does not use memory backed by tpm_chip, this is the first patch in the series that introduces that concept, so it is the first place that needs to do this. > > - for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) > > + for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > > + inode = d_inode(chip->bios_dir[i]); > > + inode_lock(inode); > > + inode->i_private = NULL; > > + inode_unlock(inode); > > Why is this is done (inode_lock + setting to NULL), and if so, why > it wasn't done in 1/7? I'm sure I've explained this before, but again, the inode lock is a work around for lack of removal fencing in securityfs. We null the i_private here during remove and test for null during open under this lock. This design ensures that open either safely gets a kref or fails. Holding the kref as long as the securityfs file is open by userspace ensures there are no use-after-frees in this code. A comment to that effect would be good. Jason ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote: > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote: > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote: > > > Currently, the event log file operations are not serialized with > > > tpm_chip_unregister(), which can possibly cause a race condition. > > > > > > This patch fixes the race condition by: > > > - moving read_log() from fops to chip register. > > > > What is "chip register"? Please use exact names. > > > > > - disallowing event log file operations when chip unregister is in > > > progress. > > > > Could you elaborate this sentence? > > > > > - guarding event log memory using chip krefs. > > > > Could you elaborate this sentence? > > > > Please describe how the race condition could happen and provide the > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit > > message won't make any sense. I cannot apply this commit with this > > commit message. > > > > The commit message does not make much sense... > > Lets get this moving along, it is hard to keep everything straight > over months.. > > Nayna: This commit message should work: > > tpm: Have eventlog use the tpm_chip > > Move the backing memory for the eventlog into tpm_chip and push > the tpm_chip into read_log. This optimizes read_log processing by > only doing it once and prepares things for the next patches in the > series which require the tpm_chip to locate the event log via > ACPI and OF handles instead of searching. > > This is straightfoward except for the issue of passing a kref through > i_private with securityfs. Since securityfs_remove does not have any > removal fencing like sysfs we use the inode lock to safely get a > kref on the tpm_chip. Perfect. Thank you. With this Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> No need for resend. /Jarkko ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Mon, Nov 07, 2016 at 04:26:43PM -0800, Jarkko Sakkinen wrote: > On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote: > > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote: > > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote: > > > > Currently, the event log file operations are not serialized with > > > > tpm_chip_unregister(), which can possibly cause a race condition. > > > > > > > > This patch fixes the race condition by: > > > > - moving read_log() from fops to chip register. > > > > > > What is "chip register"? Please use exact names. > > > > > > > - disallowing event log file operations when chip unregister is in > > > > progress. > > > > > > Could you elaborate this sentence? > > > > > > > - guarding event log memory using chip krefs. > > > > > > Could you elaborate this sentence? > > > > > > Please describe how the race condition could happen and provide the > > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit > > > message won't make any sense. I cannot apply this commit with this > > > commit message. > > > > > > The commit message does not make much sense... > > > > Lets get this moving along, it is hard to keep everything straight > > over months.. > > > > Nayna: This commit message should work: > > > > tpm: Have eventlog use the tpm_chip > > > > Move the backing memory for the eventlog into tpm_chip and push > > the tpm_chip into read_log. This optimizes read_log processing by > > only doing it once and prepares things for the next patches in the > > series which require the tpm_chip to locate the event log via > > ACPI and OF handles instead of searching. > > > > This is straightfoward except for the issue of passing a kref through > > i_private with securityfs. Since securityfs_remove does not have any > > removal fencing like sysfs we use the inode lock to safely get a > > kref on the tpm_chip. > > Perfect. Thank you. > > With this > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > No need for resend. The missing null still needs to be fixed, and the inode lock needs to be held only over get_device in open() Jason ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Mon, Nov 07, 2016 at 04:26:42PM -0800, Jarkko Sakkinen wrote: > On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote: > > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote: > > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote: > > > > Currently, the event log file operations are not serialized with > > > > tpm_chip_unregister(), which can possibly cause a race condition. > > > > > > > > This patch fixes the race condition by: > > > > - moving read_log() from fops to chip register. > > > > > > What is "chip register"? Please use exact names. > > > > > > > - disallowing event log file operations when chip unregister is in > > > > progress. > > > > > > Could you elaborate this sentence? > > > > > > > - guarding event log memory using chip krefs. > > > > > > Could you elaborate this sentence? > > > > > > Please describe how the race condition could happen and provide the > > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit > > > message won't make any sense. I cannot apply this commit with this > > > commit message. > > > > > > The commit message does not make much sense... > > > > Lets get this moving along, it is hard to keep everything straight > > over months.. > > > > Nayna: This commit message should work: > > > > tpm: Have eventlog use the tpm_chip > > > > Move the backing memory for the eventlog into tpm_chip and push > > the tpm_chip into read_log. This optimizes read_log processing by > > only doing it once and prepares things for the next patches in the > > series which require the tpm_chip to locate the event log via > > ACPI and OF handles instead of searching. > > > > This is straightfoward except for the issue of passing a kref through > > i_private with securityfs. Since securityfs_remove does not have any > > removal fencing like sysfs we use the inode lock to safely get a > > kref on the tpm_chip. > > Perfect. Thank you. > > With this > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > No need for resend. There actually *is* need for one more resend because the patces were not CC'd to linux-security-module. I discussed about this at LPC with James Morris and his take was that *major* changes should be CC'd to linux-security-module. This is such a major set of changes. If this series is sent one more time with CC to linux-kernel and linux-security-module and the commit message is changed for this commit from my part it is ready for to be applied. You can add my Reviewed-by to this patch. /Jarkko ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Mon, Nov 07, 2016 at 05:29:43PM -0700, Jason Gunthorpe wrote: > On Mon, Nov 07, 2016 at 04:26:43PM -0800, Jarkko Sakkinen wrote: > > On Mon, Nov 07, 2016 at 04:48:39PM -0700, Jason Gunthorpe wrote: > > > On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote: > > > > On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote: > > > > > Currently, the event log file operations are not serialized with > > > > > tpm_chip_unregister(), which can possibly cause a race condition. > > > > > > > > > > This patch fixes the race condition by: > > > > > - moving read_log() from fops to chip register. > > > > > > > > What is "chip register"? Please use exact names. > > > > > > > > > - disallowing event log file operations when chip unregister is in > > > > > progress. > > > > > > > > Could you elaborate this sentence? > > > > > > > > > - guarding event log memory using chip krefs. > > > > > > > > Could you elaborate this sentence? > > > > > > > > Please describe how the race condition could happen and provide the > > > > "Fixes:" line for the commit ID that caused it. Otherwise, your commit > > > > message won't make any sense. I cannot apply this commit with this > > > > commit message. > > > > > > > > The commit message does not make much sense... > > > > > > Lets get this moving along, it is hard to keep everything straight > > > over months.. > > > > > > Nayna: This commit message should work: > > > > > > tpm: Have eventlog use the tpm_chip > > > > > > Move the backing memory for the eventlog into tpm_chip and push > > > the tpm_chip into read_log. This optimizes read_log processing by > > > only doing it once and prepares things for the next patches in the > > > series which require the tpm_chip to locate the event log via > > > ACPI and OF handles instead of searching. > > > > > > This is straightfoward except for the issue of passing a kref through > > > i_private with securityfs. Since securityfs_remove does not have any > > > removal fencing like sysfs we use the inode lock to safely get a > > > kref on the tpm_chip. > > > > Perfect. Thank you. > > > > With this > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > No need for resend. > > The missing null still needs to be fixed, and the inode lock needs to > be held only over get_device in open() Right, I'm sorry I forgot that... Another reason for one more resend. /Jarkko ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index eac1f10..813e0d7 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev) idr_remove(&dev_nums_idr, chip->dev_num); mutex_unlock(&idr_lock); + kfree(chip->log.bios_event_log); kfree(chip); } diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 4c118a4..bfe93e6 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -35,6 +35,8 @@ #include <linux/cdev.h> #include <linux/highmem.h> +#include "tpm_eventlog.h" + enum tpm_const { TPM_MINOR = 224, /* officially assigned */ TPM_BUFSIZE = 4096, @@ -146,6 +148,11 @@ enum tpm_chip_flags { TPM_CHIP_FLAG_VIRTUAL = BIT(3), }; +struct tpm_chip_seqops { + struct tpm_chip *chip; + const struct seq_operations *seqops; +}; + struct tpm_chip { struct device dev; struct cdev cdev; @@ -157,6 +164,10 @@ struct tpm_chip { struct rw_semaphore ops_sem; const struct tpm_class_ops *ops; + struct tpm_bios_log log; + struct tpm_chip_seqops bin_log_seqops; + struct tpm_chip_seqops ascii_log_seqops; + unsigned int flags; int dev_num; /* /dev/tpm# */ diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c index 565a947..4d6c2d7 100644 --- a/drivers/char/tpm/tpm_acpi.c +++ b/drivers/char/tpm/tpm_acpi.c @@ -45,13 +45,15 @@ struct acpi_tcpa { }; /* read binary bios log */ -int read_log(struct tpm_bios_log *log) +int read_log(struct tpm_chip *chip) { struct acpi_tcpa *buff; acpi_status status; void __iomem *virt; u64 len, start; + struct tpm_bios_log *log; + log = &chip->log; if (log->bios_event_log != NULL) { printk(KERN_ERR "%s: ERROR - Eventlog already initialized\n", @@ -97,13 +99,17 @@ int read_log(struct tpm_bios_log *log) virt = acpi_os_map_iomem(start, len); if (!virt) { - kfree(log->bios_event_log); printk("%s: ERROR - Unable to map memory\n", __func__); - return -EIO; + goto err; } memcpy_fromio(log->bios_event_log, virt, len); acpi_os_unmap_iomem(virt, len); return 0; + +err: + kfree(log->bios_event_log); + return -EIO; + } diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c index 753e92d..bb142f2 100644 --- a/drivers/char/tpm/tpm_eventlog.c +++ b/drivers/char/tpm/tpm_eventlog.c @@ -73,7 +73,8 @@ static const char* tcpa_pc_event_id_strings[] = { static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos) { loff_t i; - struct tpm_bios_log *log = m->private; + struct tpm_chip *chip = m->private; + struct tpm_bios_log *log = &chip->log; void *addr = log->bios_event_log; void *limit = log->bios_event_log_end; struct tcpa_event *event; @@ -120,7 +121,8 @@ static void *tpm_bios_measurements_next(struct seq_file *m, void *v, loff_t *pos) { struct tcpa_event *event = v; - struct tpm_bios_log *log = m->private; + struct tpm_chip *chip = m->private; + struct tpm_bios_log *log = &chip->log; void *limit = log->bios_event_log_end; u32 converted_event_size; u32 converted_event_type; @@ -261,13 +263,10 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v) static int tpm_bios_measurements_release(struct inode *inode, struct file *file) { - struct seq_file *seq = file->private_data; - struct tpm_bios_log *log = seq->private; + struct seq_file *seq = (struct seq_file *)file->private_data; + struct tpm_chip *chip = (struct tpm_chip *)seq->private; - if (log) { - kfree(log->bios_event_log); - kfree(log); - } + put_device(&chip->dev); return seq_release(inode, file); } @@ -323,36 +322,34 @@ static int tpm_bios_measurements_open(struct inode *inode, struct file *file) { int err; - struct tpm_bios_log *log; struct seq_file *seq; - const struct seq_operations *seqops = - (const struct seq_operations *)inode->i_private; - - log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL); - if (!log) - return -ENOMEM; - - if ((err = read_log(log))) - goto out_free; + struct tpm_chip_seqops *chip_seqops; + const struct seq_operations *seqops; + struct tpm_chip *chip; + + inode_lock(inode); + if (!inode->i_private) { + inode_unlock(inode); + return -ENODEV; + } + chip_seqops = (struct tpm_chip_seqops *)inode->i_private; + seqops = chip_seqops->seqops; + chip = chip_seqops->chip; + get_device(&chip->dev); /* now register seq file */ err = seq_open(file, seqops); if (!err) { seq = file->private_data; - seq->private = log; - } else { - goto out_free; + seq->private = chip; } -out: + inode_unlock(inode); return err; -out_free: - kfree(log->bios_event_log); - kfree(log); - goto out; } static const struct file_operations tpm_bios_measurements_ops = { + .owner = THIS_MODULE, .open = tpm_bios_measurements_open, .read = seq_read, .llseek = seq_lseek, @@ -372,10 +369,22 @@ int tpm_bios_log_setup(struct tpm_chip *chip) { const char *name = dev_name(&chip->dev); unsigned int cnt; + int rc = 0; if (chip->flags & TPM_CHIP_FLAG_TPM2) return 0; + rc = read_log(chip); + /* + * read_log failure means event log is not supported except for ENOMEM + */ + if (rc < 0) { + if (rc == -ENOMEM) + return rc; + else + return 0; + } + cnt = 0; chip->bios_dir[cnt] = securityfs_create_dir(name, NULL); @@ -383,19 +392,25 @@ int tpm_bios_log_setup(struct tpm_chip *chip) goto err; cnt++; + chip->bin_log_seqops.chip = chip; + chip->bin_log_seqops.seqops = &tpm_binary_b_measurments_seqops; + chip->bios_dir[cnt] = securityfs_create_file("binary_bios_measurements", S_IRUSR | S_IRGRP, chip->bios_dir[0], - (void *)&tpm_binary_b_measurments_seqops, + (void *)&chip->bin_log_seqops, &tpm_bios_measurements_ops); if (is_bad(chip->bios_dir[cnt])) goto err; cnt++; + chip->ascii_log_seqops.chip = chip; + chip->ascii_log_seqops.seqops = &tpm_ascii_b_measurments_seqops; + chip->bios_dir[cnt] = securityfs_create_file("ascii_bios_measurements", S_IRUSR | S_IRGRP, chip->bios_dir[0], - (void *)&tpm_ascii_b_measurments_seqops, + (void *)&chip->ascii_log_seqops, &tpm_bios_measurements_ops); if (is_bad(chip->bios_dir[cnt])) goto err; @@ -412,7 +427,14 @@ err: void tpm_bios_log_teardown(struct tpm_chip *chip) { int i; + struct inode *inode; - for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) + for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { + inode = d_inode(chip->bios_dir[i]); + inode_lock(inode); + inode->i_private = NULL; + inode_unlock(inode); securityfs_remove(chip->bios_dir[i]); + } + } diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h index fd3357e..6df2f8e 100644 --- a/drivers/char/tpm/tpm_eventlog.h +++ b/drivers/char/tpm/tpm_eventlog.h @@ -73,7 +73,7 @@ enum tcpa_pc_event_ids { HOST_TABLE_OF_DEVICES, }; -int read_log(struct tpm_bios_log *log); +int read_log(struct tpm_chip *chip); #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \ defined(CONFIG_ACPI) diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c index 570f30c..68d891a 100644 --- a/drivers/char/tpm/tpm_of.c +++ b/drivers/char/tpm/tpm_of.c @@ -20,12 +20,14 @@ #include "tpm.h" #include "tpm_eventlog.h" -int read_log(struct tpm_bios_log *log) +int read_log(struct tpm_chip *chip) { struct device_node *np; const u32 *sizep; const u64 *basep; + struct tpm_bios_log *log; + log = &chip->log; if (log->bios_event_log != NULL) { pr_err("%s: ERROR - Eventlog already initialized\n", __func__); return -EFAULT;
Currently, the event log file operations are not serialized with tpm_chip_unregister(), which can possibly cause a race condition. This patch fixes the race condition by: - moving read_log() from fops to chip register. - disallowing event log file operations when chip unregister is in progress. - guarding event log memory using chip krefs. Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> --- drivers/char/tpm/tpm-chip.c | 1 + drivers/char/tpm/tpm.h | 11 ++++++ drivers/char/tpm/tpm_acpi.c | 12 +++++-- drivers/char/tpm/tpm_eventlog.c | 80 ++++++++++++++++++++++++++--------------- drivers/char/tpm/tpm_eventlog.h | 2 +- drivers/char/tpm/tpm_of.c | 4 ++- 6 files changed, 76 insertions(+), 34 deletions(-)