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

[Eric Richter]

On 5/25/22 3:56 PM, Kenneth Goldman wrote:
> 
> 
> -----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?
> 

The name should only be 34 bytes, as it should be SHA256. If it is not, it will fail elsewhere in the code since we are expecting a very specific hash for the 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
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot 
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot