subject:"\[Xen\-devel\] \[PATCH\] xen\: Add EFI_LOAD

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-04 Thread Jan Beulich

>>> On 04.01.18 at 17:35,  wrote:
> On Thu, Jan 4, 2018 at 9:25 AM, Jan Beulich  wrote:
> On 04.01.18 at 17:16,  wrote:
>>> On Thu, Jan 4, 2018 at 8:00 AM, Jan Beulich  wrote:
>>> On 04.01.18 at 15:39,  wrote:
> On Thu, Jan 4, 2018 at 3:43 AM, Jan Beulich  wrote:
>> Just looking at the low bit of the first
>> byte before assuming this could be a load option structure,
>> however, is too weak a check for my taste.
>
> There is more done in this patch then just checking the low bit of the
> Attributes field: calculating where the OptionalData should be if this
> is indeed an EFI_LOAD_OPTION and then sanity checking if that address
> is sane or not. This requires the buffer to have a layout that at
> least parses properly as an EFI_LOAD_OPTION, hence it likely is one..

 But that calculation is not doing all the checking that would be
 needed to be reasonably safe. In particular, you mustn't call
 wstrlen() when you don't know whether all of the supposed
 string is actually inside the given memory block size. And I'm
 also not entirely certain whether caring of overflow in only one
 (late) place is sufficient.
>>>
>>> Fair enough. How about if we check whether the supposed string start
>>> spot is within the buffer and then use wstrnlen in place such that it
>>> can't go past the buffer? Would that be enough sanity checking?
>>
>> That should be enough, yes. Do we have wstrnlen() though?
>> Rather than introducing it, open-coding the search for the nul
>> terminator may be more appropriate in a case like this one.
> 
> We don't have it currently. I would prefer introducing it as a
> separate function but if you feel strongly about it being implemented
> in-place, I can do that.

The main reason I wouldn't want it to be introduced in this case
is that it's not strictly that function that's needed. wmemchr()
or wstrchr() could both be used as well (and perhaps a few more),
so which one in particular to use to avoid the open coding I'd like
to decide at the point when another user of any of the options
would appear. That way we don't end up with two functions used
just once. But then again I don't feel _really_ strongly about this.

Jan


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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-04 Thread Tamas K Lengyel

On Thu, Jan 4, 2018 at 9:25 AM, Jan Beulich  wrote:
 On 04.01.18 at 17:16,  wrote:
>> On Thu, Jan 4, 2018 at 8:00 AM, Jan Beulich  wrote:
>> On 04.01.18 at 15:39,  wrote:
 On Thu, Jan 4, 2018 at 3:43 AM, Jan Beulich  wrote:
> Just looking at the low bit of the first
> byte before assuming this could be a load option structure,
> however, is too weak a check for my taste.

 There is more done in this patch then just checking the low bit of the
 Attributes field: calculating where the OptionalData should be if this
 is indeed an EFI_LOAD_OPTION and then sanity checking if that address
 is sane or not. This requires the buffer to have a layout that at
 least parses properly as an EFI_LOAD_OPTION, hence it likely is one..
>>>
>>> But that calculation is not doing all the checking that would be
>>> needed to be reasonably safe. In particular, you mustn't call
>>> wstrlen() when you don't know whether all of the supposed
>>> string is actually inside the given memory block size. And I'm
>>> also not entirely certain whether caring of overflow in only one
>>> (late) place is sufficient.
>>
>> Fair enough. How about if we check whether the supposed string start
>> spot is within the buffer and then use wstrnlen in place such that it
>> can't go past the buffer? Would that be enough sanity checking?
>
> That should be enough, yes. Do we have wstrnlen() though?
> Rather than introducing it, open-coding the search for the nul
> terminator may be more appropriate in a case like this one.

We don't have it currently. I would prefer introducing it as a
separate function but if you feel strongly about it being implemented
in-place, I can do that.

Tamas

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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-04 Thread Jan Beulich

>>> On 04.01.18 at 17:16,  wrote:
> On Thu, Jan 4, 2018 at 8:00 AM, Jan Beulich  wrote:
> On 04.01.18 at 15:39,  wrote:
>>> On Thu, Jan 4, 2018 at 3:43 AM, Jan Beulich  wrote:
 Just looking at the low bit of the first
 byte before assuming this could be a load option structure,
 however, is too weak a check for my taste.
>>>
>>> There is more done in this patch then just checking the low bit of the
>>> Attributes field: calculating where the OptionalData should be if this
>>> is indeed an EFI_LOAD_OPTION and then sanity checking if that address
>>> is sane or not. This requires the buffer to have a layout that at
>>> least parses properly as an EFI_LOAD_OPTION, hence it likely is one..
>>
>> But that calculation is not doing all the checking that would be
>> needed to be reasonably safe. In particular, you mustn't call
>> wstrlen() when you don't know whether all of the supposed
>> string is actually inside the given memory block size. And I'm
>> also not entirely certain whether caring of overflow in only one
>> (late) place is sufficient.
> 
> Fair enough. How about if we check whether the supposed string start
> spot is within the buffer and then use wstrnlen in place such that it
> can't go past the buffer? Would that be enough sanity checking?

That should be enough, yes. Do we have wstrnlen() though?
Rather than introducing it, open-coding the search for the nul
terminator may be more appropriate in a case like this one.

Jan


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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-04 Thread Tamas K Lengyel

On Thu, Jan 4, 2018 at 8:00 AM, Jan Beulich  wrote:
 On 04.01.18 at 15:39,  wrote:
>> On Thu, Jan 4, 2018 at 3:43 AM, Jan Beulich  wrote:
>>> Just looking at the low bit of the first
>>> byte before assuming this could be a load option structure,
>>> however, is too weak a check for my taste.
>>
>> There is more done in this patch then just checking the low bit of the
>> Attributes field: calculating where the OptionalData should be if this
>> is indeed an EFI_LOAD_OPTION and then sanity checking if that address
>> is sane or not. This requires the buffer to have a layout that at
>> least parses properly as an EFI_LOAD_OPTION, hence it likely is one..
>
> But that calculation is not doing all the checking that would be
> needed to be reasonably safe. In particular, you mustn't call
> wstrlen() when you don't know whether all of the supposed
> string is actually inside the given memory block size. And I'm
> also not entirely certain whether caring of overflow in only one
> (late) place is sufficient.

Fair enough. How about if we check whether the supposed string start
spot is within the buffer and then use wstrnlen in place such that it
can't go past the buffer? Would that be enough sanity checking?

Tamas

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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-04 Thread Jan Beulich

>>> On 04.01.18 at 15:39,  wrote:
> On Thu, Jan 4, 2018 at 3:43 AM, Jan Beulich  wrote:
>> Just looking at the low bit of the first
>> byte before assuming this could be a load option structure,
>> however, is too weak a check for my taste.
> 
> There is more done in this patch then just checking the low bit of the
> Attributes field: calculating where the OptionalData should be if this
> is indeed an EFI_LOAD_OPTION and then sanity checking if that address
> is sane or not. This requires the buffer to have a layout that at
> least parses properly as an EFI_LOAD_OPTION, hence it likely is one..

But that calculation is not doing all the checking that would be
needed to be reasonably safe. In particular, you mustn't call
wstrlen() when you don't know whether all of the supposed
string is actually inside the given memory block size. And I'm
also not entirely certain whether caring of overflow in only one
(late) place is sufficient.

Btw, looking at the code again, I see two more cosmetic things
for you to change: Please use sizeof(CHAR16) both in place of
sizeof(L'\0') and instead of the literal 2 there.

Jan

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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-04 Thread Tamas K Lengyel

On Thu, Jan 4, 2018 at 3:43 AM, Jan Beulich  wrote:
 On 03.01.18 at 17:53,  wrote:
>> On Wed, Jan 3, 2018 at 9:36 AM, Jan Beulich  wrote:
>> On 03.01.18 at 17:04,  wrote:
 On Wed, Jan 3, 2018 at 4:20 AM, Jan Beulich  wrote:
 On 02.01.18 at 16:56,  wrote:
>> +if ( elo->Attributes & LOAD_OPTION_ACTIVE )
>
> Without any other (earlier) check, how can you reliably tell this
> being a pointer to EFI_LOAD_OPTION from it being the
> currently used one to CHAR16? Is the distinction perhaps UEFI
> version dependent? In the 2.3 spec I can't find any information
> on the layout of what ->LoadOptions points to.

 AFAICT there is no clear cut way to distinguish what is being passed
 here. When launched via UEFI Xen receives the EFI_LOAD_OPTION buffer
 here already, which it tries to parse as a string and fails. Take a
 look at the same problem in the shim:
 https://github.com/rhboot/shim/blob/master/shim.c#L2501. If there is a
 clearer way to distinguish what is being passed here or more checks
 that can be done I would be open for suggestions.
>>>
>>> Well, first of all the code you point to tells me that this situation is
>>> indeed as bad as it can be. However, the code also shows you
>>> some heuristics you could re-use.
>>
>> From what I've seen the only heuristic we could copy is counting ucs2
>> strings in the buffer. I wasn't entirely convinced that it's necessary
>> and rather went with the "if it looks like a duck, swims like a duck,
>> and quacks like a duck, then it probably is a duck" test. What are the
>> chances we could encounter a string that also properly parses as an
>> EFI_LOAD_OPTION buffer passing the checks in place in this patch?
>> Anyway, if the ucs2 string counting is something we also want to
>> utilize, I have nothing against it.
>
> Actually that string counting seemed rather fragile, but maybe I
> didn't fully understand it.

That was my impression of it too.

> Just looking at the low bit of the first
> byte before assuming this could be a load option structure,
> however, is too weak a check for my taste.

There is more done in this patch then just checking the low bit of the
Attributes field: calculating where the OptionalData should be if this
is indeed an EFI_LOAD_OPTION and then sanity checking if that address
is sane or not. This requires the buffer to have a layout that at
least parses properly as an EFI_LOAD_OPTION, hence it likely is one..

Tamas

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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-04 Thread Jan Beulich

>>> On 03.01.18 at 17:53,  wrote:
> On Wed, Jan 3, 2018 at 9:36 AM, Jan Beulich  wrote:
> On 03.01.18 at 17:04,  wrote:
>>> On Wed, Jan 3, 2018 at 4:20 AM, Jan Beulich  wrote:
>>> On 02.01.18 at 16:56,  wrote:
> +if ( elo->Attributes & LOAD_OPTION_ACTIVE )

 Without any other (earlier) check, how can you reliably tell this
 being a pointer to EFI_LOAD_OPTION from it being the
 currently used one to CHAR16? Is the distinction perhaps UEFI
 version dependent? In the 2.3 spec I can't find any information
 on the layout of what ->LoadOptions points to.
>>>
>>> AFAICT there is no clear cut way to distinguish what is being passed
>>> here. When launched via UEFI Xen receives the EFI_LOAD_OPTION buffer
>>> here already, which it tries to parse as a string and fails. Take a
>>> look at the same problem in the shim:
>>> https://github.com/rhboot/shim/blob/master/shim.c#L2501. If there is a
>>> clearer way to distinguish what is being passed here or more checks
>>> that can be done I would be open for suggestions.
>>
>> Well, first of all the code you point to tells me that this situation is
>> indeed as bad as it can be. However, the code also shows you
>> some heuristics you could re-use.
> 
> From what I've seen the only heuristic we could copy is counting ucs2
> strings in the buffer. I wasn't entirely convinced that it's necessary
> and rather went with the "if it looks like a duck, swims like a duck,
> and quacks like a duck, then it probably is a duck" test. What are the
> chances we could encounter a string that also properly parses as an
> EFI_LOAD_OPTION buffer passing the checks in place in this patch?
> Anyway, if the ucs2 string counting is something we also want to
> utilize, I have nothing against it.

Actually that string counting seemed rather fragile, but maybe I
didn't fully understand it. Just looking at the low bit of the first
byte before assuming this could be a load option structure,
however, is too weak a check for my taste.

Jan


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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-03 Thread Tamas K Lengyel

On Wed, Jan 3, 2018 at 9:36 AM, Jan Beulich  wrote:
 On 03.01.18 at 17:04,  wrote:
>> On Wed, Jan 3, 2018 at 4:20 AM, Jan Beulich  wrote:
>> On 02.01.18 at 16:56,  wrote:
 +if ( elo->Attributes & LOAD_OPTION_ACTIVE )
>>>
>>> Without any other (earlier) check, how can you reliably tell this
>>> being a pointer to EFI_LOAD_OPTION from it being the
>>> currently used one to CHAR16? Is the distinction perhaps UEFI
>>> version dependent? In the 2.3 spec I can't find any information
>>> on the layout of what ->LoadOptions points to.
>>
>> AFAICT there is no clear cut way to distinguish what is being passed
>> here. When launched via UEFI Xen receives the EFI_LOAD_OPTION buffer
>> here already, which it tries to parse as a string and fails. Take a
>> look at the same problem in the shim:
>> https://github.com/rhboot/shim/blob/master/shim.c#L2501. If there is a
>> clearer way to distinguish what is being passed here or more checks
>> that can be done I would be open for suggestions.
>
> Well, first of all the code you point to tells me that this situation is
> indeed as bad as it can be. However, the code also shows you
> some heuristics you could re-use.

From what I've seen the only heuristic we could copy is counting ucs2
strings in the buffer. I wasn't entirely convinced that it's necessary
and rather went with the "if it looks like a duck, swims like a duck,
and quacks like a duck, then it probably is a duck" test. What are the
chances we could encounter a string that also properly parses as an
EFI_LOAD_OPTION buffer passing the checks in place in this patch?
Anyway, if the ucs2 string counting is something we also want to
utilize, I have nothing against it.

> What they don't seem to utilize
> is the fact that as long as there are few enough bits defined for
> the Attributes field, checking that one for being a printable
> character (and the upper 16 bits to be non-null) might give a good
> indication what form we're dealing with. In fact it might be that
> when it's a string, the first character is always '\', but I'm afraid
> that's not written down anywhere.
>

I don't think that's true, when I printed this buffer when launched
through the shell (after entering the xen folder on the ESP) it
started with "xen.efi".

Tamas

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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-03 Thread Jan Beulich

>>> On 03.01.18 at 17:04,  wrote:
> On Wed, Jan 3, 2018 at 4:20 AM, Jan Beulich  wrote:
> On 02.01.18 at 16:56,  wrote:
>>> +if ( elo->Attributes & LOAD_OPTION_ACTIVE )
>>
>> Without any other (earlier) check, how can you reliably tell this
>> being a pointer to EFI_LOAD_OPTION from it being the
>> currently used one to CHAR16? Is the distinction perhaps UEFI
>> version dependent? In the 2.3 spec I can't find any information
>> on the layout of what ->LoadOptions points to.
> 
> AFAICT there is no clear cut way to distinguish what is being passed
> here. When launched via UEFI Xen receives the EFI_LOAD_OPTION buffer
> here already, which it tries to parse as a string and fails. Take a
> look at the same problem in the shim:
> https://github.com/rhboot/shim/blob/master/shim.c#L2501. If there is a
> clearer way to distinguish what is being passed here or more checks
> that can be done I would be open for suggestions.

Well, first of all the code you point to tells me that this situation is
indeed as bad as it can be. However, the code also shows you
some heuristics you could re-use. What they don't seem to utilize
is the fact that as long as there are few enough bits defined for
the Attributes field, checking that one for being a printable
character (and the upper 16 bits to be non-null) might give a good
indication what form we're dealing with. In fact it might be that
when it's a string, the first character is always '\', but I'm afraid
that's not written down anywhere.

Jan

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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-03 Thread Tamas K Lengyel

On Wed, Jan 3, 2018 at 4:20 AM, Jan Beulich  wrote:
 On 02.01.18 at 16:56,  wrote:
>> 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 Xen is started through an EFI Shell.
>
> Is this true? I thought that EFI Boot Manager entries can very well
> have command line options. And other boot loaders (e.g. grub2)
> should provide their own means to hand over options.

Maybe the comment is inaccurate in that if you use grub or some other
bootloader it can also specify which section to use from the Xen
config. I only tried it with the Shell though so I can't speak for
other bootloaders. If Xen is being launched directly by UEFI (or by
the shim) the only way to specify the section is with
EFI_LOAD_OPTION's OptionalData. When you do that though the current
command line parsing code won't work because it's not a string:

# efibootmgr -w -L "Xen" -l "\EFI\xen\xen.efi" -c -d /dev/nvme0n1
--part 1 -e 3 -u "section5"
# efivar -p --name 8be4df61-93ca-11d2-aa0d-00e098032b8c-Boot
GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
Name: "Boot"
Attributes:
Non-Volatile
Boot Service Access
Runtime Service Access
Value:
  01 00 00 00 7c 00 58 00  65 00 6e 00 00 00 02 01  ||.X.e.n.|
0010  0c 00 d0 41 03 0a 00 00  00 00 01 01 06 00 00 1d  |...A|
0020  01 01 06 00 00 00 03 17  10 00 01 00 00 00 00 08  ||
0030  0d 03 00 0b a8 e0 04 01  2a 00 01 00 00 00 00 08  |*...|
0040  00 00 00 00 00 00 00 00  10 00 00 00 00 00 9b e2  ||
0050  c5 ff 57 fa 3d 48 a5 de  00 53 f8 7a bd c4 02 02  |..W.=H...S.z|
0060  04 04 26 00 5c 00 45 00  46 00 49 00 5c 00 78 00  |..&.\.E.F.I.\.x.|
0070  65 00 6e 00 5c 00 78 00  65 00 6e 00 2e 00 65 00  |e.n.\.x.e.n...e.|
0080  66 00 69 00 00 00 7f ff  04 00 73 00 65 00 63 00  |f.i...s.e.c.|
0090  74 00 69 00 6f 00 6e 00  35 00|t.i.o.n.5.  |

>> +if ( elo->Attributes & LOAD_OPTION_ACTIVE )
>
> Without any other (earlier) check, how can you reliably tell this
> being a pointer to EFI_LOAD_OPTION from it being the
> currently used one to CHAR16? Is the distinction perhaps UEFI
> version dependent? In the 2.3 spec I can't find any information
> on the layout of what ->LoadOptions points to.

AFAICT there is no clear cut way to distinguish what is being passed
here. When launched via UEFI Xen receives the EFI_LOAD_OPTION buffer
here already, which it tries to parse as a string and fails. Take a
look at the same problem in the shim:
https://github.com/rhboot/shim/blob/master/shim.c#L2501. If there is a
clearer way to distinguish what is being passed here or more checks
that can be done I would be open for suggestions.

>> --- a/xen/include/efi/efiapi.h
>> +++ b/xen/include/efi/efiapi.h
>
> Generally additions to this file are expected to come from the gnu-efi
> package, which it was originally cloned from. I've just checked and
> see that 3.0.6 doesn't appear to have any of this (yet). In such a
> case at the very least you should match pre-existing style (e.g.
> indentation). Commonly, however, we introduce such private
> (and temporary) definitions into the source file that needs them (see
> e.g. the Apple Properties interface).

This part I took as-is from
tools/firmware/etherboot/ipxe/src/include/ipxe/efi/Uefi/UefiSpec.h as
found in the Xen source tree.

Tamas

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

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-03 Thread Jan Beulich

>>> On 02.01.18 at 16:56,  wrote:
> 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 Xen is started through an EFI Shell.

Is this true? I thought that EFI Boot Manager entries can very well
have command line options. And other boot loaders (e.g. grub2)
should provide their own means to hand over options.

> @@ -375,12 +375,40 @@ 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) )
> +{
> +/*
> + * See include/efi/efiapi.h for more info about the following
> + */

This should be a single line comment (also elsewhere).

> +EFI_LOAD_OPTION *elo = (EFI_LOAD_OPTION*)cmdline;

Missing blank before * (also elsewhere). And please make this a
pointer to const (and wherever else this would be suitable).

> +if ( elo->Attributes & LOAD_OPTION_ACTIVE )

Without any other (earlier) check, how can you reliably tell this
being a pointer to EFI_LOAD_OPTION from it being the
currently used one to CHAR16? Is the distinction perhaps UEFI
version dependent? In the 2.3 spec I can't find any information
on the layout of what ->LoadOptions points to.

> +{
> +UINT8 *_opts = (UINT8*)elo;
> +UINTN _cmdsize = cmdsize;

Leading underscores should only be used on file scope variables.

> @@ -1074,6 +1102,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  bool base_video = false;
>  char *option_str;
>  bool use_cfg_file;
> +bool elo_active = false;

Please add to one of the existing bool lines instead of introducing
yet another one.

> --- a/xen/include/efi/efiapi.h
> +++ b/xen/include/efi/efiapi.h

Generally additions to this file are expected to come from the gnu-efi
package, which it was originally cloned from. I've just checked and
see that 3.0.6 doesn't appear to have any of this (yet). In such a
case at the very least you should match pre-existing style (e.g.
indentation). Commonly, however, we introduce such private
(and temporary) definitions into the source file that needs them (see
e.g. the Apple Properties interface).

> +typedef struct __packed _EFI_LOAD_OPTION {

Is the __packed here really needed? I'd much rather uncomment
the Description[] field (allowing you to get at it without using
sizeof(the-whole-structure).

Jan

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

[Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

2018-01-02 Thread 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 Xen is started through an EFI Shell. As UEFI
provides a standard to pass optional arguments to an application 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 
---
Cc: Jan Beulich 
Cc: ope...@googlegroups.com
---
 xen/common/efi/boot.c| 41 ++--
 xen/include/efi/efiapi.h | 54 
 2 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 469bf980cc..b12261b662 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -236,7 +236,7 @@ static void __init DisplayUint(UINT64 Val, INTN Width)
 PrintStr(PrintString);
 }
 
-static size_t __init __maybe_unused wstrlen(const CHAR16 *s)
+static size_t __init wstrlen(const CHAR16 *s)
 {
 const CHAR16 *sc;
 
@@ -375,12 +375,40 @@ 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) )
+{
+/*
+ * See include/efi/efiapi.h for more info about the following
+ */
+EFI_LOAD_OPTION *elo = (EFI_LOAD_OPTION*)cmdline;
+
+if ( elo->Attributes & LOAD_OPTION_ACTIVE )
+{
+UINT8 *_opts = (UINT8*)elo;
+UINTN _cmdsize = cmdsize;
+
+_opts += sizeof(elo->Attributes) + sizeof(elo->FilePathListLength);
+_opts += sizeof(L'\0') + 2*wstrlen((CHAR16*)_opts) + 
elo->FilePathListLength;
+_cmdsize -= _opts - (UINT8*)elo;
+
+/*
+ * Sanity check the new cmdsize to avoid an underflow
+ */
+if ( _cmdsize < cmdsize )
+{
+*elo_active = true;
+cmdline = (CHAR16*)_opts;
+cmdsize = _cmdsize;
+}
+}
+}
+
+for ( ; cmdsize >= sizeof(*cmdline) && *cmdline;
 cmdsize -= sizeof(*cmdline), ++cmdline )
 {
 bool cur_sep = *cmdline == L' ' || *cmdline == L'\t';
@@ -1074,6 +1102,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
*SystemTable)
 bool base_video = false;
 char *option_str;
 bool use_cfg_file;
+bool elo_active = false;
 
 __set_bit(EFI_BOOT, _flags);
 __set_bit(EFI_LOADER, _flags);
@@ -1096,17 +1125,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, _active);
 if ( argc > 0 &&
  efi_bs->AllocatePool(EfiLoaderData,
   (argc + 1) * sizeof(*argv) +
   loaded_image->LoadOptionsSize,
   (void **)) == EFI_SUCCESS )
 get_argv(argc, argv, loaded_image->LoadOptions,
- loaded_image->LoadOptionsSize, );
+ loaded_image->LoadOptionsSize, , _active);
 else
 argc = 0;
-for ( i = 1; i < argc; ++i )
+for ( i = !elo_active; i < argc; ++i )
 {
 CHAR16 *ptr = argv[i];
 
diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
index a616d1238a..c3dc902ac5 100644
--- a/xen/include/efi/efiapi.h
+++ b/xen/include/efi/efiapi.h
@@ -922,5 +922,59 @@ typedef struct _EFI_SYSTEM_TABLE {
 
 } EFI_SYSTEM_TABLE;
 
+//
+// EFI Load Option. This data structure describes format of UEFI boot option 
variables.
+//
+// NOTE: EFI Load Option is a byte packed buffer of variable length fields.
+// The first two fields have fixed length. They are declared as members of the
+// EFI_LOAD_OPTION structure. All the other fields are variable length fields.
+// They are listed in the comment block below for reference purposes.
+//
+typedef struct __packed _EFI_LOAD_OPTION {
+  ///
+  /// The attributes for this load option entry. All unused bits must be zero
+  /// and are reserved by the UEFI specification for future growth.
+  ///
+  UINT32   Attributes;
+  ///
+  /// Length in bytes of the FilePathList. OptionalData starts at offset
+  /// sizeof(UINT32) + sizeof(UINT16) +

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

[Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

12 matches

Site Navigation

Mail list logo

Footer information