| Message ID | 20240131170824.6183-3-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] tpm: protect against locality counter underflow | expand |
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
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
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 --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;