RE: [PATCH v2] tpm: Fix the driver cleanup code

[Shaikh, Azhar]

>-----Original Message-----
>From: linux-integrity-owner@xxxxxxxxxxxxxxx [mailto:linux-integrity-
>owner@xxxxxxxxxxxxxxx] On Behalf Of Shaikh, Azhar
>Sent: Friday, December 22, 2017 11:15 AM
>To: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
>Cc: jgg@xxxxxxxx; javierm@xxxxxxxxxx; peterhuewe@xxxxxx; linux-security-
>module@xxxxxxxxxxxxxxx; linux-integrity@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx
>Subject: RE: [PATCH v2] tpm: Fix the driver cleanup code
>
>
>
>>-----Original Message-----
>>From: Jarkko Sakkinen [mailto:jarkko.sakkinen@xxxxxxxxxxxxxxx]
>>Sent: Friday, December 22, 2017 10:42 AM
>>To: Shaikh, Azhar <azhar.shaikh@xxxxxxxxx>
>>Cc: jgg@xxxxxxxx; javierm@xxxxxxxxxx; peterhuewe@xxxxxx;
>>linux-security- module@xxxxxxxxxxxxxxx;
>>linux-integrity@xxxxxxxxxxxxxxx; linux- kernel@xxxxxxxxxxxxxxx
>>Subject: Re: [PATCH v2] tpm: Fix the driver cleanup code
>>
>>On Thu, Dec 21, 2017 at 03:13:39PM -0800, Azhar Shaikh wrote:
>>> Commit 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout the
>>> duration of transmit_cmd()") added code which accessed
>>> chip->ops, even after it was set to NULL in tpm_del_char_device(),
>>> called from tpm_chip_unregister() in error / driver exit paths.
>>> So fix this code.
>>>
>>> Fixes: 3c1701339284353c41 ("tpm: Keep CLKRUN enabled throughout the
>>> duration of transmit_cmd()")
>>>
>>> Suggested-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
>>> Suggested-by: Jason Gunthorpe <jgg@xxxxxxxx>
>>> Signed-off-by: Azhar Shaikh <azhar.shaikh@xxxxxxxxx>
>>> ---
>>> Changes in v2:
>>> - Remove TPM_CHIP_FLAG_DO_NOT_CLEAR_OPS flag, instead call
>>>   tpm_tis_clkrun_enable() directly in tpm_tis_remove()
>>> - Add comment to tpm_tis_clkrun_enable()
>>>
>>>  drivers/char/tpm/tpm_tis.c      |  6 ------
>>>  drivers/char/tpm/tpm_tis_core.c | 29 +++++++++++++++++------------
>>>  2 files changed, 17 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
>>> index d29add49b033..c847fc69a2fc 100644
>>> --- a/drivers/char/tpm/tpm_tis.c
>>> +++ b/drivers/char/tpm/tpm_tis.c
>>> @@ -275,9 +275,6 @@ static void tpm_tis_pnp_remove(struct pnp_dev
>>> *dev)
>>>
>>>  	tpm_chip_unregister(chip);
>>>  	tpm_tis_remove(chip);
>>> -	if (is_bsw())
>>> -		iounmap(priv->ilb_base_addr);
>>> -
>>>  }
>>>
>>>  static struct pnp_driver tis_pnp_driver = { @@ -329,9 +326,6 @@
>>> static int tpm_tis_plat_remove(struct platform_device *pdev)
>>>  	tpm_chip_unregister(chip);
>>>  	tpm_tis_remove(chip);
>>>
>>> -	if (is_bsw())
>>> -		iounmap(priv->ilb_base_addr);
>>> -
>>>  	return 0;
>>>  }
>>>
>>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>>> b/drivers/char/tpm/tpm_tis_core.c index c2227983ed88..519e4a78c8f8
>>> 100644
>>> --- a/drivers/char/tpm/tpm_tis_core.c
>>> +++ b/drivers/char/tpm/tpm_tis_core.c
>>> @@ -37,6 +37,8 @@
>>>   */
>>>  #define TPM_POLL_SLEEP	1  /* msec */
>>>
>>> +static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool
>>> +value);
>>> +
>>>  static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
>>>  					bool check_cancel, bool *canceled)  {
>>@@ -716,8 +718,7 @@ void
>>> tpm_tis_remove(struct tpm_chip *chip)
>>>  	u32 interrupt;
>>>  	int rc;
>>>
>>> -	if (chip->ops->clk_enable != NULL)
>>> -		chip->ops->clk_enable(chip, true);
>>> +	tpm_tis_clkrun_enable(chip, true);
>>>
>>>  	rc = tpm_tis_read32(priv, reg, &interrupt);
>>>  	if (rc < 0)
>>> @@ -725,8 +726,10 @@ void tpm_tis_remove(struct tpm_chip *chip)
>>>
>>>  	tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
>>>
>>> -	if (chip->ops->clk_enable != NULL)
>>> -		chip->ops->clk_enable(chip, false);
>>> +	tpm_tis_clkrun_enable(chip, false);
>>> +
>>> +	if (priv->ilb_base_addr)
>>> +		iounmap(priv->ilb_base_addr);
>>>  }
>>>  EXPORT_SYMBOL_GPL(tpm_tis_remove);
>>>
>>> @@ -736,6 +739,9 @@ void tpm_tis_remove(struct tpm_chip *chip)
>>>   * @chip:	TPM chip to use
>>>   * @value:	1 - Disable CLKRUN protocol, so that clocks are free running
>>>   *		0 - Enable CLKRUN protocol
>>> + *
>>> + * Call this function directly in tpm_tis_remove() in error or
>>> + driver removal
>>> + * path, since the chip->ops is set to NULL in tpm_chip_unregister().
>>>   */
>>>  static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value)
>>> { @@ -922,21 +928,20 @@ int tpm_tis_core_init(struct device *dev,
>>> struct tpm_tis_data *priv, int irq,
>>>  	}
>>>
>>>  	rc = tpm_chip_register(chip);
>>> -	if (rc && is_bsw())
>>> -		iounmap(priv->ilb_base_addr);
>>> +	if (rc)
>>> +		goto out_err;
>>>
>>>  	if (chip->ops->clk_enable != NULL)
>>>  		chip->ops->clk_enable(chip, false);
>>>
>>> -	return rc;
>>> -out_err:
>>> -	tpm_tis_remove(chip);
>>> -	if (is_bsw())
>>> -		iounmap(priv->ilb_base_addr);
>>> +	return 0;
>>>
>>> -	if (chip->ops->clk_enable != NULL)
>>> +out_err:
>>> +	if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL))
>>>  		chip->ops->clk_enable(chip, false);
>>>
>>> +	tpm_tis_remove(chip);
>>> +
>>>  	return rc;
>>>  }
>>>  EXPORT_SYMBOL_GPL(tpm_tis_core_init);
>>> --
>>> 1.9.1
>>>
>>
>>Since the original commit is not in the mainline yet, you should rather
>>send an updated version of that commit.
>>
>
>Ok, I will send an updated version of the original commit "tpm: Keep CLKRUN
>enabled throughout the duration of transmit_cmd()"

Should I send the updated commit on top of these 4?

https://patchwork.kernel.org/patch/10125543/ 
https://patchwork.kernel.org/patch/10125535/
https://patchwork.kernel.org/patch/10125509/ 
https://patchwork.kernel.org/patch/10125511/ 

>
>>/Jarkko
>
>Regards,
>Azhar Shaikh

Regards,
Azhar Shaikh