| Message ID | 20241029124934.3167208-3-stefanb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement 2 missing TPM related firmware API calls | expand |
On Tue, 29 Oct 2024, at 23:49, Stefan Berger wrote: > From: Stefan Berger <stefanb@linux.ibm.com> > > Implement the firmware API call pass-through-to-tpm that allows a caller > to pass a TPM command to the TPM. Since the buffer provided by the user > will be used for returning the TPM's response it must be sufficiently > large. To be safe, it should be of the size returned by the firmware API > call tpm-get-maximum-cmd-size. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > board-qemu/slof/vio-vtpm-cdriver.fs | 11 +++++++++++ > lib/libtpm/tcgbios.c | 25 +++++++++++++++++++++++++ > lib/libtpm/tcgbios.h | 1 + > lib/libtpm/tpm.code | 11 +++++++++++ > lib/libtpm/tpm.in | 1 + > 5 files changed, 49 insertions(+) > > diff --git a/board-qemu/slof/vio-vtpm-cdriver.fs b/board-qemu/slof/vio-vtpm-cdriver.fs > index 21c2190..ced2ac0 100644 > --- a/board-qemu/slof/vio-vtpm-cdriver.fs > +++ b/board-qemu/slof/vio-vtpm-cdriver.fs > @@ -57,6 +57,17 @@ LOG-SIZE BUFFER: log-base > THEN > ; > > +\ firmware API call > +: pass-through-to-tpm ( buf-addr cmd-size -- rsp-size ) > + vtpm-debug? IF > + ." Call to pass-through-to-tpm" cr > + THEN > + tpm-pass-through-to-tpm ( rsp-size ) > + vtpm-debug? IF > + ." VTPM: tpm-pass-through-to-tpm returned size: " dup . cr > + THEN > +; > + > \ firmware API call > : get-maximum-cmd-size ( -- max-size ) > vtpm-debug? IF > diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c > index a64afde..366eda9 100644 > --- a/lib/libtpm/tcgbios.c > +++ b/lib/libtpm/tcgbios.c > @@ -972,6 +972,31 @@ uint32_t tpm_get_maximum_cmd_size(void) > return PAPR_VTPM_MAX_BUFFER_SIZE; > } > > +uint32_t tpm_pass_through_to_tpm(void *buffer, uint32_t cmd_size) > +{ > + unsigned char respbuffer[PAPR_VTPM_MAX_BUFFER_SIZE]; > + uint32_t respbufferlen = sizeof(respbuffer); > + struct tpm_req_header *hdr = buffer; > + uint32_t totlen; uint32_t totlen = be32_to_cpu(hdr->totlen); and drop memcpy(&totlen,...) below? > + int ret; A nit: not really needed, could do "if (spapr_transmit())" (only because I am commenting on other things) > + > + if (cmd_size < sizeof(struct tpm_req_header)) > + return 0; > + > + memcpy(&totlen, &hdr->totlen, sizeof(totlen)); > + if (cmd_size != be32_to_cpu(totlen)) So the caller has to pass the length twice - in @buffer (==header) and in @cmd_size, can we drop @cmd_size? Is it a total size of the buffer (and then passing @cmd_size makes sense but checking cmd_size!=totlen does not) or a size of a TPM command (looks like this is the case but then this function does not know if @buffer is big enough for the response)? > + return 0; > + > + ret = spapr_transmit(0, buffer, respbuffer, &respbufferlen, > + TPM_DURATION_TYPE_LONG); > + if (ret) > + return 0; > + > + memcpy(buffer, respbuffer, respbufferlen); This assumes @buffer has enough space and respbufferlen<=cmd_size but does not check for this. Probably may be also worth checking if the returned hdr->totlen not out of boundaries too, or the returned buffer does not have a header? Thanks, > + > + return respbufferlen; > +} > + > /* > * Add an EV_ACTION measurement to the list of measurements > */ > diff --git a/lib/libtpm/tcgbios.h b/lib/libtpm/tcgbios.h > index 83148e0..0e98e63 100644 > --- a/lib/libtpm/tcgbios.h > +++ b/lib/libtpm/tcgbios.h > @@ -42,5 +42,6 @@ uint32_t tpm_2hash_ext_log(uint32_t pcrindex, > const char *info, uint32_t infolen, > const void *data, uint64_t datalen); > uint32_t tpm_get_maximum_cmd_size(void); > +uint32_t tpm_pass_through_to_tpm(void *buffer, uint32_t cmdsize); > > #endif /* TCGBIOS_H */ > diff --git a/lib/libtpm/tpm.code b/lib/libtpm/tpm.code > index 23075b8..27a87c9 100644 > --- a/lib/libtpm/tpm.code > +++ b/lib/libtpm/tpm.code > @@ -216,3 +216,14 @@ PRIM(tpm_X2d_get_X2d_maximum_X2d_cmd_X2d_size) > PUSH; > TOS.u = tpm_get_maximum_cmd_size(); > MIRP > + > +/****************************************************************************************/ > +/* SLOF: tpm-pass-through-to-tpm ( buf-addr cmd-size -- rsp-size ) */ > +/* LIBTPM: rsp_size = tpm-pass-through-to-tpm */ > +/****************************************************************************************/ > +PRIM(tpm_X2d_pass_X2d_through_X2d_to_X2d_tpm) > + uint32_t cmd_size = TOS.u; POP; > + void *buf = TOS.a; > + > + TOS.u = tpm_pass_through_to_tpm(buf, cmd_size); > +MIRP > diff --git a/lib/libtpm/tpm.in b/lib/libtpm/tpm.in > index d76c479..b413a24 100644 > --- a/lib/libtpm/tpm.in > +++ b/lib/libtpm/tpm.in > @@ -31,3 +31,4 @@ cod(tpm-measure-gpt) > cod(tpm-hash-log-extend-event-buffer) > cod(tpm-2hash-ext-log) > cod(tpm-get-maximum-cmd-size) > +cod(tpm-pass-through-to-tpm) > -- > 2.25.1 > >
On 10/31/24 9:52 PM, Alexey Kardashevskiy wrote: > > > On Tue, 29 Oct 2024, at 23:49, Stefan Berger wrote: >> From: Stefan Berger <stefanb@linux.ibm.com> >> >> Implement the firmware API call pass-through-to-tpm that allows a caller >> to pass a TPM command to the TPM. Since the buffer provided by the user >> will be used for returning the TPM's response it must be sufficiently >> large. To be safe, it should be of the size returned by the firmware API >> call tpm-get-maximum-cmd-size. >> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> --- >> board-qemu/slof/vio-vtpm-cdriver.fs | 11 +++++++++++ >> lib/libtpm/tcgbios.c | 25 +++++++++++++++++++++++++ >> lib/libtpm/tcgbios.h | 1 + >> lib/libtpm/tpm.code | 11 +++++++++++ >> lib/libtpm/tpm.in | 1 + >> 5 files changed, 49 insertions(+) >> >> diff --git a/board-qemu/slof/vio-vtpm-cdriver.fs b/board-qemu/slof/vio-vtpm-cdriver.fs >> index 21c2190..ced2ac0 100644 >> --- a/board-qemu/slof/vio-vtpm-cdriver.fs >> +++ b/board-qemu/slof/vio-vtpm-cdriver.fs >> @@ -57,6 +57,17 @@ LOG-SIZE BUFFER: log-base >> THEN >> ; >> >> +\ firmware API call >> +: pass-through-to-tpm ( buf-addr cmd-size -- rsp-size ) >> + vtpm-debug? IF >> + ." Call to pass-through-to-tpm" cr >> + THEN >> + tpm-pass-through-to-tpm ( rsp-size ) >> + vtpm-debug? IF >> + ." VTPM: tpm-pass-through-to-tpm returned size: " dup . cr >> + THEN >> +; >> + >> \ firmware API call >> : get-maximum-cmd-size ( -- max-size ) >> vtpm-debug? IF >> diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c >> index a64afde..366eda9 100644 >> --- a/lib/libtpm/tcgbios.c >> +++ b/lib/libtpm/tcgbios.c >> @@ -972,6 +972,31 @@ uint32_t tpm_get_maximum_cmd_size(void) >> return PAPR_VTPM_MAX_BUFFER_SIZE; >> } >> >> +uint32_t tpm_pass_through_to_tpm(void *buffer, uint32_t cmd_size) >> +{ >> + unsigned char respbuffer[PAPR_VTPM_MAX_BUFFER_SIZE]; >> + uint32_t respbufferlen = sizeof(respbuffer); >> + struct tpm_req_header *hdr = buffer; >> + uint32_t totlen; > > uint32_t totlen = be32_to_cpu(hdr->totlen); > and drop memcpy(&totlen,...) below? I did this not wanting to assume alignment. > >> + int ret; > > A nit: not really needed, could do "if (spapr_transmit())" > (only because I am commenting on other things) > >> + >> + if (cmd_size < sizeof(struct tpm_req_header)) >> + return 0; >> + >> + memcpy(&totlen, &hdr->totlen, sizeof(totlen)); >> + if (cmd_size != be32_to_cpu(totlen)) > > So the caller has to pass the length twice - in @buffer (==header) and in @cmd_size, can we drop @cmd_size? cmd_size is redundant. > Is it a total size of the buffer (and then passing @cmd_size makes sense but checking cmd_size!=totlen does not) or a size of a TPM command (looks like this is the case but then this function does not know if @buffer is big enough for the response)? The spec says: "The method takes as arguments the address and size of a fully formed TPM command – which is passed to the vTPM. The entire response is copied back over the input buffer – which must be large enough to accept the expected response or else the buffer will be overrun ." > >> + return 0; >> + >> + ret = spapr_transmit(0, buffer, respbuffer, &respbufferlen, >> + TPM_DURATION_TYPE_LONG); >> + if (ret) >> + return 0; >> + >> + memcpy(buffer, respbuffer, respbufferlen); > > This assumes @buffer has enough space and respbufferlen<=cmd_size but does not check for this. > Probably may be also worth checking if the returned hdr->totlen not out of boundaries too, or the returned buffer does not have a header? Thanks, See quote from spec.
On Fri, 1 Nov 2024, at 22:47, Stefan Berger wrote: > > > On 10/31/24 9:52 PM, Alexey Kardashevskiy wrote: > > > > > > On Tue, 29 Oct 2024, at 23:49, Stefan Berger wrote: > >> From: Stefan Berger <stefanb@linux.ibm.com> > >> > >> Implement the firmware API call pass-through-to-tpm that allows a caller > >> to pass a TPM command to the TPM. Since the buffer provided by the user > >> will be used for returning the TPM's response it must be sufficiently > >> large. To be safe, it should be of the size returned by the firmware API > >> call tpm-get-maximum-cmd-size. > >> > >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > >> --- > >> board-qemu/slof/vio-vtpm-cdriver.fs | 11 +++++++++++ > >> lib/libtpm/tcgbios.c | 25 +++++++++++++++++++++++++ > >> lib/libtpm/tcgbios.h | 1 + > >> lib/libtpm/tpm.code | 11 +++++++++++ > >> lib/libtpm/tpm.in | 1 + > >> 5 files changed, 49 insertions(+) > >> > >> diff --git a/board-qemu/slof/vio-vtpm-cdriver.fs b/board-qemu/slof/vio-vtpm-cdriver.fs > >> index 21c2190..ced2ac0 100644 > >> --- a/board-qemu/slof/vio-vtpm-cdriver.fs > >> +++ b/board-qemu/slof/vio-vtpm-cdriver.fs > >> @@ -57,6 +57,17 @@ LOG-SIZE BUFFER: log-base > >> THEN > >> ; > >> > >> +\ firmware API call > >> +: pass-through-to-tpm ( buf-addr cmd-size -- rsp-size ) > >> + vtpm-debug? IF > >> + ." Call to pass-through-to-tpm" cr > >> + THEN > >> + tpm-pass-through-to-tpm ( rsp-size ) > >> + vtpm-debug? IF > >> + ." VTPM: tpm-pass-through-to-tpm returned size: " dup . cr > >> + THEN > >> +; > >> + > >> \ firmware API call > >> : get-maximum-cmd-size ( -- max-size ) > >> vtpm-debug? IF > >> diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c > >> index a64afde..366eda9 100644 > >> --- a/lib/libtpm/tcgbios.c > >> +++ b/lib/libtpm/tcgbios.c > >> @@ -972,6 +972,31 @@ uint32_t tpm_get_maximum_cmd_size(void) > >> return PAPR_VTPM_MAX_BUFFER_SIZE; > >> } > >> > >> +uint32_t tpm_pass_through_to_tpm(void *buffer, uint32_t cmd_size) > >> +{ > >> + unsigned char respbuffer[PAPR_VTPM_MAX_BUFFER_SIZE]; > >> + uint32_t respbufferlen = sizeof(respbuffer); > >> + struct tpm_req_header *hdr = buffer; > >> + uint32_t totlen; > > > > uint32_t totlen = be32_to_cpu(hdr->totlen); > > and drop memcpy(&totlen,...) below? > > I did this not wanting to assume alignment. Why would unaligned access not work here? > > > >> + int ret; > > > > A nit: not really needed, could do "if (spapr_transmit())" > > (only because I am commenting on other things) > > > >> + > >> + if (cmd_size < sizeof(struct tpm_req_header)) > >> + return 0; > >> + > >> + memcpy(&totlen, &hdr->totlen, sizeof(totlen)); > >> + if (cmd_size != be32_to_cpu(totlen)) > > > > So the caller has to pass the length twice - in @buffer (==header) and in @cmd_size, can we drop @cmd_size? > > cmd_size is redundant. then drop it? > > Is it a total size of the buffer (and then passing @cmd_size makes sense but checking cmd_size!=totlen does not) or a size of a TPM command (looks like this is the case but then this function does not know if @buffer is big enough for the response)? > > The spec says: "The method takes as arguments the address and size of a > fully formed TPM command – which is passed to the vTPM. The entire > response is copied back over the input buffer – which must be large > enough to accept the expected response or else the buffer will be overrun ." What a lovely spec :) What does "large enough" mean there, a page? I can see 1K is the bare minimum for spapr_vtpm.buffer_size, can we use this parameter, or it is a wrong buffer size? I just think we better not allow buffer overrun even if the spec is fine about it. Thanks, > > > >> + return 0; > >> + > >> + ret = spapr_transmit(0, buffer, respbuffer, &respbufferlen, > >> + TPM_DURATION_TYPE_LONG); > >> + if (ret) > >> + return 0; > >> + > >> + memcpy(buffer, respbuffer, respbufferlen); > > > > This assumes @buffer has enough space and respbufferlen<=cmd_size but does not check for this. > > Probably may be also worth checking if the returned hdr->totlen not out of boundaries too, or the returned buffer does not have a header? Thanks, > > See quote from spec. > >
On 11/1/24 11:54 PM, Alexey Kardashevskiy wrote: > > > On Fri, 1 Nov 2024, at 22:47, Stefan Berger wrote: >> >> >> On 10/31/24 9:52 PM, Alexey Kardashevskiy wrote: >>> >>> >>> On Tue, 29 Oct 2024, at 23:49, Stefan Berger wrote: >>>> From: Stefan Berger <stefanb@linux.ibm.com> >>>> >>>> Implement the firmware API call pass-through-to-tpm that allows a caller >>>> to pass a TPM command to the TPM. Since the buffer provided by the user >>>> will be used for returning the TPM's response it must be sufficiently >>>> large. To be safe, it should be of the size returned by the firmware API >>>> call tpm-get-maximum-cmd-size. >>>> >>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>> --- >>>> board-qemu/slof/vio-vtpm-cdriver.fs | 11 +++++++++++ >>>> lib/libtpm/tcgbios.c | 25 +++++++++++++++++++++++++ >>>> lib/libtpm/tcgbios.h | 1 + >>>> lib/libtpm/tpm.code | 11 +++++++++++ >>>> lib/libtpm/tpm.in | 1 + >>>> 5 files changed, 49 insertions(+) >>>> >>>> diff --git a/board-qemu/slof/vio-vtpm-cdriver.fs b/board-qemu/slof/vio-vtpm-cdriver.fs >>>> index 21c2190..ced2ac0 100644 >>>> --- a/board-qemu/slof/vio-vtpm-cdriver.fs >>>> +++ b/board-qemu/slof/vio-vtpm-cdriver.fs >>>> @@ -57,6 +57,17 @@ LOG-SIZE BUFFER: log-base >>>> THEN >>>> ; >>>> >>>> +\ firmware API call >>>> +: pass-through-to-tpm ( buf-addr cmd-size -- rsp-size ) >>>> + vtpm-debug? IF >>>> + ." Call to pass-through-to-tpm" cr >>>> + THEN >>>> + tpm-pass-through-to-tpm ( rsp-size ) >>>> + vtpm-debug? IF >>>> + ." VTPM: tpm-pass-through-to-tpm returned size: " dup . cr >>>> + THEN >>>> +; >>>> + >>>> \ firmware API call >>>> : get-maximum-cmd-size ( -- max-size ) >>>> vtpm-debug? IF >>>> diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c >>>> index a64afde..366eda9 100644 >>>> --- a/lib/libtpm/tcgbios.c >>>> +++ b/lib/libtpm/tcgbios.c >>>> @@ -972,6 +972,31 @@ uint32_t tpm_get_maximum_cmd_size(void) >>>> return PAPR_VTPM_MAX_BUFFER_SIZE; >>>> } >>>> >>>> +uint32_t tpm_pass_through_to_tpm(void *buffer, uint32_t cmd_size) >>>> +{ >>>> + unsigned char respbuffer[PAPR_VTPM_MAX_BUFFER_SIZE]; >>>> + uint32_t respbufferlen = sizeof(respbuffer); >>>> + struct tpm_req_header *hdr = buffer; >>>> + uint32_t totlen; >>> >>> uint32_t totlen = be32_to_cpu(hdr->totlen); >>> and drop memcpy(&totlen,...) below? >> >> I did this not wanting to assume alignment. > > Why would unaligned access not work here? If hdr->totlen was on an odd address we'd probably get an exception. > >>> >>>> + int ret; >>> >>> A nit: not really needed, could do "if (spapr_transmit())" >>> (only because I am commenting on other things) >>> >>>> + >>>> + if (cmd_size < sizeof(struct tpm_req_header)) >>>> + return 0; >>>> + >>>> + memcpy(&totlen, &hdr->totlen, sizeof(totlen)); >>>> + if (cmd_size != be32_to_cpu(totlen)) >>> >>> So the caller has to pass the length twice - in @buffer (==header) and in @cmd_size, can we drop @cmd_size? >> >> cmd_size is redundant. > > then drop it? Sure, I can drop it at an earlier point in the functions call path. > >>> Is it a total size of the buffer (and then passing @cmd_size makes sense but checking cmd_size!=totlen does not) or a size of a TPM command (looks like this is the case but then this function does not know if @buffer is big enough for the response)? >> >> The spec says: "The method takes as arguments the address and size of a >> fully formed TPM command – which is passed to the vTPM. The entire >> response is copied back over the input buffer – which must be large >> enough to accept the expected response or else the buffer will be overrun ." > > What a lovely spec :) What does "large enough" mean there, a page? I can see 1K is the bare minimum for spapr_vtpm.buffer_size, can we use this parameter, or it is a wrong buffer size? I just think we better not allow buffer overrun even if the spec is fine about it. Thanks, It basically means the caller should use a buffer of size returned by tpm-get-maximum-cmd-size, as described in the commit message. There's no size of the callers' buffer given via API so that we could restrict the copy size. > >>> >>>> + return 0; >>>> + >>>> + ret = spapr_transmit(0, buffer, respbuffer, &respbufferlen, >>>> + TPM_DURATION_TYPE_LONG); >>>> + if (ret) >>>> + return 0; >>>> + >>>> + memcpy(buffer, respbuffer, respbufferlen); >>> >>> This assumes @buffer has enough space and respbufferlen<=cmd_size but does not check for this. >>> Probably may be also worth checking if the returned hdr->totlen not out of boundaries too, or the returned buffer does not have a header? Thanks, >> >> See quote from spec. >> >>
On Sat, 2 Nov 2024, at 23:50, Stefan Berger wrote: > > > On 11/1/24 11:54 PM, Alexey Kardashevskiy wrote: > > > > > > On Fri, 1 Nov 2024, at 22:47, Stefan Berger wrote: > >> > >> > >> On 10/31/24 9:52 PM, Alexey Kardashevskiy wrote: > >>> > >>> > >>> On Tue, 29 Oct 2024, at 23:49, Stefan Berger wrote: > >>>> From: Stefan Berger <stefanb@linux.ibm.com> > >>>> > >>>> Implement the firmware API call pass-through-to-tpm that allows a caller > >>>> to pass a TPM command to the TPM. Since the buffer provided by the user > >>>> will be used for returning the TPM's response it must be sufficiently > >>>> large. To be safe, it should be of the size returned by the firmware API > >>>> call tpm-get-maximum-cmd-size. > >>>> > >>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > >>>> --- > >>>> board-qemu/slof/vio-vtpm-cdriver.fs | 11 +++++++++++ > >>>> lib/libtpm/tcgbios.c | 25 +++++++++++++++++++++++++ > >>>> lib/libtpm/tcgbios.h | 1 + > >>>> lib/libtpm/tpm.code | 11 +++++++++++ > >>>> lib/libtpm/tpm.in | 1 + > >>>> 5 files changed, 49 insertions(+) > >>>> > >>>> diff --git a/board-qemu/slof/vio-vtpm-cdriver.fs b/board-qemu/slof/vio-vtpm-cdriver.fs > >>>> index 21c2190..ced2ac0 100644 > >>>> --- a/board-qemu/slof/vio-vtpm-cdriver.fs > >>>> +++ b/board-qemu/slof/vio-vtpm-cdriver.fs > >>>> @@ -57,6 +57,17 @@ LOG-SIZE BUFFER: log-base > >>>> THEN > >>>> ; > >>>> > >>>> +\ firmware API call > >>>> +: pass-through-to-tpm ( buf-addr cmd-size -- rsp-size ) > >>>> + vtpm-debug? IF > >>>> + ." Call to pass-through-to-tpm" cr > >>>> + THEN > >>>> + tpm-pass-through-to-tpm ( rsp-size ) > >>>> + vtpm-debug? IF > >>>> + ." VTPM: tpm-pass-through-to-tpm returned size: " dup . cr > >>>> + THEN > >>>> +; > >>>> + > >>>> \ firmware API call > >>>> : get-maximum-cmd-size ( -- max-size ) > >>>> vtpm-debug? IF > >>>> diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c > >>>> index a64afde..366eda9 100644 > >>>> --- a/lib/libtpm/tcgbios.c > >>>> +++ b/lib/libtpm/tcgbios.c > >>>> @@ -972,6 +972,31 @@ uint32_t tpm_get_maximum_cmd_size(void) > >>>> return PAPR_VTPM_MAX_BUFFER_SIZE; > >>>> } > >>>> > >>>> +uint32_t tpm_pass_through_to_tpm(void *buffer, uint32_t cmd_size) > >>>> +{ > >>>> + unsigned char respbuffer[PAPR_VTPM_MAX_BUFFER_SIZE]; > >>>> + uint32_t respbufferlen = sizeof(respbuffer); > >>>> + struct tpm_req_header *hdr = buffer; > >>>> + uint32_t totlen; > >>> > >>> uint32_t totlen = be32_to_cpu(hdr->totlen); > >>> and drop memcpy(&totlen,...) below? > >> > >> I did this not wanting to assume alignment. > > > > Why would unaligned access not work here? > > If hdr->totlen was on an odd address we'd probably get an exception. Seems unlikely as modern POWER should be able handle it but I do not have the hw to try (just for fun) adding "1" to the address and see if it throws an exception. > > > >>> > >>>> + int ret; > >>> > >>> A nit: not really needed, could do "if (spapr_transmit())" > >>> (only because I am commenting on other things) > >>> > >>>> + > >>>> + if (cmd_size < sizeof(struct tpm_req_header)) > >>>> + return 0; > >>>> + > >>>> + memcpy(&totlen, &hdr->totlen, sizeof(totlen)); > >>>> + if (cmd_size != be32_to_cpu(totlen)) > >>> > >>> So the caller has to pass the length twice - in @buffer (==header) and in @cmd_size, can we drop @cmd_size? > >> > >> cmd_size is redundant. > > > > then drop it? > > Sure, I can drop it at an earlier point in the functions call path. > > > > >>> Is it a total size of the buffer (and then passing @cmd_size makes sense but checking cmd_size!=totlen does not) or a size of a TPM command (looks like this is the case but then this function does not know if @buffer is big enough for the response)? > >> > >> The spec says: "The method takes as arguments the address and size of a > >> fully formed TPM command – which is passed to the vTPM. The entire > >> response is copied back over the input buffer – which must be large > >> enough to accept the expected response or else the buffer will be overrun ." > > > > What a lovely spec :) What does "large enough" mean there, a page? I can see 1K is the bare minimum for spapr_vtpm.buffer_size, can we use this parameter, or it is a wrong buffer size? I just think we better not allow buffer overrun even if the spec is fine about it. Thanks, > > It basically means the caller should use a buffer of size returned by > tpm-get-maximum-cmd-size, as described in the commit message. > There's no size of the callers' buffer given via API so that we could > restrict the copy size. Have a link to the spec handy? I could not easily google one, and the one I have does not mention tpm-pass-through-to-tpm. Thanks, > > > > >>> > >>>> + return 0; > >>>> + > >>>> + ret = spapr_transmit(0, buffer, respbuffer, &respbufferlen, > >>>> + TPM_DURATION_TYPE_LONG); > >>>> + if (ret) > >>>> + return 0; > >>>> + > >>>> + memcpy(buffer, respbuffer, respbufferlen); > >>> > >>> This assumes @buffer has enough space and respbufferlen<=cmd_size but does not check for this. > >>> Probably may be also worth checking if the returned hdr->totlen not out of boundaries too, or the returned buffer does not have a header? Thanks, > >> > >> See quote from spec. > >> > >> > >
On 11/3/24 5:16 AM, Alexey Kardashevskiy wrote: > > > On Sat, 2 Nov 2024, at 23:50, Stefan Berger wrote: >> >> >> On 11/1/24 11:54 PM, Alexey Kardashevskiy wrote: >>> >>> >>> On Fri, 1 Nov 2024, at 22:47, Stefan Berger wrote: >>>> >>>> >>>> On 10/31/24 9:52 PM, Alexey Kardashevskiy wrote: >>>>> >>>>> >>>>> On Tue, 29 Oct 2024, at 23:49, Stefan Berger wrote: >>>>>> From: Stefan Berger <stefanb@linux.ibm.com> >>>>>> >>>>>> Implement the firmware API call pass-through-to-tpm that allows a caller >>>>>> to pass a TPM command to the TPM. Since the buffer provided by the user >>>>>> will be used for returning the TPM's response it must be sufficiently >>>>>> large. To be safe, it should be of the size returned by the firmware API >>>>>> call tpm-get-maximum-cmd-size. >>>>>> >>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>>>> --- >>>>>> board-qemu/slof/vio-vtpm-cdriver.fs | 11 +++++++++++ >>>>>> lib/libtpm/tcgbios.c | 25 +++++++++++++++++++++++++ >>>>>> lib/libtpm/tcgbios.h | 1 + >>>>>> lib/libtpm/tpm.code | 11 +++++++++++ >>>>>> lib/libtpm/tpm.in | 1 + >>>>>> 5 files changed, 49 insertions(+) >>>>>> >>>>>> diff --git a/board-qemu/slof/vio-vtpm-cdriver.fs b/board-qemu/slof/vio-vtpm-cdriver.fs >>>>>> index 21c2190..ced2ac0 100644 >>>>>> --- a/board-qemu/slof/vio-vtpm-cdriver.fs >>>>>> +++ b/board-qemu/slof/vio-vtpm-cdriver.fs >>>>>> @@ -57,6 +57,17 @@ LOG-SIZE BUFFER: log-base >>>>>> THEN >>>>>> ; >>>>>> >>>>>> +\ firmware API call >>>>>> +: pass-through-to-tpm ( buf-addr cmd-size -- rsp-size ) >>>>>> + vtpm-debug? IF >>>>>> + ." Call to pass-through-to-tpm" cr >>>>>> + THEN >>>>>> + tpm-pass-through-to-tpm ( rsp-size ) >>>>>> + vtpm-debug? IF >>>>>> + ." VTPM: tpm-pass-through-to-tpm returned size: " dup . cr >>>>>> + THEN >>>>>> +; >>>>>> + >>>>>> \ firmware API call >>>>>> : get-maximum-cmd-size ( -- max-size ) >>>>>> vtpm-debug? IF >>>>>> diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c >>>>>> index a64afde..366eda9 100644 >>>>>> --- a/lib/libtpm/tcgbios.c >>>>>> +++ b/lib/libtpm/tcgbios.c >>>>>> @@ -972,6 +972,31 @@ uint32_t tpm_get_maximum_cmd_size(void) >>>>>> return PAPR_VTPM_MAX_BUFFER_SIZE; >>>>>> } >>>>>> >>>>>> +uint32_t tpm_pass_through_to_tpm(void *buffer, uint32_t cmd_size) >>>>>> +{ >>>>>> + unsigned char respbuffer[PAPR_VTPM_MAX_BUFFER_SIZE]; >>>>>> + uint32_t respbufferlen = sizeof(respbuffer); >>>>>> + struct tpm_req_header *hdr = buffer; >>>>>> + uint32_t totlen; >>>>> >>>>> uint32_t totlen = be32_to_cpu(hdr->totlen); >>>>> and drop memcpy(&totlen,...) below? >>>> >>>> I did this not wanting to assume alignment. >>> >>> Why would unaligned access not work here? >> >> If hdr->totlen was on an odd address we'd probably get an exception. > > Seems unlikely as modern POWER should be able handle it but I do not have the hw to try (just for fun) adding "1" to the address and see if it throws an exception. It can handle it at odd addresses as well. Still, removing the check now but would pass the cmd_size all the way down into this C function but leave it unused. >> >>> >>>>> Is it a total size of the buffer (and then passing @cmd_size makes sense but checking cmd_size!=totlen does not) or a size of a TPM command (looks like this is the case but then this function does not know if @buffer is big enough for the response)? >>>> >>>> The spec says: "The method takes as arguments the address and size of a >>>> fully formed TPM command – which is passed to the vTPM. The entire >>>> response is copied back over the input buffer – which must be large >>>> enough to accept the expected response or else the buffer will be overrun ." >>> >>> What a lovely spec :) What does "large enough" mean there, a page? I can see 1K is the bare minimum for spapr_vtpm.buffer_size, can we use this parameter, or it is a wrong buffer size? I just think we better not allow buffer overrun even if the spec is fine about it. Thanks, >> >> It basically means the caller should use a buffer of size returned by >> tpm-get-maximum-cmd-size, as described in the commit message. >> There's no size of the callers' buffer given via API so that we could >> restrict the copy size. > > Have a link to the spec handy? I could not easily google one, and the one I have does not mention tpm-pass-through-to-tpm. Thanks, Afaik it's not public. I am asking whether it can be published.
diff --git a/board-qemu/slof/vio-vtpm-cdriver.fs b/board-qemu/slof/vio-vtpm-cdriver.fs index 21c2190..ced2ac0 100644 --- a/board-qemu/slof/vio-vtpm-cdriver.fs +++ b/board-qemu/slof/vio-vtpm-cdriver.fs @@ -57,6 +57,17 @@ LOG-SIZE BUFFER: log-base THEN ; +\ firmware API call +: pass-through-to-tpm ( buf-addr cmd-size -- rsp-size ) + vtpm-debug? IF + ." Call to pass-through-to-tpm" cr + THEN + tpm-pass-through-to-tpm ( rsp-size ) + vtpm-debug? IF + ." VTPM: tpm-pass-through-to-tpm returned size: " dup . cr + THEN +; + \ firmware API call : get-maximum-cmd-size ( -- max-size ) vtpm-debug? IF diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c index a64afde..366eda9 100644 --- a/lib/libtpm/tcgbios.c +++ b/lib/libtpm/tcgbios.c @@ -972,6 +972,31 @@ uint32_t tpm_get_maximum_cmd_size(void) return PAPR_VTPM_MAX_BUFFER_SIZE; } +uint32_t tpm_pass_through_to_tpm(void *buffer, uint32_t cmd_size) +{ + unsigned char respbuffer[PAPR_VTPM_MAX_BUFFER_SIZE]; + uint32_t respbufferlen = sizeof(respbuffer); + struct tpm_req_header *hdr = buffer; + uint32_t totlen; + int ret; + + if (cmd_size < sizeof(struct tpm_req_header)) + return 0; + + memcpy(&totlen, &hdr->totlen, sizeof(totlen)); + if (cmd_size != be32_to_cpu(totlen)) + return 0; + + ret = spapr_transmit(0, buffer, respbuffer, &respbufferlen, + TPM_DURATION_TYPE_LONG); + if (ret) + return 0; + + memcpy(buffer, respbuffer, respbufferlen); + + return respbufferlen; +} + /* * Add an EV_ACTION measurement to the list of measurements */ diff --git a/lib/libtpm/tcgbios.h b/lib/libtpm/tcgbios.h index 83148e0..0e98e63 100644 --- a/lib/libtpm/tcgbios.h +++ b/lib/libtpm/tcgbios.h @@ -42,5 +42,6 @@ uint32_t tpm_2hash_ext_log(uint32_t pcrindex, const char *info, uint32_t infolen, const void *data, uint64_t datalen); uint32_t tpm_get_maximum_cmd_size(void); +uint32_t tpm_pass_through_to_tpm(void *buffer, uint32_t cmdsize); #endif /* TCGBIOS_H */ diff --git a/lib/libtpm/tpm.code b/lib/libtpm/tpm.code index 23075b8..27a87c9 100644 --- a/lib/libtpm/tpm.code +++ b/lib/libtpm/tpm.code @@ -216,3 +216,14 @@ PRIM(tpm_X2d_get_X2d_maximum_X2d_cmd_X2d_size) PUSH; TOS.u = tpm_get_maximum_cmd_size(); MIRP + +/****************************************************************************************/ +/* SLOF: tpm-pass-through-to-tpm ( buf-addr cmd-size -- rsp-size ) */ +/* LIBTPM: rsp_size = tpm-pass-through-to-tpm */ +/****************************************************************************************/ +PRIM(tpm_X2d_pass_X2d_through_X2d_to_X2d_tpm) + uint32_t cmd_size = TOS.u; POP; + void *buf = TOS.a; + + TOS.u = tpm_pass_through_to_tpm(buf, cmd_size); +MIRP diff --git a/lib/libtpm/tpm.in b/lib/libtpm/tpm.in index d76c479..b413a24 100644 --- a/lib/libtpm/tpm.in +++ b/lib/libtpm/tpm.in @@ -31,3 +31,4 @@ cod(tpm-measure-gpt) cod(tpm-hash-log-extend-event-buffer) cod(tpm-2hash-ext-log) cod(tpm-get-maximum-cmd-size) +cod(tpm-pass-through-to-tpm)