[Skiboot] [PATCH] libstb: Fix memcpy overread in fakenv_readpublic()

Reza Arbab arbab at linux.ibm.com
Thu May 26 06:29:22 AEST 2022


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

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



More information about the Skiboot mailing list