[Skiboot] [PATCH] libstb: Fix memcpy overread in fakenv_readpublic() - is truncation correct?

[Kenneth Goldman]

-----Original Message-----
From: Skiboot <skiboot-bounces+kgoldman=us.ibm.com at lists.ozlabs.org> On Behalf Of Reza Arbab
Sent: Wednesday, May 25, 2022 4:29 PM
To: skiboot at lists.ozlabs.org
Subject: [EXTERNAL] [Skiboot] [PATCH] libstb: Fix memcpy overread in fakenv_readpublic()

Caught by `make check` on fedora-rawhide (GCC 12):

  libstb/secvar/test/../storage/fakenv_ops.c: In function 'fakenv_readpublic':
  libstb/secvar/test/../storage/fakenv_ops.c:155:17: error: 'memcpy' reading 134 bytes from a region of size 34 [-Werror=stringop-overread]
    155 |                 memcpy(&nv_name->t.name, tpmnv_vars_name, sizeof(TPM2B_NAME));
        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from libstb/secvar/test/secvar-test-secboot-tpm.c:5:
  libstb/secvar/test/../storage/secboot_tpm.c:35:15: note: source object 'tpmnv_vars_name' of size 34
     35 | const uint8_t tpmnv_vars_name[] = {
        |               ^~~~~~~~~~~~~~~
  libstb/secvar/test/../storage/fakenv_ops.c:158:17: error: 'memcpy' reading 134 bytes from a region of size 34 [-Werror=stringop-overread]
    158 |                 memcpy(&nv_name->t.name, tpmnv_control_name, sizeof(TPM2B_NAME));
        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  libstb/secvar/test/../storage/secboot_tpm.c:41:15: note: source object 'tpmnv_control_name' of size 34
     41 | const uint8_t tpmnv_control_name[] = {
        |               ^~~~~~~~~~~~~~~~~~

Using the first error as an example; there are a couple of things to fix here. The types involved in the memcpy are

  const uint8_t tpmnv_vars_name[34];
  TPM2B_NAME *nv_name;

So the current memcpy line was likely intended to be

  memcpy(&nv_name->t.name, tpmnv_vars_name, sizeof(nv_name->t.name));

Second, as the error indicates, the memcpy size should be no more than the the size of the source or the destination, so

  memcpy(&nv_name->t.name, tpmnv_vars_name,
         MIN(sizeof(nv_name->t.name), sizeof(tpmnv_vars_name)));

[kgold] 
[kgold] Is truncating the Name the right solution?

I would have thought that, if the source length is greater than the target length, an error results.  The size of the target should be that of the largest hash algorithm.  If a Name is larger, didn't something go badly wrong?

Fix both errors similarly.

Signed-off-by: Reza Arbab <arbab at linux.ibm.com>
---
 libstb/secvar/storage/fakenv_ops.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libstb/secvar/storage/fakenv_ops.c b/libstb/secvar/storage/fakenv_ops.c
index 224ac2a5de7d..35d9dcdde5c0 100644
--- a/libstb/secvar/storage/fakenv_ops.c
+++ b/libstb/secvar/storage/fakenv_ops.c
@@ -152,10 +152,12 @@ static int fakenv_readpublic(TPMI_RH_NV_INDEX index, TPMS_NV_PUBLIC *nv_public,
 
 	switch (index) {
 	case SECBOOT_TPMNV_VARS_INDEX:
-		memcpy(&nv_name->t.name, tpmnv_vars_name, sizeof(TPM2B_NAME));
+		memcpy(&nv_name->t.name, tpmnv_vars_name,
+		       MIN(sizeof(nv_name->t.name), sizeof(tpmnv_vars_name)));
 		break;
 	case SECBOOT_TPMNV_CONTROL_INDEX:
-		memcpy(&nv_name->t.name, tpmnv_control_name, sizeof(TPM2B_NAME));
+		memcpy(&nv_name->t.name, tpmnv_control_name,
+		       MIN(sizeof(nv_name->t.name), sizeof(tpmnv_control_name)));
 		break;
 	default:
 		return OPAL_INTERNAL_ERROR;
--
2.31.1

_______________________________________________
Skiboot mailing list
Skiboot at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/skiboot