| Message ID | 20210504124842.220445-10-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | vtpmmgr: Some fixes - still incomplete | expand |
Jason Andryuk, le mar. 04 mai 2021 08:48:42 -0400, a ecrit: > GetRandom passthrough currently fails when using vtpmmgr with a hardware > TPM 2.0. > vtpmmgr (8): INFO[VTPM]: Passthrough: TPM_GetRandom > vtpm (12): vtpm_cmd.c:120: Error: TPM_GetRandom() failed with error code (30) > > When running on TPM 2.0 hardware, vtpmmgr needs to convert the TPM 1.2 > TPM_ORD_GetRandom into a TPM2 TPM_CC_GetRandom command. Besides the > differing ordinal, the TPM 1.2 uses 32bit sizes for the request and > response (vs. 16bit for TPM2). > > Place the random output directly into the tpmcmd->resp and build the > packet around it. This avoids bouncing through an extra buffer, but the > header has to be written after grabbing the random bytes so we have the > number of bytes to include in the size. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > --- > stubdom/vtpmmgr/marshal.h | 10 +++++++ > stubdom/vtpmmgr/vtpm_cmd_handler.c | 48 ++++++++++++++++++++++++++++++ > 2 files changed, 58 insertions(+) > > diff --git a/stubdom/vtpmmgr/marshal.h b/stubdom/vtpmmgr/marshal.h > index dce19c6439..20da22af09 100644 > --- a/stubdom/vtpmmgr/marshal.h > +++ b/stubdom/vtpmmgr/marshal.h > @@ -890,6 +890,15 @@ inline int sizeof_TPM_AUTH_SESSION(const TPM_AUTH_SESSION* auth) { > return rv; > } > > +static > +inline int sizeof_TPM_RQU_HEADER(BYTE* ptr) { > + int rv = 0; > + rv += sizeof_UINT16(ptr); > + rv += sizeof_UINT32(ptr); > + rv += sizeof_UINT32(ptr); > + return rv; > +} > + > static > inline BYTE* pack_TPM_RQU_HEADER(BYTE* ptr, > TPM_TAG tag, > @@ -923,5 +932,6 @@ inline int unpack3_TPM_RQU_HEADER(BYTE* ptr, UINT32* pos, UINT32 max, > #define pack_TPM_RSP_HEADER(p, t, s, r) pack_TPM_RQU_HEADER(p, t, s, r) > #define unpack_TPM_RSP_HEADER(p, t, s, r) unpack_TPM_RQU_HEADER(p, t, s, r) > #define unpack3_TPM_RSP_HEADER(p, l, m, t, s, r) unpack3_TPM_RQU_HEADER(p, l, m, t, s, r) > +#define sizeof_TPM_RSP_HEADER(p) sizeof_TPM_RQU_HEADER(p) > > #endif > diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c > index 2ac14fae77..7ca1d9df94 100644 > --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c > +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c > @@ -47,6 +47,7 @@ > #include "vtpm_disk.h" > #include "vtpmmgr.h" > #include "tpm.h" > +#include "tpm2.h" > #include "tpmrsa.h" > #include "tcg.h" > #include "mgmt_authority.h" > @@ -772,6 +773,52 @@ static int vtpmmgr_permcheck(struct tpm_opaque *opq) > return 1; > } > > +TPM_RESULT vtpmmgr_handle_getrandom(struct tpm_opaque *opaque, > + tpmcmd_t* tpmcmd) > +{ > + TPM_RESULT status = TPM_SUCCESS; > + TPM_TAG tag; > + UINT32 size; > + UINT32 rand_offset; > + UINT32 rand_size; > + TPM_COMMAND_CODE ord; > + BYTE *p; > + > + p = unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord); > + > + if (!hw_is_tpm2()) { > + size = TCPA_MAX_BUFFER_LENGTH; > + TPMTRYRETURN(TPM_TransmitData(tpmcmd->req, tpmcmd->req_len, > + tpmcmd->resp, &size)); > + tpmcmd->resp_len = size; > + > + return TPM_SUCCESS; > + } We need to check for the size of the request before unpacking (which doesn't check for it), don't we? > + /* TPM_GetRandom req: <header><uint32 num bytes> */ > + unpack_UINT32(p, &rand_size); > + > + /* Call TPM2_GetRandom but return a TPM_GetRandom response. */ > + /* TPM_GetRandom resp: <header><uint32 num bytes><num random bytes> */ > + rand_offset = sizeof_TPM_RSP_HEADER(tpmcmd->resp) + > + sizeof_UINT32(tpmcmd->resp); There is a spurious indentation here, at first sight I even thought it was part of the comment. We also need to check that rand_size is not too large? - that the returned data won't overflow tpmcmd->resp + rand_offset - that it fits in a UINT16 Also, TPM2_GetRandom casts bytesRequested into UINT16*, that's bogus, it should use a local UINT16 variable and assign *bytesRequested. > + TPMTRYRETURN(TPM2_GetRandom(&rand_size, tpmcmd->resp + rand_offset)); > + > + p = pack_TPM_RSP_HEADER(tpmcmd->resp, TPM_TAG_RSP_COMMAND, > + rand_offset + rand_size, status); > + p = pack_UINT32(p, rand_size); > + tpmcmd->resp_len = rand_offset + rand_size; > + > + return status; > + > +abort_egress: > + tpmcmd->resp_len = VTPM_COMMAND_HEADER_SIZE; > + pack_TPM_RSP_HEADER(tpmcmd->resp, tag + 3, tpmcmd->resp_len, status); > + > + return status; > +} > + > TPM_RESULT vtpmmgr_handle_cmd( > struct tpm_opaque *opaque, > tpmcmd_t* tpmcmd) > @@ -842,6 +889,7 @@ TPM_RESULT vtpmmgr_handle_cmd( > switch(ord) { > case TPM_ORD_GetRandom: > vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n"); > + return vtpmmgr_handle_getrandom(opaque, tpmcmd); > break; Drop the break, then. I would say also move (or drop) the log, like the other cases. > case TPM_ORD_PcrRead: > vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_PcrRead\n"); > -- > 2.30.2 >
On Tue, May 4, 2021 at 9:33 AM Samuel Thibault <samuel.thibault@ens-lyon.org> wrote: > > Jason Andryuk, le mar. 04 mai 2021 08:48:42 -0400, a ecrit: > > GetRandom passthrough currently fails when using vtpmmgr with a hardware > > TPM 2.0. > > vtpmmgr (8): INFO[VTPM]: Passthrough: TPM_GetRandom > > vtpm (12): vtpm_cmd.c:120: Error: TPM_GetRandom() failed with error code (30) > > > > When running on TPM 2.0 hardware, vtpmmgr needs to convert the TPM 1.2 > > TPM_ORD_GetRandom into a TPM2 TPM_CC_GetRandom command. Besides the > > differing ordinal, the TPM 1.2 uses 32bit sizes for the request and > > response (vs. 16bit for TPM2). > > > > Place the random output directly into the tpmcmd->resp and build the > > packet around it. This avoids bouncing through an extra buffer, but the > > header has to be written after grabbing the random bytes so we have the > > number of bytes to include in the size. > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > --- > > stubdom/vtpmmgr/marshal.h | 10 +++++++ > > stubdom/vtpmmgr/vtpm_cmd_handler.c | 48 ++++++++++++++++++++++++++++++ > > 2 files changed, 58 insertions(+) > > > > diff --git a/stubdom/vtpmmgr/marshal.h b/stubdom/vtpmmgr/marshal.h > > index dce19c6439..20da22af09 100644 > > --- a/stubdom/vtpmmgr/marshal.h > > +++ b/stubdom/vtpmmgr/marshal.h > > @@ -890,6 +890,15 @@ inline int sizeof_TPM_AUTH_SESSION(const TPM_AUTH_SESSION* auth) { > > return rv; > > } > > > > +static > > +inline int sizeof_TPM_RQU_HEADER(BYTE* ptr) { > > + int rv = 0; > > + rv += sizeof_UINT16(ptr); > > + rv += sizeof_UINT32(ptr); > > + rv += sizeof_UINT32(ptr); > > + return rv; > > +} > > + > > static > > inline BYTE* pack_TPM_RQU_HEADER(BYTE* ptr, > > TPM_TAG tag, > > @@ -923,5 +932,6 @@ inline int unpack3_TPM_RQU_HEADER(BYTE* ptr, UINT32* pos, UINT32 max, > > #define pack_TPM_RSP_HEADER(p, t, s, r) pack_TPM_RQU_HEADER(p, t, s, r) > > #define unpack_TPM_RSP_HEADER(p, t, s, r) unpack_TPM_RQU_HEADER(p, t, s, r) > > #define unpack3_TPM_RSP_HEADER(p, l, m, t, s, r) unpack3_TPM_RQU_HEADER(p, l, m, t, s, r) > > +#define sizeof_TPM_RSP_HEADER(p) sizeof_TPM_RQU_HEADER(p) > > > > #endif > > diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c > > index 2ac14fae77..7ca1d9df94 100644 > > --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c > > +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c > > @@ -47,6 +47,7 @@ > > #include "vtpm_disk.h" > > #include "vtpmmgr.h" > > #include "tpm.h" > > +#include "tpm2.h" > > #include "tpmrsa.h" > > #include "tcg.h" > > #include "mgmt_authority.h" > > @@ -772,6 +773,52 @@ static int vtpmmgr_permcheck(struct tpm_opaque *opq) > > return 1; > > } > > > > +TPM_RESULT vtpmmgr_handle_getrandom(struct tpm_opaque *opaque, > > + tpmcmd_t* tpmcmd) > > +{ > > + TPM_RESULT status = TPM_SUCCESS; > > + TPM_TAG tag; > > + UINT32 size; > > + UINT32 rand_offset; > > + UINT32 rand_size; > > + TPM_COMMAND_CODE ord; > > + BYTE *p; > > + > > + p = unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord); > > + > > + if (!hw_is_tpm2()) { > > + size = TCPA_MAX_BUFFER_LENGTH; > > + TPMTRYRETURN(TPM_TransmitData(tpmcmd->req, tpmcmd->req_len, > > + tpmcmd->resp, &size)); > > + tpmcmd->resp_len = size; > > + > > + return TPM_SUCCESS; > > + } > > > We need to check for the size of the request before unpacking (which > doesn't check for it), don't we? Yes, good catch. vtpmmgr_handle_cmd doesn't check either. > > + /* TPM_GetRandom req: <header><uint32 num bytes> */ > > + unpack_UINT32(p, &rand_size); > > + > > + /* Call TPM2_GetRandom but return a TPM_GetRandom response. */ > > + /* TPM_GetRandom resp: <header><uint32 num bytes><num random bytes> */ > > + rand_offset = sizeof_TPM_RSP_HEADER(tpmcmd->resp) + > > + sizeof_UINT32(tpmcmd->resp); > > There is a spurious indentation here, at first sight I even thought it > was part of the comment. Sorry about that - it was 8 spaces instead of a tab. > We also need to check that rand_size is not too large? > - that the returned data won't overflow tpmcmd->resp + rand_offset > - that it fits in a UINT16 Yes, will do. > Also, TPM2_GetRandom casts bytesRequested into UINT16*, that's bogus, it > should use a local UINT16 variable and assign *bytesRequested. Good catch. I'll do that in a new patch. > > + TPMTRYRETURN(TPM2_GetRandom(&rand_size, tpmcmd->resp + rand_offset)); > > + > > + p = pack_TPM_RSP_HEADER(tpmcmd->resp, TPM_TAG_RSP_COMMAND, > > + rand_offset + rand_size, status); > > + p = pack_UINT32(p, rand_size); > > + tpmcmd->resp_len = rand_offset + rand_size; > > + > > + return status; > > + > > +abort_egress: > > + tpmcmd->resp_len = VTPM_COMMAND_HEADER_SIZE; > > + pack_TPM_RSP_HEADER(tpmcmd->resp, tag + 3, tpmcmd->resp_len, status); > > + > > + return status; > > +} > > + > > TPM_RESULT vtpmmgr_handle_cmd( > > struct tpm_opaque *opaque, > > tpmcmd_t* tpmcmd) > > @@ -842,6 +889,7 @@ TPM_RESULT vtpmmgr_handle_cmd( > > switch(ord) { > > case TPM_ORD_GetRandom: > > vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n"); > > + return vtpmmgr_handle_getrandom(opaque, tpmcmd); > > break; > > Drop the break, then. I would say also move (or drop) the log, like the > other cases. Will drop the break. I would just leave the log since it matches the other cases in this case statement. But I can remove it if you want. Regards, Jason
Jason Andryuk, le mar. 04 mai 2021 13:23:57 -0400, a ecrit: > > > @@ -842,6 +889,7 @@ TPM_RESULT vtpmmgr_handle_cmd( > > > switch(ord) { > > > case TPM_ORD_GetRandom: > > > vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n"); > > > + return vtpmmgr_handle_getrandom(opaque, tpmcmd); > > > break; > > > > Drop the break, then. I would say also move (or drop) the log, like the > > other cases. > > Will drop the break. I would just leave the log since it matches the > other cases in this case statement. Mmm, right these are all pass-through cases so better log them the same. Samuel
diff --git a/stubdom/vtpmmgr/marshal.h b/stubdom/vtpmmgr/marshal.h index dce19c6439..20da22af09 100644 --- a/stubdom/vtpmmgr/marshal.h +++ b/stubdom/vtpmmgr/marshal.h @@ -890,6 +890,15 @@ inline int sizeof_TPM_AUTH_SESSION(const TPM_AUTH_SESSION* auth) { return rv; } +static +inline int sizeof_TPM_RQU_HEADER(BYTE* ptr) { + int rv = 0; + rv += sizeof_UINT16(ptr); + rv += sizeof_UINT32(ptr); + rv += sizeof_UINT32(ptr); + return rv; +} + static inline BYTE* pack_TPM_RQU_HEADER(BYTE* ptr, TPM_TAG tag, @@ -923,5 +932,6 @@ inline int unpack3_TPM_RQU_HEADER(BYTE* ptr, UINT32* pos, UINT32 max, #define pack_TPM_RSP_HEADER(p, t, s, r) pack_TPM_RQU_HEADER(p, t, s, r) #define unpack_TPM_RSP_HEADER(p, t, s, r) unpack_TPM_RQU_HEADER(p, t, s, r) #define unpack3_TPM_RSP_HEADER(p, l, m, t, s, r) unpack3_TPM_RQU_HEADER(p, l, m, t, s, r) +#define sizeof_TPM_RSP_HEADER(p) sizeof_TPM_RQU_HEADER(p) #endif diff --git a/stubdom/vtpmmgr/vtpm_cmd_handler.c b/stubdom/vtpmmgr/vtpm_cmd_handler.c index 2ac14fae77..7ca1d9df94 100644 --- a/stubdom/vtpmmgr/vtpm_cmd_handler.c +++ b/stubdom/vtpmmgr/vtpm_cmd_handler.c @@ -47,6 +47,7 @@ #include "vtpm_disk.h" #include "vtpmmgr.h" #include "tpm.h" +#include "tpm2.h" #include "tpmrsa.h" #include "tcg.h" #include "mgmt_authority.h" @@ -772,6 +773,52 @@ static int vtpmmgr_permcheck(struct tpm_opaque *opq) return 1; } +TPM_RESULT vtpmmgr_handle_getrandom(struct tpm_opaque *opaque, + tpmcmd_t* tpmcmd) +{ + TPM_RESULT status = TPM_SUCCESS; + TPM_TAG tag; + UINT32 size; + UINT32 rand_offset; + UINT32 rand_size; + TPM_COMMAND_CODE ord; + BYTE *p; + + p = unpack_TPM_RQU_HEADER(tpmcmd->req, &tag, &size, &ord); + + if (!hw_is_tpm2()) { + size = TCPA_MAX_BUFFER_LENGTH; + TPMTRYRETURN(TPM_TransmitData(tpmcmd->req, tpmcmd->req_len, + tpmcmd->resp, &size)); + tpmcmd->resp_len = size; + + return TPM_SUCCESS; + } + + /* TPM_GetRandom req: <header><uint32 num bytes> */ + unpack_UINT32(p, &rand_size); + + /* Call TPM2_GetRandom but return a TPM_GetRandom response. */ + /* TPM_GetRandom resp: <header><uint32 num bytes><num random bytes> */ + rand_offset = sizeof_TPM_RSP_HEADER(tpmcmd->resp) + + sizeof_UINT32(tpmcmd->resp); + + TPMTRYRETURN(TPM2_GetRandom(&rand_size, tpmcmd->resp + rand_offset)); + + p = pack_TPM_RSP_HEADER(tpmcmd->resp, TPM_TAG_RSP_COMMAND, + rand_offset + rand_size, status); + p = pack_UINT32(p, rand_size); + tpmcmd->resp_len = rand_offset + rand_size; + + return status; + +abort_egress: + tpmcmd->resp_len = VTPM_COMMAND_HEADER_SIZE; + pack_TPM_RSP_HEADER(tpmcmd->resp, tag + 3, tpmcmd->resp_len, status); + + return status; +} + TPM_RESULT vtpmmgr_handle_cmd( struct tpm_opaque *opaque, tpmcmd_t* tpmcmd) @@ -842,6 +889,7 @@ TPM_RESULT vtpmmgr_handle_cmd( switch(ord) { case TPM_ORD_GetRandom: vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_GetRandom\n"); + return vtpmmgr_handle_getrandom(opaque, tpmcmd); break; case TPM_ORD_PcrRead: vtpmloginfo(VTPM_LOG_VTPM, "Passthrough: TPM_PcrRead\n");
GetRandom passthrough currently fails when using vtpmmgr with a hardware TPM 2.0. vtpmmgr (8): INFO[VTPM]: Passthrough: TPM_GetRandom vtpm (12): vtpm_cmd.c:120: Error: TPM_GetRandom() failed with error code (30) When running on TPM 2.0 hardware, vtpmmgr needs to convert the TPM 1.2 TPM_ORD_GetRandom into a TPM2 TPM_CC_GetRandom command. Besides the differing ordinal, the TPM 1.2 uses 32bit sizes for the request and response (vs. 16bit for TPM2). Place the random output directly into the tpmcmd->resp and build the packet around it. This avoids bouncing through an extra buffer, but the header has to be written after grabbing the random bytes so we have the number of bytes to include in the size. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- stubdom/vtpmmgr/marshal.h | 10 +++++++ stubdom/vtpmmgr/vtpm_cmd_handler.c | 48 ++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+)