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

Daniel Axtens dja at axtens.net
Wed Nov 3 16:04:06 AEDT 2021


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


More information about the Skiboot mailing list