From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takahiro Akashi Date: Wed, 8 May 2019 08:56:29 +0900 Subject: [U-Boot] [PATCH v2 1/1] efi_loader: optional data in load options are binary In-Reply-To: <715443d8-5ede-b33b-2313-7fb5f8205172@gmx.de> References: <20190430061115.29960-1-xypron.glpk@gmx.de> <20190507015308.GA11160@linaro.org> <6974f8d4-1b1a-1414-0a04-dcd8e4248476@gmx.de> <20190507061610.GG11160@linaro.org> <20190507073046.GI11160@linaro.org> <715443d8-5ede-b33b-2313-7fb5f8205172@gmx.de> Message-ID: <20190507235627.GJ11160@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 > >>>>>>>--- > >>>>>>>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 > >>>>>>>   #include > >>>>>>>   #include > >>>>>>>+#include > >>>>>>>   #include > >>>>>>>   #include > >>>>>>>   #include > >>>>>>>@@ -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 > >>>>>>> > >>>>>> > >>>>> > >>>>> > >>>> > >>> > >> > > >