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 <[email protected]>
Suggested-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Azhar Shaikh <[email protected]>
---
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
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 <[email protected]>
> Suggested-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Azhar Shaikh <[email protected]>
> 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()
Looks Ok to me
Reviewed-by: Jason Gunthorpe <[email protected]>
Jason
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 <[email protected]>
> Suggested-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Azhar Shaikh <[email protected]>
> ---
> 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.
/Jarkko
>-----Original Message-----
>From: Jarkko Sakkinen [mailto:[email protected]]
>Sent: Friday, December 22, 2017 10:42 AM
>To: Shaikh, Azhar <[email protected]>
>Cc: [email protected]; [email protected]; [email protected]; linux-security-
>[email protected]; [email protected]; linux-
>[email protected]
>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 <[email protected]>
>> Suggested-by: Jason Gunthorpe <[email protected]>
>> Signed-off-by: Azhar Shaikh <[email protected]>
>> ---
>> 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()"
>/Jarkko
Regards,
Azhar Shaikh
>-----Original Message-----
>From: [email protected] [mailto:linux-integrity-
>[email protected]] On Behalf Of Shaikh, Azhar
>Sent: Friday, December 22, 2017 11:15 AM
>To: Jarkko Sakkinen <[email protected]>
>Cc: [email protected]; [email protected]; [email protected]; linux-security-
>[email protected]; [email protected]; linux-
>[email protected]
>Subject: RE: [PATCH v2] tpm: Fix the driver cleanup code
>
>
>
>>-----Original Message-----
>>From: Jarkko Sakkinen [mailto:[email protected]]
>>Sent: Friday, December 22, 2017 10:42 AM
>>To: Shaikh, Azhar <[email protected]>
>>Cc: [email protected]; [email protected]; [email protected];
>>linux-security- [email protected];
>>[email protected]; linux- [email protected]
>>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 <[email protected]>
>>> Suggested-by: Jason Gunthorpe <[email protected]>
>>> Signed-off-by: Azhar Shaikh <[email protected]>
>>> ---
>>> 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