[PATCH v3 0/3] fix and refactoring of efi_disk.c
This series fixes the memory leak issue in lib/efi_loader/efi_disk.c and refactor the efi_disk_remove() implementation.
[Changelog] v2 -> v3 - remove unnecessary NULL check
v1 -> v2 - remove already merged patch - remove container_of() calls - add TODO comments - return immediately in UCLASS_EFI_LOADER removal
Masahisa Kojima (3): efi_loader: avoid pointer access after calling efi_delete_handle efi_loader: create common function to free struct efi_disk_obj efi_loader: return immediately in UCLASS_EFI_LOADER removal
lib/efi_loader/efi_disk.c | 52 ++++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 14 deletions(-)
efi_delete_handle() calls efi_purge_handle(), then it finally frees the EFI handle. Both diskobj and handle variables in efi_disk_remove() have the same pointer, we can not access diskobj->dp after calling efi_delete_handle().
This commit saves the struct efi_device_path pointer before calling efi_delete_handle(). This commit also fixes the missing free for volume member in struct efi_disk_obj.
This commit also removes the container_of() calls, and adds the TODO comment of missing efi_close_protocol() call for the parent EFI_BLOCK_IO_PROTOCOL.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/efi_loader/efi_disk.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 013842f077..105f080125 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -707,7 +707,9 @@ int efi_disk_remove(void *ctx, struct event *event) struct udevice *dev = event->data.dm.dev; efi_handle_t handle; struct blk_desc *desc; + struct efi_device_path *dp = NULL; struct efi_disk_obj *diskobj = NULL; + struct efi_simple_file_system_protocol *volume = NULL; efi_status_t ret;
if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) @@ -718,24 +720,30 @@ int efi_disk_remove(void *ctx, struct event *event) case UCLASS_BLK: desc = dev_get_uclass_plat(dev); if (desc && desc->uclass_id != UCLASS_EFI_LOADER) - diskobj = container_of(handle, struct efi_disk_obj, - header); + diskobj = (struct efi_disk_obj *)handle; break; case UCLASS_PARTITION: - diskobj = container_of(handle, struct efi_disk_obj, header); + diskobj = (struct efi_disk_obj *)handle; + + /* TODO: closing the parent EFI_BLOCK_IO_PROTOCOL is missing. */ + break; default: return 0; }
+ if (diskobj) { + dp = diskobj->dp; + volume = diskobj->volume; + } + ret = efi_delete_handle(handle); /* Do not delete DM device if there are still EFI drivers attached. */ if (ret != EFI_SUCCESS) return -1;
- if (diskobj) - efi_free_pool(diskobj->dp); - + efi_free_pool(dp); + free(volume); dev_tag_del(dev, DM_TAG_EFI);
return 0;
Current error handling of creating raw disk/partition has following issues. - duplicate free for EFI handle, EFI handle is already freed in efi_delete_handle() - missing free for struct efi_device_path and struct efi_simple_file_system_protocol in some error paths
To address those issues, this commit creates the common function to free the struct efi_disk_obj resources and calls it in case of error.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/efi_loader/efi_disk.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 105f080125..e2edc69fcf 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -371,6 +371,20 @@ static int efi_fs_exists(struct blk_desc *desc, int part) return 1; }
+static void efi_disk_free_diskobj(struct efi_disk_obj *diskobj) +{ + struct efi_device_path *dp = diskobj->dp; + struct efi_simple_file_system_protocol *volume = diskobj->volume; + + /* + * ignore error of efi_delete_handle() since this function + * is expected to be called in error path. + */ + efi_delete_handle(&diskobj->header); + efi_free_pool(dp); + free(volume); +} + /** * efi_disk_add_dev() - create a handle for a partition or disk * @@ -528,9 +542,7 @@ static efi_status_t efi_disk_add_dev( } return EFI_SUCCESS; error: - efi_delete_handle(&diskobj->header); - free(diskobj->volume); - free(diskobj); + efi_disk_free_diskobj(diskobj); return ret; }
@@ -569,8 +581,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) return ret; } if (efi_link_dev(&disk->header, dev)) { - efi_free_pool(disk->dp); - efi_delete_handle(&disk->header); + efi_disk_free_diskobj(disk);
return -EINVAL; } @@ -624,8 +635,9 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) return -1; } if (efi_link_dev(&disk->header, dev)) { - efi_free_pool(disk->dp); - efi_delete_handle(&disk->header); + efi_disk_free_diskobj(disk); + + /* TODO: closing the parent EFI_BLOCK_IO_PROTOCOL is missing. */
return -1; }
In case of UCLASS_EFI_LOADER, EFI handles are managed by EFI application/driver, we must not delete EFI handles.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org --- lib/efi_loader/efi_disk.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index e2edc69fcf..b1739d9920 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -731,8 +731,14 @@ int efi_disk_remove(void *ctx, struct event *event) switch (id) { case UCLASS_BLK: desc = dev_get_uclass_plat(dev); - if (desc && desc->uclass_id != UCLASS_EFI_LOADER) - diskobj = (struct efi_disk_obj *)handle; + if (desc && desc->uclass_id == UCLASS_EFI_LOADER) + /* + * EFI application/driver manages the EFI handle, + * no need to delete EFI handle. + */ + return 0; + + diskobj = (struct efi_disk_obj *)handle; break; case UCLASS_PARTITION: diskobj = (struct efi_disk_obj *)handle; @@ -744,10 +750,8 @@ int efi_disk_remove(void *ctx, struct event *event) return 0; }
- if (diskobj) { - dp = diskobj->dp; - volume = diskobj->volume; - } + dp = diskobj->dp; + volume = diskobj->volume;
ret = efi_delete_handle(handle); /* Do not delete DM device if there are still EFI drivers attached. */
participants (1)
-
Masahisa Kojima