Re: [PATCH v4 3/8] tpm: validate event log access before tpm_bios_log_setup

[Nayna <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]

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-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> Signed-off-by: Nayna Jain <nayna-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> ---
>>   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