Re: [PATCH v3 3/7] tpm: Validate the eventlog access before tpm_bios_log_setup

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

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

------------------------------------------------------------------------------