Re: [U-Boot] [PATCH v2 2/3] efi_loader: device_path: lift the upper limit in dp-to-text conversion
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
---
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
#include
+#include
#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(, ...) */
+ 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"----" 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) {
+ }
+
+
Re: [U-Boot] [PATCH v2 2/3] efi_loader: device_path: lift the upper limit in dp-to-text conversion
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
> >---
> > 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
> > #include
> >+#include
> >
> > #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(, ...) */
> >+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"----" 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;
> >+
Re: [U-Boot] [PATCH v2 2/3] efi_loader: device_path: lift the upper limit in dp-to-text conversion
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
---
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
#include
+#include
#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(, ...) */
+ return 8 + 36 + (dp->length - sizeof(*dp)) * 2 + 1;
Isn't this off by factor 2?
L"VenHw(,)" is 16 bytes long.
L"----" 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;
