| Message ID | 20240131170824.6183-2-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: > Commit 933bfc5ad213 introduced the use of a locality counter to control when a > locality request is allowed to be sent to the TPM. In the commit, the counter > is indiscriminately decremented. Thus creating a situation for an integer > underflow of the counter. What is the sequence of events that leads to this triggering the underflow? This information should be represent in the commit message. > > 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_tis_core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 1b350412d8a6..4176d3bd1f04 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); BR, Jarkko
On 01.02.24 23:21, Jarkko Sakkinen wrote: > > On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >> Commit 933bfc5ad213 introduced the use of a locality counter to control when a >> locality request is allowed to be sent to the TPM. In the commit, the counter >> is indiscriminately decremented. Thus creating a situation for an integer >> underflow of the counter. > > What is the sequence of events that leads to this triggering the > underflow? This information should be represent in the commit message. > AFAIU this is: 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() for the first time, but since a locality is (unexpectedly) already active check_locality() and consequently __tpm_tis_request_locality() return "true". This prevents the locality_counter from being increased to 1, see ret = __tpm_tis_request_locality(chip, l); if (!ret) /* Counter not increased since ret == 1 */ priv->locality_count++; in tpm_tis_request_locality(). If now the locality is released the counter is decreased to below zero (resulting in an underflow since "locality_counter" is an unsigned int.
On Fri Feb 2, 2024 at 5:08 AM EET, Lino Sanfilippo wrote: > > > On 01.02.24 23:21, Jarkko Sakkinen wrote: > > > > > On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: > >> Commit 933bfc5ad213 introduced the use of a locality counter to control when a > >> locality request is allowed to be sent to the TPM. In the commit, the counter > >> is indiscriminately decremented. Thus creating a situation for an integer > >> underflow of the counter. > > > > What is the sequence of events that leads to this triggering the > > underflow? This information should be represent in the commit message. > > > > AFAIU this is: > > 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() > for the first time, but since a locality is (unexpectedly) already active check_locality() and consequently > __tpm_tis_request_locality() return "true". This prevents the locality_counter from being increased > to 1, see > > ret = __tpm_tis_request_locality(chip, l); > if (!ret) /* Counter not increased since ret == 1 */ > priv->locality_count++; > > in tpm_tis_request_locality(). > > If now the locality is released the counter is decreased to below zero (resulting > in an underflow since "locality_counter" is an unsigned int. Thanks, Daniel, can you transcript this to the commit message? BR, Jarkko
On 2/12/24 15:05, Jarkko Sakkinen wrote: > On Fri Feb 2, 2024 at 5:08 AM EET, Lino Sanfilippo wrote: >> >> >> On 01.02.24 23:21, Jarkko Sakkinen wrote: >> >>> >>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >>>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a >>>> locality request is allowed to be sent to the TPM. In the commit, the counter >>>> is indiscriminately decremented. Thus creating a situation for an integer >>>> underflow of the counter. >>> >>> What is the sequence of events that leads to this triggering the >>> underflow? This information should be represent in the commit message. >>> >> >> AFAIU this is: >> >> 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() >> for the first time, but since a locality is (unexpectedly) already active check_locality() and consequently >> __tpm_tis_request_locality() return "true". This prevents the locality_counter from being increased >> to 1, see >> >> ret = __tpm_tis_request_locality(chip, l); >> if (!ret) /* Counter not increased since ret == 1 */ >> priv->locality_count++; >> >> in tpm_tis_request_locality(). >> >> If now the locality is released the counter is decreased to below zero (resulting >> in an underflow since "locality_counter" is an unsigned int. > > Thanks, Daniel, can you transcript this to the commit message? ack v/r, dps
On 02.02.2024 04:08, Lino Sanfilippo wrote: > On 01.02.24 23:21, Jarkko Sakkinen wrote: > >> >> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a >>> locality request is allowed to be sent to the TPM. In the commit, the counter >>> is indiscriminately decremented. Thus creating a situation for an integer >>> underflow of the counter. >> >> What is the sequence of events that leads to this triggering the >> underflow? This information should be represent in the commit message. >> > > AFAIU this is: > > 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() > for the first time, but since a locality is (unexpectedly) already active > check_locality() and consequently __tpm_tis_request_locality() return "true". check_locality() returns true, but __tpm_tis_request_locality() returns the requested locality. Currently, this is always 0, so the check for !ret will always correctly indicate success and increment the locality_count. But since theoretically a locality != 0 could be requested, the correct fix would be to check for something like ret >= 0 or ret == l instead of !ret. Then the counter will also be incremented correctly for localities != 0, and no underflow will happen later on. Therefore, explicitly checking for an underflow is unnecessary and hides the real problem. > This prevents the locality_counter from being increased to 1, see > > ret = __tpm_tis_request_locality(chip, l); > if (!ret) /* Counter not increased since ret == 1 */ > priv->locality_count++; > > in tpm_tis_request_locality(). > > If now the locality is released the counter is decreased to below zero (resulting > in an underflow since "locality_counter" is an unsigned int.
On Tue Feb 20, 2024 at 8:42 PM EET, Alexander Steffen wrote: > On 02.02.2024 04:08, Lino Sanfilippo wrote: > > On 01.02.24 23:21, Jarkko Sakkinen wrote: > > > >> > >> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: > >>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a > >>> locality request is allowed to be sent to the TPM. In the commit, the counter > >>> is indiscriminately decremented. Thus creating a situation for an integer > >>> underflow of the counter. > >> > >> What is the sequence of events that leads to this triggering the > >> underflow? This information should be represent in the commit message. > >> > > > > AFAIU this is: > > > > 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() > > for the first time, but since a locality is (unexpectedly) already active > > check_locality() and consequently __tpm_tis_request_locality() return "true". > > check_locality() returns true, but __tpm_tis_request_locality() returns > the requested locality. Currently, this is always 0, so the check for > !ret will always correctly indicate success and increment the > locality_count. > > But since theoretically a locality != 0 could be requested, the correct > fix would be to check for something like ret >= 0 or ret == l instead of > !ret. Then the counter will also be incremented correctly for localities > != 0, and no underflow will happen later on. Therefore, explicitly > checking for an underflow is unnecessary and hides the real problem. Good point. I think that the check should contain info-level klog message of the event together with the check against the underflow. I think this is very useful info for live systems. BR, Jarkko
Hi, On 20.02.24 19:42, Alexander Steffen wrote: > ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover. > > > On 02.02.2024 04:08, Lino Sanfilippo wrote: >> On 01.02.24 23:21, Jarkko Sakkinen wrote: >> >>> >>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >>>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a >>>> locality request is allowed to be sent to the TPM. In the commit, the counter >>>> is indiscriminately decremented. Thus creating a situation for an integer >>>> underflow of the counter. >>> >>> What is the sequence of events that leads to this triggering the >>> underflow? This information should be represent in the commit message. >>> >> >> AFAIU this is: >> >> 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() >> for the first time, but since a locality is (unexpectedly) already active >> check_locality() and consequently __tpm_tis_request_locality() return "true". > > check_locality() returns true, but __tpm_tis_request_locality() returns > the requested locality. Currently, this is always 0, so the check for > !ret will always correctly indicate success and increment the > locality_count. > Will the TPM TIS CORE ever (have to) request another locality than 0? Maybe the best would be to hardcode TPM_ACCESS(0) and get rid of all the locality parameters that are passed from one function to another. But this is rather code optimization and not really required to fix the reported bug. As I already wrote in a former comment, the actual bug fix would IMHO simply be to make sure that no localities are held at the beginning of tpm_tis_core_init(): for (i = 0; i <= MAX_LOCALITY; i++) __tpm_tis_relinquish_locality(priv, i); before wait_startup() should be sufficient. Regards, Lino
On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: > Hi, > > On 20.02.24 19:42, Alexander Steffen wrote: > > ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover. > > > > > > On 02.02.2024 04:08, Lino Sanfilippo wrote: > >> On 01.02.24 23:21, Jarkko Sakkinen wrote: > >> > >>> > >>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: > >>>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a > >>>> locality request is allowed to be sent to the TPM. In the commit, the counter > >>>> is indiscriminately decremented. Thus creating a situation for an integer > >>>> underflow of the counter. > >>> > >>> What is the sequence of events that leads to this triggering the > >>> underflow? This information should be represent in the commit message. > >>> > >> > >> AFAIU this is: > >> > >> 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() > >> for the first time, but since a locality is (unexpectedly) already active > >> check_locality() and consequently __tpm_tis_request_locality() return "true". > > > > check_locality() returns true, but __tpm_tis_request_locality() returns > > the requested locality. Currently, this is always 0, so the check for > > !ret will always correctly indicate success and increment the > > locality_count. > > > > Will the TPM TIS CORE ever (have to) request another locality than 0? Maybe the best would > be to hardcode TPM_ACCESS(0) and get rid of all the locality parameters that are > passed from one function to another. Usually, or at least use cases I'm aware of, localities are per component. E.g. Intel TXT has one and Linux has another. There's been some proposals in the past here for hypervisor specific locality here at LKML they didn't lead to anything. If you are suggesting of removing "int l" parameter altogether, I do support that idea. > But this is rather code optimization and not really required to fix > the reported bug. Just adding here that I wish we also had a log transcript of bug, which is right now missing. The explanation believable enough to move forward but I still wish to see a log transcript. A/B testing of the bug and fix is something I'm lacking here. If anyone has ideas how to use QEMU to simulate what Intel TXT does with localities that would help. Most of us do not carry Intel TXT setup anywhere (home or office). Also even tho 0/3 has an explanation bug 1/3 does not have anything at all to make it to be counted as a bug fix. Pretty difficult to compare any possible proposals for a fix on this playground tbh. BR, Jarkko
On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: > for (i = 0; i <= MAX_LOCALITY; i++) > __tpm_tis_relinquish_locality(priv, i); I'm pretty unfamiliar with Intel TXT so asking a dummy question: if Intel TXT uses locality 2 I suppose we should not try to relinquish it, or? AFAIK, we don't have a symbol called MAX_LOCALITY. BR, Jarkko
On Tue Feb 20, 2024 at 10:26 PM UTC, Jarkko Sakkinen wrote: > On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: > > for (i = 0; i <= MAX_LOCALITY; i++) > > __tpm_tis_relinquish_locality(priv, i); > > I'm pretty unfamiliar with Intel TXT so asking a dummy question: > if Intel TXT uses locality 2 I suppose we should not try to > relinquish it, or? > > AFAIK, we don't have a symbol called MAX_LOCALITY. OK it was called TPM_MAX_LOCALITY :-) I had the patch set applied in one branch but looked up with wrong symbol name. So I reformalize my question to two parts: 1. Why does TXT leave locality 2 open in the first place? I did not see explanation. Isn't this a bug in TXT? 2. Because localities are not too useful these days given TPM2's policy mechanism I cannot recall out of top of my head can you have two localities open at same time. So what kind of conflict happens when you try to open locality 0 and have locality 2 open? BR, Jarkko
On 2/20/24 2:26 PM, Jarkko Sakkinen wrote: > On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: >> for (i = 0; i <= MAX_LOCALITY; i++) >> __tpm_tis_relinquish_locality(priv, i); > > I'm pretty unfamiliar with Intel TXT so asking a dummy question: > if Intel TXT uses locality 2 I suppose we should not try to > relinquish it, or? The TPM has five localities (0 - 4). Localities 1 - 4 are for DRTM support. For TXT, locality 4 is hard wired to the CPU - nothing else can touch it. Locality 3 is only ever accessible when the CPU is executing an AC (Authenticated Code) module. That leaves 1 and 2 for the DRTM software environment to use. If the DRTM software opens 1 or 2, it should close them before exiting the DRTM. > > AFAIK, we don't have a symbol called MAX_LOCALITY. Daniel added it in the patch set. Thanks Ross > > BR, Jarkko
On Tue Feb 20, 2024 at 10:57 PM UTC, wrote: > On 2/20/24 2:26 PM, Jarkko Sakkinen wrote: > > On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: > >> for (i = 0; i <= MAX_LOCALITY; i++) > >> __tpm_tis_relinquish_locality(priv, i); > > > > I'm pretty unfamiliar with Intel TXT so asking a dummy question: > > if Intel TXT uses locality 2 I suppose we should not try to > > relinquish it, or? > > The TPM has five localities (0 - 4). Localities 1 - 4 are for DRTM > support. For TXT, locality 4 is hard wired to the CPU - nothing else can Locality 4 is familiar because it comes across from time to time. If I recall correctly DRTM should use only localities 3-4 and localities 0-2 should be reserved for the OS use. So this does not match what I recall unfortunately but I'm not really expert with this stuff. The patches has zero explanations SINIT ACM's behaviour on locality use and without that this cannot move forward because there's neither way to reproduce any of this. Actually there's zero effort on anything related to SINIT. > an AC (Authenticated Code) module. That leaves 1 and 2 for the DRTM > software environment to use. If the DRTM software opens 1 or 2, it > should close them before exiting the DRTM. > > > > > AFAIK, we don't have a symbol called MAX_LOCALITY. > > Daniel added it in the patch set. Got this, my symbol lookup just failed in my Git tree but looking at the patch set there was a symbol called *TPM_*MAX_LOCALITY :-) BR, Jarkko
On Tue Feb 20, 2024 at 11:10 PM UTC, Jarkko Sakkinen wrote > On Tue Feb 20, 2024 at 10:57 PM UTC, wrote: > > On 2/20/24 2:26 PM, Jarkko Sakkinen wrote: > > > On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: > > >> for (i = 0; i <= MAX_LOCALITY; i++) > > >> __tpm_tis_relinquish_locality(priv, i); > > > > > > I'm pretty unfamiliar with Intel TXT so asking a dummy question: > > > if Intel TXT uses locality 2 I suppose we should not try to > > > relinquish it, or? > > > > The TPM has five localities (0 - 4). Localities 1 - 4 are for DRTM > > support. For TXT, locality 4 is hard wired to the CPU - nothing else can > > Locality 4 is familiar because it comes across from time to time. > > If I recall correctly DRTM should use only localities 3-4 and > localities 0-2 should be reserved for the OS use. > > So this does not match what I recall unfortunately but I'm not > really expert with this stuff. > > The patches has zero explanations SINIT ACM's behaviour on > locality use and without that this cannot move forward because > there's neither way to reproduce any of this. > > Actually there's zero effort on anything related to SINIT. To put short we need a clearer sequence what Intel TXT does causing leaving localities does. If that is nailed to less abstract description then we can review this. If we know this blackbox model then there's a chance to make simulation of it e.g. with QEMU. Alternatively we need a more trivial scenario to reproduce a bug in locality handling than Intel TXT. There's no enough beef ATM to really make good decisions what sort of code change would be best for Linux. BR, Jarkko
On 20.02.24 23:23, Jarkko Sakkinen wrote: > On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: >> Hi, >> >> On 20.02.24 19:42, Alexander Steffen wrote: >>> ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover. >>> >>> >>> On 02.02.2024 04:08, Lino Sanfilippo wrote: >>>> On 01.02.24 23:21, Jarkko Sakkinen wrote: >>>> >>>>> >>>>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >>>>>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a >>>>>> locality request is allowed to be sent to the TPM. In the commit, the counter >>>>>> is indiscriminately decremented. Thus creating a situation for an integer >>>>>> underflow of the counter. >>>>> >>>>> What is the sequence of events that leads to this triggering the >>>>> underflow? This information should be represent in the commit message. >>>>> >>>> >>>> AFAIU this is: >>>> >>>> 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() >>>> for the first time, but since a locality is (unexpectedly) already active >>>> check_locality() and consequently __tpm_tis_request_locality() return "true". >>> >>> check_locality() returns true, but __tpm_tis_request_locality() returns >>> the requested locality. Currently, this is always 0, so the check for >>> !ret will always correctly indicate success and increment the >>> locality_count. >>> >> >> Will the TPM TIS CORE ever (have to) request another locality than 0? Maybe the best would >> be to hardcode TPM_ACCESS(0) and get rid of all the locality parameters that are >> passed from one function to another. > > Usually, or at least use cases I'm aware of, localities are per > component. E.g. Intel TXT has one and Linux has another. > > There's been some proposals in the past here for hypervisor specific > locality here at LKML they didn't lead to anything. > > If you are suggesting of removing "int l" parameter altogether, I > do support that idea. > Yes, removing the "l" parameter is what I meant. I can prepare a patch for the removal. Regards, Lino >> But this is rather code optimization and not really required to fix >> the reported bug. > > Just adding here that I wish we also had a log transcript of bug, which > is right now missing. The explanation believable enough to move forward > but I still wish to see a log transcript. > > A/B testing of the bug and fix is something I'm lacking here. If anyone > has ideas how to use QEMU to simulate what Intel TXT does with > localities that would help. > > Most of us do not carry Intel TXT setup anywhere (home or office). > > Also even tho 0/3 has an explanation bug 1/3 does not have anything at > all to make it to be counted as a bug fix. Pretty difficult to compare > any possible proposals for a fix on this playground tbh. > > BR, Jarkko >
On 20.02.24 23:31, Jarkko Sakkinen wrote: > ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover. > > > On Tue Feb 20, 2024 at 10:26 PM UTC, Jarkko Sakkinen wrote: >> On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: >>> for (i = 0; i <= MAX_LOCALITY; i++) >>> __tpm_tis_relinquish_locality(priv, i); >> >> I'm pretty unfamiliar with Intel TXT so asking a dummy question: >> if Intel TXT uses locality 2 I suppose we should not try to >> relinquish it, or? >> >> AFAIK, we don't have a symbol called MAX_LOCALITY. > > OK it was called TPM_MAX_LOCALITY :-) I had the patch set applied > in one branch but looked up with wrong symbol name. > Sorry for the confusion. The code was just meant to sketch a solution, it was written out of my head and I just remembered that Daniels patch set introduced some define for the max number of the localities. I did not look up the correct name though.
On Tue Feb 20, 2024 at 11:19 PM UTC, Lino Sanfilippo wrote: > > > On 20.02.24 23:23, Jarkko Sakkinen wrote: > > On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: > >> Hi, > >> > >> On 20.02.24 19:42, Alexander Steffen wrote: > >>> ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover. > >>> > >>> > >>> On 02.02.2024 04:08, Lino Sanfilippo wrote: > >>>> On 01.02.24 23:21, Jarkko Sakkinen wrote: > >>>> > >>>>> > >>>>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: > >>>>>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a > >>>>>> locality request is allowed to be sent to the TPM. In the commit, the counter > >>>>>> is indiscriminately decremented. Thus creating a situation for an integer > >>>>>> underflow of the counter. > >>>>> > >>>>> What is the sequence of events that leads to this triggering the > >>>>> underflow? This information should be represent in the commit message. > >>>>> > >>>> > >>>> AFAIU this is: > >>>> > >>>> 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() > >>>> for the first time, but since a locality is (unexpectedly) already active > >>>> check_locality() and consequently __tpm_tis_request_locality() return "true". > >>> > >>> check_locality() returns true, but __tpm_tis_request_locality() returns > >>> the requested locality. Currently, this is always 0, so the check for > >>> !ret will always correctly indicate success and increment the > >>> locality_count. > >>> > >> > >> Will the TPM TIS CORE ever (have to) request another locality than 0? Maybe the best would > >> be to hardcode TPM_ACCESS(0) and get rid of all the locality parameters that are > >> passed from one function to another. > > > > Usually, or at least use cases I'm aware of, localities are per > > component. E.g. Intel TXT has one and Linux has another. > > > > There's been some proposals in the past here for hypervisor specific > > locality here at LKML they didn't lead to anything. > > > > If you are suggesting of removing "int l" parameter altogether, I > > do support that idea. > > > > Yes, removing the "l" parameter is what I meant. I can prepare a patch for > the removal. This change BTW does not need to be supported by any bug per se as removing useless code is always welcome. If we wanted ever use let's say separate locality for hypervisor, we would want to design such feature from ground up. I don't think this will happen tho since localities are sort of trend that died with TPM 1.2... It had only authorization value and locality brought some additional granularity to it. As for this patch set I also don't think kernel should care about localities beyond 2 or at least not ever try relinquish them. I.e. it should at most relinquish localities 0-2. The only action taken for 3-4 should really be perhaps rollbacking the driver init and report to klog that these localities have been left open by the platform. BR, Jarkko
On Tue Feb 20, 2024 at 11:26 PM UTC, Lino Sanfilippo wrote: > > > On 20.02.24 23:31, Jarkko Sakkinen wrote: > > ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover. > > > > > > On Tue Feb 20, 2024 at 10:26 PM UTC, Jarkko Sakkinen wrote: > >> On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: > >>> for (i = 0; i <= MAX_LOCALITY; i++) > >>> __tpm_tis_relinquish_locality(priv, i); > >> > >> I'm pretty unfamiliar with Intel TXT so asking a dummy question: > >> if Intel TXT uses locality 2 I suppose we should not try to > >> relinquish it, or? > >> > >> AFAIK, we don't have a symbol called MAX_LOCALITY. > > > > OK it was called TPM_MAX_LOCALITY :-) I had the patch set applied > > in one branch but looked up with wrong symbol name. > > > > Sorry for the confusion. The code was just meant to sketch a solution, it was > written out of my head and I just remembered that Daniels patch set introduced > some define for the max number of the localities. I did not look up the correct > name though. NP BR, Jarkko
On Tue, 2024-02-20 at 22:31 +0000, Jarkko Sakkinen wrote: > > 2. Because localities are not too useful these days given TPM2's > policy mechanism Localitites are useful to the TPM2 policy mechanism. When we get key policy in the kernel it will give us a way to create TPM wrapped keys that can only be unwrapped in the kernel if we run the kernel in a different locality from userspace (I already have demo patches doing this). > I cannot recall out of top of my head can > you have two localities open at same time. I think there's a misunderstanding about what localities are: they're effectively an additional platform supplied tag to a command. Each command can therefore have one and only one locality. The TPM doesn't have a concept of which locality is open; if you look at the reference implementation, the simulator has a __plat__LocalitySet() function which places all commands in the just set locality until you change to a different one. However, since the way localities are implemented (i.e. what triggers _plat__LocalitySet()) is implementation defined, each physical TPM device has a different way of doing the set (for instance, for TIS TPM's locality is a function of the port set used to address the TPM; for CRB TPMs it can be an additional tag on the buffer for command submission). I think the locality request/relinquish was modelled after some other HW, but I don't know what. James
On Wed Feb 21, 2024 at 12:37 PM UTC, James Bottomley wrote: > On Tue, 2024-02-20 at 22:31 +0000, Jarkko Sakkinen wrote: > > > > 2. Because localities are not too useful these days given TPM2's > > policy mechanism > > Localitites are useful to the TPM2 policy mechanism. When we get key > policy in the kernel it will give us a way to create TPM wrapped keys > that can only be unwrapped in the kernel if we run the kernel in a > different locality from userspace (I already have demo patches doing > this). Let's keep this discussion in scope, please. Removing useless code using registers that you might have some actually useful use is not wrong thing to do. It is better to look at things from clean slate when the time comes. > > I cannot recall out of top of my head can > > you have two localities open at same time. > > I think there's a misunderstanding about what localities are: they're > effectively an additional platform supplied tag to a command. Each > command can therefore have one and only one locality. The TPM doesn't Actually this was not unclear at all. I even read the chapters from Ariel Segall's yesterday as a refresher. I was merely asking that if TPM_ACCESS_X is not properly cleared and you se TPM_ACCESS_Y where Y < X how does the hardware react as the bug report is pretty open ended and not very clear of the steps leading to unwanted results. With a quick check from [1] could not spot the conflict reaction but it is probably there. > submission). I think the locality request/relinquish was modelled > after some other HW, but I don't know what. My wild guess: first implementation was made when TPM's became available and there was no analytical thinking other than getting something that runs :-) > James [1] https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.pdf BR, Jarkko
On Wed Feb 21, 2024 at 7:43 PM UTC, Jarkko Sakkinen wrote: > On Wed Feb 21, 2024 at 12:37 PM UTC, James Bottomley wrote: > > On Tue, 2024-02-20 at 22:31 +0000, Jarkko Sakkinen wrote: > > > > > > 2. Because localities are not too useful these days given TPM2's > > > policy mechanism > > > > Localitites are useful to the TPM2 policy mechanism. When we get key > > policy in the kernel it will give us a way to create TPM wrapped keys > > that can only be unwrapped in the kernel if we run the kernel in a > > different locality from userspace (I already have demo patches doing > > this). > > Let's keep this discussion in scope, please. > > Removing useless code using registers that you might have some actually > useful use is not wrong thing to do. It is better to look at things from > clean slate when the time comes. > > > > I cannot recall out of top of my head can > > > you have two localities open at same time. > > > > I think there's a misunderstanding about what localities are: they're > > effectively an additional platform supplied tag to a command. Each > > command can therefore have one and only one locality. The TPM doesn't > > Actually this was not unclear at all. I even read the chapters from > Ariel Segall's yesterday as a refresher. Refering to https://www.amazon.com/Trusted-Platform-Modules-Computing-Networks/dp/1849198934 SBR, Jarkko
On Wed, 2024-02-21 at 19:43 +0000, Jarkko Sakkinen wrote: > On Wed Feb 21, 2024 at 12:37 PM UTC, James Bottomley wrote: > > On Tue, 2024-02-20 at 22:31 +0000, Jarkko Sakkinen wrote: [...] > > > I cannot recall out of top of my head can > > > you have two localities open at same time. > > > > I think there's a misunderstanding about what localities are: > > they're effectively an additional platform supplied tag to a > > command. Each command can therefore have one and only one > > locality. The TPM doesn't > > Actually this was not unclear at all. I even read the chapters from > Ariel Segall's yesterday as a refresher. > > I was merely asking that if TPM_ACCESS_X is not properly cleared and > you se TPM_ACCESS_Y where Y < X how does the hardware react as the > bug report is pretty open ended and not very clear of the steps > leading to unwanted results. So TPM_ACCESS_X is *not* a generic TPM thing, it's a TIS interface specific thing. Now the TIS interface seems to be dominating, so perhaps it is the correct programming model for us to follow, but not all current TPMs adhere to it. > With a quick check from [1] could not spot the conflict reaction but > it is probably there. The way platforms should handle localities is now detailed in the TCG library code snippets (part 4 Supporting Routines - Code): https://trustedcomputinggroup.org/resource/tpm-library-specification/ It's the _plat__LocalityGet/Set in Appendix C The implementation documented there is what the TPM reference implementation follows. James
On Thu Feb 22, 2024 at 11:06 AM EET, James Bottomley wrote: > On Wed, 2024-02-21 at 19:43 +0000, Jarkko Sakkinen wrote: > > On Wed Feb 21, 2024 at 12:37 PM UTC, James Bottomley wrote: > > > On Tue, 2024-02-20 at 22:31 +0000, Jarkko Sakkinen wrote: > [...] > > > > I cannot recall out of top of my head can > > > > you have two localities open at same time. > > > > > > I think there's a misunderstanding about what localities are: > > > they're effectively an additional platform supplied tag to a > > > command. Each command can therefore have one and only one > > > locality. The TPM doesn't > > > > Actually this was not unclear at all. I even read the chapters from > > Ariel Segall's yesterday as a refresher. > > > > I was merely asking that if TPM_ACCESS_X is not properly cleared and > > you se TPM_ACCESS_Y where Y < X how does the hardware react as the > > bug report is pretty open ended and not very clear of the steps > > leading to unwanted results. > > So TPM_ACCESS_X is *not* a generic TPM thing, it's a TIS interface > specific thing. Now the TIS interface seems to be dominating, so > perhaps it is the correct programming model for us to follow, but not > all current TPMs adhere to it. I know, I only have CRB based TPMs in my host machines but here the context is TIS interface so in this scope it's what we care about. We're trying to fix a bug here, not speculate what additional features could be done with localities. BR, Jarkko
On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: > Commit 933bfc5ad213 introduced the use of a locality counter to control when a > locality request is allowed to be sent to the TPM. In the commit, the counter > is indiscriminately decremented. Thus creating a situation for an integer > underflow of the counter. > > 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_tis_core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 1b350412d8a6..4176d3bd1f04 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); To make this patch set better the whole story should be scenario based. Starting from cover letter the explanation is way too rounded to guide to the conclusion that these are actually best possible code changes to fix the issue. I agree fully on that the problem should be fixed but given that the scenarios are fuzzy deciding whether this the right things done right is the open question. I.e. we need the steps for destruction and how these patches change those steps to make this right. Since the whole topic is complicated I'd use PC Client spec as reference. BR, Jarkko
On 2/20/24 13:42, Alexander Steffen wrote: > On 02.02.2024 04:08, Lino Sanfilippo wrote: >> On 01.02.24 23:21, Jarkko Sakkinen wrote: >> >>> >>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >>>> Commit 933bfc5ad213 introduced the use of a locality counter to >>>> control when a >>>> locality request is allowed to be sent to the TPM. In the commit, >>>> the counter >>>> is indiscriminately decremented. Thus creating a situation for an >>>> integer >>>> underflow of the counter. >>> >>> What is the sequence of events that leads to this triggering the >>> underflow? This information should be represent in the commit message. >>> >> >> AFAIU this is: >> >> 1. We start with a locality_counter of 0 and then we call >> tpm_tis_request_locality() >> for the first time, but since a locality is (unexpectedly) already active >> check_locality() and consequently __tpm_tis_request_locality() return >> "true". > > check_locality() returns true, but __tpm_tis_request_locality() returns > the requested locality. Currently, this is always 0, so the check for > !ret will always correctly indicate success and increment the > locality_count. > > But since theoretically a locality != 0 could be requested, the correct > fix would be to check for something like ret >= 0 or ret == l instead of > !ret. Then the counter will also be incremented correctly for localities > != 0, and no underflow will happen later on. Therefore, explicitly > checking for an underflow is unnecessary and hides the real problem. > My apologies, but I will have to humbly disagree from a fundamental level here. If a state variable has bounds, then those bounds should be enforced when the variable is being manipulated. Assuming that every path leading to the variable manipulation code has ensured proper manipulation is just that, an assumption. When assumptions fail is how bugs and vulnerabilities occur. To your point, does this full address the situation experienced, I would say it does not. IMHO, the situation is really a combination of both patch 1 and patch 2, but the request was to split the changes for individual discussion. We selected this one as being the fixes for two reasons. First, it blocks the underflow such that when the Secure Launch series opens Locality 2, it will get incremented at that time and the internal locality tracking state variables will end up with the correct values. Thus leading to the relinquish succeeding at kernel shutdown. Second, it provides a stronger defensive coding practice. Another reason that this works as a fix is that the TPM specification requires the registers to be mirrored across all localities, regardless of the active locality. While all the request/relinquishes for Locality 0 sent by the early code do not succeed, obtaining the values via the Locality 0 registers are still guaranteed to be correct. v/r, dps
On 2/20/24 15:54, Lino Sanfilippo wrote: > Hi, > > On 20.02.24 19:42, Alexander Steffen wrote: >> ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover. >> >> >> On 02.02.2024 04:08, Lino Sanfilippo wrote: >>> On 01.02.24 23:21, Jarkko Sakkinen wrote: >>> >>>> >>>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >>>>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a >>>>> locality request is allowed to be sent to the TPM. In the commit, the counter >>>>> is indiscriminately decremented. Thus creating a situation for an integer >>>>> underflow of the counter. >>>> >>>> What is the sequence of events that leads to this triggering the >>>> underflow? This information should be represent in the commit message. >>>> >>> >>> AFAIU this is: >>> >>> 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() >>> for the first time, but since a locality is (unexpectedly) already active >>> check_locality() and consequently __tpm_tis_request_locality() return "true". >> >> check_locality() returns true, but __tpm_tis_request_locality() returns >> the requested locality. Currently, this is always 0, so the check for >> !ret will always correctly indicate success and increment the >> locality_count. >> > > Will the TPM TIS CORE ever (have to) request another locality than 0? Maybe the best would > be to hardcode TPM_ACCESS(0) and get rid of all the locality parameters that are > passed from one function to another. > But this is rather code optimization and not really required to fix the reported bug. Actually, doing so will break the TPM API. The function tpm_tis_request_locality() is registered as the locality handler, int (*request_locality)(struct tpm_chip *chip, int loc), in the tis instance of struct tpm_class_ops{}. This is the API used by the Secure Launch series to open Locality2 for the measurements it must record. v/r, dps
On 2/20/24 17:31, Jarkko Sakkinen wrote: > On Tue Feb 20, 2024 at 10:26 PM UTC, Jarkko Sakkinen wrote: >> On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: >>> for (i = 0; i <= MAX_LOCALITY; i++) >>> __tpm_tis_relinquish_locality(priv, i); >> >> I'm pretty unfamiliar with Intel TXT so asking a dummy question: >> if Intel TXT uses locality 2 I suppose we should not try to >> relinquish it, or? >> >> AFAIK, we don't have a symbol called MAX_LOCALITY. > > OK it was called TPM_MAX_LOCALITY :-) I had the patch set applied > in one branch but looked up with wrong symbol name. > > So I reformalize my question to two parts: > > 1. Why does TXT leave locality 2 open in the first place? I did > not see explanation. Isn't this a bug in TXT? It does so because that is what the TCG D-RTM specification requires. See Section 5.3.4.10 of the TCG D-RTM specification[1], the first requirement is, "The DLME SHALL receive control with access to Locality 2." > 2. Because localities are not too useful these days given TPM2's > policy mechanism I cannot recall out of top of my head can > you have two localities open at same time. So what kind of > conflict happens when you try to open locality 0 and have > locality 2 open? I would disagree and would call your attention to the TCG's definition/motivation for localities, Section 3.2 of Client PTP specification[2]. "“Locality” is an assertion to the TPM that a command’s source is associated with a particular component. Locality can be thought of as a hardware-based authorization. The TPM is not actually aware of the nature of the relationship between the locality and the component. The ability to reset and extend notwithstanding, it is important to note that, from a PCR “usage” perspective, there is no hierarchical relationship between different localities. The TPM simply enforces locality restrictions on TPM assets (such as PCR or SEALed blobs)." As stated, from the TPM specification perspective, it is not aware of this mapping to components and leaves it to the platform to enforce. "The protection and separation of the localities (and therefore the association with the associated components) is entirely the responsibility of the platform components. Platform components, including the OS, may provide the separation of localities using protection mechanisms such as virtual memory or paging." The x86 manufactures opted to adopt the D-RTM specification which defines the components as follows: Locality 4: Usually associated with the CPU executing microcode. This is used to establish the Dynamic RTM. Locality 3: Auxiliary components. Use of this is optional and, if used, it is implementation dependent. Locality 2: Dynamically Launched OS (Dynamic OS) “runtime” environment. Locality 1: An environment for use by the Dynamic OS. Locality 0: The Static RTM, its chain of trust and its environment. And the means to protect and separate those localities are encoded in the x86 chipset, i.e A D-RTM Event must be used to access any of the D-RTM Localities (Locality1 - Locality4). For Intel, Locality 4 can only be accessed when a dedicated signal between the CPU and the chipset is raised, thus only allowing the CPU to utilize Locality 4. The CPU will then close Locality 4, authenticate and give control to the ACM with access to Locality 3. When the ACM is complete, it will instruct the chipset to lock Locality 3 and give control to the DLME (MLE in Intel parlance) with Locality 2 open. It is up to the DLME, the Linux kernel in this case, to decide how to assign components to Locality 1 and 2. As to proposals to utilize localities by the Linux kernel, the only one I was aware of was dropped because they couldn't open the higher localities. I would also highlight that the D-RTM implementation guide for Arm allows for a hardware D-RTM event, which the vendor may choose to implement a hardware/CPU enforced access to TPM localities. Thus, the ability to support localities will also become a requirement for certain Arm CPUs. [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_D-RTM_Architecture_v1-0_Published_06172013.pdf [2] https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.pdf
On 2/21/24 14:43, Jarkko Sakkinen wrote: > On Wed Feb 21, 2024 at 12:37 PM UTC, James Bottomley wrote: >> On Tue, 2024-02-20 at 22:31 +0000, Jarkko Sakkinen wrote: >>> >>> 2. Because localities are not too useful these days given TPM2's >>> policy mechanism >> >> Localitites are useful to the TPM2 policy mechanism. When we get key >> policy in the kernel it will give us a way to create TPM wrapped keys >> that can only be unwrapped in the kernel if we run the kernel in a >> different locality from userspace (I already have demo patches doing >> this). > > Let's keep this discussion in scope, please. > > Removing useless code using registers that you might have some actually > useful use is not wrong thing to do. It is better to look at things from > clean slate when the time comes. > >>> I cannot recall out of top of my head can >>> you have two localities open at same time. >> >> I think there's a misunderstanding about what localities are: they're >> effectively an additional platform supplied tag to a command. Each >> command can therefore have one and only one locality. The TPM doesn't > > Actually this was not unclear at all. I even read the chapters from > Ariel Segall's yesterday as a refresher. > > I was merely asking that if TPM_ACCESS_X is not properly cleared and you > se TPM_ACCESS_Y where Y < X how does the hardware react as the bug > report is pretty open ended and not very clear of the steps leading to > unwanted results. > > With a quick check from [1] could not spot the conflict reaction but > it is probably there. The expected behavior is explained in the Informative Comment of section 6.5.2.4 of the Client PTP spec[1]: "The purpose of this register is to allow the processes operating at the various localities to share the TPM. The basic notion is that any locality can request access to the TPM by setting the TPM_ACCESS_x.requestUse field using its assigned TPM_ACCESS_x register address. If there is no currently set locality, the TPM sets current locality to the requesting one and allows operations only from that locality. If the TPM is currently at another locality, the TPM keeps the request pending until the currently executing locality frees the TPM. Software relinquishes the TPM’s locality by writing a 1 to the TPM_ACCESS_x.activeLocality field. Upon release, the TPM honors the highest locality request pending. If there is no pending request, the TPM enters the “free” state." >> submission). I think the locality request/relinquish was modelled >> after some other HW, but I don't know what. > > My wild guess: first implementation was made when TPM's became available > and there was no analytical thinking other than getting something that > runs :-) Actually, no that is not how it was done. IIRC, localities were designed in conjunction with D-RTM when Intel and MS started the LeGrande effort back in 2000. It was then generalized for the TPM 1.1b specification. My first introduction to LeGrande/TXT wasn't until 2005 as part of an early access program. So most of my historical understanding is from discussions I luckily got to have with one of the architects and a few of the original TCG committee members. [1] https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.pdf v/r, dps
On 2/20/24 17:23, Jarkko Sakkinen wrote: > On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: >> Hi, >> >> On 20.02.24 19:42, Alexander Steffen wrote: >>> ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover. >>> >>> >>> On 02.02.2024 04:08, Lino Sanfilippo wrote: >>>> On 01.02.24 23:21, Jarkko Sakkinen wrote: >>>> >>>>> >>>>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >>>>>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a >>>>>> locality request is allowed to be sent to the TPM. In the commit, the counter >>>>>> is indiscriminately decremented. Thus creating a situation for an integer >>>>>> underflow of the counter. >>>>> >>>>> What is the sequence of events that leads to this triggering the >>>>> underflow? This information should be represent in the commit message. >>>>> >>>> >>>> AFAIU this is: >>>> >>>> 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() >>>> for the first time, but since a locality is (unexpectedly) already active >>>> check_locality() and consequently __tpm_tis_request_locality() return "true". >>> >>> check_locality() returns true, but __tpm_tis_request_locality() returns >>> the requested locality. Currently, this is always 0, so the check for >>> !ret will always correctly indicate success and increment the >>> locality_count. >>> >> >> Will the TPM TIS CORE ever (have to) request another locality than 0? Maybe the best would >> be to hardcode TPM_ACCESS(0) and get rid of all the locality parameters that are >> passed from one function to another. > > Usually, or at least use cases I'm aware of, localities are per > component. E.g. Intel TXT has one and Linux has another. > > There's been some proposals in the past here for hypervisor specific > locality here at LKML they didn't lead to anything. > > If you are suggesting of removing "int l" parameter altogether, I > do support that idea. > >> But this is rather code optimization and not really required to fix >> the reported bug. > > Just adding here that I wish we also had a log transcript of bug, which > is right now missing. The explanation believable enough to move forward > but I still wish to see a log transcript. That will be forth coming. v/r, dps
On Fri Feb 23, 2024 at 3:58 AM EET, Daniel P. Smith wrote: > > Just adding here that I wish we also had a log transcript of bug, which > > is right now missing. The explanation believable enough to move forward > > but I still wish to see a log transcript. > > That will be forth coming. I did not respond yet to other responses that you've given in the past 12'ish hours or so (just woke up) but I started to think how all this great and useful information would be best kept in memory. Some of it has been discussed in the past but there is lot of small details that are too easily forgotten. I'd think the best "documentation" approach here would be inject the spec references to the sites where locality behaviour is changed so that it is easy in future cross-reference them, and least of risk of having code changes that would break anything. I think this way all the information that you provided is best preserved for the future. Thanks a lot for great and informative responses! > v/r, > dps BR, Jarkko
On Fri Feb 23, 2024 at 3:57 AM EET, Daniel P. Smith wrote: > On 2/21/24 14:43, Jarkko Sakkinen wrote: > > On Wed Feb 21, 2024 at 12:37 PM UTC, James Bottomley wrote: > >> On Tue, 2024-02-20 at 22:31 +0000, Jarkko Sakkinen wrote: > >>> > >>> 2. Because localities are not too useful these days given TPM2's > >>> policy mechanism > >> > >> Localitites are useful to the TPM2 policy mechanism. When we get key > >> policy in the kernel it will give us a way to create TPM wrapped keys > >> that can only be unwrapped in the kernel if we run the kernel in a > >> different locality from userspace (I already have demo patches doing > >> this). > > > > Let's keep this discussion in scope, please. > > > > Removing useless code using registers that you might have some actually > > useful use is not wrong thing to do. It is better to look at things from > > clean slate when the time comes. > > > >>> I cannot recall out of top of my head can > >>> you have two localities open at same time. > >> > >> I think there's a misunderstanding about what localities are: they're > >> effectively an additional platform supplied tag to a command. Each > >> command can therefore have one and only one locality. The TPM doesn't > > > > Actually this was not unclear at all. I even read the chapters from > > Ariel Segall's yesterday as a refresher. > > > > I was merely asking that if TPM_ACCESS_X is not properly cleared and you > > se TPM_ACCESS_Y where Y < X how does the hardware react as the bug > > report is pretty open ended and not very clear of the steps leading to > > unwanted results. > > > > With a quick check from [1] could not spot the conflict reaction but > > it is probably there. > > The expected behavior is explained in the Informative Comment of section > 6.5.2.4 of the Client PTP spec[1]: > > "The purpose of this register is to allow the processes operating at the > various localities to share the TPM. The basic notion is that any > locality can request access to the TPM by setting the > TPM_ACCESS_x.requestUse field using its assigned TPM_ACCESS_x register > address. If there is no currently set locality, the TPM sets current > locality to the requesting one and allows operations only from that > locality. If the TPM is currently at another locality, the TPM keeps the > request pending until the currently executing locality frees the TPM. Right. I'd think it would make sense to document the basic dance like this as part of kdoc for request_locality: * Setting TPM_ACCESS_x.requestUse: * 1. No locality reserved => set locality. * 2. Locality reserved => set pending. I.e. easy reminder with enough granularity. > Software relinquishes the TPM’s locality by writing a 1 to the > TPM_ACCESS_x.activeLocality field. Upon release, the TPM honors the > highest locality request pending. If there is no pending request, the > TPM enters the “free” state." And this for relinquish_locality: * Setting TPM_ACCESS_x.activeLocality: * 1. No locality pending => free. * 2. Localities pending => reserve for highest. > >> submission). I think the locality request/relinquish was modelled > >> after some other HW, but I don't know what. > > > > My wild guess: first implementation was made when TPM's became available > > and there was no analytical thinking other than getting something that > > runs :-) > > Actually, no that is not how it was done. IIRC, localities were designed > in conjunction with D-RTM when Intel and MS started the LeGrande effort > back in 2000. It was then generalized for the TPM 1.1b specification. My OK, thanks for this bit of information! I did not know this. > first introduction to LeGrande/TXT wasn't until 2005 as part of an early > access program. So most of my historical understanding is from > discussions I luckily got to have with one of the architects and a few > of the original TCG committee members. Thanks alot for sharing this. > > [1] > https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.pdf > > v/r, > dps BR, Jarkko
On Fri Feb 23, 2024 at 10:40 PM EET, Jarkko Sakkinen wrote: > On Fri Feb 23, 2024 at 3:57 AM EET, Daniel P. Smith wrote: > > On 2/21/24 14:43, Jarkko Sakkinen wrote: > > > On Wed Feb 21, 2024 at 12:37 PM UTC, James Bottomley wrote: > > >> On Tue, 2024-02-20 at 22:31 +0000, Jarkko Sakkinen wrote: > > >>> > > >>> 2. Because localities are not too useful these days given TPM2's > > >>> policy mechanism > > >> > > >> Localitites are useful to the TPM2 policy mechanism. When we get key > > >> policy in the kernel it will give us a way to create TPM wrapped keys > > >> that can only be unwrapped in the kernel if we run the kernel in a > > >> different locality from userspace (I already have demo patches doing > > >> this). > > > > > > Let's keep this discussion in scope, please. > > > > > > Removing useless code using registers that you might have some actually > > > useful use is not wrong thing to do. It is better to look at things from > > > clean slate when the time comes. > > > > > >>> I cannot recall out of top of my head can > > >>> you have two localities open at same time. > > >> > > >> I think there's a misunderstanding about what localities are: they're > > >> effectively an additional platform supplied tag to a command. Each > > >> command can therefore have one and only one locality. The TPM doesn't > > > > > > Actually this was not unclear at all. I even read the chapters from > > > Ariel Segall's yesterday as a refresher. > > > > > > I was merely asking that if TPM_ACCESS_X is not properly cleared and you > > > se TPM_ACCESS_Y where Y < X how does the hardware react as the bug > > > report is pretty open ended and not very clear of the steps leading to > > > unwanted results. > > > > > > With a quick check from [1] could not spot the conflict reaction but > > > it is probably there. > > > > The expected behavior is explained in the Informative Comment of section > > 6.5.2.4 of the Client PTP spec[1]: > > > > "The purpose of this register is to allow the processes operating at the > > various localities to share the TPM. The basic notion is that any > > locality can request access to the TPM by setting the > > TPM_ACCESS_x.requestUse field using its assigned TPM_ACCESS_x register > > address. If there is no currently set locality, the TPM sets current > > locality to the requesting one and allows operations only from that > > locality. If the TPM is currently at another locality, the TPM keeps the > > request pending until the currently executing locality frees the TPM. > > Right. > > I'd think it would make sense to document the basic dance like this as > part of kdoc for request_locality: > > * Setting TPM_ACCESS_x.requestUse: > * 1. No locality reserved => set locality. > * 2. Locality reserved => set pending. > > I.e. easy reminder with enough granularity. Also for any non-TPM kernel developer this should be enough to get the basic gist of the mechanism without spending too much time reading. BR, Jarkko
On Fri Feb 23, 2024 at 3:56 AM EET, Daniel P. Smith wrote: > On 2/20/24 15:54, Lino Sanfilippo wrote: > > Hi, > > > > On 20.02.24 19:42, Alexander Steffen wrote: > >> ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover. > >> > >> > >> On 02.02.2024 04:08, Lino Sanfilippo wrote: > >>> On 01.02.24 23:21, Jarkko Sakkinen wrote: > >>> > >>>> > >>>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: > >>>>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a > >>>>> locality request is allowed to be sent to the TPM. In the commit, the counter > >>>>> is indiscriminately decremented. Thus creating a situation for an integer > >>>>> underflow of the counter. > >>>> > >>>> What is the sequence of events that leads to this triggering the > >>>> underflow? This information should be represent in the commit message. > >>>> > >>> > >>> AFAIU this is: > >>> > >>> 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() > >>> for the first time, but since a locality is (unexpectedly) already active > >>> check_locality() and consequently __tpm_tis_request_locality() return "true". > >> > >> check_locality() returns true, but __tpm_tis_request_locality() returns > >> the requested locality. Currently, this is always 0, so the check for > >> !ret will always correctly indicate success and increment the > >> locality_count. > >> > > > > Will the TPM TIS CORE ever (have to) request another locality than 0? Maybe the best would > > be to hardcode TPM_ACCESS(0) and get rid of all the locality parameters that are > > passed from one function to another. > > But this is rather code optimization and not really required to fix the reported bug. > > Actually, doing so will break the TPM API. The function > tpm_tis_request_locality() is registered as the locality handler, > int (*request_locality)(struct tpm_chip *chip, int loc), in the tis > instance of struct tpm_class_ops{}. This is the API used by the Secure > Launch series to open Locality2 for the measurements it must record. OK, based on James' earlier feedback on possibility to have kernel specific locality and this , and some reconsideration of my position on the topic, and reading all these great and informative responses, I think I went too far with this :-) > > v/r, > dps BR, Jarkko
On Fri Feb 23, 2024 at 3:57 AM EET, Daniel P. Smith wrote: > On 2/20/24 17:31, Jarkko Sakkinen wrote: > > On Tue Feb 20, 2024 at 10:26 PM UTC, Jarkko Sakkinen wrote: > >> On Tue Feb 20, 2024 at 8:54 PM UTC, Lino Sanfilippo wrote: > >>> for (i = 0; i <= MAX_LOCALITY; i++) > >>> __tpm_tis_relinquish_locality(priv, i); > >> > >> I'm pretty unfamiliar with Intel TXT so asking a dummy question: > >> if Intel TXT uses locality 2 I suppose we should not try to > >> relinquish it, or? > >> > >> AFAIK, we don't have a symbol called MAX_LOCALITY. > > > > OK it was called TPM_MAX_LOCALITY :-) I had the patch set applied > > in one branch but looked up with wrong symbol name. > > > > So I reformalize my question to two parts: > > > > 1. Why does TXT leave locality 2 open in the first place? I did > > not see explanation. Isn't this a bug in TXT? > > It does so because that is what the TCG D-RTM specification requires. > See Section 5.3.4.10 of the TCG D-RTM specification[1], the first > requirement is, "The DLME SHALL receive control with access to Locality 2." From below also the locality enumeration would be good to have documented (as a reminder). > > > 2. Because localities are not too useful these days given TPM2's > > policy mechanism I cannot recall out of top of my head can > > you have two localities open at same time. So what kind of > > conflict happens when you try to open locality 0 and have > > locality 2 open? > > I would disagree and would call your attention to the TCG's > definition/motivation for localities, Section 3.2 of Client PTP > specification[2]. > > "“Locality” is an assertion to the TPM that a command’s source is > associated with a particular component. Locality can be thought of as a > hardware-based authorization. The TPM is not actually aware of the > nature of the relationship between the locality and the component. The > ability to reset and extend notwithstanding, it is important to note > that, from a PCR “usage” perspective, there is no hierarchical > relationship between different localities. The TPM simply enforces > locality restrictions on TPM assets (such as PCR or SEALed blobs)." > > As stated, from the TPM specification perspective, it is not aware of > this mapping to components and leaves it to the platform to enforce. Yeah, TPM is a passive component, not active actor, in everything. The way I see locality as way to separate e.g. kernel and user space driver TPM transactions is pretty much like actor-dependent salt (e.g. if 0 was for user space and 1 was for kernel). > > "The protection and separation of the localities (and therefore the > association with the associated components) is entirely the > responsibility of the platform components. Platform components, > including the OS, may provide the separation of localities using > protection mechanisms such as virtual memory or paging." > > The x86 manufactures opted to adopt the D-RTM specification which > defines the components as follows: > > Locality 4: Usually associated with the CPU executing microcode. This is > used to establish the Dynamic RTM. > Locality 3: Auxiliary components. Use of this is optional and, if used, > it is implementation dependent. > Locality 2: Dynamically Launched OS (Dynamic OS) “runtime” environment. > Locality 1: An environment for use by the Dynamic OS. > Locality 0: The Static RTM, its chain of trust and its environment. > > And the means to protect and separate those localities are encoded in > the x86 chipset, i.e A D-RTM Event must be used to access any of the > D-RTM Localities (Locality1 - Locality4). > > For Intel, Locality 4 can only be accessed when a dedicated signal > between the CPU and the chipset is raised, thus only allowing the CPU to > utilize Locality 4. The CPU will then close Locality 4, authenticate and > give control to the ACM with access to Locality 3. When the ACM is > complete, it will instruct the chipset to lock Locality 3 and give > control to the DLME (MLE in Intel parlance) with Locality 2 open. It is > up to the DLME, the Linux kernel in this case, to decide how to assign > components to Locality 1 and 2. > > As to proposals to utilize localities by the Linux kernel, the only one > I was aware of was dropped because they couldn't open the higher localities. > > I would also highlight that the D-RTM implementation guide for Arm > allows for a hardware D-RTM event, which the vendor may choose to > implement a hardware/CPU enforced access to TPM localities. Thus, the > ability to support localities will also become a requirement for certain > Arm CPUs. > > [1] > https://trustedcomputinggroup.org/wp-content/uploads/TCG_D-RTM_Architecture_v1-0_Published_06172013.pdf > [2] > https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.pdf BR, Jarkko
Hi, On 20.02.24 19:42, Alexander Steffen wrote: > On 02.02.2024 04:08, Lino Sanfilippo wrote: >> On 01.02.24 23:21, Jarkko Sakkinen wrote: >> >>> >>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >>>> Commit 933bfc5ad213 introduced the use of a locality counter to control when a >>>> locality request is allowed to be sent to the TPM. In the commit, the counter >>>> is indiscriminately decremented. Thus creating a situation for an integer >>>> underflow of the counter. >>> >>> What is the sequence of events that leads to this triggering the >>> underflow? This information should be represent in the commit message. >>> >> >> AFAIU this is: >> >> 1. We start with a locality_counter of 0 and then we call tpm_tis_request_locality() >> for the first time, but since a locality is (unexpectedly) already active >> check_locality() and consequently __tpm_tis_request_locality() return "true". > > check_locality() returns true, but __tpm_tis_request_locality() returns the requested locality. Currently, this is always 0, so the check for !ret will always correctly indicate success and increment the locality_count. So how was the reported underflow triggered then? We only request locality 0 in TPM TIS core, no other locality. > > But since theoretically a locality != 0 could be requested, the correct fix would be to check for something like ret >= 0 or ret == l instead of !ret. I thought that the underflow issue has actually been triggered and is not only a theoretical problem. But now I wonder how this could ever happen. BR, Lino
On 23.02.24 02:56, Daniel P. Smith wrote: >> >> Will the TPM TIS CORE ever (have to) request another locality than 0? Maybe the best would >> be to hardcode TPM_ACCESS(0) and get rid of all the locality parameters that are >> passed from one function to another. >> But this is rather code optimization and not really required to fix the reported bug. > > Actually, doing so will break the TPM API. The function > tpm_tis_request_locality() is registered as the locality handler, > int (*request_locality)(struct tpm_chip *chip, int loc), in the tis > instance of struct tpm_class_ops{}. This is the API used by the Secure > Launch series to open Locality2 for the measurements it must record. > I dont understand this. How do you use locality 2 with the current mainline API? Do you adjust the mainline code to use locality 2 instead of 0? This would at least explain how you ran into the underflow issue which from the source code seems to be impossible when using locality 0. But then I wonder why this has not been made clear in this discussion. And then we are talking about fixing a bug that does not even exist in the upstream code. BR, Lino
On 2/23/24 07:58, Jarkko Sakkinen wrote: > On Fri Feb 23, 2024 at 3:58 AM EET, Daniel P. Smith wrote: >>> Just adding here that I wish we also had a log transcript of bug, which >>> is right now missing. The explanation believable enough to move forward >>> but I still wish to see a log transcript. >> >> That will be forth coming. > > I did not respond yet to other responses that you've given in the past > 12'ish hours or so (just woke up) but I started to think how all this > great and useful information would be best kept in memory. Some of it > has been discussed in the past but there is lot of small details that > are too easily forgotten. > > I'd think the best "documentation" approach here would be inject the > spec references to the sites where locality behaviour is changed so > that it is easy in future cross-reference them, and least of risk > of having code changes that would break anything. I think this way > all the information that you provided is best preserved for the > future. > > Thanks a lot for great and informative responses! No problem at all. Here is a serial output[1] from a dynamic launch using Linux Secure Launch v7[2] with one additional patch[3] to dump TPM driver state. [1] https://paste.debian.net/1308538/ [2] https://lore.kernel.org/lkml/scpu273f2mprr2uvjlyk2xrjjtcduhse2eb45lmj7givn6jh4u@i2v4f2vbldu4/T/ [3] https://paste.debian.net/1308539/
On Sat Feb 24, 2024 at 4:34 AM EET, Lino Sanfilippo wrote: > > > On 23.02.24 02:56, Daniel P. Smith wrote: > > >> > >> Will the TPM TIS CORE ever (have to) request another locality than 0? Maybe the best would > >> be to hardcode TPM_ACCESS(0) and get rid of all the locality parameters that are > >> passed from one function to another. > >> But this is rather code optimization and not really required to fix the reported bug. > > > > Actually, doing so will break the TPM API. The function > > tpm_tis_request_locality() is registered as the locality handler, > > int (*request_locality)(struct tpm_chip *chip, int loc), in the tis > > instance of struct tpm_class_ops{}. This is the API used by the Secure > > Launch series to open Locality2 for the measurements it must record. > > > > I dont understand this. How do you use locality 2 with the current mainline > API? Do you adjust the mainline code to use locality 2 instead of 0? This would > at least explain how you ran into the underflow issue which from > the source code seems to be impossible when using locality 0. But then I wonder why > this has not been made clear in this discussion. And then we are talking > about fixing a bug that does not even exist in the upstream code. Thanks for bringing this up, now I finally figured out what confuses me in this series. Daniel, I also have troubles understanding why locality_count would ever be greater than zero exactly in the mainline kernel, *without* [1]? [1] https://lore.kernel.org/linux-integrity/20240214221847.2066632-1-ross.philipson@oracle.com/ BR, Jarkko
On Sun Feb 25, 2024 at 1:23 PM EET, Daniel P. Smith wrote: > On 2/23/24 07:58, Jarkko Sakkinen wrote: > > On Fri Feb 23, 2024 at 3:58 AM EET, Daniel P. Smith wrote: > >>> Just adding here that I wish we also had a log transcript of bug, which > >>> is right now missing. The explanation believable enough to move forward > >>> but I still wish to see a log transcript. > >> > >> That will be forth coming. > > > > I did not respond yet to other responses that you've given in the past > > 12'ish hours or so (just woke up) but I started to think how all this > > great and useful information would be best kept in memory. Some of it > > has been discussed in the past but there is lot of small details that > > are too easily forgotten. > > > > I'd think the best "documentation" approach here would be inject the > > spec references to the sites where locality behaviour is changed so > > that it is easy in future cross-reference them, and least of risk > > of having code changes that would break anything. I think this way > > all the information that you provided is best preserved for the > > future. > > > > Thanks a lot for great and informative responses! > > No problem at all. > > Here is a serial output[1] from a dynamic launch using Linux Secure > Launch v7[2] with one additional patch[3] to dump TPM driver state. But are this fixes for a kernel tree with [2] applied. If the bugs do not occur in the mainline tree without the out-of-tree feature, they are not bug fixes. They should then really be part of that series. BR, Jarkko
On 23.02.2024 02:55, Daniel P. Smith wrote: > On 2/20/24 13:42, Alexander Steffen wrote: >> On 02.02.2024 04:08, Lino Sanfilippo wrote: >>> On 01.02.24 23:21, Jarkko Sakkinen wrote: >>> >>>> >>>> On Wed Jan 31, 2024 at 7:08 PM EET, Daniel P. Smith wrote: >>>>> Commit 933bfc5ad213 introduced the use of a locality counter to >>>>> control when a >>>>> locality request is allowed to be sent to the TPM. In the commit, >>>>> the counter >>>>> is indiscriminately decremented. Thus creating a situation for an >>>>> integer >>>>> underflow of the counter. >>>> >>>> What is the sequence of events that leads to this triggering the >>>> underflow? This information should be represent in the commit message. >>>> >>> >>> AFAIU this is: >>> >>> 1. We start with a locality_counter of 0 and then we call >>> tpm_tis_request_locality() >>> for the first time, but since a locality is (unexpectedly) already >>> active >>> check_locality() and consequently __tpm_tis_request_locality() return >>> "true". >> >> check_locality() returns true, but __tpm_tis_request_locality() returns >> the requested locality. Currently, this is always 0, so the check for >> !ret will always correctly indicate success and increment the >> locality_count. >> >> But since theoretically a locality != 0 could be requested, the correct >> fix would be to check for something like ret >= 0 or ret == l instead of >> !ret. Then the counter will also be incremented correctly for localities >> != 0, and no underflow will happen later on. Therefore, explicitly >> checking for an underflow is unnecessary and hides the real problem. >> > > My apologies, but I will have to humbly disagree from a fundamental > level here. If a state variable has bounds, then those bounds should be > enforced when the variable is being manipulated. That's fine, but that is not what your proposed fix does. tpm_tis_request_locality and tpm_tis_relinquish_locality are meant to be called in pairs: for every (successful) call to tpm_tis_request_locality there *must* be a corresponding call to tpm_tis_relinquish_locality afterwards. Unfortunately, in C there is no language construct to enforce that (nothing like a Python context manager), so instead locality_count is used to count the number of successful calls to tpm_tis_request_locality, so that tpm_tis_relinquish_locality can wait to actually relinquish the locality until the last expected call has happened (you can think of that as a Python RLock, to stay with the Python analogies). So if locality_count ever gets negative, that is certainly a bug somewhere. But your proposed fix hides this bug, by allowing tpm_tis_relinquish_locality to be called more often than tpm_tis_request_locality. You could have added something like BUG_ON(priv->locality_count == 0) before decrementing the counter. That would really enforce the bounds, without hiding the bug, and I would be fine with that. Of course, that still leaves the actual bug to be fixed. In this case, there is no mismatch between the calls to tpm_tis_request_locality and tpm_tis_relinquish_locality. It is just (as I said before) that the counting of successful calls in tpm_tis_request_locality is broken for localities != 0, so that is what you need to fix. > Assuming that every > path leading to the variable manipulation code has ensured proper > manipulation is just that, an assumption. When assumptions fail is how > bugs and vulnerabilities occur. > > To your point, does this full address the situation experienced, I would > say it does not. IMHO, the situation is really a combination of both > patch 1 and patch 2, but the request was to split the changes for > individual discussion. We selected this one as being the fixes for two > reasons. First, it blocks the underflow such that when the Secure Launch > series opens Locality 2, it will get incremented at that time and the > internal locality tracking state variables will end up with the correct > values. Thus leading to the relinquish succeeding at kernel shutdown. > Second, it provides a stronger defensive coding practice. > > Another reason that this works as a fix is that the TPM specification > requires the registers to be mirrored across all localities, regardless > of the active locality. While all the request/relinquishes for Locality > 0 sent by the early code do not succeed, obtaining the values via the > Locality 0 registers are still guaranteed to be correct. > > v/r, > dps
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 1b350412d8a6..4176d3bd1f04 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);