[U-Boot] [PATCH v2 1/1] efi_loader: optional data in load options are binary

Takahiro Akashi takahiro.akashi at linaro.org
Tue May 7 23:56:29 UTC 2019


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