linux-kernel - Re: [PATCH] tpm: make locality handling resilient

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 19 Jan 2024 21:38:32 +0000
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Alexander Steffen" <Alexander.Steffen@...ineon.com>, "Daniel P. Smith"
 <dpsmith@...rtussolutions.com>, "Jason Gunthorpe" <jgg@...pe.ca>, "Lino
 Sanfilippo" <l.sanfilippo@...bus.com>, "Sasha Levin" <sashal@...nel.org>,
 <linux-integrity@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Cc: "Ross Philipson" <ross.philipson@...cle.com>, "Kanth Ghatraju"
 <kanth.ghatraju@...cle.com>, "Peter Huewe" <peterhuewe@....de>
Subject: Re: [PATCH] tpm: make locality handling resilient

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 created 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 coupled
> > with the fact that the commit indiscriminately decremented the counter created
> > the condition for integer underflow of the counter. The underflow was triggered
> > 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 flipping
> > between the underflow value and 0. The result is that it appeared all calls to
> > request/relinquish were successful, but they were not. The end result is 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[SEXIT]
> > instruction is called, the PCH will close access to Locality 2 MMIO address
> > space, leaving the TPM locked in Locality 2 with no means to relinquish the
> > locality until system reset.
> > 
> > The commit seeks to address this situation through three changes.
>
> Could you split this up into multiple patches then, so that they can be 
> 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