Andrey Pronin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41100 )
Change subject: security: tcg-2.0: Improve error response handling, fix Cr50 boot mode ......................................................................
Patch Set 1:
(1 comment)
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 556: ibuf_nr_read(ib) == resp->hdr.tpm_size
and the header return code should also be examined before this function is invoked, so there is no n […]
examining it before the function is what I originally had in mind to minimize the patch.
but passing it here 1) keeps tpm_unmarshal_response generic as it should be, and encapsulates vendor-command-specific logic in this function - lower risk to miss this logic during some future change. 2) allows to add special cases (when we do want to get some info from the payload for vendor command error) there. though I suspect we never need those special cases.
bottomline: no strong preferences from my side. the choice is between a smaller patch and better separation of concerns.
another approach could be applying this logic (don't look at the payload for errors) to all commands, not just vendor commands. but that's a bigger change to the overall approach, and easier to break with future changes than this vendor-command-specific behavior for errors.