diff mbox series

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

Message ID 20200928220609.10479-2-erichte@linux.ibm.com
State Accepted
Headers show
Series Initial secure variable drivers addendum | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch fail Failed to apply to any branch
snowpatch_ozlabs/apply_patch warning Failed to apply on branch master (d362ae4f4c521a7faffb1befe2fbba467f2c4d18)

Commit Message

Eric Richter Sept. 28, 2020, 10:06 p.m. UTC
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@linux.ibm.com>
---
 libstb/tss2/tssskiboot.c | 82 ++++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 28 deletions(-)

Comments

Kenneth Goldman Sept. 29, 2020, 1:08 p.m. UTC | #1
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@linux.ibm.com>
> To: skiboot@lists.ozlabs.org
> Cc: klaus@linux.ibm.com, nayna@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@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@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@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=

>
diff mbox series

Patch

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);