Here is an updated version of the patch.
Please bear with me while I learn this email based code review process. I am trying to find resources on this, but there are a lot and I don't know what applies here.
Anyway, I believe there was another leak. If the call to device_bind fails both plat and plat->device_path need to be freed. I think I captured this in the new version.
The other open issue was the duplication of efi_dp_size code. But if I understand correctly this was not available in the configuration that we are using. Not sure what the path forward should be here.
With kind regards, Janis
On Wed, Dec 11, 2024 at 12:23 PM Simon Glass sjg@chromium.org wrote:
Hi Janis,
On Wed, 11 Dec 2024 at 10:50, Janis Danisevskis jdanisevskis@aurora.tech wrote:
Hi everyone,
Good catch finding this leak. Would you like me to provide an updated
patch?
I am good either way. Let Simon take credit for the fix on the git
history. I can live with
shame :-).
Thanks for the email. If you're able to send a v2, take a look at this series which has a few other things:
https://patchwork.ozlabs.org/project/uboot/list/?series=436150
You can incorporate any or all of those and there's no need to keep me as the author of anything in particular.
Regards, Simon
Janis
On Tue, Dec 10, 2024 at 9:11 AM Tom Rini trini@konsulko.com wrote:
On Tue, Dec 10, 2024 at 09:17:26AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 10 Dec 2024 at 01:50, Heinrich Schuchardt xypron.glpk@gmx.de
wrote:
On 23.11.24 20:55, Matthew Garrett wrote:
From: Janis Danisevskis jdanisevskis@aurora.tech
efi_bind_block had two issues.
- A pointer to a the stack was inserted as plat structure and
thus used
beyond its life time. 2. Only the first segment of the device path was copied into the platfom data structure resulting in an unterminated device path.
Signed-off-by: Janis Danisevskis jdanisevskis@aurora.tech Signed-off-by: Matthew Garrett mgarrett@aurora.tech
lib/efi/efi_app_init.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c index 9704020b749..cc91e1d74b8 100644 --- a/lib/efi/efi_app_init.c +++ b/lib/efi/efi_app_init.c @@ -19,6 +19,15 @@
DECLARE_GLOBAL_DATA_PTR;
+static size_t device_path_length(const struct efi_device_path
*device_path)
+{
const struct efi_device_path *d;for (d = device_path; d->type != DEVICE_PATH_TYPE_END; d =(void *)d + d->length) {
}return (void *)d - (void *)device_path + d->length;+}
- /**
- efi_bind_block() - bind a new block device to an EFI device
@@ -39,19 +48,23 @@ int efi_bind_block(efi_handle_t handle,
struct efi_block_io *blkio,
struct efi_device_path *device_path, int len, struct udevice **devp){
struct efi_media_plat plat;
struct efi_media_plat *plat; struct udevice *dev; char name[18]; int ret;
plat.handle = handle;plat.blkio = blkio;plat.device_path = malloc(device_path->length);if (!plat.device_path)
size_t device_path_len = device_path_length(device_path);plat = malloc(sizeof(struct efi_media_plat));if (!plat)return log_msg_ret("plat", -ENOMEM);plat->handle = handle;plat->blkio = blkio;plat->device_path = malloc(device_path_len);if (!plat->device_path) return log_msg_ret("path", -ENOMEM);
memcpy(plat.device_path, device_path, device_path->length);
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);
plat, ofnode_null(), &dev); if (ret) return log_msg_ret("bind", ret);Here you have a memory leak for pointer plat if ret != 0.
Simon suggested a follow up patch. Instead we should fix it in this
patch.
@Simon The logic in the caller setup_block() looks wrong.
LocateHandle() does not ensure that when you try to read from the
block
device the same still does exist. E.g. if a USB device is removed
the
block IO protocol could be uninstalled and the handle deleted and
the
EFI app may crash. Please, call OpenProtocol() with attribute
BY_DRIVER.
HandleProtocol() is considered deprecated. Please, use
OpenProtocol()
instead.
Can you please send a follow-up with your ideas on this?
Simon, it's in your tree. The normal path here would be that the submitter would correct this in v2. But you're trying to short-circuit that path for your own reasons. No one should be making work on top of your tree except for you.
-- Tom