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 <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
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
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
