[Skiboot] [PATCH v6a 1/4] tssskiboot.c: chunk reads/writes in 1024-sized buffers to support larger nv indices

[Kenneth Goldman]

A few comments:

1024 is not guaranteed, and the PC client uses 512.  The safest approach is
to use getcapability.  You also have to handle back level TPMs that don't
implement that capability. See readNvBufferMax() for the implementation.

A int64_t  signed value is strange for a two byte unsigned length  Letting
it go negative is risky, especially when adding it to a pointer.  Always
subtracting 1024 is a C trick - as a minimum, it needs lots of
comments.  .I think it's cleaner to calculate the bytes to read once and
put that value into in->size, then subtract in->size, terminating at zero.


> From: Eric Richter <erichte at linux.ibm.com>
> To: skiboot at lists.ozlabs.org
> Cc: klaus at linux.ibm.com, nayna at linux.ibm.com
> Date: 09/28/2020 06:07 PM
> Subject: [EXTERNAL] [Skiboot] [PATCH v6a 1/4] tssskiboot.c: chunk
> reads/writes in 1024-sized buffers to support larger nv indices
> Sent by: "Skiboot" <skiboot-bounces+kgoldman=us.ibm.com at lists.ozlabs.org>
>
> The Nuvoton npct650 chip has a command buffer max size of 1024.
> Attempting to read or write from an NV index larger than this value
> would return an error.
>
> This patch changes the tss_nv_read and tss_nv_write commands to chunk
> their operations in 1024-byte batches to allow support for larger NV
> indices.
>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
>  libstb/tss2/tssskiboot.c | 82 ++++++++++++++++++++++++++--------------
>  1 file changed, 54 insertions(+), 28 deletions(-)
>
> diff --git a/libstb/tss2/tssskiboot.c b/libstb/tss2/tssskiboot.c
> index 5f6d7611..b97159e7 100644
> --- a/libstb/tss2/tssskiboot.c
> +++ b/libstb/tss2/tssskiboot.c
> @@ -13,6 +13,8 @@
>  #include <ibmtss/tssmarshal.h>
>  #include <ibmtss/tssresponsecode.h>
>
> +#define TSS_MAX_NV_BUFFER_SIZE 1024
> +
>  /*
>   * Helper to string-fy TSS error response codes.
>   */
> @@ -98,6 +100,7 @@ int tss_nv_read(TPMI_RH_NV_INDEX nv_index, void
*buffer,
>     NV_Read_Out *out = NULL;
>     NV_Read_In *in = NULL;
>     TPM_RC rc = OPAL_SUCCESS;
> +   int64_t buffer_remaining;
>
>     if (!buffer) {
>        rc = OPAL_PARAMETER;
> @@ -125,21 +128,30 @@ int tss_nv_read(TPMI_RH_NV_INDEX nv_index, void
*buffer,
>
>     in->nvIndex = nv_index;
>     in->authHandle = nv_index;
> -   in->offset = offset;
> -   in->size = buffer_size;
>
> -   rc = TSS_Execute(context,
> -          (RESPONSE_PARAMETERS *) out,
> -          (COMMAND_PARAMETERS *) in,
> -          NULL,
> -          TPM_CC_NV_Read,
> -          TPM_RS_PW, NULL, 0,
> -          TPM_RH_NULL, NULL, 0);
> +   buffer_remaining = buffer_size;
> +   while (buffer_remaining > 0) {
> +      in->offset = offset;
> +      in->size = MIN(TSS_MAX_NV_BUFFER_SIZE, buffer_remaining);
>
> -   if (!rc)
> -      memcpy(buffer, out->data.b.buffer, buffer_size);
> -   else
> -      tss_error_trace("tss_nv_read", rc);
> +      rc = TSS_Execute(context,
> +             (RESPONSE_PARAMETERS *) out,
> +             (COMMAND_PARAMETERS *) in,
> +             NULL,
> +             TPM_CC_NV_Read,
> +             TPM_RS_PW, NULL, 0,
> +             TPM_RH_NULL, NULL, 0);
> +
> +      if (rc) {
> +         tss_error_trace("tss_nv_read", rc);
> +         goto cleanup;
> +      }
> +
> +      memcpy(buffer, out->data.b.buffer, in->size);
> +      buffer += TSS_MAX_NV_BUFFER_SIZE;
> +      buffer_remaining -= TSS_MAX_NV_BUFFER_SIZE;
> +      offset += TSS_MAX_NV_BUFFER_SIZE;
> +   }
>
>  cleanup:
>     TSS_Delete(context);
> @@ -161,6 +173,7 @@ int tss_nv_write(TPMI_RH_NV_INDEX nv_index, void
*buffer,
>     TSS_CONTEXT *context = NULL;
>     NV_Write_In *in = NULL;
>     TPM_RC rc = OPAL_SUCCESS;
> +   int64_t buffer_remaining;
>
>     if (!buffer) {
>        rc = OPAL_PARAMETER;
> @@ -182,23 +195,36 @@ int tss_nv_write(TPMI_RH_NV_INDEX nv_index,
> void *buffer,
>
>     in->nvIndex = nv_index;
>     in->authHandle = TPM_RH_PLATFORM;
> -   in->offset = offset;
> -   rc = TSS_TPM2B_Create(&in->data.b, buffer, buffer_size,
> -               sizeof(in->data.t.buffer));
> -   if (rc) {
> -      tss_error_trace("tss_nv_write", rc);
> -      goto cleanup;
> +
> +   buffer_remaining = buffer_size;
> +   while (buffer_remaining > 0) {
> +      in->offset = offset;
> +      rc = TSS_TPM2B_Create(&in->data.b, buffer,
> +                  MIN(TSS_MAX_NV_BUFFER_SIZE, buffer_remaining),
> +                  sizeof(in->data.t.buffer));
> +
> +      if (rc) {
> +         tss_error_trace("tss_nv_write", rc);
> +         goto cleanup;
> +      }
> +
> +      rc = TSS_Execute(context,
> +             NULL,
> +             (COMMAND_PARAMETERS *) in,
> +             NULL,
> +             TPM_CC_NV_Write,
> +             TPM_RS_PW, NULL, 0,
> +             TPM_RH_NULL, NULL, 0);
> +      if (rc) {
> +         tss_error_trace("tss_nv_write", rc);
> +         goto cleanup;
> +      }
> +
> +      buffer += TSS_MAX_NV_BUFFER_SIZE;
> +      buffer_remaining -= TSS_MAX_NV_BUFFER_SIZE;
> +      offset += TSS_MAX_NV_BUFFER_SIZE;
>     }
>
> -   rc = TSS_Execute(context,
> -          NULL,
> -          (COMMAND_PARAMETERS *) in,
> -          NULL,
> -          TPM_CC_NV_Write,
> -          TPM_RS_PW, NULL, 0,
> -          TPM_RH_NULL, NULL, 0);
> -   if (rc)
> -      tss_error_trace("tss_nv_write", rc);
>  cleanup:
>     TSS_Delete(context);
>     free(in);
> --
> 2.21.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> INVALID URI REMOVED
> u=https-3A__lists.ozlabs.org_listinfo_skiboot&d=DwICAg&c=jf_iaSHvJObTbx-
> siA1ZOg&r=DZCVG43VcL8GTneMZb8k8lEwb-O1GZktFfre1-
>
mlmiA&m=Lhg9Fbycamsl8ljo_qgfAqufCe7ygbaEdyA5oatS8Sg&s=UCqK31nTMpsZxNo632V67tYvuDdZbwEZudPrVrWuZnQ&e=

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20200929/3d1d83f8/attachment.htm>