Hi Ilias,
On Fri, 11 Nov 2022 at 21:33, Ilias Apalodimas
<ilias.apalodi...@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.apalodi...@linaro.org>
Thanks,
Masahisa Kojima
>
