Message-ID: <2a243379-f18e-4461-95ad-c17ce49e6641@oracle.com>
Date: Fri, 22 Nov 2024 15:37:36 -0800
From: ross.philipson@...cle.com
To: Andy Lutomirski <luto@...capital.net>
Cc: James Bottomley <James.Bottomley@...senpartnership.com>,
        "Daniel P. Smith" <dpsmith@...rtussolutions.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Eric Biggers <ebiggers@...nel.org>, linux-kernel@...r.kernel.org,
        x86@...nel.org, linux-integrity@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-crypto@...r.kernel.org,
        kexec@...ts.infradead.org, linux-efi@...r.kernel.org,
        iommu@...ts.linux-foundation.org, mingo@...hat.com, bp@...en8.de,
        hpa@...or.com, dave.hansen@...ux.intel.com, ardb@...nel.org,
        mjg59@...f.ucam.org, peterhuewe@....de, jarkko@...nel.org,
        jgg@...pe.ca, nivedita@...m.mit.edu, herbert@...dor.apana.org.au,
        davem@...emloft.net, corbet@....net, dwmw2@...radead.org,
        baolu.lu@...ux.intel.com, kanth.ghatraju@...cle.com,
        andrew.cooper3@...rix.com, trenchboot-devel@...glegroups.com
Subject: Re: [PATCH v9 06/19] x86: Add early SHA-1 support for Secure Launch
 early measurements

On 11/21/24 2:42 PM, Andy Lutomirski wrote:
> On Thu, Nov 21, 2024 at 12:54 PM Andy Lutomirski <luto@...capital.net> wrote:
>>
>> On Thu, Nov 21, 2024 at 12:11 PM <ross.philipson@...cle.com> wrote:
>>>
>>> On 11/18/24 12:02 PM, Andy Lutomirski wrote:
>>
>>>> If the vendor of an attestation-dependent thing trusts SM3 but *Linux*
>>>> does not like SM3, then the vendor's software should not become wildly
>>>> insecure because Linux does not like SM3.  And, as that 2004 CVE
>>>> shows, even two groups that are nominally associated with Microsoft
>>>> can disagree on which banks they like, causing a vulnerability.
>>>
>>> Thanks everyone for all the feedback and discussions on this. I
>>> understand it is important and perhaps the Linux TPM code should be
>>> modified to do the extend operations differently but this seems like it
>>> is outside the scope of our Secure Launch feature patch set.
>>
>> It's absolutely not outside the scope.  Look, this is quoted verbatim
>> from your patchset (v11, but I don't think this has materially
>> changed):
> 

Concerning my previous response, I realized that I did not fully 
understand what you were suggesting/proposing so I am sorry about that. 
You are correct that addressing this is within the scope of what we are 
doing. I have reread all the emails again and I/we now understand what 
you are saying.

We are now exploring how we might use TPM2_PCR_Event, whether it 
introduces other issues with respect to how TXT/ACM might behave and 
what would be needed to adopt this approach. More to come on that front. 
I will mention that the TPM code in the Linux kernel does not currently 
support the TPM2_PCR_Event command. The functionality needs to be added 
and that needs buy in from the TPM maintainers (probably guidance too).

A bit more below to clarify a few things...

> 
> ... I apologize -- I've misread the code.  That code is still wrong, I
> think, but for an entirely different reason:
> 
>>
>> +       /* Early SL code ensured there was a max count of 2 digests */
>> +       for (i = 0; i < event->count; i++) {
>> +               dptr = (u8 *)alg_id_field + sizeof(u16);
>> +
>> +               for (j = 0; j < tpm->nr_allocated_banks; j++) {
>> +                       if (digests[j].alg_id != *alg_id_field)
>> +                               continue;
>>
>> ^^^^^^^^^^^^^^^^^^^^^ excuse me?
>>
>> +
>> +                       switch (digests[j].alg_id) {
>> +                       case TPM_ALG_SHA256:
>> +                               memcpy(&digests[j].digest[0], dptr,
>> +                                      SHA256_DIGEST_SIZE);
>> +                               alg_id_field = (u16 *)((u8 *)alg_id_field +
>> +                                       SHA256_DIGEST_SIZE + sizeof(u16));
>> +                               break;
>> +                       case TPM_ALG_SHA1:
>> +                               memcpy(&digests[j].digest[0], dptr,
>> +                                      SHA1_DIGEST_SIZE);
>> +                               alg_id_field = (u16 *)((u8 *)alg_id_field +
>> +                                       SHA1_DIGEST_SIZE + sizeof(u16));
>> +                               break;
>> +                       default:
>> +                               break;
>> +                       }
>> +               }
>> +       }
> 
> If we fall off the end of the loop, we never increase alg_id_field,
> and subsequent iterations will malfunction.  But we apparently will
> write zeros (or fail?) if we have an unsupported algorithm, because we
> are asking to extend all allocated banks.  I think.  This code is
> gross.  It's plausible that this whole sequence is impossible unless
> something malicious is going on.

Noted, there does look like there is an issue there. Thank you for the 
analysis.

> 
> Also, and I'm sort of replying to the wrong patch here, how
> trustworthy is the data that's used to populate tpm_algs in the stub?
> I don't think the results will be very pretty if tpm_algs ends up
> being incorrect.

We gather the list of algorithms used from the DRTM event log which the 
TXT/ACM phase initializes and begins populating. One thing to note here 
is that in the early setup kernel stub code, we would fail to boot if we 
saw an algorithm we did not support so we would not reach a state where 
we were simply ignoring an active bank as you mentioned in an earlier 
reply. But I also understand your point about limiting the functionality 
to just a subset of algorithms.

Thank you for your feedback,
Ross

Powered by blists - more mailing lists