[U-Boot] [PATCH] efi_loader: device_path: allow for arbitrary length of file path

[U-Boot] [PATCH] efi_loader: device_path: allow for arbitrary length of file path

[AKASHI Takahiro]

This patch will lift the upper limit of maximum path length.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_device_path.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 86297bb7c116..13a2b5e29db1 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -1017,8 +1017,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
 	struct blk_desc *desc = NULL;
 	disk_partition_t fs_partition;
 	int part = 0;
-	char filename[32] = { 0 }; /* dp->str is u16[32] long */
-	char *s;
+	char *filename, *s;
 
 	if (path && !file)
 		return EFI_INVALID_PARAMETER;
@@ -1042,13 +1041,16 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
 	if (!path)
 		return EFI_SUCCESS;
 
-	snprintf(filename, sizeof(filename), "%s", path);
+	filename = strdup(path);
+	if (!filename)
+		return EFI_OUT_OF_RESOURCES;
 	/* DOS style file path: */
 	s = filename;
 	while ((s = strchr(s, '/')))
 		*s++ = '\\';
 	*file = efi_dp_from_file(((!is_net && device) ? desc : NULL),
 				 part, filename);
+	free(filename);
 
 	return EFI_SUCCESS;
 }
-- 
2.21.0

[U-Boot] [PATCH] efi_loader: device_path: allow for arbitrary length of file path

[Heinrich Schuchardt]

On 10/4/19 6:45 AM, AKASHI Takahiro wrote:
> This patch will lift the upper limit of maximum path length.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Thanks for addressing this issue.

Please, have a look at the side effects to our implementation of the
device path to text protocol. Here I assumed that a device node is at
most 512 bytes long. This will no longer hold true after your patch.

I think we should fix the device path to text protocol first before
merging this patch.

> ---
>   lib/efi_loader/efi_device_path.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 86297bb7c116..13a2b5e29db1 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -1017,8 +1017,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>   	struct blk_desc *desc = NULL;
>   	disk_partition_t fs_partition;
>   	int part = 0;
> -	char filename[32] = { 0 }; /* dp->str is u16[32] long */
> -	char *s;
> +	char *filename, *s;

Below we are replacing slashes '/' by backslashes '\'. So shouldn't this
variable be called filepath?

>
>   	if (path && !file)
>   		return EFI_INVALID_PARAMETER;
> @@ -1042,13 +1041,16 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>   	if (!path)
>   		return EFI_SUCCESS;
>
> -	snprintf(filename, sizeof(filename), "%s", path);
> +	filename = strdup(path);

Windows has a maximum file path length of 260 characters. Can we assume
in EFI that a path has a maximum length of 260 characters too?

At least we must ensure that the node length does not exceed 65535
bytes, the maximum possible value in the node length field.

Best regards

Heinrich

> +	if (!filename)
> +		return EFI_OUT_OF_RESOURCES;
>   	/* DOS style file path: */
>   	s = filename;
>   	while ((s = strchr(s, '/')))
>   		*s++ = '\\';
>   	*file = efi_dp_from_file(((!is_net && device) ? desc : NULL),
>   				 part, filename);
> +	free(filename);
>
>   	return EFI_SUCCESS;
>   }
>

[U-Boot] [PATCH] efi_loader: device_path: allow for arbitrary length of file path

[AKASHI Takahiro]

On Fri, Oct 04, 2019 at 08:09:57AM +0200, Heinrich Schuchardt wrote:
> On 10/4/19 6:45 AM, AKASHI Takahiro wrote:
> >This patch will lift the upper limit of maximum path length.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> Thanks for addressing this issue.
> 
> Please, have a look at the side effects to our implementation of the
> device path to text protocol. Here I assumed that a device node is at
> most 512 bytes long. This will no longer hold true after your patch.

Whether my patch is applied or not, we may possibly hit this limitation.

> I think we should fix the device path to text protocol first before
> merging this patch.

Anyhow, I will check.

> >---
> >  lib/efi_loader/efi_device_path.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> >diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> >index 86297bb7c116..13a2b5e29db1 100644
> >--- a/lib/efi_loader/efi_device_path.c
> >+++ b/lib/efi_loader/efi_device_path.c
> >@@ -1017,8 +1017,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
> >  	struct blk_desc *desc = NULL;
> >  	disk_partition_t fs_partition;
> >  	int part = 0;
> >-	char filename[32] = { 0 }; /* dp->str is u16[32] long */
> >-	char *s;
> >+	char *filename, *s;
> 
> Below we are replacing slashes '/' by backslashes '\'. So shouldn't this
> variable be called filepath?

Okay.

> >
> >  	if (path && !file)
> >  		return EFI_INVALID_PARAMETER;
> >@@ -1042,13 +1041,16 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
> >  	if (!path)
> >  		return EFI_SUCCESS;
> >
> >-	snprintf(filename, sizeof(filename), "%s", path);
> >+	filename = strdup(path);
> 
> Windows has a maximum file path length of 260 characters. Can we assume
> in EFI that a path has a maximum length of 260 characters too?

Why do you stick to Windows here?
In my patch of "install FILE_SYSTEM_PROTOCOL to whole disk,"
you wanted to allow file systems other than FAT.

> At least we must ensure that the node length does not exceed 65535
> bytes, the maximum possible value in the node length field.

Okay.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >+	if (!filename)
> >+		return EFI_OUT_OF_RESOURCES;
> >  	/* DOS style file path: */
> >  	s = filename;
> >  	while ((s = strchr(s, '/')))
> >  		*s++ = '\\';
> >  	*file = efi_dp_from_file(((!is_net && device) ? desc : NULL),
> >  				 part, filename);
> >+	free(filename);
> >
> >  	return EFI_SUCCESS;
> >  }
> >
>