[PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Ilias Apalodimas]

On Wed, 6 Oct 2021 at 10:21, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
>
>
> On 10/6/21 08:29, Ilias Apalodimas wrote:
> > On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
> >> Simplify efi_sigstore_parse_sigdb() by using existing functions.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> >> ---
> >> v3:
> >>      Keep error handling in efi_sigstore_parse_sigdb()
> >> v2:
> >>      remove a superfluous check
> >> ---
> >>   lib/efi_loader/efi_signature.c | 11 ++---------
> >>   1 file changed, 2 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> >> index bdd09881fc..97f6dfacd9 100644
> >> --- a/lib/efi_loader/efi_signature.c
> >> +++ b/lib/efi_loader/efi_signature.c
> >> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
> >>      efi_uintn_t db_size;
> >>      efi_status_t ret;
> >>
> >> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
> >> -            vendor = &efi_global_variable_guid;
> >> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
> >> -            vendor = &efi_guid_image_security_database;
> >> -    } else {
> >> -            EFI_PRINT("unknown signature database, %ls\n", name);
> >> -            return NULL;
> >> -    }
> >> +    vendor = efi_auth_var_get_guid(name);
> >
> > Should we return NULL if we get back the default guid?
>
> efi_sigstore_parse_sigdb() is only called with fixed values of 'name'.
> So how should this occur?

Bugs that slip through maybe?  I generally prefer being more pedantic
with security related code

Regards
/Ilias
>
> Best regards
>
> Heinrich
>
> >
> >>
> >>      /* retrieve variable data */
> >>      db_size = 0;
> >> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
> >> +    ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
> >>      if (ret == EFI_NOT_FOUND) {
> >>              EFI_PRINT("variable, %ls, not found\n", name);
> >>              sigstore = calloc(sizeof(*sigstore), 1);
> >> --
> >> 2.32.0
> >>
> >
> > Regards
> > /Ilias
> >