Re: [PATCH v8 5/5] eficonfig: add "Show Signature Database" menu entry

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

[...]

...

+/**

• • prepare_signature_list_menu() - create the signature list menu entry
• • @efimenu: pointer to the efimenu structure
• • @varname: pointer to the variable name
• • @db: pointer to the variable raw data
• • @db_size: variable data size
• • @func: callback of each entry
• • Return: status code
• */

+static efi_status_t prepare_signature_list_menu(struct efimenu *efi_menu, void *varname,

•                                         void *db, efi_uintn_t db_size,
  

•                                         eficonfig_entry_func func)
  

+{

• u32 num = 0;
  

• efi_uintn_t size;
  

• struct eficonfig_sig_data *sg;
  

• struct efi_signature_list *esl;
  

• struct efi_signature_data *esd;
  

• efi_status_t ret = EFI_SUCCESS;
  

• INIT_LIST_HEAD(&efi_menu->list);
  

• esl = db;
  

• size = db_size;
  

• while (size > 0) {
  

•         u32 remain;
  

•         esd = (struct efi_signature_data *)((u8 *)esl +
  

•                                             (sizeof(struct efi_signature_list) +
  

•                                             esl->signature_header_size));
  

•         remain = esl->signature_list_size - sizeof(struct efi_signature_list) -
  

•                  esl->signature_header_size;
  

•         for (; remain > 0; remain -= esl->signature_size) {
  

•                 char buf[40];
  

•                 char *title;
  

•                 if (num >= EFICONFIG_ENTRY_NUM_MAX - 1) {
  

•                         ret = EFI_OUT_OF_RESOURCES;
  

•                         goto out;
  

•                 }
  

•                 sg = calloc(1, sizeof(struct eficonfig_sig_data));
  

•                 if (!sg) {
  

•                         ret = EFI_OUT_OF_RESOURCES;
  

•                         goto err;
  

•                 }
  

•                 snprintf(buf, sizeof(buf), "%pUL", &esd->signature_owner);
  

•                 title = calloc(1, (strlen(buf) + 1));
  

•                 if (!title) {
  

•                         free(sg);
  

•                         ret = EFI_OUT_OF_RESOURCES;
  

•                         goto err;
  

•                 }
  

•                 strlcpy(title, buf, strlen(buf) + 1);
  

•                 sg->esl = esl;
  

•                 sg->esd = esd;
  

•                 sg->varname = varname;
  

•                 ret = eficonfig_append_menu_entry(efi_menu, title, func, sg);
  

•                 if (ret != EFI_SUCCESS) {
  

•                         free(sg);
  

•                         free(title);
  

•                         goto err;
  

•                 }
  

title and sg are allocated on a for loop. I assume that those will eventually be freed by eficonfig_destroy(). But shouldn't we call that on failures as well instead of just freeing the last instance?

Title is freed by eficonfig_destroy(), and entry->data is freed by the caller because it depends on the caller's decision whether entry->data must be freed or not. eficonfig_destroy() also frees "struct efimenu" pointer and it is allocated by the caller. As a design same as other eficonfig menu, eficonfig_destroy() is called by caller, and entry->data is freed by caller. The menu creation function(e.g. prepare_signature_list_menu()) frees the allocated variable when eficonfig_append_menu_entry() fails because these allocated variables are not in the efi_menu->list.

Thanks, Masahisa Kojima

...

•                 esd = (struct efi_signature_data *)((u8 *)esd + esl->signature_size);
  

•                 num++;
  

•         }
  

•         size -= esl->signature_list_size;
  

•         esl = (struct efi_signature_list *)((u8 *)esl + esl->signature_list_size);
  

• }
  

+out:

• ret = eficonfig_append_quit_entry(efi_menu);
  

+err:

• return ret;
  

+}

+/**

[...]

Regards /Ilias