Received-SPF: pass (google.com: domain of linux-kernel+bounces-31583-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Fri, 19 Jan 2024 21:38:32 +0000
Message-Id: <CYJ0APT6N1KL.CSHV5R4VRWHB@seitikki>
To: "Alexander Steffen" <Alexander.Steffen@infineon.com>, "Daniel P. Smith"
 <dpsmith@apertussolutions.com>, "Jason Gunthorpe" <jgg@ziepe.ca>, "Lino
 Sanfilippo" <l.sanfilippo@kunbus.com>, "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] tpm: make locality handling resilient
From: "Jarkko Sakkinen" <jarkko@kernel.org>
References: <20240115011546.21193-1-dpsmith@apertussolutions.com>
 <711d659f-3f57-48e4-b5b3-efbc2fe236c8@infineon.com>
In-Reply-To: <711d659f-3f57-48e4-b5b3-efbc2fe236c8@infineon.com>

On Wed Jan 17, 2024 at 8:44 AM UTC, Alexander Steffen wrote:
> On 15.01.2024 02:15, Daniel P. Smith wrote:
> > Commit 933bfc5ad213 introduced the use of a locality counter to control=
 when
> > locality request was actually sent to the TPM. This locality counter cr=
eated a
> > hard enforcement that the TPM had no active locality at the time of the=
 driver
> > initialization. The reality is that this may not always be the case cou=
pled
> > with the fact that the commit indiscriminately decremented the counter =
created
> > the condition for integer underflow of the counter. The underflow was t=
riggered
> > by the first pair of request/relinquish calls made in tpm_tis_init_core=
 and all
> > subsequent calls to request/relinquished calls would have the counter f=
lipping
> > between the underflow value and 0. The result is that it appeared all c=
alls to
> > request/relinquish were successful, but they were not. The end result i=
s that
> > the locality that was active when the driver loaded would always remain=
 active,
> > to include after the driver shutdown. This creates a significant issue =
when
> > using Intel TXT and Locality 2 is active at boot. After the GETSEC[SEXI=
T]
> > instruction is called, the PCH will close access to Locality 2 MMIO add=
ress
> > space, leaving the TPM locked in Locality 2 with no means to relinquish=
 the
> > locality until system reset.
> >=20
> > The commit seeks to address this situation through three changes.
>
> Could you split this up into multiple patches then, so that they can be=
=20
> discussed separately?

I have to agree with you ttly.

Yeah also the text above is not exactly in the ballpark.

I did not understand what I read. I had to read the code change instead
to get an idea. A huge pile of text does not equal to stronger story.

Like for any essay, scientific paper or a kernel message one should do
also few edit rounds. The commit message is more important than the code
change itself in bug fixes...

There is trigger (TXT) and solution. A great commit message should have
motivation and implementation parts and somewhat concise story where
things lead to another. It should essentially make *any* reader who
knows the basics of kernel code base convinced, not confused. This is
at leat a good aim even tho sometimes unreachable.

BR, Jarkko