diff mbox series

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

Message ID 20231225044356.626900-4-masahisa.kojima@linaro.org
State Superseded
Headers show
Series fix and refactoring of efi_disk.c | expand

Commit Message

Masahisa Kojima Dec. 25, 2023, 4:43 a.m. UTC
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 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.kojima@linaro.org>
---
 lib/efi_loader/efi_disk.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Heinrich Schuchardt Dec. 25, 2023, 10:31 a.m. UTC | #1
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.kojima@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;
>   	}
Masahisa Kojima Dec. 26, 2023, 1:46 a.m. UTC | #2
On Mon, 25 Dec 2023 at 19:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> 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.

Duplicate free() calls occur here in the original code:
> > -     efi_delete_handle(&diskobj->header);
> > -     free(diskobj->volume);
> > -     free(diskobj);

efi_delete_handle(&diskobj->header) will free the handle(=
&diskobj->header, = diskobj).
So calling  free(diskobj) should be avoided.

>
> >   - 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.kojima@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.

Yes, initialize it with diskobj->dp.

>
> > +     struct efi_simple_file_system_protocol *volume = NULL;
>
> ditto

OK.

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

I assume this function efi_disk_free_diskobj() is called only from
the error path.(I should note as a comment)
I think it is better to ignore errors in efi_delete_handle() and proceed to
free the other resources.

>
> > +     efi_free_pool(dp);
>
> The device path may only be freed if it has been uninstalled from the
> handle.

Sorry, I'm not sure if I understand this comment correctly, but device_path
is uninstalled in efi_delete_handle().

>
> > +     free(volume);
>
> The simple file protocol interface may only be freed if it has been
> uninstalled from the handle.

Same as above, all protocols on the handle are uninstalled in
efi_delete_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.

OK, I will check on this.

Thanks,
Masahisa Kojima


>
> Best regards
>
> Heinrich
>
> >
> >               return -1;
> >       }
>
diff mbox series

Patch

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;
+	struct efi_simple_file_system_protocol *volume = NULL;
+
+	dp = diskobj->dp;
+	volume = diskobj->volume;
+
+	efi_delete_handle(&diskobj->header);
+	efi_free_pool(dp);
+	free(volume);
+}
+
 /**
  * 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);
 
 		return -1;
 	}