[SLOF] [PATCH v3 2/2] tpm: Implement firmware API call pass-through-to-tpm

[Alexey Kardashevskiy]

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 at 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 at 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.
> 
>