| Message ID | 20211101220513.835940-5-erichte@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Secvar adjustments and fixes | expand |
Eric Richter <erichte@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@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
On 11/3/21 12:04 AM, Daniel Axtens wrote: > Eric Richter <erichte@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@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 >
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); } 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++) { 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; /* Verify the signature. sha256 is 32 bytes long. */ - rc = verify_signature(auth, hash, 32, avar); + rc = verify_signature(auth, hash, 32, key_authority[i]); /* Break if signature verification is successful */ if (rc == OPAL_SUCCESS) { - prlog(PR_INFO, "Key %s successfully verified by authority %s\n", update->key, key_authority[i]); + prlog(PR_INFO, "Key %s successfully verified by authority %s\n", update->key, key_authority[i]->key); break; } }
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@linux.ibm.com> --- libstb/secvar/backend/edk2-compat-process.c | 32 +++++++++------------ 1 file changed, 14 insertions(+), 18 deletions(-)