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

[Eric Richter]

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 at linux.ibm.com>

Reviewed-by: Eric Richter <erichte 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;