| Message ID | 20220525202922.489421-1-arbab@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libstb: Fix memcpy overread in fakenv_readpublic() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-Docker_builds_and_checks | fail | check_build (ubuntu-rolling) failed at step Create Docker image. |
On 5/25/22 3:29 PM, Reza Arbab wrote: > 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@linux.ibm.com> Reviewed-by: Eric Richter <erichte@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;
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;
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@linux.ibm.com> --- libstb/secvar/storage/fakenv_ops.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)