Re: [PATCH 1/3] tpm: protect against locality counter underflow

From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Lino Sanfilippo" <l.sanfilippo@kunbus.com>,
	"Alexander Steffen" <Alexander.Steffen@infineon.com>,
	"Daniel P. Smith" <dpsmith@apertussolutions.com>,
	"Jason Gunthorpe" <jgg@ziepe.ca>,
	"Sasha Levin" <sashal@kernel.org>,
	<linux-integrity@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Cc: "Ross Philipson" <ross.philipson@oracle.com>,
	"Kanth Ghatraju" <kanth.ghatraju@oracle.com>,
	"Peter Huewe" <peterhuewe@gmx.de>
Subject: Re: [PATCH 1/3] tpm: protect against locality counter underflow
Date: Tue, 20 Feb 2024 22:23:13 +0000	[thread overview]
Message-ID: <CZA9ADCGOTQT.LB5XHZZVTWVH@seitikki> (raw)
In-Reply-To: <91f600ef-867b-4523-89be-1c0ba34f8a4c@kunbus.com>

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