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

[Stefan Berger]

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 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?

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