On 11/21/24 2:42 PM, Andy Lutomirski wrote:
On Thu, Nov 21, 2024 at 12:54 PM Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
On Thu, Nov 21, 2024 at 12:11 PM <ross.philipson@xxxxxxxxxx> 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
