Re: [PATCH] tpm: fix: sanitized code paths in tpm_chip_register()

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

Resending v2 soon. This version was from wrong branch :( Sorry.

On Wed, Mar 18, 2015 at 06:50:01AM +0200, Jarkko Sakkinen wrote:
> I started to work with PPI interface so that it would be available
> under character device sysfs directory and realized that chip
> registeration was still too messy.
> 
> In TPM 1.x in some rare scenarios (errors that almost never occur)
> wrong order in deinitialization steps was taken in teardown. I
> reproduced these scenarios by manually inserting error codes in the
> place of the corresponding function calls.
> 
> The key problem is that the teardown is messy with two separate code
> paths (this was inherited when moving code from tpm-interface.c).
> 
> Moved TPM 1.x specific register/unregister functionality to own helper
> functions and added single code path for teardown in tpm_chip_register().
> Now the code paths have been fixed and it should be easier to review
> later on this part of the code.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-chip.c | 56 +++++++++++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index cf43f4b..e5b0481 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -170,6 +170,33 @@ static void tpm_dev_del_device(struct tpm_chip *chip)
>  	device_unregister(&chip->dev);
>  }
>  
> +static int tpm1_chip_register(struct tpm_chip *chip)
> +{
> +	int rc;
> +
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		return 0;
> +
> +	rc = tpm_sysfs_add_device(chip);
> +	if (rc)
> +		return rc;
> +
> +	chip->bios_dir = tpm_bios_log_setup(chip->devname);
> +
> +	return 0;
> +}
> +
> +static void tpm1_chip_unregister(struct tpm_chip *chip)
> +{
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		return;
> +
> +	if (chip->bios_dir)
> +		tpm_bios_log_teardown(chip->bios_dir);
> +
> +	tpm_sysfs_del_device(chip);
> +}
> +
>  /*
>   * tpm_chip_register() - create a character device for the TPM chip
>   * @chip: TPM chip to use.
> @@ -185,22 +212,17 @@ int tpm_chip_register(struct tpm_chip *chip)
>  {
>  	int rc;
>  
> -	/* Populate sysfs for TPM1 devices. */
> -	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> -		rc = tpm_sysfs_add_device(chip);
> -		if (rc)
> -			goto del_misc;
> -
> -		chip->bios_dir = tpm_bios_log_setup(chip->devname);
> -	}
> +	rc = tpm1_chip_register(chip);
> +	if (rc)
> +		return rc;
>  
>  	rc = tpm_add_ppi(chip);
>  	if (rc)
> -		goto del_sysfs;
> +		goto out_err;
>  
>  	rc = tpm_dev_add_device(chip);
>  	if (rc)
> -		return rc;
> +		goto out_err;
>  
>  	/* Make the chip available. */
>  	spin_lock(&driver_lock);
> @@ -210,10 +232,9 @@ int tpm_chip_register(struct tpm_chip *chip)
>  	chip->flags |= TPM_CHIP_FLAG_REGISTERED;
>  
>  	return 0;
> -del_sysfs:
> -	tpm_sysfs_del_device(chip);
> -del_misc:
> -	tpm_dev_del_device(chip);
> +out_err:
> +	tpm_remove_ppi(chip);
> +	tpm1_chip_unregister(chip);
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_register);
> @@ -238,13 +259,8 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  	spin_unlock(&driver_lock);
>  	synchronize_rcu();
>  
> -	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> -		if (chip->bios_dir)
> -			tpm_bios_log_teardown(chip->bios_dir);
> -		tpm_sysfs_del_device(chip);
> -	}
> -
>  	tpm_remove_ppi(chip);
> +	tpm1_chip_unregister(chip);
>  	tpm_dev_del_device(chip);
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_unregister);
> -- 
> 2.1.4
>