efi: Avoid a memory leak in efi_bind_block() on error path

Free the memory used if an error occurs.

Series-to: u-boot
Series-cc: Matthew Garrett <mgarrett@aurora.tech>, efi
Cover-letter:
efi: Add docs and tests and a few tweaks
This series addresses feedback provided on the original series,
including:

- adding documentation and tests for the two new commands
- fixing an issue that somehow didn't fail in CI
- splitting up a large function into two parts
- using lmb for the allocation, instead of looking at internal tables
- dropping use of config.h

Link: https://lore.kernel.org/u-boot/20241123195616.305687-1-mjg59@srcf.ucam.org/
Link: https://patchwork.ozlabs.org/project/uboot/list/?series=433949&state=*&archive=both
END

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Simon Glass <sjg@chromium.org>

cmd/Kconfig

@@ -130,6 +130,7 @@ config CMD_ACPI
 
 config CMD_ADDR_FIND
         bool "addr_find"
+        default y if SANDBOX || EFI_APP
 	help
 	  This command searches for an unused region of address space
 	  sufficiently large to hold a file. If successful, it sets the
@@ -1552,6 +1553,7 @@ config CMD_PART
 config CMD_PART_FIND
 	bool "part_find"
 	depends on PARTITIONS
+	default y if SANDBOX || EFI_APP
 	select HAVE_BLOCK_DEVICE
 	select PARTITION_UUIDS
 	select PARTITION_TYPE_GUID

cmd/addr_find.c

@@ -5,7 +5,6 @@
  */
 
 #include <blk.h>
-#include <config.h>
 #include <command.h>
 #include <env.h>
 #include <fs.h>
@@ -16,12 +15,13 @@ DECLARE_GLOBAL_DATA_PTR;
 
 int do_addr_find(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	struct lmb_region *mem, *reserved;
 	const char *filename;
-	struct lmb lmb;
+	phys_addr_t start;
 	loff_t size;
 	int ret;
-	int i, j;
+
+	if (argc < 3)
+		return CMD_RET_USAGE;
 
 	if (!gd->fdt_blob) {
 		log_err("No FDT setup\n");
@@ -49,36 +49,21 @@ int do_addr_find(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		return CMD_RET_FAILURE;
 	}
 
-	lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
-	mem = &lmb.memory;
-	reserved = &lmb.reserved;
+	start = lmb_alloc(size, SZ_2M);
+	if ((long)start < 0) {
+		log_err("Failed to find enough RAM for 0x%llx bytes\n", size);
 
-	for (i = 0; i < mem->cnt; i++) {
-		unsigned long long start, end;
-
-		start = mem->region[i].base;
-		end = mem->region[i].base + mem->region[i].size - 1;
-		if ((start + size) > end)
-			continue;
-		for (j = 0; j < reserved->cnt; j++) {
-			if ((reserved->region[j].base + reserved->region[j].size) < start)
-				continue;
-			if ((start + size) > reserved->region[j].base)
-				start = reserved->region[j].base + reserved->region[j].size;
-		}
-		if ((start + size) <= end) {
-			env_set_hex("loadaddr", start);
-			debug("Set loadaddr to 0x%llx\n", start);
-			return CMD_RET_SUCCESS;
-		}
+		return CMD_RET_FAILURE;
 	}
 
-	log_err("Failed to find enough RAM for 0x%llx bytes\n", size);
-	return CMD_RET_FAILURE;
+	env_set_hex("loadaddr", start);
+	debug("Set loadaddr to %llx\n", (u64)start);
+
+	return 0;
 }
 
 U_BOOT_CMD(
-	addr_find, 7, 1, do_addr_find,
+	addr_find, 4, 1, do_addr_find,
 	"find a load address suitable for a file",
 	"<interface> [<dev[:part]>] <filename>\n"
 	"- find a consecutive region of memory sufficiently large to hold\n"

cmd/part_find.c

@@ -5,12 +5,10 @@
  */
 
 #include <blk.h>
-#include <config.h>
 #include <command.h>
 #include <dm.h>
 #include <env.h>
 #include <part.h>
-#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)
 #include <efi.h>
 #include <efi_api.h>
 
@@ -47,21 +45,86 @@ static bool partition_is_on_device(const struct efi_device_path *device,
 	}
 	return false;
 }
-#endif
+
+/**
+ * part_self_find() - Check if a device contains the loaded-image path
+ *
+ * @udev: Block device to check
+ * @loaded_image_path: EFI path of the loaded image
+ * Return 0 if found, -ENOENT if not, other -ve value on error
+ */
+static int part_self_find(struct udevice *udev,
+			  struct efi_device_path *loaded_image_path)
+{
+	struct blk_desc *desc = dev_get_uclass_plat(udev);
+
+	if (desc->uclass_id == UCLASS_EFI_MEDIA) {
+		struct efi_media_plat *plat = dev_get_plat(udev->parent);
+		u32 loader_part_no;
+
+		if (partition_is_on_device(plat->device_path, loaded_image_path,
+					   &loader_part_no)) {
+			char env[256];
+			int ret;
+
+			ret = snprintf(env, sizeof(env), "%s %x:%x",
+				       blk_get_uclass_name(desc->uclass_id),
+				       desc->devnum, loader_part_no);
+			if (ret < 0 || ret == sizeof(env))
+				return -ENOSPC;
+			if (env_set("target_part", env))
+				return -ENOMEM;
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}
+
+/**
+ * part_blk_find() - Check if a device contains a partition with a type uuid
+ *
+ * @udev: Block device to check
+ * @uuid: UUID to search for (in string form)
+ * Return 0 if found, -ENOENT if not, other -ve value on error
+ */
+static int part_blk_find(struct udevice *udev, const char *uuid)
+{
+	struct blk_desc *desc = dev_get_uclass_plat(udev);
+	int i;
+
+	for (i = 1; i <= MAX_SEARCH_PARTITIONS; i++) {
+		struct disk_partition info;
+		int ret;
+
+		ret = part_get_info(desc, i, &info);
+		if (ret)
+			break;
+		if (strcasecmp(uuid, info.type_guid) == 0) {
+			char env[256];
+
+			ret = snprintf(env, sizeof(env), "%s %x:%x",
+				       blk_get_uclass_name(desc->uclass_id),
+				       desc->devnum, i);
+			if (ret < 0 || ret == sizeof(env))
+				return -ENOSPC;
+			debug("Setting target_part to %s\n", env);
+			if (env_set("target_part", env))
+				return -ENOMEM;
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}
 
 static int part_find(int argc, char *const argv[])
 {
-#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)
 	efi_guid_t efi_devpath_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
 	struct efi_device_path *loaded_image_path = NULL;
-	struct efi_boot_services *boot = efi_get_boot();
-	struct efi_priv *priv = efi_get_priv();
 	bool part_self = false;
-#endif
 	struct driver *d = ll_entry_start(struct driver, driver);
 	const int n_ents = ll_entry_count(struct driver, driver);
-	struct disk_partition info;
-	struct blk_desc *desc;
 	struct driver *entry;
 	struct udevice *udev;
 	struct uclass *uc;
@@ -70,16 +133,20 @@ static int part_find(int argc, char *const argv[])
 	if (argc != 2)
 		return CMD_RET_USAGE;
 
-#if defined(CONFIG_EFI) || defined (CONFIG_EFI_APP)
-	part_self = !strncmp(argv[1], "self", 6);
-	if (part_self) {
-		ret = boot->handle_protocol(priv->loaded_image->device_handle,
-					&efi_devpath_guid,
-					(void **)&loaded_image_path);
-		if (ret)
-			log_warning("failed to get device path for loaded image (ret=%d)", ret);
+	if (IS_ENABLED(CONFIG_EFI)) {
+		struct efi_boot_services *boot = efi_get_boot();
+		struct efi_priv *priv = efi_get_priv();
+
+		part_self = !strncmp(argv[1], "self", 6);
+		if (part_self) {
+			ret = boot->handle_protocol(priv->loaded_image->device_handle,
+						    &efi_devpath_guid,
+						    (void **)&loaded_image_path);
+			if (ret)
+				log_warning("failed to get device path for loaded image (ret=%d)",
+					    ret);
+		}
 	}
-#endif
 
 	ret = uclass_get(UCLASS_BLK, &uc);
 	if (ret) {
@@ -90,50 +157,16 @@ static int part_find(int argc, char *const argv[])
 		if (entry->id != UCLASS_BLK)
 			continue;
 		uclass_foreach_dev(udev, uc) {
-			int i;
-
 			if (udev->driver != entry)
 				continue;
-			desc = dev_get_uclass_plat(udev);
-#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)
-			if (part_self) {
-				if (desc->if_type == IF_TYPE_EFI_MEDIA) {
-					struct efi_media_plat *plat =
-						dev_get_plat(udev->parent);
-					__u32 loader_part_no;
-
-					if (partition_is_on_device(plat->device_path,
-								   loaded_image_path,
-								   &loader_part_no)) {
-						char env[256];
-
-						ret = snprintf(env, sizeof(env), "%s %d:%d", blk_get_if_type_name(desc->if_type), desc->devnum, loader_part_no);
-						if (ret < 0 || ret == sizeof(env))
-							return CMD_RET_FAILURE;
-						if (env_set("target_part", env))
-							return CMD_RET_FAILURE;
-						return CMD_RET_SUCCESS;
-					}
-				}
-			} else {
-#endif
-				for (i = 1; i <= MAX_SEARCH_PARTITIONS; i++) {
-					ret = part_get_info(desc, i, &info);
-					if (ret)
-						break;
-					if (strcasecmp(argv[1], info.type_guid) == 0) {
-						char env[256];
-						ret = snprintf(env, sizeof(env), "%s %d:%d", blk_get_if_type_name(desc->if_type), desc->devnum, i);
-						if (ret < 0 || ret == sizeof(env))
-							return CMD_RET_FAILURE;
-						env_set("target_part", env);
-						debug("Setting target_part to %s\n", env);
-						return CMD_RET_SUCCESS;
-					}
-				}
-#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)
-			}
-#endif
+			if (IS_ENABLED(CONFIG_EFI) && part_self)
+				ret = part_self_find(udev, loaded_image_path);
+			else
+				ret = part_blk_find(udev, argv[1]);
+			if (!ret)
+				return 0;
+			if (ret != -ENOENT)
+				break;
 		}
 	}
 

doc/usage/cmd/addr_find.rst

@@ -0,0 +1,63 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+.. index::
+   single: addr_find (command)
+
+addr_find command
+=================
+
+Synopsis
+--------
+
+::
+
+    addr_find <interface> [<dev[:part]> [<filename>]]
+
+Description
+-----------
+
+The addr_find command is used to find a consecutive region of memory
+sufficiently large to hold a file, ensuring that the memory is not currently in
+use for another file, etc.
+
+If successful, 'loadaddr' is set to the located address.
+
+The number of transferred bytes is saved in the environment variable filesize.
+The load address is saved in the environment variable fileaddr.
+
+interface
+    interface for accessing the block device (mmc, sata, scsi, usb, ....)
+
+dev
+    device number
+
+part
+    partition number, defaults to 0 (whole device)
+
+filename
+    path to file, defaults to environment variable 'bootfile'
+
+Example
+-------
+
+This shows obtaining an address suitable for a file on an mmc disk::
+
+    => ls mmc 1
+                extlinux/
+     97135227   initramfs-5.3.7-301.fc31.armv7hl.img
+                dtb-5.3.7-301.fc31.armv7hl/
+     12531628   vmlinuz-5.3.7-301.fc31.armv7hl
+
+    2 file(s), 2 dir(s)
+
+    => addr_find mmc 1 vmlinuz-5.3.7-301.fc31.armv7hl
+    => print loadaddr
+    loadaddr=7c00000
+    =>
+
+
+Return value
+------------
+
+The return value $? is set to 0 (true) if the command succeeds. If no suitable
+address could be found, the return value $? is set to 1 (false).

doc/usage/cmd/part_find.rst

@@ -0,0 +1,119 @@
+.. SPDX-License-Identifier: GPL-2.0+:
+
+.. index::
+   single: part_find (command)
+
+part_find command
+=================
+
+Synopsis
+--------
+
+::
+
+    part_find <uuid>
+    part_find self
+
+Description
+-----------
+
+The `part_find` command is used to find a partition with a given type GUID. When
+it finds one, it sets the target_part environment variable to the corresponding
+``interface dev:part`` string.
+
+uuid
+    Universally Unique Identifier (UUID) to search, expressed as a string
+
+self
+    This is only permitted in the EFI app. It indicates that the required
+    partition is the one from which the app was started.
+
+Example
+-------
+
+This shows searching for an EFI system partition and looking at the files on
+that partition::
+
+    => host bind 1 mmc5.img
+    => part list host 0
+
+    Partition Map for host device 0  --   Partition Type: EFI
+
+    Part    Start LBA       End LBA         Name
+            Attributes
+            Type GUID
+            Partition GUID
+    1     0x0000202f      0x0000282e      ""
+            attrs:  0x0000000000000000
+            type:   ebd0a0a2-b9e5-4433-87c0-68b6b72699c7
+                    (data)
+            guid:   6b1e51e3-427c-9f45-a947-e467b7216356
+    2     0x0000002d      0x0000082c      ""
+            attrs:  0x0000000000000000
+            type:   fe3a2a5d-4f32-41a7-b725-accc3285a309
+                    (cros-kern)
+            guid:   dece619f-4876-e140-a6c9-8c208a0c9099
+    3     0x0000202e      0x0000202e      ""
+            attrs:  0x0000000000000000
+            type:   3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec
+                    (cros-root)
+            guid:   078cee87-a195-ae4c-a974-8ba3a3d783b3
+    4     0x0000082d      0x0000102c      ""
+            attrs:  0x0000000000000000
+            type:   fe3a2a5d-4f32-41a7-b725-accc3285a309
+                    (cros-kern)
+            guid:   08d2f20f-d941-fc43-96f6-948931289d71
+    5     0x0000202d      0x0000202d      ""
+            attrs:  0x0000000000000000
+            type:   3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec
+                    (cros-root)
+            guid:   0b23ba00-a11c-ed4e-8b49-5e8738899569
+    6     0x00000029      0x00000029      ""
+            attrs:  0x0000000000000000
+            type:   fe3a2a5d-4f32-41a7-b725-accc3285a309
+                    (cros-kern)
+            guid:   6d8158a8-f82d-0d4d-8983-a3ada4eb9b73
+    7     0x0000002a      0x0000002a      ""
+            attrs:  0x0000000000000000
+            type:   3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec
+                    (cros-root)
+            guid:   76e8f9b0-7db7-3844-8f18-21de93485211
+    8     0x0000102d      0x0000182c      ""
+            attrs:  0x0000000000000000
+            type:   ebd0a0a2-b9e5-4433-87c0-68b6b72699c7
+                    (data)
+            guid:   071dfd2d-173c-f64b-9474-3318665e1d24
+    9     0x0000002b      0x0000002b      ""
+            attrs:  0x0000000000000000
+            type:   2e0a753d-9e48-43b0-8337-b15192cb1b5e
+                    (cros-rsrv)
+            guid:   b9d078c3-bafa-cd48-b771-a0aaa18d5008
+    10     0x0000002c      0x0000002c      ""
+            attrs:  0x0000000000000000
+            type:   2e0a753d-9e48-43b0-8337-b15192cb1b5e
+                    (cros-rsrv)
+            guid:   7b0c0234-1a29-0c4f-bceb-40fae8f7b27c
+    11     0x00000028      0x00000028      ""
+            attrs:  0x0000000000000000
+            type:   cab6e88e-abf3-4102-a07a-d4bb9be3c1d3
+                    (cros-fw)
+            guid:   aced715d-cd1f-394a-9e3e-24b54a7b1472
+    12     0x0000182d      0x0000202c      ""
+            attrs:  0x0000000000000000
+            type:   c12a7328-f81f-11d2-ba4b-00a0c93ec93b
+                    (system)
+            guid:   e1672afd-75ee-d74e-be95-8726b12b5e74
+    => part_find c12a7328-f81f-11d2-ba4b-00a0c93ec93b
+    => print target_part
+    target_part=host 0:c
+    => ls $target_part
+                EFI/
+
+    0 file(s), 1 dir(s)
+
+
+Return value
+------------
+
+The return value $? is set to 0 (true) if the command succeeds. If no partition
+could be found, the return value $? is set to 1 (false).

doc/usage/index.rst

@@ -24,6 +24,7 @@ Shell commands
    :maxdepth: 1
 
    cmd/acpi
+   cmd/addr_find
    cmd/addrmap
    cmd/armffa
    cmd/askenv
@@ -95,6 +96,7 @@ Shell commands
    cmd/mtrr
    cmd/panic
    cmd/part
+   cmd/part_find
    cmd/pause
    cmd/pinmux
    cmd/printenv

drivers/net/efi_net.c

@@ -4,7 +4,6 @@
  *
  */
 
-#include <config.h>
 #include <dm.h>
 #include <efi.h>
 #include <efi_api.h>

drivers/tpm/tpm2_efi.c

@@ -4,7 +4,6 @@
  *
  */
 
-#include <config.h>
 #include <dm.h>
 #include <efi.h>
 #include <efi_api.h>

lib/efi/efi_app_init.c

@@ -60,8 +60,10 @@ int efi_bind_block(efi_handle_t handle, struct efi_block_io *blkio,
 	plat->handle = handle;
 	plat->blkio = blkio;
 	plat->device_path = malloc(device_path_len);
-	if (!plat->device_path)
+	if (!plat->device_path) {
+		free(plat);
 		return log_msg_ret("path", -ENOMEM);
+	}
 	memcpy(plat->device_path, device_path, device_path_len);
 	ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
 			  plat, ofnode_null(), &dev);

lib/fdtdec.c

@@ -1238,7 +1238,7 @@ static void *fdt_find_separate(void)
 		fdt_blob = (ulong *)__bss_end;
 #elif defined CONFIG_EFI_APP
 	/* FDT is in a separate section */
-	fdt_blob = (ulong *)__dtb;
+	fdt_blob = (ulong *)_dtb;
 #else
 	/* FDT is at end of image */
 	fdt_blob = (ulong *)_end;

test/cmd/Makefile

@@ -14,6 +14,7 @@ obj-$(CONFIG_CMD_PAUSE) += test_pause.o
 endif
 obj-y += exit.o mem.o
 obj-$(CONFIG_X86) += cpuid.o msr.o
+obj-$(CONFIG_CMD_ADDR_FIND) += addr_find.o
 obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o
 obj-$(CONFIG_CMD_BDI) += bdinfo.o
 obj-$(CONFIG_COREBOOT_SYSINFO) += coreboot.o
@@ -25,6 +26,7 @@ obj-$(CONFIG_CMD_LOADM) += loadm.o
 obj-$(CONFIG_CMD_MEMINFO) += meminfo.o
 obj-$(CONFIG_CMD_MEMORY) += mem_copy.o
 obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o
+obj-$(CONFIG_CMD_PART_FIND) += part_find.o
 ifdef CONFIG_CMD_PCI
 obj-$(CONFIG_CMD_PCI_MPS) += pci_mps.o
 endif

test/cmd/addr_find.c

@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test for 'part_find' command
+ *
+ * Copyright 2024 Google LLC
+ * Written by Simon Glass <sjg@chromium.org>
+ */
+
+#include <dm/device-internal.h>
+#include <dm/lists.h>
+#include <dm/ofnode.h>
+#include <dm/test.h>
+#include <test/cmd.h>
+#include <test/ut.h>
+
+/* Test 'addr_find' command */
+static int cmd_test_addr_find(struct unit_test_state *uts)
+{
+	ut_assertok(env_set("loadaddr", NULL));
+	ut_assertok(run_command("addr_find mmc 1:1 vmlinuz-5.3.7-301.fc31.armv7hl", 0));
+	ut_assert_console_end();
+
+	ut_assertnonnull(env_get("loadaddr"));
+
+	return 0;
+}
+CMD_TEST(cmd_test_addr_find, UTF_CONSOLE | UTF_DM | UTF_SCAN_FDT);

test/cmd/part_find.c

@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test for 'part_find' command
+ *
+ * Copyright 2024 Google LLC
+ * Written by Simon Glass <sjg@chromium.org>
+ */
+
+#include <dm/device-internal.h>
+#include <dm/lists.h>
+#include <dm/ofnode.h>
+#include <dm/test.h>
+#include <test/cmd.h>
+#include <test/ut.h>
+
+/* Test 'part_find' command */
+static int cmd_test_part_find(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+	ofnode root, node;
+
+	/* Enable the requested mmc node since we need a second bootflow */
+	root = oftree_root(oftree_default());
+	node = ofnode_find_subnode(root, "mmc5");
+	ut_assert(ofnode_valid(node));
+	ut_assertok(lists_bind_fdt(gd->dm_root, node, &dev, NULL, false));
+
+	ut_assertok(device_probe(dev));
+
+	ut_assertok(env_set("target_part", NULL));
+	ut_assertok(run_command("part_find c12a7328-f81f-11d2-ba4b-00a0c93ec93b", 0));
+	ut_assert_console_end();
+	ut_asserteq_str("mmc 5:c", env_get("target_part"));
+
+	ut_asserteq(1, run_command("part_find invalid", 0));
+	ut_asserteq_str("mmc 5:c", env_get("target_part"));
+
+	ut_assert_console_end();
+
+	return 0;
+}
+CMD_TEST(cmd_test_part_find, UTF_CONSOLE | UTF_DM | UTF_SCAN_FDT);