[PATCHv3] xen: Add EFI_LOAD_OPTION support

[PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel]

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 <lengyelt@ainfosec.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: openxt@googlegroups.com

v3: simplify sanity checking logic
v2: move EFI_LOAD_OPTION definition into file that uses it
    add more sanity checks to validate the buffer
---
 xen/common/efi/boot.c | 49 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 469bf980cc..3537fe9588 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -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              0x00000001
+#define LOAD_OPTION_FORCE_RECONNECT     0x00000002
+#define LOAD_OPTION_HIDDEN              0x00000008
+
 union string {
     CHAR16 *w;
     char *s;
@@ -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' )
+    {
+        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;
+            }
+        }
+    }
+
+    for ( ; cmdsize >= sizeof(*cmdline) && *cmdline;
             cmdsize -= sizeof(*cmdline), ++cmdline )
     {
         bool cur_sep = *cmdline == L' ' || *cmdline == L'\t';
@@ -1071,7 +1108,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
     union string section = { NULL }, name;
-    bool base_video = false;
+    bool base_video = false, elo_active = false;
     char *option_str;
     bool use_cfg_file;
 
@@ -1096,17 +1133,17 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     if ( use_cfg_file )
     {
         argc = get_argv(0, NULL, loaded_image->LoadOptions,
-                        loaded_image->LoadOptionsSize, NULL);
+                        loaded_image->LoadOptionsSize, NULL, &elo_active);
         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, &options, &elo_active);
         else
             argc = 0;
-        for ( i = 1; i < argc; ++i )
+        for ( i = !elo_active; i < argc; ++i )
         {
             CHAR16 *ptr = argv[i];
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Jan Beulich]

>>> On 23.01.18 at 01:21, <tamas@tklengyel.com> 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              0x00000001
> +#define LOAD_OPTION_FORCE_RECONNECT     0x00000002
> +#define LOAD_OPTION_HIDDEN              0x00000008

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

Re: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel]

On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 23.01.18 at 01:21, <tamas@tklengyel.com> 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              0x00000001
>> +#define LOAD_OPTION_FORCE_RECONNECT     0x00000002
>> +#define LOAD_OPTION_HIDDEN              0x00000008
>
> 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: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Jan Beulich]

>>> On 26.01.18 at 18:35, <tamas@tklengyel.com> wrote:
> On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 23.01.18 at 01:21, <tamas@tklengyel.com> 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: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel]

On Mon, Jan 29, 2018 at 2:18 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.01.18 at 18:35, <tamas@tklengyel.com> wrote:
>> On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 23.01.18 at 01:21, <tamas@tklengyel.com> 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: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel]

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 <tamas@tklengyel.com> wrote:
> On Mon, Jan 29, 2018 at 2:18 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 26.01.18 at 18:35, <tamas@tklengyel.com> wrote:
>>> On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 23.01.18 at 01:21, <tamas@tklengyel.com> 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: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Jan Beulich]

>>> On 12.03.18 at 16:00, <tamas@tklengyel.com> 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: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel]

On Tue, Mar 13, 2018 at 1:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 12.03.18 at 16:00, <tamas@tklengyel.com> 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: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Jan Beulich]

>>> On 07.02.18 at 17:00, <tamas@tklengyel.com> 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 <tamas@tklengyel.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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              0x00000001
+
 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: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel]

On Thu, May 17, 2018 at 2:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 07.02.18 at 17:00, <tamas@tklengyel.com> 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,0000,0003,0004,0005,0006,0007
Boot0000* Xen
HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x100000)/File(\EFI\xen\xen.efi)
Boot0001* Xen altp2m
HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x100000)/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 <tamas@tklengyel.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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              0x00000001
> +
>  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: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel]

On Thu, May 17, 2018 at 11:42 AM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> On Thu, May 17, 2018 at 2:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 07.02.18 at 17:00, <tamas@tklengyel.com> 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,0000,0003,0004,0005,0006,0007
> Boot0000* Xen
> HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x100000)/File(\EFI\xen\xen.efi)
> Boot0001* Xen altp2m
> HD(1,GPT,ffc5e29b-fa57-483d-a5de-0053f87abdc4,0x800,0x100000)/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 <tamas@tklengyel.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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              0x00000001
>> +
>>  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;

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.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Jan Beulich]

>>> On 21.05.18 at 18:59, <tamas@tklengyel.com> 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: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Jan Beulich]

>>> On 21.05.18 at 18:59, <tamas@tklengyel.com> 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 <tamas@tklengyel.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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              0x00000001
+
 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 ImageHandle, EFI_SY
             efi_bs->FreePool(name.w);
         }
 
+        if ( argc && !*argv )
+        {
+            EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
+                                                       &file_name);
+
+            handle->Close(handle);
+            *argv = file_name;
+        }
+
         name.s = get_value(&cfg, section.s, "options");
         efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s);
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel]

On Tue, May 22, 2018 at 6:24 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 21.05.18 at 18:59, <tamas@tklengyel.com> 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 <tamas@tklengyel.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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              0x00000001
> +
>  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 ImageHandle, EFI_SY
>              efi_bs->FreePool(name.w);
>          }
>
> +        if ( argc && !*argv )
> +        {
> +            EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
> +                                                       &file_name);
> +
> +            handle->Close(handle);
> +            *argv = file_name;
> +        }
> +
>          name.s = get_value(&cfg, section.s, "options");
>          efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s);
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel