Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
On Tue, May 22, 2018 at 6:24 AM, Jan Beulich wrote: On 21.05.18 at 18:59, wrote: >> After closer inspection the problem is with the following line here: >> for ( i = 1; i < argc; ++i ) >> >> This assumes that argv[0] is the EFI executable filename, which is not >> true when EFI_LOAD_OPTION is used. That's why in my v3 patch I had the >> "elo_active" variable to determine whether to start the iteration from >> 0 or from 1. > > How about this one then? > Success! Although I have to say it's pretty hard to follow the code and what it's actually doing. Perhaps adding more comments would be helpful. Tamas > > > EFI: add EFI_LOAD_OPTION support > > When booting Xen via UEFI the Xen config file can contain multiple > sections each describing different boot options. It is currently only > possible to choose which section to boot with if the buffer contains a > string. UEFI provides a different standard to pass optional arguments > to an application, and in this patch we make Xen properly parse this > buffer, thus making it possible to have separate EFI boot options > present for the different config sections. > > Signed-off-by: Tamas K Lengyel > Signed-off-by: Jan Beulich > --- > v4: Address my own review comments. > > --- unstable.orig/xen/common/efi/boot.c > +++ unstable/xen/common/efi/boot.c > @@ -88,6 +88,14 @@ typedef struct _EFI_APPLE_PROPERTIES { > EFI_APPLE_PROPERTIES_GETALL GetAll; > } EFI_APPLE_PROPERTIES; > > +typedef struct _EFI_LOAD_OPTION { > +UINT32 Attributes; > +UINT16 FilePathListLength; > +CHAR16 Description[]; > +} EFI_LOAD_OPTION; > + > +#define LOAD_OPTION_ACTIVE 0x0001 > + > union string { > CHAR16 *w; > char *s; > @@ -275,6 +283,16 @@ static int __init wstrncmp(const CHAR16 > return n ? *s1 - *s2 : 0; > } > > +static const CHAR16 *__init wmemchr(const CHAR16 *s, CHAR16 c, UINTN n) > +{ > +while ( n && *s != c ) > +{ > +--n; > +++s; > +} > +return n ? s : NULL; > +} > + > static CHAR16 *__init s2w(union string *str) > { > const char *s = str->s; > @@ -374,14 +392,58 @@ static void __init PrintErrMesg(const CH > } > > static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, > -CHAR16 *cmdline, UINTN cmdsize, > +VOID *data, UINTN size, UINTN *offset, > CHAR16 **options) > { > -CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; > +CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL; > bool prev_sep = true; > > -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; > -cmdsize -= sizeof(*cmdline), ++cmdline ) > +if ( argc ) > +{ > +cmdline = data + *offset; > +/* EFI_LOAD_OPTION does not supply an image name as first component. > */ > +if ( *offset ) > +*argv++ = NULL; > +} > +else if ( size > sizeof(*cmdline) && !(size % sizeof(*cmdline)) && > + (wmemchr(data, 0, size / sizeof(*cmdline)) == > + data + size - sizeof(*cmdline)) ) > +{ > +*offset = 0; > +cmdline = data; > +} > +else if ( size > sizeof(EFI_LOAD_OPTION) ) > +{ > +const EFI_LOAD_OPTION *elo = data; > +/* The minimum size the buffer needs to be. */ > +size_t elo_min = offsetof(EFI_LOAD_OPTION, Description[1]) + > + elo->FilePathListLength; > + > +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size > elo_min && > + !((size - elo_min) % sizeof(*cmdline)) ) > +{ > +const CHAR16 *desc = elo->Description; > +const CHAR16 *end = wmemchr(desc, 0, > +(size - elo_min) / sizeof(*desc) + > 1); > + > +if ( end ) > +{ > +*offset = elo_min + (end - desc) * sizeof(*desc); > +if ( (size -= *offset) > sizeof(*cmdline) ) > +{ > +cmdline = data + *offset; > +/* Cater for the image name as first component. */ > +++argc; > +} > +} > +} > +} > + > +if ( !cmdline ) > +return 0; > + > +for ( ; size > sizeof(*cmdline) && *cmdline; > +size -= sizeof(*cmdline), ++cmdline ) > { > bool cur_sep = *cmdline == L' ' || *cmdline == L'\t'; > > @@ -1095,15 +1157,17 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY > > if ( use_cfg_file ) > { > +UINTN offset = 0; > + > argc = get_argv(0, NULL, loaded_image->LoadOptions, > -loaded_image->LoadOptionsSize, NULL); > +loaded_image->LoadOptionsSize, &offset, NULL); > if ( argc > 0 && > efi_bs->AllocatePool(EfiLoaderData, >(argc + 1) * sizeof(*argv) + >
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
>>> On 21.05.18 at 18:59, wrote: > After closer inspection the problem is with the following line here: > >>> for ( i = 1; i < argc; ++i ) > > This assumes that argv[0] is the EFI executable filename, which is not > true when EFI_LOAD_OPTION is used. That's why in my v3 patch I had the > "elo_active" variable to determine whether to start the iteration from > 0 or from 1. How about this one then? Jan EFI: add EFI_LOAD_OPTION support When booting Xen via UEFI the Xen config file can contain multiple sections each describing different boot options. It is currently only possible to choose which section to boot with if the buffer contains a string. UEFI provides a different standard to pass optional arguments to an application, and in this patch we make Xen properly parse this buffer, thus making it possible to have separate EFI boot options present for the different config sections. Signed-off-by: Tamas K Lengyel Signed-off-by: Jan Beulich --- v4: Address my own review comments. --- unstable.orig/xen/common/efi/boot.c +++ unstable/xen/common/efi/boot.c @@ -88,6 +88,14 @@ typedef struct _EFI_APPLE_PROPERTIES { EFI_APPLE_PROPERTIES_GETALL GetAll; } EFI_APPLE_PROPERTIES; +typedef struct _EFI_LOAD_OPTION { +UINT32 Attributes; +UINT16 FilePathListLength; +CHAR16 Description[]; +} EFI_LOAD_OPTION; + +#define LOAD_OPTION_ACTIVE 0x0001 + union string { CHAR16 *w; char *s; @@ -275,6 +283,16 @@ static int __init wstrncmp(const CHAR16 return n ? *s1 - *s2 : 0; } +static const CHAR16 *__init wmemchr(const CHAR16 *s, CHAR16 c, UINTN n) +{ +while ( n && *s != c ) +{ +--n; +++s; +} +return n ? s : NULL; +} + static CHAR16 *__init s2w(union string *str) { const char *s = str->s; @@ -374,14 +392,58 @@ static void __init PrintErrMesg(const CH } static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, -CHAR16 *cmdline, UINTN cmdsize, +VOID *data, UINTN size, UINTN *offset, CHAR16 **options) { -CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; +CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL; bool prev_sep = true; -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; -cmdsize -= sizeof(*cmdline), ++cmdline ) +if ( argc ) +{ +cmdline = data + *offset; +/* EFI_LOAD_OPTION does not supply an image name as first component. */ +if ( *offset ) +*argv++ = NULL; +} +else if ( size > sizeof(*cmdline) && !(size % sizeof(*cmdline)) && + (wmemchr(data, 0, size / sizeof(*cmdline)) == + data + size - sizeof(*cmdline)) ) +{ +*offset = 0; +cmdline = data; +} +else if ( size > sizeof(EFI_LOAD_OPTION) ) +{ +const EFI_LOAD_OPTION *elo = data; +/* The minimum size the buffer needs to be. */ +size_t elo_min = offsetof(EFI_LOAD_OPTION, Description[1]) + + elo->FilePathListLength; + +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size > elo_min && + !((size - elo_min) % sizeof(*cmdline)) ) +{ +const CHAR16 *desc = elo->Description; +const CHAR16 *end = wmemchr(desc, 0, +(size - elo_min) / sizeof(*desc) + 1); + +if ( end ) +{ +*offset = elo_min + (end - desc) * sizeof(*desc); +if ( (size -= *offset) > sizeof(*cmdline) ) +{ +cmdline = data + *offset; +/* Cater for the image name as first component. */ +++argc; +} +} +} +} + +if ( !cmdline ) +return 0; + +for ( ; size > sizeof(*cmdline) && *cmdline; +size -= sizeof(*cmdline), ++cmdline ) { bool cur_sep = *cmdline == L' ' || *cmdline == L'\t'; @@ -1095,15 +1157,17 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY if ( use_cfg_file ) { +UINTN offset = 0; + argc = get_argv(0, NULL, loaded_image->LoadOptions, -loaded_image->LoadOptionsSize, NULL); +loaded_image->LoadOptionsSize, &offset, NULL); if ( argc > 0 && efi_bs->AllocatePool(EfiLoaderData, (argc + 1) * sizeof(*argv) + loaded_image->LoadOptionsSize, (void **)&argv) == EFI_SUCCESS ) get_argv(argc, argv, loaded_image->LoadOptions, - loaded_image->LoadOptionsSize, &options); + loaded_image->LoadOptionsSize, &offset, &options); else argc = 0; for ( i = 1; i < argc; ++i ) @@ -1244,6 +1308,15 @@ efi_start(EFI_HANDLE ImageHa
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
>>> On 21.05.18 at 18:59, wrote: > After closer inspection the problem is with the following line here: > >>> for ( i = 1; i < argc; ++i ) > > This assumes that argv[0] is the EFI executable filename, which is not > true when EFI_LOAD_OPTION is used. That's why in my v3 patch I had the > "elo_active" variable to determine whether to start the iteration from > 0 or from 1. Oh, I see. I didn't mean to drop that code of yours, but I also didn't mean to use it as is; I simply forgot that I need to make further changes here. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
On Thu, May 17, 2018 at 11:42 AM, Tamas K Lengyel wrote: > On Thu, May 17, 2018 at 2:03 AM, Jan Beulich wrote: > On 07.02.18 at 17:00, wrote: >>> This patch as-is correctly tells the two possible formats apart. I >>> tested and Xen boots correctly both from the Shell and from the >>> firmware boot menu. I would not like to start addressing hypothetical >>> scenarios that I have no reasonable way to test against. If you are >>> inclined to do that, that's your call but I'll just leave this patch >>> here for now and I hope you would consider merging it. >> >> Would you mind giving the tentative v4 (below) a try? > > Unfortunately this does not seem to work as intended: > > # cat /boot/efi/EFI/xen/xen.cfg > [global] > default=old > > [old] > options=console=vga > kernel=vmlinuz-4.9.0-6-amd64 > root=UUID=19f184db-23a8-42c6-8dfa-67816c822573 ro quiet > ramdisk=initrd.img-4.9.0-6-amd64 > > [new] > options=console=vga,com1 com1=115200,8n1,amt loglvl=all > guest_loglvl=all altp2m=1 > kernel=vmlinuz-4.9.0-6-amd64 > root=UUID=19f184db-23a8-42c6-8dfa-67816c822573 ro quiet > ramdisk=initrd.img-4.9.0-6-amd64 > > > # efibootmgr -v > BootCurrent: 0001 > Timeout: 0 seconds > BootOrder: 0001,,0003,0004,0005,0006,0007 > Boot* Xen > HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x10)/File(\EFI\xen\xen.efi) > Boot0001* Xen altp2m > HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x10)/File(\EFI\xen\xen.efi)n.e.w. > > # xl info > ... > xen_commandline: console=vga > > As you can see boot option 1 (Xen altp2m) was used for booting but Xen > still used the default global option from the config file instead of > the one specified by the OptionalData. > > Tamas > >> >> Jan >> >> EFI: add EFI_LOAD_OPTION support >> >> When booting Xen via UEFI the Xen config file can contain multiple >> sections each describing different boot options. It is currently only >> possible to choose which section to boot with if the buffer contains a >> string. UEFI provides a different standard to pass optional arguments >> to an application, and in this patch we make Xen properly parse this >> buffer, thus making it possible to have separate EFI boot options >> present for the different config sections. >> >> Signed-off-by: Tamas K Lengyel >> Signed-off-by: Jan Beulich >> --- >> v4: Address my own review comments. >> >> --- unstable.orig/xen/common/efi/boot.c >> +++ unstable/xen/common/efi/boot.c >> @@ -88,6 +88,14 @@ typedef struct _EFI_APPLE_PROPERTIES { >> EFI_APPLE_PROPERTIES_GETALL GetAll; >> } EFI_APPLE_PROPERTIES; >> >> +typedef struct _EFI_LOAD_OPTION { >> +UINT32 Attributes; >> +UINT16 FilePathListLength; >> +CHAR16 Description[]; >> +} EFI_LOAD_OPTION; >> + >> +#define LOAD_OPTION_ACTIVE 0x0001 >> + >> union string { >> CHAR16 *w; >> char *s; >> @@ -275,6 +283,16 @@ static int __init wstrncmp(const CHAR16 >> return n ? *s1 - *s2 : 0; >> } >> >> +static const CHAR16 *__init wmemchr(const CHAR16 *s, CHAR16 c, UINTN n) >> +{ >> +while ( n && *s != c ) >> +{ >> +--n; >> +++s; >> +} >> +return n ? s : NULL; >> +} >> + >> static CHAR16 *__init s2w(union string *str) >> { >> const char *s = str->s; >> @@ -374,14 +392,49 @@ static void __init PrintErrMesg(const CH >> } >> >> static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, >> -CHAR16 *cmdline, UINTN cmdsize, >> +VOID *data, UINTN size, UINTN *offset, >> CHAR16 **options) >> { >> -CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; >> +CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = >> NULL; >> bool prev_sep = true; >> >> -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; >> -cmdsize -= sizeof(*cmdline), ++cmdline ) >> +if ( *offset < size ) >> +cmdline = data + *offset; >> +else if ( size > sizeof(*cmdline) && !(size % sizeof(*cmdline)) && >> + (wmemchr(data, 0, size / sizeof(*cmdline)) == >> + data + size - sizeof(*cmdline)) ) >> +{ >> +*offset = 0; >> +cmdline = data; >> +} >> +else if ( size > sizeof(EFI_LOAD_OPTION) ) >> +{ >> +const EFI_LOAD_OPTION *elo = data; >> +/* The minimum size the buffer needs to be. */ >> +size_t elo_min = offsetof(EFI_LOAD_OPTION, Description[1]) + >> + elo->FilePathListLength; >> + >> +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size > elo_min && >> + !((size - elo_min) % sizeof(*cmdline)) ) >> +{ >> +const CHAR16 *desc = elo->Description; >> +const CHAR16 *end = wmemchr(desc, 0, >> +(size - elo_min) / sizeof(*desc) + >> 1); >> + >> +if ( end ) >> +{ >> +*offset = elo_min + (end - desc) * sizeof(*de
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
On Thu, May 17, 2018 at 2:03 AM, Jan Beulich wrote: On 07.02.18 at 17:00, wrote: >> This patch as-is correctly tells the two possible formats apart. I >> tested and Xen boots correctly both from the Shell and from the >> firmware boot menu. I would not like to start addressing hypothetical >> scenarios that I have no reasonable way to test against. If you are >> inclined to do that, that's your call but I'll just leave this patch >> here for now and I hope you would consider merging it. > > Would you mind giving the tentative v4 (below) a try? Unfortunately this does not seem to work as intended: # cat /boot/efi/EFI/xen/xen.cfg [global] default=old [old] options=console=vga kernel=vmlinuz-4.9.0-6-amd64 root=UUID=19f184db-23a8-42c6-8dfa-67816c822573 ro quiet ramdisk=initrd.img-4.9.0-6-amd64 [new] options=console=vga,com1 com1=115200,8n1,amt loglvl=all guest_loglvl=all altp2m=1 kernel=vmlinuz-4.9.0-6-amd64 root=UUID=19f184db-23a8-42c6-8dfa-67816c822573 ro quiet ramdisk=initrd.img-4.9.0-6-amd64 # efibootmgr -v BootCurrent: 0001 Timeout: 0 seconds BootOrder: 0001,,0003,0004,0005,0006,0007 Boot* Xen HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x10)/File(\EFI\xen\xen.efi) Boot0001* Xen altp2m HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x10)/File(\EFI\xen\xen.efi)n.e.w. # xl info ... xen_commandline: console=vga As you can see boot option 1 (Xen altp2m) was used for booting but Xen still used the default global option from the config file instead of the one specified by the OptionalData. Tamas > > Jan > > EFI: add EFI_LOAD_OPTION support > > When booting Xen via UEFI the Xen config file can contain multiple > sections each describing different boot options. It is currently only > possible to choose which section to boot with if the buffer contains a > string. UEFI provides a different standard to pass optional arguments > to an application, and in this patch we make Xen properly parse this > buffer, thus making it possible to have separate EFI boot options > present for the different config sections. > > Signed-off-by: Tamas K Lengyel > Signed-off-by: Jan Beulich > --- > v4: Address my own review comments. > > --- unstable.orig/xen/common/efi/boot.c > +++ unstable/xen/common/efi/boot.c > @@ -88,6 +88,14 @@ typedef struct _EFI_APPLE_PROPERTIES { > EFI_APPLE_PROPERTIES_GETALL GetAll; > } EFI_APPLE_PROPERTIES; > > +typedef struct _EFI_LOAD_OPTION { > +UINT32 Attributes; > +UINT16 FilePathListLength; > +CHAR16 Description[]; > +} EFI_LOAD_OPTION; > + > +#define LOAD_OPTION_ACTIVE 0x0001 > + > union string { > CHAR16 *w; > char *s; > @@ -275,6 +283,16 @@ static int __init wstrncmp(const CHAR16 > return n ? *s1 - *s2 : 0; > } > > +static const CHAR16 *__init wmemchr(const CHAR16 *s, CHAR16 c, UINTN n) > +{ > +while ( n && *s != c ) > +{ > +--n; > +++s; > +} > +return n ? s : NULL; > +} > + > static CHAR16 *__init s2w(union string *str) > { > const char *s = str->s; > @@ -374,14 +392,49 @@ static void __init PrintErrMesg(const CH > } > > static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, > -CHAR16 *cmdline, UINTN cmdsize, > +VOID *data, UINTN size, UINTN *offset, > CHAR16 **options) > { > -CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; > +CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL; > bool prev_sep = true; > > -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; > -cmdsize -= sizeof(*cmdline), ++cmdline ) > +if ( *offset < size ) > +cmdline = data + *offset; > +else if ( size > sizeof(*cmdline) && !(size % sizeof(*cmdline)) && > + (wmemchr(data, 0, size / sizeof(*cmdline)) == > + data + size - sizeof(*cmdline)) ) > +{ > +*offset = 0; > +cmdline = data; > +} > +else if ( size > sizeof(EFI_LOAD_OPTION) ) > +{ > +const EFI_LOAD_OPTION *elo = data; > +/* The minimum size the buffer needs to be. */ > +size_t elo_min = offsetof(EFI_LOAD_OPTION, Description[1]) + > + elo->FilePathListLength; > + > +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size > elo_min && > + !((size - elo_min) % sizeof(*cmdline)) ) > +{ > +const CHAR16 *desc = elo->Description; > +const CHAR16 *end = wmemchr(desc, 0, > +(size - elo_min) / sizeof(*desc) + > 1); > + > +if ( end ) > +{ > +*offset = elo_min + (end - desc) * sizeof(*desc); > +if ( (size -= *offset) > sizeof(*cmdline) ) > +cmdline = data + *offset; > +} > +} > +} > + > +if ( !cmdline ) > +return 0; > + > +for ( ; size > sizeof
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
>>> On 07.02.18 at 17:00, wrote: > This patch as-is correctly tells the two possible formats apart. I > tested and Xen boots correctly both from the Shell and from the > firmware boot menu. I would not like to start addressing hypothetical > scenarios that I have no reasonable way to test against. If you are > inclined to do that, that's your call but I'll just leave this patch > here for now and I hope you would consider merging it. Would you mind giving the tentative v4 (below) a try? Jan EFI: add EFI_LOAD_OPTION support When booting Xen via UEFI the Xen config file can contain multiple sections each describing different boot options. It is currently only possible to choose which section to boot with if the buffer contains a string. UEFI provides a different standard to pass optional arguments to an application, and in this patch we make Xen properly parse this buffer, thus making it possible to have separate EFI boot options present for the different config sections. Signed-off-by: Tamas K Lengyel Signed-off-by: Jan Beulich --- v4: Address my own review comments. --- unstable.orig/xen/common/efi/boot.c +++ unstable/xen/common/efi/boot.c @@ -88,6 +88,14 @@ typedef struct _EFI_APPLE_PROPERTIES { EFI_APPLE_PROPERTIES_GETALL GetAll; } EFI_APPLE_PROPERTIES; +typedef struct _EFI_LOAD_OPTION { +UINT32 Attributes; +UINT16 FilePathListLength; +CHAR16 Description[]; +} EFI_LOAD_OPTION; + +#define LOAD_OPTION_ACTIVE 0x0001 + union string { CHAR16 *w; char *s; @@ -275,6 +283,16 @@ static int __init wstrncmp(const CHAR16 return n ? *s1 - *s2 : 0; } +static const CHAR16 *__init wmemchr(const CHAR16 *s, CHAR16 c, UINTN n) +{ +while ( n && *s != c ) +{ +--n; +++s; +} +return n ? s : NULL; +} + static CHAR16 *__init s2w(union string *str) { const char *s = str->s; @@ -374,14 +392,49 @@ static void __init PrintErrMesg(const CH } static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, -CHAR16 *cmdline, UINTN cmdsize, +VOID *data, UINTN size, UINTN *offset, CHAR16 **options) { -CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; +CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL; bool prev_sep = true; -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; -cmdsize -= sizeof(*cmdline), ++cmdline ) +if ( *offset < size ) +cmdline = data + *offset; +else if ( size > sizeof(*cmdline) && !(size % sizeof(*cmdline)) && + (wmemchr(data, 0, size / sizeof(*cmdline)) == + data + size - sizeof(*cmdline)) ) +{ +*offset = 0; +cmdline = data; +} +else if ( size > sizeof(EFI_LOAD_OPTION) ) +{ +const EFI_LOAD_OPTION *elo = data; +/* The minimum size the buffer needs to be. */ +size_t elo_min = offsetof(EFI_LOAD_OPTION, Description[1]) + + elo->FilePathListLength; + +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size > elo_min && + !((size - elo_min) % sizeof(*cmdline)) ) +{ +const CHAR16 *desc = elo->Description; +const CHAR16 *end = wmemchr(desc, 0, +(size - elo_min) / sizeof(*desc) + 1); + +if ( end ) +{ +*offset = elo_min + (end - desc) * sizeof(*desc); +if ( (size -= *offset) > sizeof(*cmdline) ) +cmdline = data + *offset; +} +} +} + +if ( !cmdline ) +return 0; + +for ( ; size > sizeof(*cmdline) && *cmdline; +size -= sizeof(*cmdline), ++cmdline ) { bool cur_sep = *cmdline == L' ' || *cmdline == L'\t'; @@ -1095,15 +1148,17 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY if ( use_cfg_file ) { +UINTN offset = ~(UINTN)0; + argc = get_argv(0, NULL, loaded_image->LoadOptions, -loaded_image->LoadOptionsSize, NULL); +loaded_image->LoadOptionsSize, &offset, NULL); if ( argc > 0 && efi_bs->AllocatePool(EfiLoaderData, (argc + 1) * sizeof(*argv) + loaded_image->LoadOptionsSize, (void **)&argv) == EFI_SUCCESS ) get_argv(argc, argv, loaded_image->LoadOptions, - loaded_image->LoadOptionsSize, &options); + loaded_image->LoadOptionsSize, &offset, &options); else argc = 0; for ( i = 1; i < argc; ++i ) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
On Tue, Mar 13, 2018 at 1:47 AM, Jan Beulich wrote: On 12.03.18 at 16:00, wrote: >> Patch ping. Jan, I would like to touch base once more to see if we can >> get this patch included in 4.11. The patch as-is correctly tells the >> difference between buffers provided by both an EFI shell or by the >> firmware as an EFI_LOAD_OPTION. > > Well, I've stated my opinion before, and I'm intending to provide a > replacement patch along the outline I had provided, but I didn't get > around to actually do so yet. > Thanks for the update. I would be happy to wait for your patch to rebase this on top of in still needed. Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
>>> On 12.03.18 at 16:00, wrote: > Patch ping. Jan, I would like to touch base once more to see if we can > get this patch included in 4.11. The patch as-is correctly tells the > difference between buffers provided by both an EFI shell or by the > firmware as an EFI_LOAD_OPTION. Well, I've stated my opinion before, and I'm intending to provide a replacement patch along the outline I had provided, but I didn't get around to actually do so yet. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
Patch ping. Jan, I would like to touch base once more to see if we can get this patch included in 4.11. The patch as-is correctly tells the difference between buffers provided by both an EFI shell or by the firmware as an EFI_LOAD_OPTION. Thanks, Tamas On Wed, Feb 7, 2018 at 9:00 AM, Tamas K Lengyel wrote: > On Mon, Jan 29, 2018 at 2:18 AM, Jan Beulich wrote: > On 26.01.18 at 18:35, wrote: >>> On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich wrote: >>> On 23.01.18 at 01:21, wrote: > @@ -375,12 +385,39 @@ static void __init PrintErrMesg(const CHAR16 *mesg, > EFI_STATUS ErrCode) > > static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, > CHAR16 *cmdline, UINTN cmdsize, > -CHAR16 **options) > +CHAR16 **options, bool *elo_active) > { > CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; > bool prev_sep = true; > > -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; > +if ( cmdsize > sizeof(EFI_LOAD_OPTION) && > + *(CHAR16 *)((void *)cmdline + cmdsize - sizeof(*cmdline)) != > L'\0' ) This is too lax - you should check whether the nul at that position indeed is the _first_ one. >>> >>> IMHO that check you suggest has nothing to do with EFI_LOAD_OPTION >>> support. That's sanity checking a command line buffer. It could >>> certainly be done, but I would say that belongs in a separate patch. >>> This check currently as is distinguishes an EFI_LOAD_OPTION from a >>> well-formed command line buffer. If the command line buffer has >>> multiple '\0' in it, that's a separate problem. >> >> You could view it as a separate problem if there was a non-heuristic >> way of distinguishing the formats. >> > +{ > +const EFI_LOAD_OPTION *elo = (const EFI_LOAD_OPTION *)cmdline; > + > +/* The absolute minimum the size of the buffer it needs to be */ > +size_t size_check = offsetof(EFI_LOAD_OPTION, Description[1]) + > +elo->FilePathListLength; > + > +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size_check < > cmdsize ) > +{ > +const CHAR16 *desc = elo->Description; > +size_t desc_length = 0; > + > +/* Find Description string length in its possible space */ > +while ( desc_length < cmdsize - size_check && *desc++ != > L'\0') > +desc_length += sizeof(*desc); > + > +if ( size_check + desc_length < cmdsize ) > +{ > +*elo_active = true; > +cmdline = (void *)cmdline + size_check + desc_length; > +cmdsize = cmdsize - size_check - desc_length; > +} > +} I can't help thinking that this is broken: What if you have a structure with the LOAD_OPTION_ACTIVE bit clear (leaving aside the fact that I'm not sure the meaning of the flag is what you use it for here)? That's still not to be taken as a plain command line then. >>> >>> Keep in mind that currently everything is being parsed as a plain >>> command line. So that's the default behavior. All I'm doing in this >>> patch is falling back on the default behavior if is determined that we >>> are not dealing with a well-formed EFI_LOAD_OPTION. Doing sanity >>> checking on arbitrary buffers that may end up being passed here by >>> buggy shells or buggy firmware or whatnot is beyond the scope of what >>> I'm looking to accomplish. >> >> As per above - this isn't sanity checking. It is a heuristic to tell apart >> the two possible formats. Without knowing what other formats there >> might be, there's no way the checking you do is going to be >> meaningfully more safe than the alternative I'm suggesting. Being >> given a binary blob, just simply have no way of telling its format >> without sideband information. >> > > This patch as-is correctly tells the two possible formats apart. I > tested and Xen boots correctly both from the Shell and from the > firmware boot menu. I would not like to start addressing hypothetical > scenarios that I have no reasonable way to test against. If you are > inclined to do that, that's your call but I'll just leave this patch > here for now and I hope you would consider merging it. > > Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
On Mon, Jan 29, 2018 at 2:18 AM, Jan Beulich wrote: On 26.01.18 at 18:35, wrote: >> On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich wrote: >> On 23.01.18 at 01:21, wrote: @@ -375,12 +385,39 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode) static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, CHAR16 *cmdline, UINTN cmdsize, -CHAR16 **options) +CHAR16 **options, bool *elo_active) { CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; bool prev_sep = true; -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; +if ( cmdsize > sizeof(EFI_LOAD_OPTION) && + *(CHAR16 *)((void *)cmdline + cmdsize - sizeof(*cmdline)) != L'\0' ) >>> >>> This is too lax - you should check whether the nul at that position >>> indeed is the _first_ one. >> >> IMHO that check you suggest has nothing to do with EFI_LOAD_OPTION >> support. That's sanity checking a command line buffer. It could >> certainly be done, but I would say that belongs in a separate patch. >> This check currently as is distinguishes an EFI_LOAD_OPTION from a >> well-formed command line buffer. If the command line buffer has >> multiple '\0' in it, that's a separate problem. > > You could view it as a separate problem if there was a non-heuristic > way of distinguishing the formats. > +{ +const EFI_LOAD_OPTION *elo = (const EFI_LOAD_OPTION *)cmdline; + +/* The absolute minimum the size of the buffer it needs to be */ +size_t size_check = offsetof(EFI_LOAD_OPTION, Description[1]) + +elo->FilePathListLength; + +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size_check < cmdsize ) +{ +const CHAR16 *desc = elo->Description; +size_t desc_length = 0; + +/* Find Description string length in its possible space */ +while ( desc_length < cmdsize - size_check && *desc++ != L'\0') +desc_length += sizeof(*desc); + +if ( size_check + desc_length < cmdsize ) +{ +*elo_active = true; +cmdline = (void *)cmdline + size_check + desc_length; +cmdsize = cmdsize - size_check - desc_length; +} +} >>> >>> I can't help thinking that this is broken: What if you have a structure >>> with the LOAD_OPTION_ACTIVE bit clear (leaving aside the fact that >>> I'm not sure the meaning of the flag is what you use it for here)? >>> That's still not to be taken as a plain command line then. >> >> Keep in mind that currently everything is being parsed as a plain >> command line. So that's the default behavior. All I'm doing in this >> patch is falling back on the default behavior if is determined that we >> are not dealing with a well-formed EFI_LOAD_OPTION. Doing sanity >> checking on arbitrary buffers that may end up being passed here by >> buggy shells or buggy firmware or whatnot is beyond the scope of what >> I'm looking to accomplish. > > As per above - this isn't sanity checking. It is a heuristic to tell apart > the two possible formats. Without knowing what other formats there > might be, there's no way the checking you do is going to be > meaningfully more safe than the alternative I'm suggesting. Being > given a binary blob, just simply have no way of telling its format > without sideband information. > This patch as-is correctly tells the two possible formats apart. I tested and Xen boots correctly both from the Shell and from the firmware boot menu. I would not like to start addressing hypothetical scenarios that I have no reasonable way to test against. If you are inclined to do that, that's your call but I'll just leave this patch here for now and I hope you would consider merging it. Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
>>> On 26.01.18 at 18:35, wrote: > On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich wrote: > On 23.01.18 at 01:21, wrote: >>> @@ -375,12 +385,39 @@ static void __init PrintErrMesg(const CHAR16 *mesg, >>> EFI_STATUS ErrCode) >>> >>> static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, >>> CHAR16 *cmdline, UINTN cmdsize, >>> -CHAR16 **options) >>> +CHAR16 **options, bool *elo_active) >>> { >>> CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; >>> bool prev_sep = true; >>> >>> -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; >>> +if ( cmdsize > sizeof(EFI_LOAD_OPTION) && >>> + *(CHAR16 *)((void *)cmdline + cmdsize - sizeof(*cmdline)) != >>> L'\0' ) >> >> This is too lax - you should check whether the nul at that position >> indeed is the _first_ one. > > IMHO that check you suggest has nothing to do with EFI_LOAD_OPTION > support. That's sanity checking a command line buffer. It could > certainly be done, but I would say that belongs in a separate patch. > This check currently as is distinguishes an EFI_LOAD_OPTION from a > well-formed command line buffer. If the command line buffer has > multiple '\0' in it, that's a separate problem. You could view it as a separate problem if there was a non-heuristic way of distinguishing the formats. >>> +{ >>> +const EFI_LOAD_OPTION *elo = (const EFI_LOAD_OPTION *)cmdline; >>> + >>> +/* The absolute minimum the size of the buffer it needs to be */ >>> +size_t size_check = offsetof(EFI_LOAD_OPTION, Description[1]) + >>> +elo->FilePathListLength; >>> + >>> +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size_check < >>> cmdsize ) >>> +{ >>> +const CHAR16 *desc = elo->Description; >>> +size_t desc_length = 0; >>> + >>> +/* Find Description string length in its possible space */ >>> +while ( desc_length < cmdsize - size_check && *desc++ != L'\0') >>> +desc_length += sizeof(*desc); >>> + >>> +if ( size_check + desc_length < cmdsize ) >>> +{ >>> +*elo_active = true; >>> +cmdline = (void *)cmdline + size_check + desc_length; >>> +cmdsize = cmdsize - size_check - desc_length; >>> +} >>> +} >> >> I can't help thinking that this is broken: What if you have a structure >> with the LOAD_OPTION_ACTIVE bit clear (leaving aside the fact that >> I'm not sure the meaning of the flag is what you use it for here)? >> That's still not to be taken as a plain command line then. > > Keep in mind that currently everything is being parsed as a plain > command line. So that's the default behavior. All I'm doing in this > patch is falling back on the default behavior if is determined that we > are not dealing with a well-formed EFI_LOAD_OPTION. Doing sanity > checking on arbitrary buffers that may end up being passed here by > buggy shells or buggy firmware or whatnot is beyond the scope of what > I'm looking to accomplish. As per above - this isn't sanity checking. It is a heuristic to tell apart the two possible formats. Without knowing what other formats there might be, there's no way the checking you do is going to be meaningfully more safe than the alternative I'm suggesting. Being given a binary blob, just simply have no way of telling its format without sideband information. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich wrote: On 23.01.18 at 01:21, wrote: >> @@ -88,6 +88,16 @@ typedef struct _EFI_APPLE_PROPERTIES { >> EFI_APPLE_PROPERTIES_GETALL GetAll; >> } EFI_APPLE_PROPERTIES; >> >> +typedef struct _EFI_LOAD_OPTION { >> +UINT32 Attributes; >> +UINT16 FilePathListLength; >> +CHAR16 Description[]; >> +} EFI_LOAD_OPTION; >> + >> +#define LOAD_OPTION_ACTIVE 0x0001 >> +#define LOAD_OPTION_FORCE_RECONNECT 0x0002 >> +#define LOAD_OPTION_HIDDEN 0x0008 > > If you add things we don't really need, please add everything the > current doc version specifies (i.e. also LOAD_OPTION_CATEGORY*). > But I'd also be fine if you kept only LOAD_OPTION_ACTIVE. Declaring only what is used would work fine for me. > >> @@ -375,12 +385,39 @@ static void __init PrintErrMesg(const CHAR16 *mesg, >> EFI_STATUS ErrCode) >> >> static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, >> CHAR16 *cmdline, UINTN cmdsize, >> -CHAR16 **options) >> +CHAR16 **options, bool *elo_active) >> { >> CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; >> bool prev_sep = true; >> >> -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; >> +if ( cmdsize > sizeof(EFI_LOAD_OPTION) && >> + *(CHAR16 *)((void *)cmdline + cmdsize - sizeof(*cmdline)) != L'\0' >> ) > > This is too lax - you should check whether the nul at that position > indeed is the _first_ one. IMHO that check you suggest has nothing to do with EFI_LOAD_OPTION support. That's sanity checking a command line buffer. It could certainly be done, but I would say that belongs in a separate patch. This check currently as is distinguishes an EFI_LOAD_OPTION from a well-formed command line buffer. If the command line buffer has multiple '\0' in it, that's a separate problem. > >> +{ >> +const EFI_LOAD_OPTION *elo = (const EFI_LOAD_OPTION *)cmdline; >> + >> +/* The absolute minimum the size of the buffer it needs to be */ >> +size_t size_check = offsetof(EFI_LOAD_OPTION, Description[1]) + >> +elo->FilePathListLength; >> + >> +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size_check < cmdsize >> ) >> +{ >> +const CHAR16 *desc = elo->Description; >> +size_t desc_length = 0; >> + >> +/* Find Description string length in its possible space */ >> +while ( desc_length < cmdsize - size_check && *desc++ != L'\0') >> +desc_length += sizeof(*desc); >> + >> +if ( size_check + desc_length < cmdsize ) >> +{ >> +*elo_active = true; >> +cmdline = (void *)cmdline + size_check + desc_length; >> +cmdsize = cmdsize - size_check - desc_length; >> +} >> +} > > I can't help thinking that this is broken: What if you have a structure > with the LOAD_OPTION_ACTIVE bit clear (leaving aside the fact that > I'm not sure the meaning of the flag is what you use it for here)? > That's still not to be taken as a plain command line then. Keep in mind that currently everything is being parsed as a plain command line. So that's the default behavior. All I'm doing in this patch is falling back on the default behavior if is determined that we are not dealing with a well-formed EFI_LOAD_OPTION. Doing sanity checking on arbitrary buffers that may end up being passed here by buggy shells or buggy firmware or whatnot is beyond the scope of what I'm looking to accomplish. Tamas ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCHv3] xen: Add EFI_LOAD_OPTION support
>>> On 23.01.18 at 01:21, wrote: > @@ -88,6 +88,16 @@ typedef struct _EFI_APPLE_PROPERTIES { > EFI_APPLE_PROPERTIES_GETALL GetAll; > } EFI_APPLE_PROPERTIES; > > +typedef struct _EFI_LOAD_OPTION { > +UINT32 Attributes; > +UINT16 FilePathListLength; > +CHAR16 Description[]; > +} EFI_LOAD_OPTION; > + > +#define LOAD_OPTION_ACTIVE 0x0001 > +#define LOAD_OPTION_FORCE_RECONNECT 0x0002 > +#define LOAD_OPTION_HIDDEN 0x0008 If you add things we don't really need, please add everything the current doc version specifies (i.e. also LOAD_OPTION_CATEGORY*). But I'd also be fine if you kept only LOAD_OPTION_ACTIVE. > @@ -375,12 +385,39 @@ static void __init PrintErrMesg(const CHAR16 *mesg, > EFI_STATUS ErrCode) > > static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv, > CHAR16 *cmdline, UINTN cmdsize, > -CHAR16 **options) > +CHAR16 **options, bool *elo_active) > { > CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL; > bool prev_sep = true; > > -for ( ; cmdsize > sizeof(*cmdline) && *cmdline; > +if ( cmdsize > sizeof(EFI_LOAD_OPTION) && > + *(CHAR16 *)((void *)cmdline + cmdsize - sizeof(*cmdline)) != L'\0' ) This is too lax - you should check whether the nul at that position indeed is the _first_ one. > +{ > +const EFI_LOAD_OPTION *elo = (const EFI_LOAD_OPTION *)cmdline; > + > +/* The absolute minimum the size of the buffer it needs to be */ > +size_t size_check = offsetof(EFI_LOAD_OPTION, Description[1]) + > +elo->FilePathListLength; > + > +if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size_check < cmdsize ) > +{ > +const CHAR16 *desc = elo->Description; > +size_t desc_length = 0; > + > +/* Find Description string length in its possible space */ > +while ( desc_length < cmdsize - size_check && *desc++ != L'\0') > +desc_length += sizeof(*desc); > + > +if ( size_check + desc_length < cmdsize ) > +{ > +*elo_active = true; > +cmdline = (void *)cmdline + size_check + desc_length; > +cmdsize = cmdsize - size_check - desc_length; > +} > +} I can't help thinking that this is broken: What if you have a structure with the LOAD_OPTION_ACTIVE bit clear (leaving aside the fact that I'm not sure the meaning of the flag is what you use it for here)? That's still not to be taken as a plain command line then. As I thought I had expressed earlier, you really _only_ want the (improved) earlier check. If it's a load option structure, you'll find a nul at the end of the description, which lives earlier than the indicated overall size, since OptionalData is specified to be on non-zero length (or else a NULL pointer is supposedly passed to us; maybe they actually mean a nul character, but then its size would again be non-zero). Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel