diff mbox series

tpm: make locality handling resilient

Message ID 20240115011546.21193-1-dpsmith@apertussolutions.com
State New
Headers
Series tpm: make locality handling resilient |

Commit Message

Daniel P. Smith Jan. 15, 2024, 1:15 a.m. UTC 
  Commit 933bfc5ad213 introduced the use of a locality counter to control when
locality request was actually sent to the TPM. This locality counter created a
hard enforcement that the TPM had no active locality at the time of the driver
initialization. The reality is that this may not always be the case coupled
with the fact that the commit indiscriminately decremented the counter created
the condition for integer underflow of the counter. The underflow was triggered
by the first pair of request/relinquish calls made in tpm_tis_init_core and all
subsequent calls to request/relinquished calls would have the counter flipping
between the underflow value and 0. The result is that it appeared all calls to
request/relinquish were successful, but they were not. The end result is that
the locality that was active when the driver loaded would always remain active,
to include after the driver shutdown. This creates a significant issue when
using Intel TXT and Locality 2 is active at boot. After the GETSEC[SEXIT]
instruction is called, the PCH will close access to Locality 2 MMIO address
space, leaving the TPM locked in Locality 2 with no means to relinquish the
locality until system reset.

The commit seeks to address this situation through three changes. The first is
to walk the localities during initialization and close any open localities to
ensure the TPM is in the assumed state. Next is to put guards around the
counter and the requested locality to ensure they remain within valid values.
The last change is to make the request locality functions be consistent in
their return values. The functions will either return the locality requested if
successful or a negative error code.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
Reported-by: Kanth Ghatraju <kanth.ghatraju@oracle.com>
Fixes: 933bfc5ad213 ("tpm, tpm: Implement usage counter for locality")
---
 drivers/char/tpm/tpm-chip.c     |  2 +-
 drivers/char/tpm/tpm_tis_core.c | 20 +++++++++++++++-----
 include/linux/tpm.h             |  2 ++
 3 files changed, 18 insertions(+), 6 deletions(-)

Comments

Alexander Steffen Jan. 17, 2024, 8:44 a.m. UTC | #1 
On 15.01.2024 02:15, Daniel P. Smith wrote:
> Commit 933bfc5ad213 introduced the use of a locality counter to control when
> locality request was actually sent to the TPM. This locality counter created a
> hard enforcement that the TPM had no active locality at the time of the driver
> initialization. The reality is that this may not always be the case coupled
> with the fact that the commit indiscriminately decremented the counter created
> the condition for integer underflow of the counter. The underflow was triggered
> by the first pair of request/relinquish calls made in tpm_tis_init_core and all
> subsequent calls to request/relinquished calls would have the counter flipping
> between the underflow value and 0. The result is that it appeared all calls to
> request/relinquish were successful, but they were not. The end result is that
> the locality that was active when the driver loaded would always remain active,
> to include after the driver shutdown. This creates a significant issue when
> using Intel TXT and Locality 2 is active at boot. After the GETSEC[SEXIT]
> instruction is called, the PCH will close access to Locality 2 MMIO address
> space, leaving the TPM locked in Locality 2 with no means to relinquish the
> locality until system reset.
> 
> The commit seeks to address this situation through three changes.

Could you split this up into multiple patches then, so that they can be 
discussed separately?

> The first is
> to walk the localities during initialization and close any open localities to
> ensure the TPM is in the assumed state. Next is to put guards around the
> counter and the requested locality to ensure they remain within valid values.
> The last change is to make the request locality functions be consistent in
> their return values. The functions will either return the locality requested if
> successful or a negative error code.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> Reported-by: Kanth Ghatraju <kanth.ghatraju@oracle.com>
> Fixes: 933bfc5ad213 ("tpm, tpm: Implement usage counter for locality")
> ---
>   drivers/char/tpm/tpm-chip.c     |  2 +-
>   drivers/char/tpm/tpm_tis_core.c | 20 +++++++++++++++-----
>   include/linux/tpm.h             |  2 ++
>   3 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 42b1062e33cd..e7293f85335a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -49,7 +49,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
>                  return rc;
> 
>          chip->locality = rc;
> -       return 0;
> +       return chip->locality;
>   }
> 
>   static void tpm_relinquish_locality(struct tpm_chip *chip)
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 1b350412d8a6..c8b9b0b199dc 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -180,7 +180,8 @@ static int tpm_tis_relinquish_locality(struct tpm_chip *chip, int l)
>          struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> 
>          mutex_lock(&priv->locality_count_mutex);
> -       priv->locality_count--;
> +       if (priv->locality_count > 0)
> +               priv->locality_count--;
>          if (priv->locality_count == 0)
>                  __tpm_tis_relinquish_locality(priv, l);
>          mutex_unlock(&priv->locality_count_mutex);
> @@ -226,18 +227,21 @@ static int __tpm_tis_request_locality(struct tpm_chip *chip, int l)
>                          tpm_msleep(TPM_TIMEOUT);
>                  } while (time_before(jiffies, stop));
>          }
> -       return -1;
> +       return -EBUSY;
>   }
> 
>   static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
>   {
>          struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -       int ret = 0;
> +       int ret = -EIO;
> +
> +       if (l > TPM_MAX_LOCALITY)
> +               return -EINVAL;
> 
>          mutex_lock(&priv->locality_count_mutex);
>          if (priv->locality_count == 0)
>                  ret = __tpm_tis_request_locality(chip, l);
> -       if (!ret)
> +       if (ret >= 0)
>                  priv->locality_count++;
>          mutex_unlock(&priv->locality_count_mutex);
>          return ret;

This line seems to be the most important change, that fixes the 
locality_count handling for localities != 0. It could already be 
sufficient to fix your original problem. I'm not sure all the other 
changes are really necessary.

> @@ -1108,7 +1112,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, locality;
>          struct tpm_chip *chip;
> 
>          chip = tpmm_chip_alloc(dev, &tpm_tis);
> @@ -1169,6 +1173,12 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>                  goto out_err;
>          }
> 
> +       /* It is not safe to assume localities are closed on startup */
> +       for (locality = 0; locality <= TPM_MAX_LOCALITY; locality++) {
> +               if (check_locality(chip, locality))
> +                       tpm_tis_relinquish_locality(chip, locality);
> +       }
> +
>          /* 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..f2651281f02e 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -116,6 +116,8 @@ struct tpm_chip_seqops {
>          const struct seq_operations *seqops;
>   };
> 
> +#define TPM_MAX_LOCALITY               4
> +
>   struct tpm_chip {
>          struct device dev;
>          struct device devs;
> --
> 2.30.2
>
Jarkko Sakkinen Jan. 19, 2024, 9:28 p.m. UTC | #2 
On Mon Jan 15, 2024 at 1:15 AM UTC, Daniel P. Smith wrote:
> Commit 933bfc5ad213 introduced the use of a locality counter to control when
> locality request was actually sent to the TPM. This locality counter created a
> hard enforcement that the TPM had no active locality at the time of the driver
> initialization. The reality is that this may not always be the case coupled
> with the fact that the commit indiscriminately decremented the counter created
> the condition for integer underflow of the counter. The underflow was triggered
> by the first pair of request/relinquish calls made in tpm_tis_init_core and all
> subsequent calls to request/relinquished calls would have the counter flipping
> between the underflow value and 0. The result is that it appeared all calls to
> request/relinquish were successful, but they were not. The end result is that
> the locality that was active when the driver loaded would always remain active,
> to include after the driver shutdown. This creates a significant issue when
> using Intel TXT and Locality 2 is active at boot. After the GETSEC[SEXIT]
> instruction is called, the PCH will close access to Locality 2 MMIO address
> space, leaving the TPM locked in Locality 2 with no means to relinquish the
> locality until system reset.
>
> The commit seeks to address this situation through three changes. The first is
> to walk the localities during initialization and close any open localities to
> ensure the TPM is in the assumed state. Next is to put guards around the
> counter and the requested locality to ensure they remain within valid values.
> The last change is to make the request locality functions be consistent in
> their return values. The functions will either return the locality requested if
> successful or a negative error code.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> Reported-by: Kanth Ghatraju <kanth.ghatraju@oracle.com>
> Fixes: 933bfc5ad213 ("tpm, tpm: Implement usage counter for locality")
> ---
>  drivers/char/tpm/tpm-chip.c     |  2 +-
>  drivers/char/tpm/tpm_tis_core.c | 20 +++++++++++++++-----
>  include/linux/tpm.h             |  2 ++
>  3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 42b1062e33cd..e7293f85335a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -49,7 +49,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
>  		return rc;
>  
>  	chip->locality = rc;
> -	return 0;
> +	return chip->locality;
>  }
>  
>  static void tpm_relinquish_locality(struct tpm_chip *chip)
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 1b350412d8a6..c8b9b0b199dc 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -180,7 +180,8 @@ static int tpm_tis_relinquish_locality(struct tpm_chip *chip, int l)
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>  
>  	mutex_lock(&priv->locality_count_mutex);
> -	priv->locality_count--;
> +	if (priv->locality_count > 0)
> +		priv->locality_count--;
>  	if (priv->locality_count == 0)
>  		__tpm_tis_relinquish_locality(priv, l);
>  	mutex_unlock(&priv->locality_count_mutex);
> @@ -226,18 +227,21 @@ static int __tpm_tis_request_locality(struct tpm_chip *chip, int l)
>  			tpm_msleep(TPM_TIMEOUT);
>  		} while (time_before(jiffies, stop));
>  	}
> -	return -1;
> +	return -EBUSY;
>  }
>  
>  static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	int ret = 0;
> +	int ret = -EIO;
> +
> +	if (l > TPM_MAX_LOCALITY)
> +		return -EINVAL;
>  
>  	mutex_lock(&priv->locality_count_mutex);
>  	if (priv->locality_count == 0)
>  		ret = __tpm_tis_request_locality(chip, l);
> -	if (!ret)
> +	if (ret >= 0)
>  		priv->locality_count++;
>  	mutex_unlock(&priv->locality_count_mutex);
>  	return ret;
> @@ -1108,7 +1112,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, locality;

s/locality/i/

We don't use long names for loop indices generally.

>  	struct tpm_chip *chip;
>  
>  	chip = tpmm_chip_alloc(dev, &tpm_tis);
> @@ -1169,6 +1173,12 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		goto out_err;
>  	}
>  
> +	/* It is not safe to assume localities are closed on startup */

This is somewhat useless comment.

E.g. this would be way more useful:

	/*
	 * Intel TXT starts with locality 2 active. Therefore,
	 * localities cannot be assumed to be closed on startup.
	 */

> +	for (locality = 0; locality <= TPM_MAX_LOCALITY; locality++) {
> +		if (check_locality(chip, locality))
> +			tpm_tis_relinquish_locality(chip, locality);
> +	}
> +
>  	/* 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..f2651281f02e 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -116,6 +116,8 @@ struct tpm_chip_seqops {
>  	const struct seq_operations *seqops;
>  };
>  
> +#define TPM_MAX_LOCALITY		4

Not documented.

> +
>  struct tpm_chip {
>  	struct device dev;
>  	struct device devs;

Thanks for the fix.

BR, Jarkko
Jarkko Sakkinen Jan. 19, 2024, 9:38 p.m. UTC | #3 
On Wed Jan 17, 2024 at 8:44 AM UTC, Alexander Steffen wrote:
> On 15.01.2024 02:15, Daniel P. Smith wrote:
> > Commit 933bfc5ad213 introduced the use of a locality counter to control when
> > locality request was actually sent to the TPM. This locality counter created a
> > hard enforcement that the TPM had no active locality at the time of the driver
> > initialization. The reality is that this may not always be the case coupled
> > with the fact that the commit indiscriminately decremented the counter created
> > the condition for integer underflow of the counter. The underflow was triggered
> > by the first pair of request/relinquish calls made in tpm_tis_init_core and all
> > subsequent calls to request/relinquished calls would have the counter flipping
> > between the underflow value and 0. The result is that it appeared all calls to
> > request/relinquish were successful, but they were not. The end result is that
> > the locality that was active when the driver loaded would always remain active,
> > to include after the driver shutdown. This creates a significant issue when
> > using Intel TXT and Locality 2 is active at boot. After the GETSEC[SEXIT]
> > instruction is called, the PCH will close access to Locality 2 MMIO address
> > space, leaving the TPM locked in Locality 2 with no means to relinquish the
> > locality until system reset.
> > 
> > The commit seeks to address this situation through three changes.
>
> Could you split this up into multiple patches then, so that they can be 
> discussed separately?

I have to agree with you ttly.

Yeah also the text above is not exactly in the ballpark.

I did not understand what I read. I had to read the code change instead
to get an idea. A huge pile of text does not equal to stronger story.

Like for any essay, scientific paper or a kernel message one should do
also few edit rounds. The commit message is more important than the code
change itself in bug fixes...

There is trigger (TXT) and solution. A great commit message should have
motivation and implementation parts and somewhat concise story where
things lead to another. It should essentially make *any* reader who
knows the basics of kernel code base convinced, not confused. This is
at leat a good aim even tho sometimes unreachable.

BR, Jarkko
Daniel P. Smith Jan. 25, 2024, 12:01 a.m. UTC | #4 
On 1/17/24 03:44, Alexander Steffen wrote:
> On 15.01.2024 02:15, Daniel P. Smith wrote:
>> Commit 933bfc5ad213 introduced the use of a locality counter to 
>> control when
>> locality request was actually sent to the TPM. This locality counter 
>> created a
>> hard enforcement that the TPM had no active locality at the time of 
>> the driver
>> initialization. The reality is that this may not always be the case 
>> coupled
>> with the fact that the commit indiscriminately decremented the counter 
>> created
>> the condition for integer underflow of the counter. The underflow was 
>> triggered
>> by the first pair of request/relinquish calls made in 
>> tpm_tis_init_core and all
>> subsequent calls to request/relinquished calls would have the counter 
>> flipping
>> between the underflow value and 0. The result is that it appeared all 
>> calls to
>> request/relinquish were successful, but they were not. The end result 
>> is that
>> the locality that was active when the driver loaded would always 
>> remain active,
>> to include after the driver shutdown. This creates a significant issue 
>> when
>> using Intel TXT and Locality 2 is active at boot. After the GETSEC[SEXIT]
>> instruction is called, the PCH will close access to Locality 2 MMIO 
>> address
>> space, leaving the TPM locked in Locality 2 with no means to 
>> relinquish the
>> locality until system reset.
>>
>> The commit seeks to address this situation through three changes.
> 
> Could you split this up into multiple patches then, so that they can be 
> discussed separately?

Gladly, but individually none of these fully address the situation.

>> The first is
>> to walk the localities during initialization and close any open 
>> localities to
>> ensure the TPM is in the assumed state. Next is to put guards around the
>> counter and the requested locality to ensure they remain within valid 
>> values.
>> The last change is to make the request locality functions be 
>> consistent in
>> their return values. The functions will either return the locality 
>> requested if
>> successful or a negative error code.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>> Reported-by: Kanth Ghatraju <kanth.ghatraju@oracle.com>
>> Fixes: 933bfc5ad213 ("tpm, tpm: Implement usage counter for locality")
>> ---
>>   drivers/char/tpm/tpm-chip.c     |  2 +-
>>   drivers/char/tpm/tpm_tis_core.c | 20 +++++++++++++++-----
>>   include/linux/tpm.h             |  2 ++
>>   3 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 42b1062e33cd..e7293f85335a 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -49,7 +49,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
>>                  return rc;
>>
>>          chip->locality = rc;
>> -       return 0;
>> +       return chip->locality;
>>   }
>>
>>   static void tpm_relinquish_locality(struct tpm_chip *chip)
>> diff --git a/drivers/char/tpm/tpm_tis_core.c 
>> b/drivers/char/tpm/tpm_tis_core.c
>> index 1b350412d8a6..c8b9b0b199dc 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -180,7 +180,8 @@ static int tpm_tis_relinquish_locality(struct 
>> tpm_chip *chip, int l)
>>          struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>
>>          mutex_lock(&priv->locality_count_mutex);
>> -       priv->locality_count--;
>> +       if (priv->locality_count > 0)
>> +               priv->locality_count--;
>>          if (priv->locality_count == 0)
>>                  __tpm_tis_relinquish_locality(priv, l);
>>          mutex_unlock(&priv->locality_count_mutex);
>> @@ -226,18 +227,21 @@ static int __tpm_tis_request_locality(struct 
>> tpm_chip *chip, int l)
>>                          tpm_msleep(TPM_TIMEOUT);
>>                  } while (time_before(jiffies, stop));
>>          }
>> -       return -1;
>> +       return -EBUSY;
>>   }
>>
>>   static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
>>   {
>>          struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> -       int ret = 0;
>> +       int ret = -EIO;
>> +
>> +       if (l > TPM_MAX_LOCALITY)
>> +               return -EINVAL;
>>
>>          mutex_lock(&priv->locality_count_mutex);
>>          if (priv->locality_count == 0)
>>                  ret = __tpm_tis_request_locality(chip, l);
>> -       if (!ret)
>> +       if (ret >= 0)
>>                  priv->locality_count++;
>>          mutex_unlock(&priv->locality_count_mutex);
>>          return ret;
> 
> This line seems to be the most important change, that fixes the 
> locality_count handling for localities != 0. It could already be 
> sufficient to fix your original problem. I'm not sure all the other 
> changes are really necessary.

It do not believe this fully address the issue. It does stop the 
underflow but locality still is not being properly tracked.

>> @@ -1108,7 +1112,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, locality;
>>          struct tpm_chip *chip;
>>
>>          chip = tpmm_chip_alloc(dev, &tpm_tis);
>> @@ -1169,6 +1173,12 @@ int tpm_tis_core_init(struct device *dev, 
>> struct tpm_tis_data *priv, int irq,
>>                  goto out_err;
>>          }
>>
>> +       /* It is not safe to assume localities are closed on startup */
>> +       for (locality = 0; locality <= TPM_MAX_LOCALITY; locality++) {
>> +               if (check_locality(chip, locality))
>> +                       tpm_tis_relinquish_locality(chip, locality);
>> +       }
>> +
>>          /* 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..f2651281f02e 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -116,6 +116,8 @@ struct tpm_chip_seqops {
>>          const struct seq_operations *seqops;
>>   };
>>
>> +#define TPM_MAX_LOCALITY               4
>> +
>>   struct tpm_chip {
>>          struct device dev;
>>          struct device devs;
>> -- 
>> 2.30.2
>>
Daniel P. Smith Jan. 25, 2024, 12:12 a.m. UTC | #5 
On 1/19/24 16:28, Jarkko Sakkinen wrote:
> On Mon Jan 15, 2024 at 1:15 AM UTC, Daniel P. Smith wrote:
>> Commit 933bfc5ad213 introduced the use of a locality counter to control when
>> locality request was actually sent to the TPM. This locality counter created a
>> hard enforcement that the TPM had no active locality at the time of the driver
>> initialization. The reality is that this may not always be the case coupled
>> with the fact that the commit indiscriminately decremented the counter created
>> the condition for integer underflow of the counter. The underflow was triggered
>> by the first pair of request/relinquish calls made in tpm_tis_init_core and all
>> subsequent calls to request/relinquished calls would have the counter flipping
>> between the underflow value and 0. The result is that it appeared all calls to
>> request/relinquish were successful, but they were not. The end result is that
>> the locality that was active when the driver loaded would always remain active,
>> to include after the driver shutdown. This creates a significant issue when
>> using Intel TXT and Locality 2 is active at boot. After the GETSEC[SEXIT]
>> instruction is called, the PCH will close access to Locality 2 MMIO address
>> space, leaving the TPM locked in Locality 2 with no means to relinquish the
>> locality until system reset.
>>
>> The commit seeks to address this situation through three changes. The first is
>> to walk the localities during initialization and close any open localities to
>> ensure the TPM is in the assumed state. Next is to put guards around the
>> counter and the requested locality to ensure they remain within valid values.
>> The last change is to make the request locality functions be consistent in
>> their return values. The functions will either return the locality requested if
>> successful or a negative error code.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
>> Reported-by: Kanth Ghatraju <kanth.ghatraju@oracle.com>
>> Fixes: 933bfc5ad213 ("tpm, tpm: Implement usage counter for locality")
>> ---
>>   drivers/char/tpm/tpm-chip.c     |  2 +-
>>   drivers/char/tpm/tpm_tis_core.c | 20 +++++++++++++++-----
>>   include/linux/tpm.h             |  2 ++
>>   3 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 42b1062e33cd..e7293f85335a 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -49,7 +49,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
>>   		return rc;
>>   
>>   	chip->locality = rc;
>> -	return 0;
>> +	return chip->locality;
>>   }
>>   
>>   static void tpm_relinquish_locality(struct tpm_chip *chip)
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 1b350412d8a6..c8b9b0b199dc 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -180,7 +180,8 @@ static int tpm_tis_relinquish_locality(struct tpm_chip *chip, int l)
>>   	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>   
>>   	mutex_lock(&priv->locality_count_mutex);
>> -	priv->locality_count--;
>> +	if (priv->locality_count > 0)
>> +		priv->locality_count--;
>>   	if (priv->locality_count == 0)
>>   		__tpm_tis_relinquish_locality(priv, l);
>>   	mutex_unlock(&priv->locality_count_mutex);
>> @@ -226,18 +227,21 @@ static int __tpm_tis_request_locality(struct tpm_chip *chip, int l)
>>   			tpm_msleep(TPM_TIMEOUT);
>>   		} while (time_before(jiffies, stop));
>>   	}
>> -	return -1;
>> +	return -EBUSY;
>>   }
>>   
>>   static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
>>   {
>>   	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> -	int ret = 0;
>> +	int ret = -EIO;
>> +
>> +	if (l > TPM_MAX_LOCALITY)
>> +		return -EINVAL;
>>   
>>   	mutex_lock(&priv->locality_count_mutex);
>>   	if (priv->locality_count == 0)
>>   		ret = __tpm_tis_request_locality(chip, l);
>> -	if (!ret)
>> +	if (ret >= 0)
>>   		priv->locality_count++;
>>   	mutex_unlock(&priv->locality_count_mutex);
>>   	return ret;
>> @@ -1108,7 +1112,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, locality;
> 
> s/locality/i/
> 
> We don't use long names for loop indices generally.

No problem, will change.

>>   	struct tpm_chip *chip;
>>   
>>   	chip = tpmm_chip_alloc(dev, &tpm_tis);
>> @@ -1169,6 +1173,12 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>>   		goto out_err;
>>   	}
>>   
>> +	/* It is not safe to assume localities are closed on startup */
> 
> This is somewhat useless comment.
> 
> E.g. this would be way more useful:
> 
> 	/*
> 	 * Intel TXT starts with locality 2 active. Therefore,
> 	 * localities cannot be assumed to be closed on startup.
> 	 */

Okay, will expand.

>> +	for (locality = 0; locality <= TPM_MAX_LOCALITY; locality++) {
>> +		if (check_locality(chip, locality))
>> +			tpm_tis_relinquish_locality(chip, locality);
>> +	}
>> +
>>   	/* 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..f2651281f02e 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -116,6 +116,8 @@ struct tpm_chip_seqops {
>>   	const struct seq_operations *seqops;
>>   };
>>   
>> +#define TPM_MAX_LOCALITY		4
> 
> Not documented.

Okay, will add spec ref.

>> +
>>   struct tpm_chip {
>>   	struct device dev;
>>   	struct device devs;
> 
> Thanks for the fix.

Your welcome.

> BR, Jarkko

V/r,
DPS
Jarkko Sakkinen Feb. 1, 2024, 11:51 p.m. UTC | #6 
On Thu Jan 25, 2024 at 2:01 AM EET, Daniel P. Smith wrote:
> > Could you split this up into multiple patches then, so that they can be 
> > discussed separately?
>
> Gladly, but individually none of these fully address the situation.

The total number of scenarios described was exactly zero meaning for us
that a situation does not exist with our current pool of knowledge :-)

I wonder what sort of scenario requires all those changes applied at
once in order to get fixed.

BR, Jarkko
Lino Sanfilippo Feb. 2, 2024, 2:52 a.m. UTC | #7 
Hi,

On 15.01.24 02:15, Daniel P. Smith wrote:

> Commit 933bfc5ad213 introduced the use of a locality counter to control when
> locality request was actually sent to the TPM. This locality counter created a
> hard enforcement that the TPM had no active locality at the time of the driver
> initialization. The reality is that this may not always be the case coupled
> with the fact that the commit indiscriminately decremented the counter created
> the condition for integer underflow of the counter. The underflow was triggered
> by the first pair of request/relinquish calls made in tpm_tis_init_core and all
> subsequent calls to request/relinquished calls would have the counter flipping
> between the underflow value and 0. The result is that it appeared all calls to
> request/relinquish were successful, but they were not. The end result is that
> the locality that was active when the driver loaded would always remain active,
> to include after the driver shutdown. This creates a significant issue when
> using Intel TXT and Locality 2 is active at boot. After the GETSEC[SEXIT]
> instruction is called, the PCH will close access to Locality 2 MMIO address
> space, leaving the TPM locked in Locality 2 with no means to relinquish the
> locality until system reset.
> 
> The commit seeks to address this situation through three changes. The first is
> to walk the localities during initialization and close any open localities to
> ensure the TPM is in the assumed state. Next is to put guards around the
> counter and the requested locality to ensure they remain within valid values.
> The last change is to make the request locality functions be consistent in
> their return values. The functions will either return the locality requested if
> successful or a negative error code.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> Reported-by: Kanth Ghatraju <kanth.ghatraju@oracle.com>
> Fixes: 933bfc5ad213 ("tpm, tpm: Implement usage counter for locality")
> ---
>  drivers/char/tpm/tpm-chip.c     |  2 +-
>  drivers/char/tpm/tpm_tis_core.c | 20 +++++++++++++++-----
>  include/linux/tpm.h             |  2 ++
>  3 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 42b1062e33cd..e7293f85335a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -49,7 +49,7 @@ static int tpm_request_locality(struct tpm_chip *chip)
>                 return rc;
> 
>         chip->locality = rc;
> -       return 0;
> +       return chip->locality;
>  }
> 
>  static void tpm_relinquish_locality(struct tpm_chip *chip)
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 1b350412d8a6..c8b9b0b199dc 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -180,7 +180,8 @@ static int tpm_tis_relinquish_locality(struct tpm_chip *chip, int l)
>         struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> 
>         mutex_lock(&priv->locality_count_mutex);
> -       priv->locality_count--;
> +       if (priv->locality_count > 0)
> +               priv->locality_count--;
>         if (priv->locality_count == 0)
>                 __tpm_tis_relinquish_locality(priv, l);
>         mutex_unlock(&priv->locality_count_mutex);
> @@ -226,18 +227,21 @@ static int __tpm_tis_request_locality(struct tpm_chip *chip, int l)
>                         tpm_msleep(TPM_TIMEOUT);
>                 } while (time_before(jiffies, stop));
>         }
> -       return -1;
> +       return -EBUSY;

Why do we want to return -EBUSY now? This does not seem to have anything to do with the
issue you are trying to solve.

>  }
> 
>  static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
>  {
>         struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -       int ret = 0;
> +       int ret = -EIO;
> +
> +       if (l > TPM_MAX_LOCALITY)
> +               return -EINVAL;

How can it happen that l > TPM_MAX_LOCALITY?

> 
>         mutex_lock(&priv->locality_count_mutex);
>         if (priv->locality_count == 0)
>                 ret = __tpm_tis_request_locality(chip, l);
> -       if (!ret)
> +       if (ret >= 0)
>                 priv->locality_count++;
>         mutex_unlock(&priv->locality_count_mutex);
>         return ret;
> @@ -1108,7 +1112,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, locality;
>         struct tpm_chip *chip;
> 
>         chip = tpmm_chip_alloc(dev, &tpm_tis);
> @@ -1169,6 +1173,12 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>                 goto out_err;
>         }
> 
> +       /* It is not safe to assume localities are closed on startup */
> +       for (locality = 0; locality <= TPM_MAX_LOCALITY; locality++) {
> +               if (check_locality(chip, locality))
> +                       tpm_tis_relinquish_locality(chip, locality);
> +       }
> +

wait_startup() already needs a locality, so this has to be done before that function.
Furthermore you can simply use __tpm_tis_relinquish_locality() as there
is not concurrency involved at this point.
With that you can IMHO spare everything else and the complete fix can be broken down to:

		for (i = 0; i <= MAX_LOCALITY; i++)
			__tpm_tis_relinquish_locality(priv, 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..f2651281f02e 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -116,6 +116,8 @@ struct tpm_chip_seqops {
>         const struct seq_operations *seqops;
>  };
> 
> +#define TPM_MAX_LOCALITY               4
> +
>  struct tpm_chip {
>         struct device dev;
>         struct device devs;
> --
> 2.30.2
> 

Regards,
Lino
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 42b1062e33cd..e7293f85335a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -49,7 +49,7 @@  static int tpm_request_locality(struct tpm_chip *chip)
 		return rc;
 
 	chip->locality = rc;
-	return 0;
+	return chip->locality;
 }
 
 static void tpm_relinquish_locality(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 1b350412d8a6..c8b9b0b199dc 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -180,7 +180,8 @@  static int tpm_tis_relinquish_locality(struct tpm_chip *chip, int l)
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
 
 	mutex_lock(&priv->locality_count_mutex);
-	priv->locality_count--;
+	if (priv->locality_count > 0)
+		priv->locality_count--;
 	if (priv->locality_count == 0)
 		__tpm_tis_relinquish_locality(priv, l);
 	mutex_unlock(&priv->locality_count_mutex);
@@ -226,18 +227,21 @@  static int __tpm_tis_request_locality(struct tpm_chip *chip, int l)
 			tpm_msleep(TPM_TIMEOUT);
 		} while (time_before(jiffies, stop));
 	}
-	return -1;
+	return -EBUSY;
 }
 
 static int tpm_tis_request_locality(struct tpm_chip *chip, int l)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
-	int ret = 0;
+	int ret = -EIO;
+
+	if (l > TPM_MAX_LOCALITY)
+		return -EINVAL;
 
 	mutex_lock(&priv->locality_count_mutex);
 	if (priv->locality_count == 0)
 		ret = __tpm_tis_request_locality(chip, l);
-	if (!ret)
+	if (ret >= 0)
 		priv->locality_count++;
 	mutex_unlock(&priv->locality_count_mutex);
 	return ret;
@@ -1108,7 +1112,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, locality;
 	struct tpm_chip *chip;
 
 	chip = tpmm_chip_alloc(dev, &tpm_tis);
@@ -1169,6 +1173,12 @@  int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 		goto out_err;
 	}
 
+	/* It is not safe to assume localities are closed on startup */
+	for (locality = 0; locality <= TPM_MAX_LOCALITY; locality++) {
+		if (check_locality(chip, locality))
+			tpm_tis_relinquish_locality(chip, locality);
+	}
+
 	/* 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..f2651281f02e 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -116,6 +116,8 @@  struct tpm_chip_seqops {
 	const struct seq_operations *seqops;
 };
 
+#define TPM_MAX_LOCALITY		4
+
 struct tpm_chip {
 	struct device dev;
 	struct device devs;