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