[PATCH] efi_loader: capsule: detect possible ESP device path

Weizhao Ouyang o451686892 at gmail.com
Wed May 8 15:00:46 CEST 2024


Hi Dan,

Thanks for the suggestion, I'll fix them in the next patch.

BR,
Weizhao

On Wed, May 8, 2024 at 8:04 PM Dan Carpenter <dan.carpenter at linaro.org> wrote:
>
> On Wed, May 08, 2024 at 07:24:01PM +0800, Weizhao Ouyang wrote:
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index de0d49ebebda..919e3cba071b 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -922,6 +922,105 @@ static bool device_is_present_and_system_part(struct efi_device_path *dp)
> >       return true;
> >  }
> >
> > +/**
> > + * get_esp_from_boot_option_file_path - get the expand device path
> > + *
> > + * Get a possible efi system partition by expanding a boot option
> > + * file path.
> > + *
> > + * @boot_dev The device path pointing to a boot option
> > + * Return:   The full ESP device path or NULL if fail
> > + */
> > +static struct efi_device_path *get_esp_from_boot_option_file_path(struct efi_device_path *boot_dev)
> > +{
> > +     efi_status_t ret = EFI_SUCCESS;
> > +     efi_handle_t handle;
> > +     struct efi_device_path *dp = boot_dev;
> > +     struct efi_device_path *full_path = NULL;
> > +
> > +     ret = EFI_CALL(efi_locate_device_path(&efi_simple_file_system_protocol_guid,
> > +                                           &dp,
> > +                                           &handle));
> > +     if (ret != EFI_SUCCESS)
> > +             ret = EFI_CALL(efi_locate_device_path(&efi_block_io_guid, &dp, &handle));
> > +
> > +     /* Expand Media Device Path */
> > +     if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) {
>
> Flip this around.  Always do failure handling.  Never do success
> handling.  full_path is always NULL.  Just return that.  Delete
> the variable.
>
>         if (ret != EFI_SUCCESS ||
>             !EFI_DP_TYPE(dp, END, END))
>                 return NULL;
>
>
> > +             struct efi_device_path *temp_dp;
> > +             struct efi_block_io *block_io;
> > +             void *buffer;
> > +             efi_handle_t *simple_file_system_handle;
> > +             efi_uintn_t number_handles, index;
> > +             u32 size;
> > +             u32 temp_size;
> > +
> > +             temp_dp = boot_dev;
> > +             ret = EFI_CALL(efi_locate_device_path(&efi_simple_file_system_protocol_guid,
> > +                                                   &temp_dp,
> > +                                                   &handle));
> > +             /*
> > +              * For device path pointing to simple file system, it only expands to one
> > +              * full path
> > +              */
> > +             if (ret == EFI_SUCCESS && EFI_DP_TYPE(temp_dp, END, END)) {
>
> "Always" was hyperbole.  This success handling is fine.
>
> > +                     if (device_is_present_and_system_part(temp_dp))
> > +                             return temp_dp;
> > +             }
> > +
> > +             /*
> > +              * For device path only pointing to the removable device handle, try to
> > +              * search all its children handles
> > +              */
> > +             ret = EFI_CALL(efi_locate_device_path(&efi_block_io_guid, &temp_dp, &handle));
>
> ret is not used.  Delete.
>
> > +             EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));
> > +
> > +             /* Align with edk2, issue a dummy read to the device to check the device change */
> > +             ret = EFI_CALL(efi_handle_protocol(handle, &efi_block_io_guid, (void **)&block_io));
> > +             if (ret == EFI_SUCCESS) {
>
>         if (ret != EFI_SUCCESS)
>                 return NULL;
>
> > +                     buffer = memalign(block_io->media->io_align, block_io->media->block_size);
> > +                     if (buffer) {
>
>         if (!buffer)
>                 return NULL;
>
>
> > +                             ret = EFI_CALL(block_io->read_blocks(block_io,
> > +                                                                  block_io->media->media_id,
> > +                                                                  0,
> > +                                                                  block_io->media->block_size,
> > +                                                                  buffer));
>
> Delete the unused "ret = " assignment.
>
> > +                             free(buffer);
> > +                     } else {
> > +                             return full_path;
> > +                     }
> > +             } else {
> > +                     return full_path;
> > +             }
> > +
> > +             /* detect the default boot file from removable media */
> > +             size = efi_dp_size(boot_dev) - sizeof(struct efi_device_path);
> > +             EFI_CALL(efi_locate_handle_buffer(BY_PROTOCOL,
> > +                                               &efi_simple_file_system_protocol_guid,
> > +                                               NULL,
> > +                                               &number_handles,
> > +                                               &simple_file_system_handle));
> > +             for (index = 0; index < number_handles; index++) {
> > +                     EFI_CALL(efi_handle_protocol(simple_file_system_handle[index],
> > +                                                  &efi_guid_device_path,
> > +                                                  (void **)&temp_dp));
> > +
> > +                     log_debug("Search ESP %pD\n", temp_dp);
> > +                     temp_size = efi_dp_size(temp_dp) - sizeof(struct efi_device_path);
> > +                     if (size <= temp_size && !memcmp(temp_dp, boot_dev, size)) {
> > +                             if (device_is_present_and_system_part(temp_dp)) {
>
> You could combine these conditions:
>
>                 if (size <= temp_size &&
>                     memcmp(temp_dp, boot_dev, size) == 0 &&
>                     device_is_present_and_system_part(temp_dp)) {
>                         efi_free_pool(simple_file_system_handle);
>                         return temp_dp;
>                 }
>
>
> > +                                     efi_free_pool(simple_file_system_handle);
> > +                                     return temp_dp;
> > +                             }
> > +                     }
> > +             }
> > +
> > +             if (simple_file_system_handle)
> > +                     efi_free_pool(simple_file_system_handle);
> > +     }
> > +
> > +     return full_path;
> > +}
> > +
> >  /**
> >   * find_boot_device - identify the boot device
> >   *
> > @@ -938,6 +1037,7 @@ static efi_status_t find_boot_device(void)
> >       int i, num;
> >       struct efi_simple_file_system_protocol *volume;
> >       struct efi_device_path *boot_dev = NULL;
> > +     struct efi_device_path *full_path = NULL;
>
> No need to initialize this to NULL, it disables static checker warnings
> for uninitialized variables so it can lead to bugs.
>
> >       efi_status_t ret;
> >
> >       /* find active boot device in BootNext */
> > @@ -961,8 +1061,14 @@ static efi_status_t find_boot_device(void)
> >                       if (device_is_present_and_system_part(boot_dev)) {
> >                               goto found;
> >                       } else {
> > -                             efi_free_pool(boot_dev);
> > -                             boot_dev = NULL;
> > +                             full_path = get_esp_from_boot_option_file_path(boot_dev);
> > +                             if (full_path) {
> > +                                     boot_dev = full_path;
>
> Later, we free full_path but do we ever free the original boot_dev?
>
> > +                                     goto found;
> > +                             } else {
> > +                                     efi_free_pool(boot_dev);
> > +                                     boot_dev = NULL;
> > +                             }
>
> Better to delete indenting where you can:
>
>                         full_path = get_esp_from_boot_option_file_path(boot_dev);
>                         if (full_path) {
>                                 boot_dev = full_path;
>                                 goto found;
>                         }
>                         efi_free_pool(boot_dev);
>                         boot_dev = NULL;
>
> regards,
> dan carpenter
>
>


More information about the U-Boot mailing list