| Message ID | cover.1611273359.git.bobbyeshleman@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support Secure Boot for multiboot2 Xen | expand |
On 22.01.2021 01:51, Bobby Eshleman wrote: > This is version 3 for a patch set sent out to the ML in 2018 [1] to > support UEFI Secure Boot for Xen on multiboot2 platforms. > > A new binary, xen.mb.efi, is built. It contains the mb2 header as well > as a hand-crafted PE/COFF header. The dom0 kernel is verified using the > shim lock protocol. > > I followed with v2 feedback and attempted to convert the PE/COFF header > into C instead of ASM. Unfortunately, this was only possible for the > first part (Legacy) of the PE/COFF header. The other parts required > addresses only available at link time (such as __2M_rwdata_end, > __pe_SizeOfImage, efi_mb_start address, etc...), which effectively ruled > out C. I don't follow the conclusion drawn, so would you mind going into further detail? Jan
On Fri, Jan 22, 2021 at 10:39:28AM +0100, Jan Beulich wrote: > On 22.01.2021 01:51, Bobby Eshleman wrote: > > I followed with v2 feedback and attempted to convert the PE/COFF header > > into C instead of ASM. Unfortunately, this was only possible for the > > first part (Legacy) of the PE/COFF header. The other parts required > > addresses only available at link time (such as __2M_rwdata_end, > > __pe_SizeOfImage, efi_mb_start address, etc...), which effectively ruled > > out C. > > I don't follow the conclusion drawn, so would you mind going into > further detail? > No problem at all, bad explanation on my part. The core issue is actually about the legality of casting 64-bit addresses to 32-bit values in constant expressions, which then is sometimes complained about by GCC in terms of load-time computability... Taking __2M_rwdata_end as an example. Attempting to use it in the PE/COFF optional header in C looks something like: extern const char __2M_rwdata_end[]; extern const char efi_pe_head_end[]; struct optional_header optional_header = { ... .code_size = (uint32_t)((unsigned long)&__2M_rwdata_end) - (uint32_t)((unsigned long)&efi_pe_head_end, ... } GCC throws "error: initializer element is not constant" because casting a 64-bit address to a 32-bit value is not a legal constant expression for static storage class objects, even though we know that in practice the address wouldn't ever be above 4GB. efi_pe_head_end could potentially be calculated by header struct sizes, but doing that predictably yields the same error. If we drop the explicit casting, GCC throws 'error: initializer element is not computable at load time'. tl;dr: I could not find a way to derive code size (data sections and all) without using a symbol location, which is an illegal constant expression operand for initializing static storage class objects... and I could not find a way to define the header in C without using an object of static storage class (global variable or static variable). -Bob
On 22.01.2021 22:18, Bobby Eshleman wrote: > On Fri, Jan 22, 2021 at 10:39:28AM +0100, Jan Beulich wrote: >> On 22.01.2021 01:51, Bobby Eshleman wrote: >>> I followed with v2 feedback and attempted to convert the PE/COFF header >>> into C instead of ASM. Unfortunately, this was only possible for the >>> first part (Legacy) of the PE/COFF header. The other parts required >>> addresses only available at link time (such as __2M_rwdata_end, >>> __pe_SizeOfImage, efi_mb_start address, etc...), which effectively ruled >>> out C. >> >> I don't follow the conclusion drawn, so would you mind going into >> further detail? >> > > No problem at all, bad explanation on my part. The core issue is > actually about the legality of casting 64-bit addresses to 32-bit values > in constant expressions, which then is sometimes complained about by GCC > in terms of load-time computability... > > Taking __2M_rwdata_end as an example. Attempting to use it in > the PE/COFF optional header in C looks something like: > > extern const char __2M_rwdata_end[]; > extern const char efi_pe_head_end[]; > > struct optional_header optional_header = { > ... > .code_size = (uint32_t)((unsigned long)&__2M_rwdata_end) - > (uint32_t)((unsigned long)&efi_pe_head_end, > ... > } > > GCC throws "error: initializer element is not constant" because casting > a 64-bit address to a 32-bit value is not a legal constant expression > for static storage class objects, even though we know that in practice > the address wouldn't ever be above 4GB. > > efi_pe_head_end could potentially be calculated by header struct sizes, > but doing that predictably yields the same error. > > If we drop the explicit casting, GCC throws 'error: initializer element > is not computable at load time'. Ah yes, I see now, and I'm aware of the compiler shortcoming. Even with the not really necessary uint32_t casts dropped the problem would still be there. So for your description this means it's not "required addresses only available at link time" but "required differences of addresses not computable or expressable at compile time". Jan > tl;dr: > > I could not find a way to derive code size (data sections and all) > without using a symbol location, which is an illegal constant expression > operand for initializing static storage class objects... and I could not > find a way to define the header in C without using an object of static > storage class (global variable or static variable). > > -Bob >
Hey all, I just wanted to request more feedback on this series and put it on the radar, while acknowledging that I'm sure given the recent code freeze it is a busy time for everybody. Best, Bob
On 22.02.2021 19:04, Bobby Eshleman wrote: > I just wanted to request more feedback on this series and put it on the radar, while acknowledging > that I'm sure given the recent code freeze it is a busy time for everybody. It is on my list of things to look at. While probably not a good excuse, my looking at previous versions of this makes we somewhat hesitant to open any of these patch mails ... But I mean to get to it. Jan
On 2/22/21 11:16 PM, Jan Beulich wrote: > It is on my list of things to look at. While probably not a good excuse, > my looking at previous versions of this makes we somewhat hesitant to > open any of these patch mails ... But I mean to get to it. > > Jan > Thanks for this response. I did comb through your v2 feedback point-by-point and incorporate it into the code, so I do hope that ends up helping. Best, Bob
This is version 3 for a patch set sent out to the ML in 2018 [1] to support UEFI Secure Boot for Xen on multiboot2 platforms. A new binary, xen.mb.efi, is built. It contains the mb2 header as well as a hand-crafted PE/COFF header. The dom0 kernel is verified using the shim lock protocol. I followed with v2 feedback and attempted to convert the PE/COFF header into C instead of ASM. Unfortunately, this was only possible for the first part (Legacy) of the PE/COFF header. The other parts required addresses only available at link time (such as __2M_rwdata_end, __pe_SizeOfImage, efi_mb_start address, etc...), which effectively ruled out C. The biggest difference between v2 and v3 is that in v3 we do not attempt to merge xen.mb.efi and xen.efi into a single binary. Instead, this will be left to a future patch set, unless requested otherwise. [1]: https://lists.xen.org/archives/html/xen-devel/2018-06/msg01292.html Changes in v3: - add requested comment clarification - remove unnecessary fake data from PE/COFF head (like linker versions) - macro-ize and refactor Makefile according to Jan's feedback - break PE/COFF header into its own file - shrink the PE/COFF to start 0x40 instead of 0x80 (my tests showed this function with no problem, on a live nested vm or using objdump/objcopy) - support SOURCE_EPOCH for posix time - removed `date` invocation that would break on FreeBSD - style changes - And obviously, ported to current HEAD Daniel Kiper (5): xen: add XEN_BUILD_POSIX_TIME xen/x86: manually build xen.mb.efi binary xen/x86: add some addresses to the Multiboot header xen/x86: add some addresses to the Multiboot2 header xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2() xen/Makefile | 22 ++++--- xen/arch/x86/Makefile | 7 +- xen/arch/x86/arch.mk | 2 + xen/arch/x86/boot/Makefile | 1 + xen/arch/x86/boot/head.S | 53 +++++++++++++-- xen/arch/x86/boot/pecoff.S | 123 +++++++++++++++++++++++++++++++++++ xen/arch/x86/efi/efi-boot.h | 30 ++++++++- xen/arch/x86/efi/stub.c | 17 ++++- xen/arch/x86/xen.lds.S | 34 ++++++++++ xen/common/efi/boot.c | 19 ++++-- xen/include/xen/compile.h.in | 1 + xen/include/xen/efi.h | 1 + 12 files changed, 283 insertions(+), 27 deletions(-) create mode 100644 xen/arch/x86/boot/pecoff.S