[Skiboot] [PATCH v2 4/6] secvar/edk2: change get_key_authority to return a list of variables instead of their names

[Daniel Axtens]

Eric Richter <erichte at linux.ibm.com> writes:

> The current get_key_authority function returns a list of variable names that
> can authorize updates to a particular variable (e.g. "KEK" can authorize
> updates to "db").
>
> As the names will have to be fetched anyway via calls to find_secvar(), this
> patch changes the behavior to operate on secvars themselves instead of just
> the names. It takes in a secvar reference, and returns a list of secvar
> references that hold the proper authority, thus cleaning up a few
> extra unnecessary temporary variables.
>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
>  libstb/secvar/backend/edk2-compat-process.c | 32 +++++++++------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index d69e066f..cdc95737 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -72,17 +72,17 @@ static char *char_to_wchar(const char *key, const size_t keylen)
>  }
>  
>  /* Returns the authority that can sign the given key update */
> -static void get_key_authority(const char *ret[3], const char *key)
> +static void get_key_authority(const struct secvar *ret[3], const struct secvar *update, struct list_head *bank)
>  {
>  	int i = 0;
>  
> -	if (key_equals(key, "PK")) {
> -		ret[i++] = "PK";
> -	} else if (key_equals(key, "KEK")) {
> -		ret[i++] = "PK";
> -	} else if (key_equals(key, "db") || key_equals(key, "dbx")) {
> -		ret[i++] = "KEK";
> -		ret[i++] = "PK";
> +	if (key_equals(update->key, "PK")) {
> +		ret[i++] = find_secvar("PK", 3, bank);
> +	} else if (key_equals(update->key, "KEK")) {
> +		ret[i++] = find_secvar("PK", 3, bank);
> +	} else if (key_equals(update->key, "db") || key_equals(update->key, "dbx")) {
> +		ret[i++] = find_secvar("KEK", 4, bank);
> +		ret[i++] = find_secvar("PK", 3, bank);
>  	}

If you have PK but not KEK, and you try to update db, ret will be:
{NULL, <secvar: PK>, NULL}. This will cause the for loop further down in
process_update to fall apart because it checks for NULLs.

>  
>  	ret[i] = NULL;
> @@ -701,9 +701,8 @@ int process_update(const struct secvar *update, char **newesl,
>  	struct efi_variable_authentication_2 *auth = NULL;
>  	void *auth_buffer = NULL;
>  	int auth_buffer_size = 0;
> -	const char *key_authority[3];
> +	const struct secvar *key_authority[3];
>  	char *hash = NULL;
> -	struct secvar *avar = NULL;
>  	int rc = 0;
>  	int i;
>  
> @@ -770,7 +769,7 @@ int process_update(const struct secvar *update, char **newesl,
>  	}
>  
>  	/* Get the authority to verify the signature */
> -	get_key_authority(key_authority, update->key);
> +	get_key_authority(key_authority, update, bank);
>  
>  	/*
>  	 * Try for all the authorities that are allowed to sign.
> @@ -778,19 +777,16 @@ int process_update(const struct secvar *update, char **newesl,
>  	 */
>  	for (i = 0; key_authority[i] != NULL; i++) {

As I mentioned above, this might go wrong if you're updating db(x) and
have PK but no KEK. I think you probably need to iterate over
ARRAY_SIZE(key_authority) and just `continue` if NULL?

>  		prlog(PR_DEBUG, "key is %s\n", update->key);
> -		prlog(PR_DEBUG, "key authority is %s\n", key_authority[i]);
> -		avar = find_secvar(key_authority[i],
> -				    strlen(key_authority[i]) + 1,
> -				    bank);
> -		if (!avar || !avar->data_size)
> +		prlog(PR_DEBUG, "key authority is %s\n", key_authority[i]->key);
> +		if (!key_authority[i] || !key_authority[i]->data_size)
>  			continue;

I thought you were doing an undefined behaviour there: dereferencing
key_authority[i] before you check whether or not it is NULL. However,
key_authority is already not NULL by virtue of the check in the for
loop. So really the test in the `if` can be simplified. It did make
sense before because avar lookup could give you NULL.

If you rejig the loop you'll need to reevaluate this, of course.

Kind regards,
Daniel