| Message ID | 1472532619-22170-4-git-send-email-nayna@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Aug 30, 2016 at 12:50:15AM -0400, Nayna Jain wrote: > Currently, securityfs files for eventlog is created irrespective of > logs properties exist or not i.e. event log base address and > size. > > This patch will create ascii and bios measurements file > only if readlog() is successful. Doesn't this also uncoditionally enable event log for TPM2? > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> > --- > drivers/char/tpm/Makefile | 10 ++++++---- > drivers/char/tpm/tpm-chip.c | 18 ++++-------------- > drivers/char/tpm/tpm.h | 4 ++++ > drivers/char/tpm/tpm_acpi.c | 14 +++++++------- > drivers/char/tpm/tpm_eventlog.c | 36 ++++++++++++------------------------ > drivers/char/tpm/tpm_eventlog.h | 17 +++-------------- > drivers/char/tpm/tpm_of.c | 12 ++++++------ > 7 files changed, 42 insertions(+), 69 deletions(-) > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile > index a385fb8..00e48e4 100644 > --- a/drivers/char/tpm/Makefile > +++ b/drivers/char/tpm/Makefile > @@ -2,14 +2,16 @@ > # Makefile for the kernel tpm device drivers. > # > obj-$(CONFIG_TCG_TPM) += tpm.o > -tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o > +tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \ > + tpm_eventlog.o > + > tpm-$(CONFIG_ACPI) += tpm_ppi.o > > ifdef CONFIG_ACPI > - tpm-y += tpm_eventlog.o tpm_acpi.o > + tpm-y += tpm_acpi.o > else > -ifdef CONFIG_TCG_IBMVTPM > - tpm-y += tpm_eventlog.o tpm_of.o Why? > +ifdef CONFIG_OF > + tpm-y += tpm_of.o > endif > endif > obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 1cd1238..307130e 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -283,19 +283,9 @@ static int tpm1_chip_register(struct tpm_chip *chip) > > tpm_sysfs_add_device(chip); > > - tpm_bios_log_setup(chip); > - > return 0; > } > > -static void tpm1_chip_unregister(struct tpm_chip *chip) > -{ > - if (chip->flags & TPM_CHIP_FLAG_TPM2) > - return; > - > - tpm_bios_log_teardown(chip); > -} > - > static void tpm_del_legacy_sysfs(struct tpm_chip *chip) > { > struct attribute **i; > @@ -369,10 +359,8 @@ int tpm_chip_register(struct tpm_chip *chip) > tpm_add_ppi(chip); > > rc = tpm_add_char_device(chip); > - if (rc) { > - tpm1_chip_unregister(chip); > + if (rc) > return rc; > - } > > chip->flags |= TPM_CHIP_FLAG_REGISTERED; > > @@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip) > return rc; > } > > + tpm_bios_log_setup(chip); > + > return 0; > } > EXPORT_SYMBOL_GPL(tpm_chip_register); > @@ -406,7 +396,7 @@ void tpm_chip_unregister(struct tpm_chip *chip) > > tpm_del_legacy_sysfs(chip); > > - tpm1_chip_unregister(chip); > + tpm_bios_log_teardown(chip); > tpm_del_char_device(chip); > } > EXPORT_SYMBOL_GPL(tpm_chip_unregister); > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 603a661..032eb7d 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -35,6 +35,8 @@ > #include <linux/cdev.h> > #include <linux/highmem.h> > > +#include "tpm_eventlog.h" > + > enum tpm_const { > TPM_MINOR = 224, /* officially assigned */ > TPM_BUFSIZE = 4096, > @@ -156,6 +158,8 @@ struct tpm_chip { > struct rw_semaphore ops_sem; > const struct tpm_class_ops *ops; > > + struct tpm_bios_log log; > + > unsigned int flags; > > int dev_num; /* /dev/tpm# */ > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c > index 565a947..05b4e8a 100644 > --- a/drivers/char/tpm/tpm_acpi.c > +++ b/drivers/char/tpm/tpm_acpi.c > @@ -45,14 +45,14 @@ struct acpi_tcpa { > }; > > /* read binary bios log */ > -int read_log(struct tpm_bios_log *log) > +int read_log(struct tpm_chip *chip) > { > struct acpi_tcpa *buff; > acpi_status status; > void __iomem *virt; > u64 len, start; > > - if (log->bios_event_log != NULL) { > + if (chip->log.bios_event_log != NULL) { > printk(KERN_ERR > "%s: ERROR - Eventlog already initialized\n", > __func__); > @@ -86,23 +86,23 @@ int read_log(struct tpm_bios_log *log) > } > > /* malloc EventLog space */ > - log->bios_event_log = kmalloc(len, GFP_KERNEL); > - if (!log->bios_event_log) { > + chip->log.bios_event_log = kmalloc(len, GFP_KERNEL); > + if (!chip->log.bios_event_log) { > printk("%s: ERROR - Not enough Memory for BIOS measurements\n", > __func__); > return -ENOMEM; > } > > - log->bios_event_log_end = log->bios_event_log + len; > + chip->log.bios_event_log_end = chip->log.bios_event_log + len; > > virt = acpi_os_map_iomem(start, len); > if (!virt) { > - kfree(log->bios_event_log); > + kfree(chip->log.bios_event_log); > printk("%s: ERROR - Unable to map memory\n", __func__); > return -EIO; > } > > - memcpy_fromio(log->bios_event_log, virt, len); > + memcpy_fromio(chip->log.bios_event_log, virt, len); > > acpi_os_unmap_iomem(virt, len); > return 0; > diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c > index 9dd69a7..d6f2477 100644 > --- a/drivers/char/tpm/tpm_eventlog.c > +++ b/drivers/char/tpm/tpm_eventlog.c > @@ -261,14 +261,6 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v) > static int tpm_bios_measurements_release(struct inode *inode, > struct file *file) > { > - struct seq_file *seq = file->private_data; > - struct tpm_bios_log *log = seq->private; > - > - if (log) { > - kfree(log->bios_event_log); > - kfree(log); > - } > - > return seq_release(inode, file); > } > > @@ -323,34 +315,22 @@ static int tpm_bios_measurements_open(struct inode *inode, > struct file *file) > { > int err; > - struct tpm_bios_log *log; > struct seq_file *seq; > + struct tpm_chip *chip; > const struct seq_operations *seqops = > (const struct seq_operations *)inode->i_private; > > - log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL); > - if (!log) > - return -ENOMEM; > - > - err = read_log(log); > - if (err) > - goto out_free; > + chip = (struct tpm_chip > + *)file->f_path.dentry->d_parent->d_inode->i_private; > > /* now register seq file */ > err = seq_open(file, seqops); > if (!err) { > seq = file->private_data; > - seq->private = log; > - } else { > - goto out_free; > + seq->private = &chip->log; > } > > -out: > return err; > -out_free: > - kfree(log->bios_event_log); > - kfree(log); > - goto out; > } > > static const struct file_operations tpm_bios_measurements_ops = { > @@ -372,12 +352,18 @@ static int is_bad(void *p) > void tpm_bios_log_setup(struct tpm_chip *chip) > { > const char *name = dev_name(&chip->dev); > + int rc = 0; > + > + rc = read_log(chip); > + if (rc < 0) > + return; > > chip->bios_dir_count = 0; > chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name, > NULL); > if (is_bad(chip->bios_dir[chip->bios_dir_count])) > goto err; > + chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip; > chip->bios_dir_count++; > > chip->bios_dir[chip->bios_dir_count] = > @@ -410,4 +396,6 @@ void tpm_bios_log_teardown(struct tpm_chip *chip) > > for (i = chip->bios_dir_count; i > 0; --i) > securityfs_remove(chip->bios_dir[i-1]); > + > + kfree(chip->log.bios_event_log); > } > diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h > index 67621c9..6a01d43 100644 > --- a/drivers/char/tpm/tpm_eventlog.h > +++ b/drivers/char/tpm/tpm_eventlog.h > @@ -73,20 +73,9 @@ enum tcpa_pc_event_ids { > HOST_TABLE_OF_DEVICES, > }; > > -int read_log(struct tpm_bios_log *log); > +int read_log(struct tpm_chip *chip); > > -#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \ > - defined(CONFIG_ACPI) > -extern void tpm_bios_log_setup(struct tpm_chip *chip); > -extern void tpm_bios_log_teardown(struct tpm_chip *chip); > -#else > -static inline void tpm_bios_log_setup(struct tpm_chip *chip) > -{ > - chip->bios_dir_count = 0; > -} > -static inline void tpm_bios_log_teardown(struct tpm_chip *chip) > -{ > -} > -#endif > +void tpm_bios_log_setup(struct tpm_chip *chip); > +void tpm_bios_log_teardown(struct tpm_chip *chip); > > #endif > diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c > index 570f30c..8e77976 100644 > --- a/drivers/char/tpm/tpm_of.c > +++ b/drivers/char/tpm/tpm_of.c > @@ -20,13 +20,13 @@ > #include "tpm.h" > #include "tpm_eventlog.h" > > -int read_log(struct tpm_bios_log *log) > +int read_log(struct tpm_chip *chip) > { > struct device_node *np; > const u32 *sizep; > const u64 *basep; > > - if (log->bios_event_log != NULL) { > + if (chip->log.bios_event_log != NULL) { > pr_err("%s: ERROR - Eventlog already initialized\n", __func__); > return -EFAULT; > } > @@ -53,17 +53,17 @@ int read_log(struct tpm_bios_log *log) > goto cleanup_eio; > } > > - log->bios_event_log = kmalloc(*sizep, GFP_KERNEL); > - if (!log->bios_event_log) { > + chip->log.bios_event_log = kmalloc(*sizep, GFP_KERNEL); > + if (!chip->log.bios_event_log) { > pr_err("%s: ERROR - Not enough memory for BIOS measurements\n", > __func__); > of_node_put(np); > return -ENOMEM; > } > > - log->bios_event_log_end = log->bios_event_log + *sizep; > + chip->log.bios_event_log_end = chip->log.bios_event_log + *sizep; > > - memcpy(log->bios_event_log, __va(*basep), *sizep); > + memcpy(chip->log.bios_event_log, __va(*basep), *sizep); > of_node_put(np); > > return 0; > -- > 2.5.0 > > > ------------------------------------------------------------------------------ > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel /Jarkko ------------------------------------------------------------------------------
On Tue, Aug 30, 2016 at 12:50:15AM -0400, Nayna Jain wrote: > @@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip) > return rc; > } > > + tpm_bios_log_setup(chip); Surely this can fail, right? At least if the security fs setup fails this should propogate that error. That is a mistake in an earlier patch now that I think about it.. > > /* malloc EventLog space */ > - log->bios_event_log = kmalloc(len, GFP_KERNEL); > - if (!log->bios_event_log) { > + chip->log.bios_event_log = kmalloc(len, GFP_KERNEL); > + if (!chip->log.bios_event_log) { > printk("%s: ERROR - Not enough Memory for BIOS measurements\n", > __func__); Please delete all prints on kmalloc failure, maybe as another patch. > return -ENOMEM; > } > > - log->bios_event_log_end = log->bios_event_log + len; > + chip->log.bios_event_log_end = chip->log.bios_event_log + len; > > virt = acpi_os_map_iomem(start, len); > if (!virt) { > - kfree(log->bios_event_log); > + kfree(chip->log.bios_event_log); It would also be nice to see this written in the standard goto-unwind idiom. > static const struct file_operations tpm_bios_measurements_ops = { > @@ -372,12 +352,18 @@ static int is_bad(void *p) > void tpm_bios_log_setup(struct tpm_chip *chip) > { > const char *name = dev_name(&chip->dev); > + int rc = 0; > + > + rc = read_log(chip); > + if (rc < 0) > + return; > > chip->bios_dir_count = 0; > chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name, > NULL); > if (is_bad(chip->bios_dir[chip->bios_dir_count])) > goto err; > + chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = > chip; Hum. So I don't know if this is right. You should get someone more familiar with securityfs to double check it. I see apparmorfs.c doing a similar approach, so that would be a good starting place to copy. Notice how it uses aa_get_(x) Still, I wonder if that is even right, is securityfs_remove() really a strong fence against open? I guess the inode locking is doing that? This also means that the file can remain held open in userspace *after* securityfs_remove returns, so the filp must hold a kref on the chip as well. At a minimum you need to do something like this: Create: chip->sfs_data_bin.chip = chip; chip->sfs_data_bin.ops = &tpm_binary_b_measurments_seqops; securityfs_create_file(...,&chip->sfs_data_bin) It must be done like that to be atomic with open, create two new members of chip to hold a struct to pass through as the private data. Do not use the dentry private. Open: chip = (struct tpm_chip *)inode->i_private; dev_get(&chip->dev); seq_open(..) seq->private = chip; Release: dev_put(&((struct tpm_chip *)seq->private)->dev); Teardown the kfree needs to move to the chip release function. > ifdef CONFIG_ACPI > - tpm-y += tpm_eventlog.o tpm_acpi.o > + tpm-y += tpm_acpi.o > else > -ifdef CONFIG_TCG_IBMVTPM > - tpm-y += tpm_eventlog.o tpm_of.o > +ifdef CONFIG_OF > + tpm-y += tpm_of.o > endif This is too early in the patch series. This change needs to go into 'Redefine the read_log method to check for ACPI/OF properties sequentially' > -#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \ > - defined(CONFIG_ACPI) Ditto Regarding Jarkko's comment, Yes, move the check for TPM2 into both of the read_log() - do not allow TPM2 to read the log until you patch the OF stuff to support the TPM2 log format. Jason ------------------------------------------------------------------------------
On 08/30/2016 11:22 PM, Jason Gunthorpe wrote: > On Tue, Aug 30, 2016 at 12:50:15AM -0400, Nayna Jain wrote: >> @@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip) >> return rc; >> } >> >> + tpm_bios_log_setup(chip); > > Surely this can fail, right? At least if the security fs setup fails > this should propogate that error. What action we want to take if it fails to do bios_log_setup ? I have done all other fixes, just am not sure that if we propogate this error, then will it mean that tpm_chip_register (where this function is called) should fail ? or it is just an error logging on failure of bios_log_setup. > > That is a mistake in an earlier patch now that I think about it.. >> >> /* malloc EventLog space */ >> - log->bios_event_log = kmalloc(len, GFP_KERNEL); >> - if (!log->bios_event_log) { >> + chip->log.bios_event_log = kmalloc(len, GFP_KERNEL); >> + if (!chip->log.bios_event_log) { >> printk("%s: ERROR - Not enough Memory for BIOS measurements\n", >> __func__); > > Please delete all prints on kmalloc failure, maybe as another patch. > >> return -ENOMEM; >> } >> >> - log->bios_event_log_end = log->bios_event_log + len; >> + chip->log.bios_event_log_end = chip->log.bios_event_log + len; >> >> virt = acpi_os_map_iomem(start, len); >> if (!virt) { >> - kfree(log->bios_event_log); >> + kfree(chip->log.bios_event_log); > > It would also be nice to see this written in the standard > goto-unwind idiom. > >> static const struct file_operations tpm_bios_measurements_ops = { >> @@ -372,12 +352,18 @@ static int is_bad(void *p) >> void tpm_bios_log_setup(struct tpm_chip *chip) >> { >> const char *name = dev_name(&chip->dev); >> + int rc = 0; >> + >> + rc = read_log(chip); >> + if (rc < 0) >> + return; >> >> chip->bios_dir_count = 0; >> chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name, >> NULL); >> if (is_bad(chip->bios_dir[chip->bios_dir_count])) >> goto err; >> + chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = >> chip; > > Hum. > > So I don't know if this is right. You should get someone more familiar > with securityfs to double check it. I see apparmorfs.c doing a similar > approach, so that would be a good starting place to copy. Notice how > it uses aa_get_(x) > > Still, I wonder if that is even right, is securityfs_remove() really a > strong fence against open? I guess the inode locking is doing that? > > This also means that the file can remain held open in userspace > *after* securityfs_remove returns, so the filp must hold a kref on the > chip as well. > > At a minimum you need to do something like this: > > Create: > > chip->sfs_data_bin.chip = chip; > chip->sfs_data_bin.ops = &tpm_binary_b_measurments_seqops; > securityfs_create_file(...,&chip->sfs_data_bin) > > It must be done like that to be atomic with open, create two new > members of chip to hold a struct to pass through as the private > data. Do not use the dentry private. > > Open: > chip = (struct tpm_chip *)inode->i_private; > dev_get(&chip->dev); > seq_open(..) > seq->private = chip; > > Release: > dev_put(&((struct tpm_chip *)seq->private)->dev); > > Teardown > the kfree needs to move to the chip release function. > >> ifdef CONFIG_ACPI >> - tpm-y += tpm_eventlog.o tpm_acpi.o >> + tpm-y += tpm_acpi.o >> else >> -ifdef CONFIG_TCG_IBMVTPM >> - tpm-y += tpm_eventlog.o tpm_of.o >> +ifdef CONFIG_OF >> + tpm-y += tpm_of.o >> endif > > This is too early in the patch series. This change needs to go into > 'Redefine the read_log method to check for ACPI/OF properties > sequentially' > >> -#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \ >> - defined(CONFIG_ACPI) > > Ditto > > Regarding Jarkko's comment, > > Yes, move the check for TPM2 into both of the read_log() - do not > allow TPM2 to read the log until you patch the OF stuff to support the > TPM2 log format. > > Jason > ------------------------------------------------------------------------------
On Fri, Sep 09, 2016 at 10:54:25PM +0530, Nayna wrote: > > > On 08/30/2016 11:22 PM, Jason Gunthorpe wrote: > >On Tue, Aug 30, 2016 at 12:50:15AM -0400, Nayna Jain wrote: > >>@@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip) > >> return rc; > >> } > >> > >>+ tpm_bios_log_setup(chip); > > > >Surely this can fail, right? At least if the security fs setup fails > >this should propogate that error. > > What action we want to take if it fails to do bios_log_setup ? > I have done all other fixes, just am not sure that if we propogate this > error, then will it mean that tpm_chip_register (where this function is > called) should fail ? or it is just an error logging on failure of > bios_log_setup. Typically we'd want to fail probe, so keep flowing it up. I'm expecting this should only happen in fatal cases like security_fs functions failing. Failures like no optional ACPI/DT description should not log or return an error up to probe.. Jason ------------------------------------------------------------------------------
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index a385fb8..00e48e4 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -2,14 +2,16 @@ # Makefile for the kernel tpm device drivers. # obj-$(CONFIG_TCG_TPM) += tpm.o -tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o +tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \ + tpm_eventlog.o + tpm-$(CONFIG_ACPI) += tpm_ppi.o ifdef CONFIG_ACPI - tpm-y += tpm_eventlog.o tpm_acpi.o + tpm-y += tpm_acpi.o else -ifdef CONFIG_TCG_IBMVTPM - tpm-y += tpm_eventlog.o tpm_of.o +ifdef CONFIG_OF + tpm-y += tpm_of.o endif endif obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 1cd1238..307130e 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -283,19 +283,9 @@ static int tpm1_chip_register(struct tpm_chip *chip) tpm_sysfs_add_device(chip); - tpm_bios_log_setup(chip); - return 0; } -static void tpm1_chip_unregister(struct tpm_chip *chip) -{ - if (chip->flags & TPM_CHIP_FLAG_TPM2) - return; - - tpm_bios_log_teardown(chip); -} - static void tpm_del_legacy_sysfs(struct tpm_chip *chip) { struct attribute **i; @@ -369,10 +359,8 @@ int tpm_chip_register(struct tpm_chip *chip) tpm_add_ppi(chip); rc = tpm_add_char_device(chip); - if (rc) { - tpm1_chip_unregister(chip); + if (rc) return rc; - } chip->flags |= TPM_CHIP_FLAG_REGISTERED; @@ -382,6 +370,8 @@ int tpm_chip_register(struct tpm_chip *chip) return rc; } + tpm_bios_log_setup(chip); + return 0; } EXPORT_SYMBOL_GPL(tpm_chip_register); @@ -406,7 +396,7 @@ void tpm_chip_unregister(struct tpm_chip *chip) tpm_del_legacy_sysfs(chip); - tpm1_chip_unregister(chip); + tpm_bios_log_teardown(chip); tpm_del_char_device(chip); } EXPORT_SYMBOL_GPL(tpm_chip_unregister); diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 603a661..032eb7d 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -35,6 +35,8 @@ #include <linux/cdev.h> #include <linux/highmem.h> +#include "tpm_eventlog.h" + enum tpm_const { TPM_MINOR = 224, /* officially assigned */ TPM_BUFSIZE = 4096, @@ -156,6 +158,8 @@ struct tpm_chip { struct rw_semaphore ops_sem; const struct tpm_class_ops *ops; + struct tpm_bios_log log; + unsigned int flags; int dev_num; /* /dev/tpm# */ diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c index 565a947..05b4e8a 100644 --- a/drivers/char/tpm/tpm_acpi.c +++ b/drivers/char/tpm/tpm_acpi.c @@ -45,14 +45,14 @@ struct acpi_tcpa { }; /* read binary bios log */ -int read_log(struct tpm_bios_log *log) +int read_log(struct tpm_chip *chip) { struct acpi_tcpa *buff; acpi_status status; void __iomem *virt; u64 len, start; - if (log->bios_event_log != NULL) { + if (chip->log.bios_event_log != NULL) { printk(KERN_ERR "%s: ERROR - Eventlog already initialized\n", __func__); @@ -86,23 +86,23 @@ int read_log(struct tpm_bios_log *log) } /* malloc EventLog space */ - log->bios_event_log = kmalloc(len, GFP_KERNEL); - if (!log->bios_event_log) { + chip->log.bios_event_log = kmalloc(len, GFP_KERNEL); + if (!chip->log.bios_event_log) { printk("%s: ERROR - Not enough Memory for BIOS measurements\n", __func__); return -ENOMEM; } - log->bios_event_log_end = log->bios_event_log + len; + chip->log.bios_event_log_end = chip->log.bios_event_log + len; virt = acpi_os_map_iomem(start, len); if (!virt) { - kfree(log->bios_event_log); + kfree(chip->log.bios_event_log); printk("%s: ERROR - Unable to map memory\n", __func__); return -EIO; } - memcpy_fromio(log->bios_event_log, virt, len); + memcpy_fromio(chip->log.bios_event_log, virt, len); acpi_os_unmap_iomem(virt, len); return 0; diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c index 9dd69a7..d6f2477 100644 --- a/drivers/char/tpm/tpm_eventlog.c +++ b/drivers/char/tpm/tpm_eventlog.c @@ -261,14 +261,6 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v) static int tpm_bios_measurements_release(struct inode *inode, struct file *file) { - struct seq_file *seq = file->private_data; - struct tpm_bios_log *log = seq->private; - - if (log) { - kfree(log->bios_event_log); - kfree(log); - } - return seq_release(inode, file); } @@ -323,34 +315,22 @@ static int tpm_bios_measurements_open(struct inode *inode, struct file *file) { int err; - struct tpm_bios_log *log; struct seq_file *seq; + struct tpm_chip *chip; const struct seq_operations *seqops = (const struct seq_operations *)inode->i_private; - log = kzalloc(sizeof(struct tpm_bios_log), GFP_KERNEL); - if (!log) - return -ENOMEM; - - err = read_log(log); - if (err) - goto out_free; + chip = (struct tpm_chip + *)file->f_path.dentry->d_parent->d_inode->i_private; /* now register seq file */ err = seq_open(file, seqops); if (!err) { seq = file->private_data; - seq->private = log; - } else { - goto out_free; + seq->private = &chip->log; } -out: return err; -out_free: - kfree(log->bios_event_log); - kfree(log); - goto out; } static const struct file_operations tpm_bios_measurements_ops = { @@ -372,12 +352,18 @@ static int is_bad(void *p) void tpm_bios_log_setup(struct tpm_chip *chip) { const char *name = dev_name(&chip->dev); + int rc = 0; + + rc = read_log(chip); + if (rc < 0) + return; chip->bios_dir_count = 0; chip->bios_dir[chip->bios_dir_count] = securityfs_create_dir(name, NULL); if (is_bad(chip->bios_dir[chip->bios_dir_count])) goto err; + chip->bios_dir[chip->bios_dir_count]->d_inode->i_private = chip; chip->bios_dir_count++; chip->bios_dir[chip->bios_dir_count] = @@ -410,4 +396,6 @@ void tpm_bios_log_teardown(struct tpm_chip *chip) for (i = chip->bios_dir_count; i > 0; --i) securityfs_remove(chip->bios_dir[i-1]); + + kfree(chip->log.bios_event_log); } diff --git a/drivers/char/tpm/tpm_eventlog.h b/drivers/char/tpm/tpm_eventlog.h index 67621c9..6a01d43 100644 --- a/drivers/char/tpm/tpm_eventlog.h +++ b/drivers/char/tpm/tpm_eventlog.h @@ -73,20 +73,9 @@ enum tcpa_pc_event_ids { HOST_TABLE_OF_DEVICES, }; -int read_log(struct tpm_bios_log *log); +int read_log(struct tpm_chip *chip); -#if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \ - defined(CONFIG_ACPI) -extern void tpm_bios_log_setup(struct tpm_chip *chip); -extern void tpm_bios_log_teardown(struct tpm_chip *chip); -#else -static inline void tpm_bios_log_setup(struct tpm_chip *chip) -{ - chip->bios_dir_count = 0; -} -static inline void tpm_bios_log_teardown(struct tpm_chip *chip) -{ -} -#endif +void tpm_bios_log_setup(struct tpm_chip *chip); +void tpm_bios_log_teardown(struct tpm_chip *chip); #endif diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c index 570f30c..8e77976 100644 --- a/drivers/char/tpm/tpm_of.c +++ b/drivers/char/tpm/tpm_of.c @@ -20,13 +20,13 @@ #include "tpm.h" #include "tpm_eventlog.h" -int read_log(struct tpm_bios_log *log) +int read_log(struct tpm_chip *chip) { struct device_node *np; const u32 *sizep; const u64 *basep; - if (log->bios_event_log != NULL) { + if (chip->log.bios_event_log != NULL) { pr_err("%s: ERROR - Eventlog already initialized\n", __func__); return -EFAULT; } @@ -53,17 +53,17 @@ int read_log(struct tpm_bios_log *log) goto cleanup_eio; } - log->bios_event_log = kmalloc(*sizep, GFP_KERNEL); - if (!log->bios_event_log) { + chip->log.bios_event_log = kmalloc(*sizep, GFP_KERNEL); + if (!chip->log.bios_event_log) { pr_err("%s: ERROR - Not enough memory for BIOS measurements\n", __func__); of_node_put(np); return -ENOMEM; } - log->bios_event_log_end = log->bios_event_log + *sizep; + chip->log.bios_event_log_end = chip->log.bios_event_log + *sizep; - memcpy(log->bios_event_log, __va(*basep), *sizep); + memcpy(chip->log.bios_event_log, __va(*basep), *sizep); of_node_put(np); return 0;
Currently, securityfs files for eventlog is created irrespective of logs properties exist or not i.e. event log base address and size. This patch will create ascii and bios measurements file only if readlog() is successful. Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> --- drivers/char/tpm/Makefile | 10 ++++++---- drivers/char/tpm/tpm-chip.c | 18 ++++-------------- drivers/char/tpm/tpm.h | 4 ++++ drivers/char/tpm/tpm_acpi.c | 14 +++++++------- drivers/char/tpm/tpm_eventlog.c | 36 ++++++++++++------------------------ drivers/char/tpm/tpm_eventlog.h | 17 +++-------------- drivers/char/tpm/tpm_of.c | 12 ++++++------ 7 files changed, 42 insertions(+), 69 deletions(-)