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.aka...@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;
  }





_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to