Message-Id: <20240131171651.15796-1-dpsmith@apertussolutions.com>
Date: Wed, 31 Jan 2024 12:16:50 -0500
From: "Daniel P. Smith" <dpsmith@...rtussolutions.com>
To: Jason Gunthorpe <jgg@...pe.ca>,
	Jarkko Sakkinen <jarkko@...nel.org>,
	Sasha Levin <sashal@...nel.org>,
	Lino Sanfilippo <l.sanfilippo@...bus.com>,
	linux-integrity@...r.kernel.org,
	linux-kernel@...r.kernel.org
Cc: "Daniel P. Smith" <dpsmith@...rtussolutions.com>,
	Ross Philipson <ross.philipson@...cle.com>,
	Kanth Ghatraju <kanth.ghatraju@...cle.com>,
	Peter Huewe <peterhuewe@....de>
Subject: 

Date: Wed, 31 Jan 2024 11:43:16 -0500
Subject: [PATCH v2 0/3] tpm: make locality handling resilient

When the kernel is started by a loader that leaves a locality other than
Locality0 open, for example Intel TXT SINIT ACM leaves Locality2 open, this
triggers a failure condition in the initialization of the TPM driver. The
result being that despite calls to request/relinquish locality, the locality
that was open at startup is never open/closed. The impact for an Intel TXT
platform is that after SMX mode is exited, access to the Locality2 register
space is blocked, and the TPM is locked in Locality 2 rendering the TPM
unusable.

The ability to trigger this failure condition was introduced in commit
933bfc5ad213, which introduced a locality counter to control when locality
requests were actually sent to the TPM. The commit introduces a series of
missing or incorrect protections in place for incrementing and decrementing the
counter. The protection around incrementing the counter was protected with an
incorrect check. The check was incorrect in two ways, 1.) it assumed that zero
was the only successful value that would be returned by
__tpm_tis_request_locality() and 2.) was evaluated outside the if block under
which __tpm_tis_request_locality() is called. The effect of (1) is that
__tpm_tis_request_locality() in fact returns the locality number that was
requested, e.g. 1-4, and therefore the protection check would fail and the
counter does not get incremented. The effect of (2) is a combination with the
ret variable being initialized to 0 and the counter not being 0. The result is
that __tpm_tis_request_locality() does not get called, but the counter is still
incremented.

Another effect seen is an indirect result of commit 933bfc5ad213 and is due to
locality being tracked in two different locations. The first is at the TIS
layer in struct tpm_tis_data and at the chip layer in struct tpm_chip. The
failures experienced by commit 933bfc5ad213 causes the locality value in these
two locations to fall out of sync with the value in tpm_tis_data reflecting
Locality0 and struct tpm_chip having a value of -1, no locality open.

This series seeks to address these conditions by introducing a protection
against underflowing the locality counter. The closing of all localities at the
beginning of initialization to begin with the TPM in the state expected by the
sequencing codified in the initialization. Lastly to adjust the return error
codes from __tpm_tis_request_locality() and tpm_tis_request_locality() to
ensure they are consistent and that the locality counter is only incremented
for a successful call to __tpm_tis_request_locality().

Changes in v2:
- split into three patches
- adjust code comments per review
- dropped incorrect return value change for tpm_request_locality in tpm-chip.c
- additional -1 replacement in __tpm_tis_relinquish_locality
- changed -EIO to -EBUSY, which is more appropriate, in tpm_tis_relinquish_locality

Daniel P. Smith (3):
  tpm: protect against locality counter underflow
  tpm: ensure tpm is in known state at startup
  tpm: make locality request return value consistent

 drivers/char/tpm/tpm_tis_core.c | 25 +++++++++++++++++++------
 include/linux/tpm.h             |  6 ++++++
 2 files changed, 25 insertions(+), 6 deletions(-)

-- 
2.30.2

Powered by blists - more mailing lists