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):
+ /* 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;
+ }
+ }
+ }
+
+ ret = tpm_pcr_extend(tpm, event->pcr_idx, digests);
+ if (ret) {
+ pr_err("Error extending TPM20 PCR, result: %d\n", ret);
+ slaunch_txt_reset(txt, "Failed to extend TPM20 PCR\n",
+ SL_ERROR_TPM_EXTEND);
+ }
I haven't even tried to see what happens if there are more than two
allocated banks, but regardless, that 'continue' statement is a
vulnerability, and it's introduced in the patchset. I'm not the
maintainer of this code, but I would NAK this.
I'm sure there's some reason that the TPM spec even makes code like
this possible, but it sure looks like the TPM2_PCR_Event operation
exists more or less to avoid this vulnerability. I think you should
either use it or you should explain, convincingly, why Linux should
add code that does not use it and thus has a vulnerability in certain,
entirely plausible, firmware configurations.
This is brand new code that is explicitly security code. I don't
think it's valid to spell "crud, we can't handle this case at all, and
failing to handle it is a security vulnerability" as "continue". If
*I* were writing this code, I would use TPM2_PCR_Event, which is
entirely immune to this particular failure as far as I can see.
--Andy
