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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Heinrich,

On Fri, Oct 01, 2021 at 06:42:14PM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 9/12/21 21:23, Ilias Apalodimas wrote:
> > Hi Heinrich
> > 
> > [...]
> > > > > -    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 {
> > > > > +    vendor = efi_auth_var_get_guid(name);
> > > > > +    if (!vendor) {
> > > > >               EFI_PRINT("unknown signature database, %ls\n", name);
> > > > >               return NULL;
> > > > >       }
> > > > 
> > > > efi_auth_var_get_guid() will return &efi_global_variable_guid if the
> > > > GUID for the variable name isn't found.
> > > 
> > > Hello Ilias, that is on purpose. In nevedit_efi we need a default GUID.
> > > I want to reuse the same function there in future.
> > > 
> > > Best regards
> > 
> > Then I guess the check can go away ?
> 
> Yes
> 
> > 
> > > 
> > > Heinrich
> > > 
> > > > 
> > > > > 
> > > > > -    /* retrieve variable data */
> > > > > -    db_size = 0;
> > > > > -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
> > > > > -    if (ret == EFI_NOT_FOUND) {
> > > > > -            EFI_PRINT("variable, %ls, not found\n", name);
> > > > > -            sigstore = calloc(sizeof(*sigstore), 1);
> > > > > -            return sigstore;
> > > > > -    } else if (ret != EFI_BUFFER_TOO_SMALL) {
> > > > > -            EFI_PRINT("Getting variable, %ls, failed\n", name);
> > > > > -            return NULL;
> > > > > -    }
> > > > > -
> > > > > -    db = malloc(db_size);
> > > > > +    db = efi_get_var(name, vendor, &db_size);
> > > > >       if (!db) {
> > > > > -            EFI_PRINT("Out of memory\n");
> > > > > -            return NULL;
> > > > > -    }
> > > > > -
> > > > > -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, db));
> > > > > -    if (ret != EFI_SUCCESS) {
> > > > > -            EFI_PRINT("Getting variable, %ls, failed\n", name);
> > > > > -            free(db);
> > > > > -            return NULL;
> > > > > +            EFI_PRINT("variable, %ls, not found\n", name);
> > > > > +            return calloc(sizeof(struct efi_signature_store), 1);
> > 
> > Why? From the patch alone it's not clear why you want to allocate
> > memory here instead of returning NULL.
> 
> This is existing code. See the same lines deleted above.

If I read the code correctly,  we are trying to be smart about the buffer
outcome.  Check for example efi_image_unsigned_authenticate().  By returning 
an empty buffer the 'dbx' check will succeed but the 'db' check a few lines 
after will fail.

But this is pointless imho... Why don't we just have 
efi_status_t efi_signature_store efi_sigstore_parse_sigdb(u16 *name, struct
														  efi_signature_store *store)

We can the control the EFI return value in efi_sigstore_parse_sigdb() and
any callers would just have to look at the result, instead of getting a
memory that contains empty data and try to reason about it.
IOW you can check for EFI_NOT_FOUND in both cases on the caller function.
If you are working with 'dbx' then that's fine and you can continue.  If
you are working with 'db' you need to fail the authentication.  This imho
is much more readable.

Regards
/Ilias