[U-Boot] [PATCH v2 0/3] efi_loader: device path: handle a long file path name

[U-Boot] [PATCH v2 0/3] efi_loader: device path: handle a long file path name

[AKASHI Takahiro]

efi_dp_from_name() uses a fixed length (32) of buffer, and so it cannot
handle a long file path name. This patch set lifts the upper limit
as well as other limitations regarding file paths.

For example, without this patch set,
=> efi boot add 1 TEST scsi 1:1 /0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef/0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef/Shell.efi ""
=> efi boot dump
Boot0001:
  attributes: A-- (0x00000001)
  label: TEST
  file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\0123456789abcdef0123456789abcd
  data:
    00000000: 00 00

	=> The path was truncated


With this patch set applied,
=> efi boot add 1 TEST scsi 1:1 /0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef/0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef/Shell.efi ""
=> efi boot dump
Boot0001:
  attributes: A-- (0x00000001)
  label: TEST
  file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000)/\0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef\0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789  data:
    00000000: 00 00

	=> still truncated, but it was due to limitation of command
	   line processing (CONFIG_SYS_CBSIZE?), not due to device path


Changes in v2 (Oct 9, 2019)
* add patch#1 after Heinrich's comment
* add patch#2 after Heinrich's comment

AKASHI Takahiro (3):
  efi_loader: device_path: check against file path length
  efi_loader: device_path: lift the upper limit in dp-to-text conversion
  efi_loader: device_path: allow for arbitrary length of file path

 lib/efi_loader/efi_device_path.c         | 23 ++++--
 lib/efi_loader/efi_device_path_to_text.c | 90 +++++++++++++++++++++---
 2 files changed, 96 insertions(+), 17 deletions(-)

-- 
2.21.0

[U-Boot] [PATCH v2 1/3] efi_loader: device_path: check against file path length

[AKASHI Takahiro]

device_path strcuture has 2 bytes of "length" field, and so
file path length should not exceed this limit, 65535.

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

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 86297bb7c116..9f772fc924fb 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -14,6 +14,7 @@
 #include <part.h>
 #include <sandboxblockdev.h>
 #include <asm-generic/unaligned.h>
+#include <linux/compat.h> /* U16_MAX */
 
 #ifdef CONFIG_SANDBOX
 const efi_guid_t efi_guid_host_dev = U_BOOT_HOST_DEV_GUID;
@@ -868,13 +869,16 @@ struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part,
 {
 	struct efi_device_path_file_path *fp;
 	void *buf, *start;
-	unsigned dpsize = 0, fpsize;
+	size_t dpsize = 0, fpsize;
 
 	if (desc)
 		dpsize = dp_part_size(desc, part);
 
 	fpsize = sizeof(struct efi_device_path) +
 		 2 * (utf8_utf16_strlen(path) + 1);
+	if (fpsize > U16_MAX)
+		return NULL;
+
 	dpsize += fpsize;
 
 	start = buf = dp_alloc(dpsize + sizeof(END));
@@ -888,7 +892,7 @@ struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part,
 	fp = buf;
 	fp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
 	fp->dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH;
-	fp->dp.length = fpsize;
+	fp->dp.length = (u16)fpsize;
 	path_to_uefi(fp->str, path);
 	buf += fpsize;
 
@@ -1050,5 +1054,8 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
 	*file = efi_dp_from_file(((!is_net && device) ? desc : NULL),
 				 part, filename);
 
+	if (!file)
+		return EFI_INVALID_PARAMETER;
+
 	return EFI_SUCCESS;
 }
-- 
2.21.0

[U-Boot] [PATCH v2 2/3] efi_loader: device_path: lift the upper limit in dp-to-text conversion

[AKASHI Takahiro]

There is no practical reason to set a maxmum length of text either for
file path or whole device path in device path-to-text conversion.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_device_path_to_text.c | 90 +++++++++++++++++++++---
 1 file changed, 80 insertions(+), 10 deletions(-)

diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
index b158641e3043..75aabe66bf77 100644
--- a/lib/efi_loader/efi_device_path_to_text.c
+++ b/lib/efi_loader/efi_device_path_to_text.c
@@ -7,12 +7,12 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <malloc.h>
 
 #define MAC_OUTPUT_LEN 22
 #define UNKNOWN_OUTPUT_LEN 23
 
 #define MAX_NODE_LEN 512
-#define MAX_PATH_LEN 1024
 
 const efi_guid_t efi_guid_device_path_to_text_protocol =
 		EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
@@ -228,8 +228,7 @@ static char *dp_media(char *s, struct efi_device_path *dp)
 		struct efi_device_path_file_path *fp =
 			(struct efi_device_path_file_path *)dp;
 		int slen = (dp->length - sizeof(*dp)) / 2;
-		if (slen > MAX_NODE_LEN - 2)
-			slen = MAX_NODE_LEN - 2;
+
 		s += sprintf(s, "%-.*ls", slen, fp->str);
 		break;
 	}
@@ -240,6 +239,31 @@ static char *dp_media(char *s, struct efi_device_path *dp)
 	return s;
 }
 
+/*
+ * Return a maximum size of buffer to be allocated for conversion
+ *
+ * @dp			device path or node
+ * @return		maximum size of buffer
+ */
+static size_t efi_dp_text_max(struct efi_device_path *dp)
+{
+	/*
+	 * We don't have be very accurate here.
+	 * Currently, there are only two cases where a maximum size
+	 * of buffer may go over MAX_NODE_LEN.
+	 */
+	if (dp->type == DEVICE_PATH_TYPE_HARDWARE_DEVICE &&
+	    dp->sub_type == DEVICE_PATH_SUB_TYPE_VENDOR)
+		/* VenHw(<guid>, xxxxxxxx...) */
+		return 8 + 36 + (dp->length - sizeof(*dp)) * 2 + 1;
+
+	if (dp->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
+	    dp->sub_type == DEVICE_PATH_SUB_TYPE_FILE_PATH)
+		return dp->length - sizeof(*dp) + 2 /* u16 NULL */;
+
+	return MAX_NODE_LEN;
+}
+
 /*
  * Converts a single node to a char string.
  *
@@ -293,16 +317,27 @@ static uint16_t EFIAPI *efi_convert_device_node_to_text(
 		bool display_only,
 		bool allow_shortcuts)
 {
-	char str[MAX_NODE_LEN];
+	char *str;
+	size_t str_size;
 	uint16_t *text = NULL;
 
 	EFI_ENTRY("%p, %d, %d", device_node, display_only, allow_shortcuts);
 
 	if (!device_node)
 		goto out;
+
+	str_size = efi_dp_text_max(device_node);
+	if (!str_size)
+		goto out;
+
+	str = malloc(str_size);
+	if (!str)
+		goto out;
+
 	efi_convert_single_device_node_to_text(str, device_node);
 
 	text = efi_str_to_u16(str);
+	free(str);
 
 out:
 	EFI_EXIT(EFI_SUCCESS);
@@ -327,24 +362,59 @@ static uint16_t EFIAPI *efi_convert_device_path_to_text(
 		bool allow_shortcuts)
 {
 	uint16_t *text = NULL;
-	char buffer[MAX_PATH_LEN];
-	char *str = buffer;
+	char *buffer = NULL, *str;
+	size_t buf_size, buf_left;
+	efi_status_t ret = EFI_SUCCESS;
 
 	EFI_ENTRY("%p, %d, %d", device_path, display_only, allow_shortcuts);
 
-	if (!device_path)
+	if (!device_path) {
+		ret = EFI_INVALID_PARAMETER;
 		goto out;
-	while (device_path &&
-	       str + MAX_NODE_LEN < buffer + MAX_PATH_LEN) {
+	}
+
+	buf_size = 1024;
+	buf_left = buf_size;
+	buffer = malloc(buf_size);
+	if (!buffer) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+	str = buffer;
+	while (device_path) {
+		char *buf_new;
+		size_t buf_new_size, dp_text_len;
+
 		*str++ = '/';
+		buf_left--;
+		dp_text_len = efi_dp_text_max(device_path);
+		if (buf_left < dp_text_len) {
+			buf_new_size = buf_size + dp_text_len;
+			buf_new = malloc(buf_new_size);
+			if (!buf_new) {
+				ret = EFI_OUT_OF_RESOURCES;
+				goto out;
+			}
+
+			memcpy(buf_new, buffer, str - buffer);
+			buf_left = buf_new_size - (str - buffer);
+			str = buf_new + (str - buffer);
+			free(buffer);
+			buffer = buf_new;
+		}
+
 		str = efi_convert_single_device_node_to_text(str, device_path);
+
 		device_path = efi_dp_next(device_path);
 	}
 
 	text = efi_str_to_u16(buffer);
 
 out:
-	EFI_EXIT(EFI_SUCCESS);
+	free(buffer);
+	EFI_EXIT(ret);
+
 	return text;
 }
 
-- 
2.21.0

[U-Boot] [PATCH v2 3/3] 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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 9f772fc924fb..8be7af2b1b7d 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -1021,8 +1021,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 *file_path, *s;
 
 	if (path && !file)
 		return EFI_INVALID_PARAMETER;
@@ -1046,13 +1045,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);
+	file_path = strdup(path);
+	if (!file_path)
+		return EFI_OUT_OF_RESOURCES;
 	/* DOS style file path: */
-	s = filename;
+	s = file_path;
 	while ((s = strchr(s, '/')))
 		*s++ = '\\';
 	*file = efi_dp_from_file(((!is_net && device) ? desc : NULL),
-				 part, filename);
+				 part, file_path);
+	free(file_path);
 
 	if (!file)
 		return EFI_INVALID_PARAMETER;
-- 
2.21.0

[U-Boot] [PATCH v2 1/3] efi_loader: device_path: check against file path length

[Heinrich Schuchardt]

On 10/9/19 9:19 AM, AKASHI Takahiro wrote:
> device_path strcuture has 2 bytes of "length" field, and so
> file path length should not exceed this limit, 65535.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>


Thanks for the patch.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[U-Boot] [PATCH v2 2/3] efi_loader: device_path: lift the upper limit in dp-to-text conversion

[Heinrich Schuchardt]

On 10/9/19 9:19 AM, AKASHI Takahiro wrote:
> There is no practical reason to set a maxmum length of text either for
> file path or whole device path in device path-to-text conversion.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   lib/efi_loader/efi_device_path_to_text.c | 90 +++++++++++++++++++++---
>   1 file changed, 80 insertions(+), 10 deletions(-)
>
> diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
> index b158641e3043..75aabe66bf77 100644
> --- a/lib/efi_loader/efi_device_path_to_text.c
> +++ b/lib/efi_loader/efi_device_path_to_text.c
> @@ -7,12 +7,12 @@
>
>   #include <common.h>
>   #include <efi_loader.h>
> +#include <malloc.h>
>
>   #define MAC_OUTPUT_LEN 22
>   #define UNKNOWN_OUTPUT_LEN 23
>
>   #define MAX_NODE_LEN 512
> -#define MAX_PATH_LEN 1024
>
>   const efi_guid_t efi_guid_device_path_to_text_protocol =
>   		EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
> @@ -228,8 +228,7 @@ static char *dp_media(char *s, struct efi_device_path *dp)
>   		struct efi_device_path_file_path *fp =
>   			(struct efi_device_path_file_path *)dp;
>   		int slen = (dp->length - sizeof(*dp)) / 2;
> -		if (slen > MAX_NODE_LEN - 2)
> -			slen = MAX_NODE_LEN - 2;
> +
>   		s += sprintf(s, "%-.*ls", slen, fp->str);
>   		break;
>   	}
> @@ -240,6 +239,31 @@ static char *dp_media(char *s, struct efi_device_path *dp)
>   	return s;
>   }
>
> +/*
> + * Return a maximum size of buffer to be allocated for conversion
> + *
> + * @dp			device path or node
> + * @return		maximum size of buffer
> + */
> +static size_t efi_dp_text_max(struct efi_device_path *dp)
> +{
> +	/*
> +	 * We don't have be very accurate here.
> +	 * Currently, there are only two cases where a maximum size
> +	 * of buffer may go over MAX_NODE_LEN.
> +	 */
> +	if (dp->type == DEVICE_PATH_TYPE_HARDWARE_DEVICE &&
> +	    dp->sub_type == DEVICE_PATH_SUB_TYPE_VENDOR)
> +		/* VenHw(<guid>, xxxxxxxx...) */
> +		return 8 + 36 + (dp->length - sizeof(*dp)) * 2 + 1;

Isn't this off by factor 2?

L"VenHw(,)" is 16 bytes long.
L"xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxxxxxx" is 72 bytes long
Each L"xx" is 4 bytes long.
The terminating L'\0' is 2 bytes long.
The length of u16[] cannot be odd.

Best regards

Heinrich

> +
> +	if (dp->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
> +	    dp->sub_type == DEVICE_PATH_SUB_TYPE_FILE_PATH)
> +		return dp->length - sizeof(*dp) + 2 /* u16 NULL */;
> +
> +	return MAX_NODE_LEN;
> +}
> +
>   /*
>    * Converts a single node to a char string.
>    *
> @@ -293,16 +317,27 @@ static uint16_t EFIAPI *efi_convert_device_node_to_text(
>   		bool display_only,
>   		bool allow_shortcuts)
>   {
> -	char str[MAX_NODE_LEN];
> +	char *str;
> +	size_t str_size;
>   	uint16_t *text = NULL;
>
>   	EFI_ENTRY("%p, %d, %d", device_node, display_only, allow_shortcuts);
>
>   	if (!device_node)
>   		goto out;
> +
> +	str_size = efi_dp_text_max(device_node);
> +	if (!str_size)
> +		goto out;
> +
> +	str = malloc(str_size);
> +	if (!str)
> +		goto out;
> +
>   	efi_convert_single_device_node_to_text(str, device_node);
>
>   	text = efi_str_to_u16(str);
> +	free(str);
>
>   out:
>   	EFI_EXIT(EFI_SUCCESS);
> @@ -327,24 +362,59 @@ static uint16_t EFIAPI *efi_convert_device_path_to_text(
>   		bool allow_shortcuts)
>   {
>   	uint16_t *text = NULL;
> -	char buffer[MAX_PATH_LEN];
> -	char *str = buffer;
> +	char *buffer = NULL, *str;
> +	size_t buf_size, buf_left;
> +	efi_status_t ret = EFI_SUCCESS;
>
>   	EFI_ENTRY("%p, %d, %d", device_path, display_only, allow_shortcuts);
>
> -	if (!device_path)
> +	if (!device_path) {
> +		ret = EFI_INVALID_PARAMETER;
>   		goto out;
> -	while (device_path &&
> -	       str + MAX_NODE_LEN < buffer + MAX_PATH_LEN) {
> +	}
> +
> +	buf_size = 1024;
> +	buf_left = buf_size;
> +	buffer = malloc(buf_size);
> +	if (!buffer) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out;
> +	}
> +
> +	str = buffer;
> +	while (device_path) {
> +		char *buf_new;
> +		size_t buf_new_size, dp_text_len;
> +
>   		*str++ = '/';
> +		buf_left--;
> +		dp_text_len = efi_dp_text_max(device_path);
> +		if (buf_left < dp_text_len) {
> +			buf_new_size = buf_size + dp_text_len;
> +			buf_new = malloc(buf_new_size);
> +			if (!buf_new) {
> +				ret = EFI_OUT_OF_RESOURCES;
> +				goto out;
> +			}
> +
> +			memcpy(buf_new, buffer, str - buffer);
> +			buf_left = buf_new_size - (str - buffer);
> +			str = buf_new + (str - buffer);
> +			free(buffer);
> +			buffer = buf_new;
> +		}
> +
>   		str = efi_convert_single_device_node_to_text(str, device_path);
> +
>   		device_path = efi_dp_next(device_path);
>   	}
>
>   	text = efi_str_to_u16(buffer);
>
>   out:
> -	EFI_EXIT(EFI_SUCCESS);
> +	free(buffer);
> +	EFI_EXIT(ret);
> +
>   	return text;
>   }
>
>

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

[Heinrich Schuchardt]

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

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[U-Boot] [PATCH v2 2/3] efi_loader: device_path: lift the upper limit in dp-to-text conversion

[AKASHI Takahiro]

On Wed, Oct 09, 2019 at 07:41:40PM +0200, Heinrich Schuchardt wrote:
> On 10/9/19 9:19 AM, AKASHI Takahiro wrote:
> >There is no practical reason to set a maxmum length of text either for
> >file path or whole device path in device path-to-text conversion.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  lib/efi_loader/efi_device_path_to_text.c | 90 +++++++++++++++++++++---
> >  1 file changed, 80 insertions(+), 10 deletions(-)
> >
> >diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
> >index b158641e3043..75aabe66bf77 100644
> >--- a/lib/efi_loader/efi_device_path_to_text.c
> >+++ b/lib/efi_loader/efi_device_path_to_text.c
> >@@ -7,12 +7,12 @@
> >
> >  #include <common.h>
> >  #include <efi_loader.h>
> >+#include <malloc.h>
> >
> >  #define MAC_OUTPUT_LEN 22
> >  #define UNKNOWN_OUTPUT_LEN 23
> >
> >  #define MAX_NODE_LEN 512
> >-#define MAX_PATH_LEN 1024
> >
> >  const efi_guid_t efi_guid_device_path_to_text_protocol =
> >  		EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
> >@@ -228,8 +228,7 @@ static char *dp_media(char *s, struct efi_device_path *dp)
> >  		struct efi_device_path_file_path *fp =
> >  			(struct efi_device_path_file_path *)dp;
> >  		int slen = (dp->length - sizeof(*dp)) / 2;
> >-		if (slen > MAX_NODE_LEN - 2)
> >-			slen = MAX_NODE_LEN - 2;
> >+
> >  		s += sprintf(s, "%-.*ls", slen, fp->str);
> >  		break;
> >  	}
> >@@ -240,6 +239,31 @@ static char *dp_media(char *s, struct efi_device_path *dp)
> >  	return s;
> >  }
> >
> >+/*
> >+ * Return a maximum size of buffer to be allocated for conversion
> >+ *
> >+ * @dp			device path or node
> >+ * @return		maximum size of buffer
> >+ */
> >+static size_t efi_dp_text_max(struct efi_device_path *dp)
> >+{
> >+	/*
> >+	 * We don't have be very accurate here.
> >+	 * Currently, there are only two cases where a maximum size
> >+	 * of buffer may go over MAX_NODE_LEN.
> >+	 */
> >+	if (dp->type == DEVICE_PATH_TYPE_HARDWARE_DEVICE &&
> >+	    dp->sub_type == DEVICE_PATH_SUB_TYPE_VENDOR)
> >+		/* VenHw(<guid>, xxxxxxxx...) */
> >+		return 8 + 36 + (dp->length - sizeof(*dp)) * 2 + 1;
> 
> Isn't this off by factor 2?

Do you mean "(8 + 36 + (dp->length - sizeof(*dp)) * 2 + 1) * 2"?
If so, no because a buffer allocated here will eventually hold a u8 string.
(See dp_xxx().) Then, efi_str_to_u16() will convert it to u16 string.

Meanwhile,
+               return dp->length - sizeof(*dp) + 2 /* u16 NULL */;
should be
                return dp->length - sizeof(*dp) + 1/;
or more optimistically,
                return (dp->length - sizeof(*dp)) / 2 + 1/;

Thanks,
-Takahiro Akashi

> L"VenHw(,)" is 16 bytes long.
> L"xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxxxxxx" is 72 bytes long
> Each L"xx" is 4 bytes long.
> The terminating L'\0' is 2 bytes long.
> The length of u16[] cannot be odd.
> 
> Best regards
> 
> Heinrich
> 
> >+
> >+	if (dp->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
> >+	    dp->sub_type == DEVICE_PATH_SUB_TYPE_FILE_PATH)
> >+		return dp->length - sizeof(*dp) + 2 /* u16 NULL */;
> >+
> >+	return MAX_NODE_LEN;
> >+}
> >+
> >  /*
> >   * Converts a single node to a char string.
> >   *
> >@@ -293,16 +317,27 @@ static uint16_t EFIAPI *efi_convert_device_node_to_text(
> >  		bool display_only,
> >  		bool allow_shortcuts)
> >  {
> >-	char str[MAX_NODE_LEN];
> >+	char *str;
> >+	size_t str_size;
> >  	uint16_t *text = NULL;
> >
> >  	EFI_ENTRY("%p, %d, %d", device_node, display_only, allow_shortcuts);
> >
> >  	if (!device_node)
> >  		goto out;
> >+
> >+	str_size = efi_dp_text_max(device_node);
> >+	if (!str_size)
> >+		goto out;
> >+
> >+	str = malloc(str_size);
> >+	if (!str)
> >+		goto out;
> >+
> >  	efi_convert_single_device_node_to_text(str, device_node);
> >
> >  	text = efi_str_to_u16(str);
> >+	free(str);
> >
> >  out:
> >  	EFI_EXIT(EFI_SUCCESS);
> >@@ -327,24 +362,59 @@ static uint16_t EFIAPI *efi_convert_device_path_to_text(
> >  		bool allow_shortcuts)
> >  {
> >  	uint16_t *text = NULL;
> >-	char buffer[MAX_PATH_LEN];
> >-	char *str = buffer;
> >+	char *buffer = NULL, *str;
> >+	size_t buf_size, buf_left;
> >+	efi_status_t ret = EFI_SUCCESS;
> >
> >  	EFI_ENTRY("%p, %d, %d", device_path, display_only, allow_shortcuts);
> >
> >-	if (!device_path)
> >+	if (!device_path) {
> >+		ret = EFI_INVALID_PARAMETER;
> >  		goto out;
> >-	while (device_path &&
> >-	       str + MAX_NODE_LEN < buffer + MAX_PATH_LEN) {
> >+	}
> >+
> >+	buf_size = 1024;
> >+	buf_left = buf_size;
> >+	buffer = malloc(buf_size);
> >+	if (!buffer) {
> >+		ret = EFI_OUT_OF_RESOURCES;
> >+		goto out;
> >+	}
> >+
> >+	str = buffer;
> >+	while (device_path) {
> >+		char *buf_new;
> >+		size_t buf_new_size, dp_text_len;
> >+
> >  		*str++ = '/';
> >+		buf_left--;
> >+		dp_text_len = efi_dp_text_max(device_path);
> >+		if (buf_left < dp_text_len) {
> >+			buf_new_size = buf_size + dp_text_len;
> >+			buf_new = malloc(buf_new_size);
> >+			if (!buf_new) {
> >+				ret = EFI_OUT_OF_RESOURCES;
> >+				goto out;
> >+			}
> >+
> >+			memcpy(buf_new, buffer, str - buffer);
> >+			buf_left = buf_new_size - (str - buffer);
> >+			str = buf_new + (str - buffer);
> >+			free(buffer);
> >+			buffer = buf_new;
> >+		}
> >+
> >  		str = efi_convert_single_device_node_to_text(str, device_path);
> >+
> >  		device_path = efi_dp_next(device_path);
> >  	}
> >
> >  	text = efi_str_to_u16(buffer);
> >
> >  out:
> >-	EFI_EXIT(EFI_SUCCESS);
> >+	free(buffer);
> >+	EFI_EXIT(ret);
> >+
> >  	return text;
> >  }
> >
> >
> 

[U-Boot] [PATCH v2 2/3] efi_loader: device_path: lift the upper limit in dp-to-text conversion

[Heinrich Schuchardt]

On 10/10/19 2:50 AM, AKASHI Takahiro wrote:
> On Wed, Oct 09, 2019 at 07:41:40PM +0200, Heinrich Schuchardt wrote:
>> On 10/9/19 9:19 AM, AKASHI Takahiro wrote:
>>> There is no practical reason to set a maxmum length of text either for
>>> file path or whole device path in device path-to-text conversion.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>   lib/efi_loader/efi_device_path_to_text.c | 90 +++++++++++++++++++++---
>>>   1 file changed, 80 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
>>> index b158641e3043..75aabe66bf77 100644
>>> --- a/lib/efi_loader/efi_device_path_to_text.c
>>> +++ b/lib/efi_loader/efi_device_path_to_text.c
>>> @@ -7,12 +7,12 @@
>>>
>>>   #include <common.h>
>>>   #include <efi_loader.h>
>>> +#include <malloc.h>
>>>
>>>   #define MAC_OUTPUT_LEN 22
>>>   #define UNKNOWN_OUTPUT_LEN 23
>>>
>>>   #define MAX_NODE_LEN 512
>>> -#define MAX_PATH_LEN 1024
>>>
>>>   const efi_guid_t efi_guid_device_path_to_text_protocol =
>>>   		EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
>>> @@ -228,8 +228,7 @@ static char *dp_media(char *s, struct efi_device_path *dp)
>>>   		struct efi_device_path_file_path *fp =
>>>   			(struct efi_device_path_file_path *)dp;
>>>   		int slen = (dp->length - sizeof(*dp)) / 2;
>>> -		if (slen > MAX_NODE_LEN - 2)
>>> -			slen = MAX_NODE_LEN - 2;
>>> +
>>>   		s += sprintf(s, "%-.*ls", slen, fp->str);
>>>   		break;
>>>   	}
>>> @@ -240,6 +239,31 @@ static char *dp_media(char *s, struct efi_device_path *dp)
>>>   	return s;
>>>   }
>>>
>>> +/*
>>> + * Return a maximum size of buffer to be allocated for conversion
>>> + *
>>> + * @dp			device path or node
>>> + * @return		maximum size of buffer
>>> + */
>>> +static size_t efi_dp_text_max(struct efi_device_path *dp)
>>> +{
>>> +	/*
>>> +	 * We don't have be very accurate here.
>>> +	 * Currently, there are only two cases where a maximum size
>>> +	 * of buffer may go over MAX_NODE_LEN.
>>> +	 */
>>> +	if (dp->type == DEVICE_PATH_TYPE_HARDWARE_DEVICE &&
>>> +	    dp->sub_type == DEVICE_PATH_SUB_TYPE_VENDOR)
>>> +		/* VenHw(<guid>, xxxxxxxx...) */
>>> +		return 8 + 36 + (dp->length - sizeof(*dp)) * 2 + 1;
>>
>> Isn't this off by factor 2?
>
> Do you mean "(8 + 36 + (dp->length - sizeof(*dp)) * 2 + 1) * 2"?
> If so, no because a buffer allocated here will eventually hold a u8 string.
> (See dp_xxx().) Then, efi_str_to_u16() will convert it to u16 string.

If you are counting UTF-8 bytes here, please, mention it in the function
description.

In this case the next statement is wrong. The terminating '\0' would be
only one byte while a single UTF-16 word may result in up to 3 UTF-8
bytes,e.g:

た in UTF-8: E3 81 9F, in UTF-16: 0x305F.

In dp_media() in case of DEVICE_PATH_SUB_TYPE_FILE_PATH we use

	int slen = (dp->length - sizeof(*dp)) / 2;

which of cause is also incorrect.

Best regards

Heinrich

>
> Meanwhile,
> +               return dp->length - sizeof(*dp) + 2 /* u16 NULL */;
> should be
>                  return dp->length - sizeof(*dp) + 1/;
> or more optimistically,
>                  return (dp->length - sizeof(*dp)) / 2 + 1/;
>
> Thanks,
> -Takahiro Akashi
>
>> L"VenHw(,)" is 16 bytes long.
>> L"xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxxxxxx" is 72 bytes long
>> Each L"xx" is 4 bytes long.
>> The terminating L'\0' is 2 bytes long.
>> The length of u16[] cannot be odd.
>>
>> Best regards
>>
>> Heinrich
>>
>>> +
>>> +	if (dp->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
>>> +	    dp->sub_type == DEVICE_PATH_SUB_TYPE_FILE_PATH)
>>> +		return dp->length - sizeof(*dp) + 2 /* u16 NULL */;
>>> +
>>> +	return MAX_NODE_LEN;
>>> +}
>>> +
>>>   /*
>>>    * Converts a single node to a char string.
>>>    *
>>> @@ -293,16 +317,27 @@ static uint16_t EFIAPI *efi_convert_device_node_to_text(
>>>   		bool display_only,
>>>   		bool allow_shortcuts)
>>>   {
>>> -	char str[MAX_NODE_LEN];
>>> +	char *str;
>>> +	size_t str_size;
>>>   	uint16_t *text = NULL;
>>>
>>>   	EFI_ENTRY("%p, %d, %d", device_node, display_only, allow_shortcuts);
>>>
>>>   	if (!device_node)
>>>   		goto out;
>>> +
>>> +	str_size = efi_dp_text_max(device_node);
>>> +	if (!str_size)
>>> +		goto out;
>>> +
>>> +	str = malloc(str_size);
>>> +	if (!str)
>>> +		goto out;
>>> +
>>>   	efi_convert_single_device_node_to_text(str, device_node);
>>>
>>>   	text = efi_str_to_u16(str);
>>> +	free(str);
>>>
>>>   out:
>>>   	EFI_EXIT(EFI_SUCCESS);
>>> @@ -327,24 +362,59 @@ static uint16_t EFIAPI *efi_convert_device_path_to_text(
>>>   		bool allow_shortcuts)
>>>   {
>>>   	uint16_t *text = NULL;
>>> -	char buffer[MAX_PATH_LEN];
>>> -	char *str = buffer;
>>> +	char *buffer = NULL, *str;
>>> +	size_t buf_size, buf_left;
>>> +	efi_status_t ret = EFI_SUCCESS;
>>>
>>>   	EFI_ENTRY("%p, %d, %d", device_path, display_only, allow_shortcuts);
>>>
>>> -	if (!device_path)
>>> +	if (!device_path) {
>>> +		ret = EFI_INVALID_PARAMETER;
>>>   		goto out;
>>> -	while (device_path &&
>>> -	       str + MAX_NODE_LEN < buffer + MAX_PATH_LEN) {
>>> +	}
>>> +
>>> +	buf_size = 1024;
>>> +	buf_left = buf_size;
>>> +	buffer = malloc(buf_size);
>>> +	if (!buffer) {
>>> +		ret = EFI_OUT_OF_RESOURCES;
>>> +		goto out;
>>> +	}
>>> +
>>> +	str = buffer;
>>> +	while (device_path) {
>>> +		char *buf_new;
>>> +		size_t buf_new_size, dp_text_len;
>>> +
>>>   		*str++ = '/';
>>> +		buf_left--;
>>> +		dp_text_len = efi_dp_text_max(device_path);
>>> +		if (buf_left < dp_text_len) {
>>> +			buf_new_size = buf_size + dp_text_len;
>>> +			buf_new = malloc(buf_new_size);
>>> +			if (!buf_new) {
>>> +				ret = EFI_OUT_OF_RESOURCES;
>>> +				goto out;
>>> +			}
>>> +
>>> +			memcpy(buf_new, buffer, str - buffer);
>>> +			buf_left = buf_new_size - (str - buffer);
>>> +			str = buf_new + (str - buffer);
>>> +			free(buffer);
>>> +			buffer = buf_new;
>>> +		}
>>> +
>>>   		str = efi_convert_single_device_node_to_text(str, device_path);
>>> +
>>>   		device_path = efi_dp_next(device_path);
>>>   	}
>>>
>>>   	text = efi_str_to_u16(buffer);
>>>
>>>   out:
>>> -	EFI_EXIT(EFI_SUCCESS);
>>> +	free(buffer);
>>> +	EFI_EXIT(ret);
>>> +
>>>   	return text;
>>>   }
>>>
>>>
>>
>