| Message ID | 20190430061115.29960-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [U-Boot,v2,1/1] efi_loader: optional data in load options are binary | expand |
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." 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 >
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." 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 >> >
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. 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 >>> >> > >
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? -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 > >>> > >> > > > > >
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. 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 >>>>> >>>> >>> >>> >> >
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. -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 > >>>>> > >>>> > >>> > >>> > >> > > >
On 07.05.19 09:30, 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. So what does the UEFI Shell do for command argument passing? Does it always pass in a utf16 string? If so, why? Alex
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 >>>>>>> >>>>>> >>>>> >>>>> >>>> >>> >> >
On Tue, May 07, 2019 at 06:54:45PM +0200, Heinrich Schuchardt wrote: > 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: With your patch, efi_serialize_load_option() always convert a given argument to utf-16 and then the resulting variable contains u16 string as optional_data. On the other hand, efi_deserialize_load_option() does *not* convert an encoded optional_data in a variable to utf-8. See what I mean? -Takahiro Akashi > 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 > >>>>>>> > >>>>>> > >>>>> > >>>>> > >>>> > >>> > >> > > >
On 5/8/19 1:56 AM, Takahiro Akashi wrote: > On Tue, May 07, 2019 at 06:54:45PM +0200, Heinrich Schuchardt wrote: >> 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: > > With your patch, > efi_serialize_load_option() always convert a given argument to > utf-16 and then the resulting variable contains u16 string as > optional_data. On the other hand, efi_deserialize_load_option() > does *not* convert an encoded optional_data in a variable to utf-8. > When we use efi_serialize_load_option() we know that it is either an UTF-8 string in the bootargs variable or UTF-8 command line parameter for the `efi boot add` command. Currently we do not foresee adding binary data. So we can simply convert the UTF-8 input to UTF-16 as will be needed when passing the boot option to the operating system. (A patch for the boot manager still to be delivered.) But this is not the only way that BootXXXX variables can be set: * An EFI application may be used to set an arbitrary binary string. * In future an operating system could put some binary data into the load option. So it is not safe to convert the load option into UTF-8 for display. That is why I chose to use print_hex_dump() for output. If your boot option only contains ASCII letters it is still legible: => efi boot add f000 lable scsi 0:1 binary.efi 'my favorite option' => efi boot dump BootF000: attributes: A-- (0x00000001) label: lable file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/HD(1,MBR,0x90381b6c,0x800,0x3fffe)/binary.efi data: 00000000: 6d 00 79 00 20 00 66 00 61 00 76 00 6f 00 72 00 m.y. .f.a.v.o.r. 00000010: 69 00 74 00 65 00 20 00 6f 00 70 00 74 00 69 00 i.t.e. .o.p.t.i. 00000020: 6f 00 6e 00 00 00 o.n... Best regards Heinrich
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; }
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. 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(-) -- 2.20.1