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):
... 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.
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.
