Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Ignore data payload for errors, fix Cr50 boot mode ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 591: if (tpm2_static_resp.hdr.tpm_size != ibuf_nr_read(ib))
ah, I misunderstood what ibuf_nr_read(ib) does
Ack
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 554: /* On errors, some subcommands do not return a payload. */
in fact, I'd even move this check before ibuf_read_be16(ib, &resp->vcr.vc_subcommand). […]
Ack
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
Why would we make a difference between vendor commands and standard commands? I think vendor command […]
Ack
https://review.coreboot.org/c/coreboot/+/41100/1/src/security/tpm/tss/tcg-2.... PS1, Line 594: (ibuf_remaining(ib) != tpm2_static_resp.hdr.tpm_size - 10)
to avoid the hardcoded header size, how about the check […]
Oh, missed this earlier... yeah, that's even better. Actually, looks like we have ibuf_capacity() to get that in one go.
https://review.coreboot.org/c/coreboot/+/41100/2/src/security/tpm/tss/tcg-2.... File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:
https://review.coreboot.org/c/coreboot/+/41100/2/src/security/tpm/tss/tcg-2.... PS2, Line 590: - 10
I would still rather avoid using bare numbers, what is wrong with comparing the size of the packet w […]
Done
https://review.coreboot.org/c/coreboot/+/41100/2/src/security/tpm/tss/tcg-2.... PS2, Line 597: On errors, we're not sure what the TPM is returning. None of the : commands we use actually expect useful data payloads for errors, so : just ignore any data after the header.
sure, this is fine with me too, the policy could be applied across the board. […]
Ack