[Skiboot] [PATCH v3 3/3] secvar: Fix signedness issues when validating ESL

[Nayna]

On 7/1/21 3:12 PM, Nick Child wrote:
> In both `validate_esl_list` and `verify_signature`, the submitted ESL
> chain and the current secvar ESL chains (respectively) were having
> their sizes typecast from their actual type.
>
> Mainly, the length of the buffer, referred to as `eslvarsize` in both
> functions, was cast from a type `size_t` to an integer. This was likely
> done to ensure the loop `while (eslvarsize > 0)` terminates. A better
> solution would be to keep `eslvarsize` as unsigned and ensure that the
> current ESL is never bigger than the remaining buffer length. As an
> effect, `eslvarsize` could never be decremented past 0. This makes sure
> that all bytes are accounted for.
>
> Currently, the length of a single ESL, referred to as `eslsize` in both
> functions, is an integer so it would be difficult to compare the two
> due to more signedess issues. Rather than cast a signed variable to
> unsigned, it would be better to treat `eslsize` as it actually is
> stored in the ESL struct: `uint32_t`. This requires a redesign of the
> function `get_esl_signature_list_size`.
>
> For these reasons, this commit merges the function
> `get_esl_signature_list_size` with `get_esl_signature_list`.
> `get_esl_signature_list` now returns an error code or success number.
> Its parameters have also changed to return the `uint32_t` size of the
> ESL as well as a pointer to the ESL struct if the user gives a pointer
> to write to. The check to ensure that the ESL structures specified size
> is never larger than the data buffer has been added as a minimal check
> that the ESL being returned is valid.
>
> Signed-off-by: Nick Child <nick.child at ibm.com>
> ---
>   libstb/secvar/backend/edk2-compat-process.c | 93 ++++++++++-----------
>   1 file changed, 45 insertions(+), 48 deletions(-)
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index bb1205a5..bcd43c5d 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -85,30 +85,37 @@ static void get_key_authority(const char *ret[3], const char *key)
>   	ret[i] = NULL;
>   }
>   
> -static EFI_SIGNATURE_LIST* get_esl_signature_list(const char *buf, size_t buflen)
> +/*
> + * Returns success or error number if buffer is not a valid ESL
> + * If success, `list` will point to ESL struct which is a `buf` internal pointer
> + * If success, `list_size` will be assigned the size of the ESL header + its data
> + * `list` can be NULL if only size of ESL is requested
> + */
> +static int get_esl_signature_list(const char *buf, size_t buflen, EFI_SIGNATURE_LIST** list, uint32_t *list_size)
>   {

get_auth_descriptor2() / get_esl_cert() returns either size if valid or 
negative error. Can we follow similar format ?

I think Nick mentioned that problem is in verify_signature() it once 
calls only to get the size.

We can probably simplify this by passing list to get_esl_cert() which is 
for now anyway calling get_esl_signature_list() internally.

I think this will also simplify validate_esl_list() and verify_signature().

Let me know if I missed something. And sorry for delay in getting back 
to this.

Thanks & Regards,

      - Nayna