diff mbox series

[2/3] tpm: ensure tpm is in known state at startup

Message ID 20240131170824.6183-3-dpsmith@apertussolutions.com
State New
Headers
Series [1/3] tpm: protect against locality counter underflow |

Commit Message

Daniel P. Smith Jan. 31, 2024, 5:08 p.m. UTC 
  When tis core initializes, it assumes all localities are closed. There
are cases when this may not be the case. This commit addresses this by
ensuring all localities are closed before initializing begins.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
---
 drivers/char/tpm/tpm_tis_core.c | 11 ++++++++++-
 include/linux/tpm.h             |  6 ++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen Feb. 1, 2024, 10:33 p.m. UTC | #1 
On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote:
> When tis core initializes, it assumes all localities are closed. There
       ~~~~~~~~
       tpm_tis_core

> are cases when this may not be the case. This commit addresses this by
> ensuring all localities are closed before initializing begins.

Remove the last sentence and replace with this paragraph:

"Address this by ensuring all the localities are closed in the beginning
of tpm_tis_core_init(). There are environments, like Intel TXT, which
may leave a locality open. Close all localities to start from a known
state."

BTW, why we should motivated to take this patch anyway?

Since the patch is not marked as a bug fix the commit message must pitch
why it is important to care.

> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 11 ++++++++++-
>  include/linux/tpm.h             |  6 ++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 4176d3bd1f04..5709f87991d9 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -1109,7 +1109,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  	u32 intmask;
>  	u32 clkrun_val;
>  	u8 rid;
> -	int rc, probe;
> +	int rc, probe, i;
>  	struct tpm_chip *chip;
>  
>  	chip = tpmm_chip_alloc(dev, &tpm_tis);
> @@ -1170,6 +1170,15 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		goto out_err;
>  	}
>  
> +	/*
> +	 * There are environments, like Intel TXT, that may leave a TPM
> +	 * locality open. Close all localities to start from a known state.
> +	 */
> +	for (i = 0; i <= TPM_MAX_LOCALITY; i++) {
> +		if (check_locality(chip, i))
> +			tpm_tis_relinquish_locality(chip, i);
> +	}
> +
>  	/* Take control of the TPM's interrupt hardware and shut it off */
>  	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
>  	if (rc < 0)
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 4ee9d13749ad..abe0d44d00ee 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -116,6 +116,12 @@ struct tpm_chip_seqops {
>  	const struct seq_operations *seqops;
>  };
>  
> +/*
> + * The maximum locality (0 - 4) for a TPM, as defined in section 3.2 of the
> + * Client Platform Profile Specification.
> + */
> +#define TPM_MAX_LOCALITY		4
> +
>  struct tpm_chip {
>  	struct device dev;
>  	struct device devs;

Is there a dependency to 1/3?

BR, Jarkko
Daniel P. Smith Feb. 19, 2024, 7:17 p.m. UTC | #2 
On 2/1/24 17:33, Jarkko Sakkinen wrote:
> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote:
>> When tis core initializes, it assumes all localities are closed. There
>         ~~~~~~~~
>         tpm_tis_core
> 
>> are cases when this may not be the case. This commit addresses this by
>> ensuring all localities are closed before initializing begins.
> 
> Remove the last sentence and replace with this paragraph:
> 
> "Address this by ensuring all the localities are closed in the beginning
> of tpm_tis_core_init(). There are environments, like Intel TXT, which
> may leave a locality open. Close all localities to start from a known
> state."

okay.

> BTW, why we should motivated to take this patch anyway?

Without this change, in this scenario the driver is unnecessarily 
thrashing the TPM with locality requests/relinquishes pairs for which 
will never take effect and that the TPM must do state change tracking. 
While I am confident that TPM chips are resilient to such abuse, I do 
not think it would be good form to knowingly allow such behavior to occur.

> Since the patch is not marked as a bug fix the commit message must pitch
> why it is important to care.

As far as I am aware, the TPM driver has always made this assumption, so 
I guess it could be written as a bug against the first commit of the 
driver. The reality is that it is more the case that the TPM driver has 
never completely supported higher localities. While there has been logic 
to support localities interface, the driver has always been hard wired 
to use locality 0.

Basically, this change makes the TPM driver function correctly when 
multiple localities are in use. I can write it up either way, just let 
me know.

>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>> ---
>>   drivers/char/tpm/tpm_tis_core.c | 11 ++++++++++-
>>   include/linux/tpm.h             |  6 ++++++
>>   2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 4176d3bd1f04..5709f87991d9 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -1109,7 +1109,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>   	u32 intmask;
>>   	u32 clkrun_val;
>>   	u8 rid;
>> -	int rc, probe;
>> +	int rc, probe, i;
>>   	struct tpm_chip *chip;
>>   
>>   	chip = tpmm_chip_alloc(dev, &tpm_tis);
>> @@ -1170,6 +1170,15 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>   		goto out_err;
>>   	}
>>   
>> +	/*
>> +	 * There are environments, like Intel TXT, that may leave a TPM
>> +	 * locality open. Close all localities to start from a known state.
>> +	 */
>> +	for (i = 0; i <= TPM_MAX_LOCALITY; i++) {
>> +		if (check_locality(chip, i))
>> +			tpm_tis_relinquish_locality(chip, i);
>> +	}
>> +
>>   	/* Take control of the TPM's interrupt hardware and shut it off */
>>   	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
>>   	if (rc < 0)
>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index 4ee9d13749ad..abe0d44d00ee 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -116,6 +116,12 @@ struct tpm_chip_seqops {
>>   	const struct seq_operations *seqops;
>>   };
>>   
>> +/*
>> + * The maximum locality (0 - 4) for a TPM, as defined in section 3.2 of the
>> + * Client Platform Profile Specification.
>> + */
>> +#define TPM_MAX_LOCALITY		4
>> +
>>   struct tpm_chip {
>>   	struct device dev;
>>   	struct device devs;
> 
> Is there a dependency to 1/3?

There is no direct dependency between these patches, but 1 and 2 are 
necessary to resolve the issue at hand while 3 corrects the return value 
behavior of the locality request function.

v/r,
dps
Jarkko Sakkinen Feb. 19, 2024, 8:17 p.m. UTC | #3 
On Mon Feb 19, 2024 at 7:17 PM UTC, Daniel P. Smith wrote:
> On 2/1/24 17:33, Jarkko Sakkinen wrote:
> > On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote:
> >> When tis core initializes, it assumes all localities are closed. There
> >         ~~~~~~~~
> >         tpm_tis_core
> > 
> >> are cases when this may not be the case. This commit addresses this by
> >> ensuring all localities are closed before initializing begins.
> > 
> > Remove the last sentence and replace with this paragraph:
> > 
> > "Address this by ensuring all the localities are closed in the beginning
> > of tpm_tis_core_init(). There are environments, like Intel TXT, which
> > may leave a locality open. Close all localities to start from a known
> > state."
>
> okay.
>
> > BTW, why we should motivated to take this patch anyway?
>
> Without this change, in this scenario the driver is unnecessarily 
> thrashing the TPM with locality requests/relinquishes pairs for which 
> will never take effect and that the TPM must do state change tracking. 
> While I am confident that TPM chips are resilient to such abuse, I do 
> not think it would be good form to knowingly allow such behavior to occur.

This would a factor better motivation part for the commit. I can 
buy this argument instead the one right now, thanks :-)

BR, Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 4176d3bd1f04..5709f87991d9 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1109,7 +1109,7 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	u32 intmask;
 	u32 clkrun_val;
 	u8 rid;
-	int rc, probe;
+	int rc, probe, i;
 	struct tpm_chip *chip;
 
 	chip = tpmm_chip_alloc(dev, &tpm_tis);
@@ -1170,6 +1170,15 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		goto out_err;
 	}
 
+	/*
+	 * There are environments, like Intel TXT, that may leave a TPM
+	 * locality open. Close all localities to start from a known state.
+	 */
+	for (i = 0; i <= TPM_MAX_LOCALITY; i++) {
+		if (check_locality(chip, i))
+			tpm_tis_relinquish_locality(chip, i);
+	}
+
 	/* Take control of the TPM's interrupt hardware and shut it off */
 	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
 	if (rc < 0)
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4ee9d13749ad..abe0d44d00ee 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -116,6 +116,12 @@  struct tpm_chip_seqops {
 	const struct seq_operations *seqops;
 };
 
+/*
+ * The maximum locality (0 - 4) for a TPM, as defined in section 3.2 of the
+ * Client Platform Profile Specification.
+ */
+#define TPM_MAX_LOCALITY		4
+
 struct tpm_chip {
 	struct device dev;
 	struct device devs;