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

[Eric Richter]

On 11/3/21 12:04 AM, Daniel Axtens wrote:
> 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.
> 

Non-obvious quirk I will update with a comment: These variables always exist
in the bank due to .pre_process() generating empty ones if they aren't loaded
from storage.

The old code effectively did this anyway, I just moved the find_secvar() calls
in here to shorten the loop.

>>  
>>  	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.
> 

Ah, yup. Find/replace at its best.

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

I'll probably end up doing this. The variables won't be NULL, but I
won't leave that to chance in case they ever are.

> 
> Kind regards,
> Daniel
>