Re: [PATCH 3/3] efi_loader: create common function to free struct efi_disk_obj

[Heinrich Schuchardt]

On 12/25/23 05:43, Masahisa Kojima wrote:

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()

I cannot see where this patch reduces the number of efi_delete_handle()
invocations.

  - missing free for struct efi_device_path and
    struct efi_simple_file_system_protocol in some error paths

To address those issue, 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.koj...@linaro.org>
---
  lib/efi_loader/efi_disk.c | 23 ++++++++++++++++-------
  1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 415d8601ba..d2ac2fab9b 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -372,6 +372,19 @@ 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 = NULL;

This NULL value is never used.

+       struct efi_simple_file_system_protocol *volume = NULL;

ditto

+
+       dp = diskobj->dp;
+       volume = diskobj->volume;
+
+       efi_delete_handle(&diskobj->header);

efi_delete_handle() may fail.

+       efi_free_pool(dp);

The device path may only be freed if it has been uninstalled from the
handle.

+       free(volume);

The simple file protocol interface may only be freed if it has been
uninstalled from the handle.

+}
+
  /**
   * efi_disk_add_dev() - create a handle for a partition or disk
   *
@@ -529,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;
  }

@@ -570,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;
        }
@@ -625,8 +635,7 @@ 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);

In efi_disk_add_dev we have opened a protocol interface. We must close
it before removing the partition handle otherwise the disk handle can
never be removed.

To do this all properly we will need to re-implement this code using
proper EFI drivers which expose a driver binding protocol. Please, add a
TODO: comment here that closing the protocol is missing.

Best regards

Heinrich

                return -1;
        }