[U-Boot] [PATCH v2 1/1] efi_loader: optional data in load options are binary
Heinrich Schuchardt
xypron.glpk at gmx.de
Tue May 7 16:54:45 UTC 2019
On 5/7/19 9:30 AM, Takahiro Akashi wrote:
> On Tue, May 07, 2019 at 09:12:56AM +0200, Heinrich Schuchardt wrote:
>> On 5/7/19 8:16 AM, Takahiro Akashi wrote:
>>> On Tue, May 07, 2019 at 08:04:26AM +0200, Heinrich Schuchardt wrote:
>>>> On 5/7/19 7:16 AM, Heinrich Schuchardt wrote:
>>>>> On 5/7/19 3:53 AM, Takahiro Akashi wrote:
>>>>>> On Tue, Apr 30, 2019 at 08:11:15AM +0200, Heinrich Schuchardt wrote:
>>>>>>> The field boot OptionalData in structure _EFI_LOAD_OPTIONS is for binary
>>>>>>> data.
>>>>>>>
>>>>>>> When we use `efidebug boot add` we should convert the 5th argument from
>>>>>>> UTF-8 to UTF-16 before putting it into the BootXXXX variable.
>>>>>>
>>>>>> While optional_data holds u8 string in calling
>>>>>> efi_serialize_load_option(),
>>>>>> it holds u16 string in leaving from efi_deserialize_load_option().
>>>>>> We should handle it in a consistent way if you want to keep optional_data
>>>>>> as "const u8."
>>>>
>>>> When communicating with Linux optional data may contain a u16 string.
>>>> But I cannot see were our coding is inconsistent.
>>>
>>> I don't get your point.
>>> Do you want to allow 'u8 *' variable to hold u16 string?#
>>
>> Yes, optional data may contain anything, in the case of Linux the
>> command line parameters as an u16 string.
>>
>> Other operating systems may use the field in other ways, e.g. store an
>> ASCII string.
>
> The problem is that with your patch optional_data is *always* converted
> to utf-16 as far as we use efidebug.
> My efidebug is not for linux only.
optional_data treated is not treated as u16 in efidebug:
include/hexdump.h:
void print_hex_dump(const char *prefix_str, int prefix_type,
int rowsize, int groupsize, const void *buf,
size_t len, bool ascii);
include/efi_loader:
struct efi_load_option {
u32 attributes;
u16 file_path_length;
u16 *label;
struct efi_device_path *file_path;
const u8 *optional_data;
};
cmd/efidebug.c
print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,
lo.optional_data, size + (u8 *)data -
(u8 *)lo.optional_data, true);
Best regards
Heinrich
>
> -Takahiro Akashi
>
>
>> Regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>> Regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> The patch is already merged so I will have to create a follow up patch.
>>>>>
>>>>> The UEFI spec has EFI_LOAD_OPTION.OptionalData as UINT8. So an odd
>>>>> number of bytes is a possibility.
>>>>>
>>>>> Best regards
>>>>>
>>>>> Heinrich
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> -Takahiro Akashi
>>>>>>
>>>>>>> When printing boot variables with `efidebug boot dump` we should support
>>>>>>> the OptionalData being arbitrary binary data. So let's dump the data as
>>>>>>> hexadecimal values.
>>>>>>>
>>>>>>> Here is an example session protocol:
>>>>>>>
>>>>>>> => efidebug boot add 00a1 label1 scsi 0:1 doit1 'my option'
>>>>>>> => efidebug boot add 00a2 label2 scsi 0:1 doit2
>>>>>>> => efidebug boot dump
>>>>>>> Boot00A0:
>>>>>>> attributes: A-- (0x00000001)
>>>>>>> label: label1
>>>>>>> file_path: .../HD(1,MBR,0xeac4e18b,0x800,0x3fffe)/doit1
>>>>>>> data:
>>>>>>> 00000000: 6d 00 79 00 20 00 6f 00 70 00 74 00 69 00 6f 00 m.y.
>>>>>>> .o.p.t.i.o.
>>>>>>> 00000010: 6e 00 00 00 n...
>>>>>>> Boot00A1:
>>>>>>> attributes: A-- (0x00000001)
>>>>>>> label: label2
>>>>>>> file_path: .../HD(1,MBR,0xeac4e18b,0x800,0x3fffe)/doit2
>>>>>>> data:
>>>>>>>
>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> remove statement without effect in efi_serialize_load_option()
>>>>>>> ---
>>>>>>> cmd/efidebug.c | 27 +++++++++++++++++----------
>>>>>>> include/efi_loader.h | 2 +-
>>>>>>> lib/efi_loader/efi_bootmgr.c | 15 ++++++++-------
>>>>>>> 3 files changed, 26 insertions(+), 18 deletions(-)
>>>>>>>
>>>>>>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>>>>>>> index efe4ea873e..c4ac9dd634 100644
>>>>>>> --- a/cmd/efidebug.c
>>>>>>> +++ b/cmd/efidebug.c
>>>>>>> @@ -11,6 +11,7 @@
>>>>>>> #include <efi_loader.h>
>>>>>>> #include <environment.h>
>>>>>>> #include <exports.h>
>>>>>>> +#include <hexdump.h>
>>>>>>> #include <malloc.h>
>>>>>>> #include <search.h>
>>>>>>> #include <linux/ctype.h>
>>>>>>> @@ -545,7 +546,10 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int
>>>>>>> flag,
>>>>>>> + sizeof(struct efi_device_path); /* for END */
>>>>>>>
>>>>>>> /* optional data */
>>>>>>> - lo.optional_data = (u8 *)(argc == 6 ? "" : argv[6]);
>>>>>>> + if (argc < 6)
>>>>>>> + lo.optional_data = NULL;
>>>>>>> + else
>>>>>>> + lo.optional_data = (const u8 *)argv[6];
>>>>>>>
>>>>>>> size = efi_serialize_load_option(&lo, (u8 **)&data);
>>>>>>> if (!size) {
>>>>>>> @@ -615,12 +619,13 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int
>>>>>>> flag,
>>>>>>> /**
>>>>>>> * show_efi_boot_opt_data() - dump UEFI load option
>>>>>>> *
>>>>>>> - * @id: Load option number
>>>>>>> - * @data: Value of UEFI load option variable
>>>>>>> + * @id: load option number
>>>>>>> + * @data: value of UEFI load option variable
>>>>>>> + * @size: size of the boot option
>>>>>>> *
>>>>>>> * Decode the value of UEFI load option variable and print
>>>>>>> information.
>>>>>>> */
>>>>>>> -static void show_efi_boot_opt_data(int id, void *data)
>>>>>>> +static void show_efi_boot_opt_data(int id, void *data, size_t size)
>>>>>>> {
>>>>>>> struct efi_load_option lo;
>>>>>>> char *label, *p;
>>>>>>> @@ -638,7 +643,7 @@ static void show_efi_boot_opt_data(int id, void
>>>>>>> *data)
>>>>>>> utf16_utf8_strncpy(&p, lo.label, label_len16);
>>>>>>>
>>>>>>> printf("Boot%04X:\n", id);
>>>>>>> - printf("\tattributes: %c%c%c (0x%08x)\n",
>>>>>>> + printf(" attributes: %c%c%c (0x%08x)\n",
>>>>>>> /* ACTIVE */
>>>>>>> lo.attributes & LOAD_OPTION_ACTIVE ? 'A' : '-',
>>>>>>> /* FORCE RECONNECT */
>>>>>>> @@ -646,14 +651,16 @@ static void show_efi_boot_opt_data(int id, void
>>>>>>> *data)
>>>>>>> /* HIDDEN */
>>>>>>> lo.attributes & LOAD_OPTION_HIDDEN ? 'H' : '-',
>>>>>>> lo.attributes);
>>>>>>> - printf("\tlabel: %s\n", label);
>>>>>>> + printf(" label: %s\n", label);
>>>>>>>
>>>>>>> dp_str = efi_dp_str(lo.file_path);
>>>>>>> - printf("\tfile_path: %ls\n", dp_str);
>>>>>>> + printf(" file_path: %ls\n", dp_str);
>>>>>>> efi_free_pool(dp_str);
>>>>>>>
>>>>>>> - printf("\tdata: %s\n", lo.optional_data);
>>>>>>> -
>>>>>>> + printf(" data:\n");
>>>>>>> + print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,
>>>>>>> + lo.optional_data, size + (u8 *)data -
>>>>>>> + (u8 *)lo.optional_data, true);
>>>>>>> free(label);
>>>>>>> }
>>>>>>>
>>>>>>> @@ -686,7 +693,7 @@ static void show_efi_boot_opt(int id)
>>>>>>> data));
>>>>>>> }
>>>>>>> if (ret == EFI_SUCCESS)
>>>>>>> - show_efi_boot_opt_data(id, data);
>>>>>>> + show_efi_boot_opt_data(id, data, size);
>>>>>>> else if (ret == EFI_NOT_FOUND)
>>>>>>> printf("Boot%04X: not found\n", id);
>>>>>>>
>>>>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>>>>> index 39ed8a6fa5..07b913d256 100644
>>>>>>> --- a/include/efi_loader.h
>>>>>>> +++ b/include/efi_loader.h
>>>>>>> @@ -560,7 +560,7 @@ struct efi_load_option {
>>>>>>> u16 file_path_length;
>>>>>>> u16 *label;
>>>>>>> struct efi_device_path *file_path;
>>>>>>> - u8 *optional_data;
>>>>>>> + const u8 *optional_data;
>>>>>>> };
>>>>>>>
>>>>>>> void efi_deserialize_load_option(struct efi_load_option *lo, u8
>>>>>>> *data);
>>>>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>>>>>> index 4ccba22875..7bf51874c1 100644
>>>>>>> --- a/lib/efi_loader/efi_bootmgr.c
>>>>>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>>>>>> @@ -53,19 +53,20 @@ void efi_deserialize_load_option(struct
>>>>>>> efi_load_option *lo, u8 *data)
>>>>>>> */
>>>>>>> unsigned long efi_serialize_load_option(struct efi_load_option *lo,
>>>>>>> u8 **data)
>>>>>>> {
>>>>>>> - unsigned long label_len, option_len;
>>>>>>> + unsigned long label_len;
>>>>>>> unsigned long size;
>>>>>>> u8 *p;
>>>>>>>
>>>>>>> label_len = (u16_strlen(lo->label) + 1) * sizeof(u16);
>>>>>>> - option_len = strlen((char *)lo->optional_data);
>>>>>>>
>>>>>>> /* total size */
>>>>>>> size = sizeof(lo->attributes);
>>>>>>> size += sizeof(lo->file_path_length);
>>>>>>> size += label_len;
>>>>>>> size += lo->file_path_length;
>>>>>>> - size += option_len + 1;
>>>>>>> + if (lo->optional_data)
>>>>>>> + size += (utf8_utf16_strlen((const char *)lo->optional_data)
>>>>>>> + + 1) * sizeof(u16);
>>>>>>> p = malloc(size);
>>>>>>> if (!p)
>>>>>>> return 0;
>>>>>>> @@ -84,10 +85,10 @@ unsigned long efi_serialize_load_option(struct
>>>>>>> efi_load_option *lo, u8 **data)
>>>>>>> memcpy(p, lo->file_path, lo->file_path_length);
>>>>>>> p += lo->file_path_length;
>>>>>>>
>>>>>>> - memcpy(p, lo->optional_data, option_len);
>>>>>>> - p += option_len;
>>>>>>> - *(char *)p = '\0';
>>>>>>> -
>>>>>>> + if (lo->optional_data) {
>>>>>>> + utf8_utf16_strcpy((u16 **)&p, (const char *)lo->optional_data);
>>>>>>> + p += sizeof(u16); /* size of trailing \0 */
>>>>>>> + }
>>>>>>> return size;
>>>>>>> }
>>>>>>>
>>>>>>> --
>>>>>>> 2.20.1
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the U-Boot
mailing list