[coreboot-gerrit] [M] Change in coreboot[main]: security/tpm: Add TPM2 NV_ReadPublic command support

[Julius Werner (Code Review)]

Attention is currently required from: Arthur Heymans, Christian Walter, Felix Held, Filip Lewiński, Martin Roth, Matt DeVillier, Michał Kopeć, Michał Żygowski.

Julius Werner has posted comments on this change by Filip Lewiński. ( https://review.coreboot.org/c/coreboot/+/82037?usp=email )

Change subject: security/tpm: Add TPM2 NV_ReadPublic command support
......................................................................

Patch Set 16:

(5 comments)

File src/security/tpm/tss/tcg-2.0/tss.c:

https://review.coreboot.org/c/coreboot/+/82037/comment/cd5f40ce_d3c84a75?usp... :
PS16, Line 395: struct nv_read_public_response *nvrp_resp
These TPM spec specific structures are usually only for use inside the TSS layer, and we prefer to unpack them into simpler primitives for the upper layers. For this case, maybe passing in separate pointers (that may be NULL if the caller doesn't care about that part) for attributes, name alg and data size, as well as optional buffers (with max size) for auth policy and name, would be the best option. Or we could create another struct that encapsulates those things but without the weirdnesses of the TSS-internal structures.

https://review.coreboot.org/c/coreboot/+/82037/comment/a49e980e_002dd4fd?usp... :
PS16, Line 410: 		return TPM_CB_NO_DEVICE;
I think this should normally be TPM_IOERROR (or maybe TPM_CB_READ_FAILURE).

https://review.coreboot.org/c/coreboot/+/82037/comment/03853837_0153d505?usp... :
PS16, Line 415: 		memcpy(nvrp_resp, &response->nvrp, sizeof(*nvrp_resp));
This is copying pointers (e.g. for the authPolicy hash) to the internal TPM transfer buffer, which will become invalid the next time we call another TPM command. That should not leak above this layer. All data returned to the caller should be deep copied at this point.

File src/security/tpm/tss/tcg-2.0/tss_marshaling.c:

https://review.coreboot.org/c/coreboot/+/82037/comment/aaf4caca_3958854d?usp... :
PS16, Line 635: 	memcpy(&name->name.digest.digest, ibuf_oob_drain(ib, digest_size), digest_size);
Shouldn't you `ibuf_check_size()` that there are still enough bytes before doing this memcpy?

File src/security/tpm/tss/tcg-2.0/tss_structures.h:

https://review.coreboot.org/c/coreboot/+/82037/comment/5757940f_c47192d6?usp... :
PS16, Line 330: 	uint8_t sha512[SHA512_DIGEST_SIZE];
I don't really understand why this digest is inline in the union while in all other cases we use a pointer to an external buffer. Maybe an inconsistency worth fixing?