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