[U-Boot] [PATCH v2 2/3] efi_loader: device_path: lift the upper limit in dp-to-text conversion
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu Oct 10 06:16:32 UTC 2019
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 at 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;
>>> }
>>>
>>>
>>
>
More information about the U-Boot
mailing list