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 <jguntho...@obsidianresearch.com>
>> Signed-off-by: Nayna Jain <na...@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
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
