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@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 >