[Skiboot] [PATCH v4 1/2] secvar: return error if validate_esl has extra data

[Daniel Axtens]

Hi Nick,

I can't get either of these patches to apply to the upstream master
branch - are there some dependencies I'm missing? From patch 2 it looks
like the patch adding a trimmed KEK test is expected?

Kind regards,
Daniel

> Currently, in `validate_esl_list`, the return code is initialized to zero
> (our success value). While looping though the ESL's in the submitted ESL
> chain, the loop will break if there is not enough data to meet minimum ESL
> requirements. This condition was not setting a return code, meaning that the
> successful return code can pass to the end of the function if there is extra
> data at the end of the ESL. As a consequence, any properly signed update can
> successfully commit any data (as long as it is less than the min size of an
> ESL) to the secvars.
>
> This commit will return an error if the described condition is met. This
> means all data in the appended ESL of an auth file must be accounted for. No
> extra bytes can be added to the end since, on success, this data will become
> the updated secvar.
>
> Additionally, a test case has been added to ensure that this commit
> addresses the issue correctly.
>
> Signed-off-by: Nick Child <nick.child at ibm.com>
> Reviewed-by: Nayna Jain <nayna at linux.ibm.com>
> Tested-by: Nayna Jain <nayna at linux.ibm.com>
> ---
>
> Previous version at https://lists.ozlabs.org/pipermail/skiboot/2021-July/017643.html
>
> ---
>
>  libstb/secvar/backend/edk2-compat-process.c  |  5 ++++-
>  libstb/secvar/test/secvar-test-edk2-compat.c | 15 +++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index 5204fdd5..983a3313 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -267,8 +267,11 @@ int validate_esl_list(const char *key, const char *esl, const size_t size)
>  
>  	while (eslvarsize > 0) {
>  		prlog(PR_DEBUG, "esl var size is %d offset is %lu\n", eslvarsize, size - eslvarsize);
> -		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
> +		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST)) {
> +			prlog(PR_ERR, "ESL with size %d is too small\n", eslvarsize);
> +			rc = OPAL_PARAMETER;
>  			break;
> +		}
>  
>  		/* Check Supported ESL Type */
>  		list = get_esl_signature_list(esl, eslvarsize);
> diff --git a/libstb/secvar/test/secvar-test-edk2-compat.c b/libstb/secvar/test/secvar-test-edk2-compat.c
> index 3cdc6ef2..1edce112 100644
> --- a/libstb/secvar/test/secvar-test-edk2-compat.c
> +++ b/libstb/secvar/test/secvar-test-edk2-compat.c
> @@ -165,6 +165,21 @@ int run_test()
>  	ASSERT(5 == list_length(&variable_bank));
>  	ASSERT(setup_mode);
>  
> +	/* Add PK with bad ESL. should fail since data is not big enough to be ESL*/
> +	printf("Add PK with invalid appended ESL");
> +	/* 1014 is length of appended ESL Header and its data */
> +	tmp = new_secvar("PK", 3, PK_auth, PK_auth_len - 1014 + sizeof(EFI_SIGNATURE_LIST) - 1, 0);
> +	ASSERT(0 == edk2_compat_validate(tmp));
> +	list_add_tail(&update_bank, &tmp->link);
> +	ASSERT(1 == list_length(&update_bank));
> +	rc = edk2_compat_process(&variable_bank, &update_bank);
> +	ASSERT(5 == list_length(&variable_bank));
> +	ASSERT(0 == list_length(&update_bank));
> +	rc = edk2_compat_post_process(&variable_bank, &update_bank);
> +	ASSERT(5 == list_length(&variable_bank));
> +	ASSERT(setup_mode);
> +
> +
>  	/* Add PK to update and .process(). */
>  	printf("Add PK");
>  	tmp = new_secvar("PK", 3, PK_auth, PK_auth_len, 0);
> -- 
> 2.25.1