Re: [PATCH v8 4/5] eficonfig: add UEFI Secure Boot Key enrollment interface

Hi Ilias,

On Fri, 11 Nov 2022 at 21:33, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hello Kojima-san!

[...]

...

• file_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
  

• if (!file_info.current_path) {
  

•         ret = EFI_OUT_OF_RESOURCES;
  

•         goto out;
  

• }
  

• ret = eficonfig_process_select_file(&file_info);
  

• if (ret != EFI_SUCCESS)
  

•         goto out;
  

• ret = efi_open_volume_int(file_info.current_volume, &root);
  

• if (ret != EFI_SUCCESS)
  

•         goto out;
  

• ret = efi_file_open_int(root, &f, file_info.current_path, EFI_FILE_MODE_READ, 0);
  

• if (ret != EFI_SUCCESS)
  

•         goto out;
  

• ret = efi_file_size(f, &size);
  

• if (ret != EFI_SUCCESS)
  

•         goto out;
  

• buf = calloc(1, size);
  

• if (!buf) {
  

•         ret = EFI_OUT_OF_RESOURCES;
  

•         goto out;
  

• }
  

• if (size == 0) {
  

•         eficonfig_print_msg("ERROR! File is empty.");
  

•         goto out;
  

• }
  

This needs to move before the allocation and if (!size)

OK.

...

• ret = efi_file_read_int(f, &size, buf);
  

• if (ret != EFI_SUCCESS) {
  

•         eficonfig_print_msg("ERROR! Failed to read file.");
  

•         goto out;
  

• }
  

• if (!file_have_auth_header(buf, size)) {
  

•         eficonfig_print_msg("ERROR! Invalid file format. Only .auth variables is allowed.");
  

•         ret = EFI_INVALID_PARAMETER;
  

•         goto out;
  

• }
  

• attr = EFI_VARIABLE_NON_VOLATILE |
  

•        EFI_VARIABLE_BOOTSERVICE_ACCESS |
  

•        EFI_VARIABLE_RUNTIME_ACCESS |
  

•        EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
  

• /* PK can enroll only one certificate */
  

• if (u16_strcmp(data, u"PK")) {
  

•         efi_uintn_t db_size = 0;
  

•         /* check the variable exists. If exists, add APPEND_WRITE attribute */
  

•         ret = efi_get_variable_int(data, efi_auth_var_get_guid(data), NULL,
  

•                                    &db_size, NULL,  NULL);
  

•         if (ret == EFI_BUFFER_TOO_SMALL)
  

•                 attr |= EFI_VARIABLE_APPEND_WRITE;
  

• }
  

• ret = efi_set_variable_int((u16 *)data, efi_auth_var_get_guid((u16 *)data),
  

•                            attr, size, buf, false);
  

• if (ret != EFI_SUCCESS)
  

•         eficonfig_print_msg("ERROR! Failed to update signature database");
  

+out:

• free(file_info.current_path);
  

• free(buf);
  

• if (f)
  

•         efi_file_close_int(f);
  

• /* return to the parent menu */
  

• ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
  

I assume we are changing the return message here because the upper layers can't handle EFI_ABORTED?

This return value change is tricky. In eficonfig menu handling, EFI_ABORTED means "exit from the current menu", it usually happens by selecting "Quit" entry or pressing ESC key. eficonfig menu implements a multi-layer menu, if the child menu returns with EFI_ABORTED, the parent menu also exit with EFI_ABORTED. This is why EFI_ABORTED is changed to EFI_NOT_READY. EFI_NOT_READY means that we stay in the current menu.

...

• return ret;
  

+}

+static struct eficonfig_item key_config_menu_items[] = {

• {"Enroll New Key", eficonfig_process_enroll_key},
  

• {"Quit", eficonfig_process_quit},
  

+};

with the size changes Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Thanks, Masahisa Kojima