On Tue, 12 Jul 2022 at 17:02, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > On 7/12/22 09:13, Masahisa Kojima wrote: > > Hi Heinrich, Takahiro, > > > > On Sun, 10 Jul 2022 at 19:10, Heinrich Schuchardt <xypron.g...@gmx.de> > > wrote: > >> > >> On 6/19/22 07:20, Masahisa Kojima wrote: > >>> This commit add the menu-driven interface to delete the > >>> signature database entry. > >>> EFI Signature Lists can contain the multiple signature > >>> entries, this menu can delete the indivisual entry. > >>> > >>> If the PK is enrolled and UEFI Secure Boot is in User Mode, > >> > >> Why don't you mention Deployed Mode? > > > > Yes, I will mention DeployedMode. > > > >> > >>> user can not delete the existing signature lists since the > >>> signature lists must be signed by KEK or PK but signing > >>> information is not stored in the signature database. > >> > >> Updating PK with an empty value must be possible if if the new value is > >> signed with the old PK. > > > > Yes, I will add this description. > > > >> > >>> > >>> Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org> > >>> --- > >>> cmd/eficonfig_sbkey.c | 218 +++++++++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 217 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/cmd/eficonfig_sbkey.c b/cmd/eficonfig_sbkey.c > >>> index 02ab8f8218..142bb4cef5 100644 > >>> --- a/cmd/eficonfig_sbkey.c > >>> +++ b/cmd/eficonfig_sbkey.c > >>> @@ -54,6 +54,29 @@ static const struct eficonfig_sigtype_to_str > >>> sigtype_to_str[] = { > >>> /* {EFI_CERT_SHA512_GUID, "SHA512", > >>> SIG_TYPE_HASH}, */ > >>> }; > >>> > >> > >> Please, add documentation to all functions. > > > > OK. > > > >> > >>> +static int eficonfig_console_yes_no(void) > >>> +{ > >>> + int esc = 0; > >>> + enum bootmenu_key key = KEY_NONE; > >>> + > >>> + while (1) { > >>> + bootmenu_loop(NULL, &key, &esc); > >>> + > >>> + switch (key) { > >>> + case KEY_SELECT: > >>> + return 1; > >>> + case KEY_QUIT: > >>> + return 0; > >>> + default: > >>> + break; > >>> + } > >>> + } > >>> + > >>> + /* never happens */ > >>> + debug("eficonfig: this should not happen"); > >>> + return 0; > >> > >> If this code is unreachable, remove it. > > > > OK. > > > >> > >>> +} > >>> + > >>> static void eficonfig_console_wait_enter(void) > >>> { > >>> int esc = 0; > >>> @@ -72,7 +95,19 @@ static void eficonfig_console_wait_enter(void) > >>> > >>> /* never happens */ > >>> debug("eficonfig: this should not happen"); > >>> - return; > >> > >> Please remove unreachable code. > > > > OK. > > > >> > >>> +} > >>> + > >>> +static bool is_setupmode(void) > >>> +{ > >>> + efi_status_t ret; > >>> + u8 setup_mode; > >>> + efi_uintn_t size; > >>> + > >>> + size = sizeof(setup_mode); > >>> + ret = efi_get_variable_int(u"SetupMode", &efi_global_variable_guid, > >>> + NULL, &size, &setup_mode, NULL); > >>> + > >>> + return setup_mode == 1; > >>> } > >>> > >>> static bool is_secureboot_enabled(void) > >>> @@ -254,6 +289,103 @@ static efi_status_t > >>> eficonfig_process_sigdata_show(void *data) > >>> return EFI_SUCCESS; > >>> } > >>> > >>> +static efi_status_t eficonfig_process_sigdata_delete(void *data) > >>> +{ > >>> + int yes_no; > >>> + struct eficonfig_sig_data *sg = data; > >>> + > >>> + display_sigdata_header(sg, "Delete"); > >>> + display_sigdata_info(sg); > >>> + > >>> + printf("\n\n Press ENTER to delete, ESC/CTRL+C to quit"); > >>> + yes_no = eficonfig_console_yes_no(); > >>> + if (!yes_no) > >>> + return EFI_NOT_READY; > >>> + > >>> + return EFI_SUCCESS; > >>> +} > >>> + > >>> +static void delete_selected_signature_data(void *db, efi_uintn_t > >>> *db_size, > >>> + struct eficonfig_sig_data > >>> *target, > >>> + struct list_head *siglist_list) > >>> +{ > >>> + u8 *dest, *start; > >>> + struct list_head *pos, *n; > >>> + u32 remain; > >>> + u32 size = *db_size; > >>> + u8 *end = (u8 *)db + size; > >>> + struct eficonfig_sig_data *sg; > >>> + > >>> + list_for_each_safe(pos, n, siglist_list) { > >>> + sg = list_entry(pos, struct eficonfig_sig_data, list); > >>> + if (sg->esl == target->esl && sg->esd == target->esd) { > >>> + remain = sg->esl->signature_list_size - > >>> + (sizeof(struct efi_signature_list) - > >>> + sg->esl->signature_header_size) - > >>> + sg->esl->signature_size; > >>> + if (remain > 0) { > >>> + /* only delete the single signature data */ > >>> + sg->esl->signature_list_size -= > >>> sg->esl->signature_size; > >>> + size -= sg->esl->signature_size; > >>> + dest = (u8 *)sg->esd; > >>> + start = (u8 *)sg->esd + > >>> sg->esl->signature_size; > >>> + } else { > >>> + /* delete entire signature list */ > >>> + dest = (u8 *)sg->esl; > >>> + start = (u8 *)sg->esl + > >>> sg->esl->signature_list_size; > >>> + size -= sg->esl->signature_list_size; > >>> + } > >>> + memmove(dest, start, (end - start)); > >>> + } > >>> + } > >>> + > >>> + *db_size = size; > >>> +} > >>> + > >>> +static efi_status_t create_time_based_payload(void *db, void **new_db, > >>> efi_uintn_t *size) > >>> +{ > >>> + efi_status_t ret; > >>> + struct efi_time time; > >>> + efi_uintn_t total_size; > >>> + struct efi_variable_authentication_2 *auth; > >>> + > >>> + *new_db = NULL; > >>> + > >>> + /* > >>> + * SetVariable() call with > >>> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS > >>> + * attribute requires EFI_VARIABLE_AUTHENTICATED_2 descriptor, > >>> prepare it > >>> + * without certificate data in it. > >>> + */ > >>> + total_size = sizeof(struct efi_variable_authentication_2) + *size; > >>> + > >>> + auth = calloc(1, total_size); > >>> + if (!auth) > >>> + return EFI_OUT_OF_RESOURCES; > >>> + > >>> + ret = EFI_CALL((*efi_runtime_services.get_time)(&time, NULL)); > >>> + if (ret != EFI_SUCCESS) { > >>> + free(auth); > >>> + return EFI_OUT_OF_RESOURCES; > >>> + } > >>> + time.pad1 = 0; > >>> + time.nanosecond = 0; > >>> + time.timezone = 0; > >>> + time.daylight = 0; > >>> + time.pad2 = 0; > >>> + memcpy(&auth->time_stamp, &time, sizeof(time)); > >>> + auth->auth_info.hdr.dwLength = sizeof(struct > >>> win_certificate_uefi_guid); > >>> + auth->auth_info.hdr.wRevision = 0x0200; > >>> + auth->auth_info.hdr.wCertificateType = WIN_CERT_TYPE_EFI_GUID; > >>> + guidcpy(&auth->auth_info.cert_type, &efi_guid_cert_type_pkcs7); > >>> + if (db) > >>> + memcpy((u8 *)auth + sizeof(struct > >>> efi_variable_authentication_2), db, *size); > >>> + > >>> + *new_db = auth; > >>> + *size = total_size; > >>> + > >>> + return EFI_SUCCESS; > >>> +} > >>> + > >>> static efi_status_t prepare_signature_db_list(struct eficonfig_item > >>> **output, void *varname, > >>> void *db, efi_uintn_t > >>> db_size, > >>> eficonfig_entry_func func, > >>> @@ -378,6 +510,68 @@ out: > >>> return ret; > >>> } > >>> > >>> +static efi_status_t process_delete_key(void *varname) > >>> +{ > >>> + u32 attr, i, count = 0; > >>> + efi_status_t ret; > >>> + struct eficonfig_item *menu_item = NULL, *iter; > >>> + void *db = NULL, *new_db = NULL; > >>> + efi_uintn_t db_size; > >>> + struct list_head siglist_list; > >>> + struct eficonfig_sig_data *selected; > >>> + > >>> + db = efi_get_var(varname, efi_auth_var_get_guid(varname), &db_size); > >>> + if (!db) { > >>> + eficonfig_print_msg("There is no entry in the signature > >>> database."); > >> > >> Please, use the terms of the UEFI specification. > >> %s/signature database/signature store/ > > > > As Takahiro already mentioned, UEFI specifications use the following term. > > - signature database > > Yes. > > The term signature database is used in references to db, dbx, dbr, dbt. > > > - signature store > > Signature store is only used in the description of dbDefault, > dbrDefault, dbtDefault, dbxDefault. > > > - signature list > > Signature list refers to one of the esl files concatenated in a > signature store.
Thank you for adding the information. The KEK variable description in UEFI specification is: "The Key Exchange Key Signature Database." So I would like to use "signature database" in this patch series. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > > > The UEFI specification has section "32.4.1 Signature Database" and > > I think "signature database" is most common in the spec. > > > >> > >>> + return EFI_NOT_FOUND; > >>> + } > >>> + > >>> + ret = prepare_signature_db_list(&menu_item, varname, db, db_size, > >>> + eficonfig_process_sigdata_delete, > >>> &selected, > >>> + &siglist_list, &count); > >>> + if (ret != EFI_SUCCESS) > >>> + goto out; > >>> + > >>> + ret = eficonfig_process_common(menu_item, count, " ** Delete Key > >>> **"); > >>> + > >>> + if (ret == EFI_SUCCESS) { > >>> + delete_selected_signature_data(db, &db_size, selected, > >>> &siglist_list); > >>> + > >>> + ret = create_time_based_payload(db, &new_db, &db_size); > >>> + if (ret != EFI_SUCCESS) > >>> + goto out; > >>> + > >>> + attr = EFI_VARIABLE_NON_VOLATILE | > >>> + EFI_VARIABLE_BOOTSERVICE_ACCESS | > >>> + EFI_VARIABLE_RUNTIME_ACCESS | > >>> + EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS; > >>> + ret = efi_set_variable_int((u16 *)varname, > >>> efi_auth_var_get_guid((u16 *)varname), > >>> + attr, db_size, new_db, false); > >>> + if (ret != EFI_SUCCESS) { > >>> + eficonfig_print_msg("ERROR! Fail to delete > >>> signature database"); > >> > >> %s/Fail/Failed/ > > > > OK. > > > >> > >> Please, use the terms of the UEFI specification. > >> %s/signature database/signature store/ > > > > Same as above. > > > >> > >>> + goto out; > >>> + } > >>> + } > >>> + > >>> +out: > >>> + if (menu_item) { > >>> + iter = menu_item; > >>> + for (i = 0; i < count - 1; iter++, i++) { > >>> + free(iter->title); > >>> + free(iter->data); > >>> + } > >>> + } > >>> + > >>> + free(menu_item); > >>> + free(db); > >>> + free(new_db); > >>> + > >>> + /* to stay the parent menu */ > >>> + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret; > >>> + > >>> + return ret; > >>> +} > >>> + > >>> static efi_status_t eficonfig_process_show_signature_db(void *data) > >>> { > >>> efi_status_t ret; > >>> @@ -394,6 +588,27 @@ static efi_status_t > >>> eficonfig_process_show_signature_db(void *data) > >>> return ret; > >>> } > >>> > >>> +static efi_status_t eficonfig_process_delete_key(void *data) > >>> +{ > >>> + efi_status_t ret; > >>> + > >>> + if (!is_setupmode()) { > >>> + eficonfig_print_msg("Not in the SetupMode, can not > >>> delete."); > >> > >> Both in Setup Mode and in Audit Mode you should be able to edit keys. > > > > Yes, I will update the code and message. > > > >> > >>> + return EFI_SUCCESS; > >>> + } > >>> + > >>> + while (1) { > >>> + ret = process_delete_key(data); > >>> + if (ret != EFI_SUCCESS) > >>> + break; > >>> + } > >>> + > >>> + /* to stay the parent menu */ > >>> + ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret; > >>> + > >>> + return ret; > >>> +} > >>> + > >>> static struct eficonfig_item key_config_pk_menu_items[] = { > >>> {"Enroll New Key", eficonfig_process_enroll_key}, > >>> {"Show Signature Database", eficonfig_process_show_signature_db}, > >> > >> %s/Signature Database/signature store/ > > > > Same as above. > > > >> > >>> @@ -403,6 +618,7 @@ static struct eficonfig_item > >>> key_config_pk_menu_items[] = { > >>> static struct eficonfig_item key_config_menu_items[] = { > >>> {"Enroll New Key", eficonfig_process_enroll_key}, > >>> {"Show Signature Database", eficonfig_process_show_signature_db}, > >> > >> %s/Signature Database/signature store/ > > > > Same as above. > > > > Thank you for your review. > > > > Regards, > > Masahias Kojima > > > >> > >> Best regards > >> > >> Heinrich > >> > >>> + {"Delete Key", eficonfig_process_delete_key}, > >>> {"Quit", eficonfig_process_quit}, > >>> }; > >>> > >> >