| Message ID | 20240115011546.21193-1-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: make locality handling resilient | expand |
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 >
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
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
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 >>
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
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
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 --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;