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

From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
To: Alexander Steffen <Alexander.Steffen@infineon.com>,
	Lino Sanfilippo <l.sanfilippo@kunbus.com>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	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: Thu, 22 Feb 2024 20:55:56 -0500	[thread overview]
Message-ID: <f52546f1-acca-4915-924c-cdd2018215d5@apertussolutions.com> (raw)
In-Reply-To: <ae3fecc4-7b76-4607-8749-045e17941923@infineon.com>

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