On Fri, 11 Sep 2020 at 02:04, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > On Thu, Sep 10, 2020 at 08:40:06PM +0000, Limonciello, Mario wrote: > > > > > > > > Ok, this is laid out in section 3.1 of the spec which defines the format > > > > of the EFI_LOAD_OPTION descriptor. Dell's BIOS is passing the entire > > > > descriptor instead of just the OptionalData part. > > > > > > > > OptionalData The remaining bytes in the load option descriptor are a > > > > binary data buffer that is passed to the loaded image. > > > > If the field is zero bytes long, a Null pointer is > > > > passed to the loaded image. The number of bytes in > > > > OptionalData can be computed by subtracting the starting > > > > offset of OptionalData from total size in bytes of the > > > > EFI_LOAD_OPTION. > > > > > > > > https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf > > > > > > > > > > This vaguely rings a bell so I have cc'ed some folks who may have run > > > into this in the past. Complete thread can be found at [0] > > > > > > > Thanks for sharing the context. This rings a bell for me with the last time > > I recall worrying about the Load Options and this commit in shim comes to > > mind: > > > > https://github.com/rhboot/shim/commit/3322257e611e2000f79726d295bb4845bbe449e7 > > > > It seems to me the shim approach to the problem isn't too big of a deal: > > count the number of strings and if it's greater or equal to 2, then throw > > out the first one. It's also already been used in production code across a > > ton of platforms for several years, so if there was major breakages I would > > expect they're covered in that code too by now. > > AFAICT, the >= 2 check is for seeing if it was invoked by the shell, and > if so, skipping the first word (which would be the name of the > executable). For handling the case we're running into here, it's > checking if loadoptions is valid UCS-2, though its idea of valid seems > to be that it must consist of characters < 0x100; and if not, trying to > parse it as a complete EFI_LOAD_OPTION. > > > > > > The firmware is obviously passing the wrong data, and I am reluctant > > > to quirk this out, since we'd have to interpret the contents of these > > > UEFI variables, and given the amount of 'value add' by the BIOS > > > vendors in this area, we may end up breaking things on other > > > platforms. > > > > > > [0] https://lore.kernel.org/linux- > > > efi/20200909203830.GA490605@xxxxxxxxxxxxxxxxxx/ > > > > In the defense of others who have interpreted the spec, as I'm reading it I'm > > not convinced that it explicitly calls out what data should be passed. In > > section 7.4 which explains how LoadImage() behaves. > > > > ``` > > Once the image is loaded, firmware creates and returns an EFI_HANDLE that identifies the image and > > supports EFI_LOADED_IMAGE_PROTOCOL and the EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL. > > The caller may fill in the image’s “load options” data > > ``` > > > > In this context the caller is most likely BDS, and it's "optional" to load > > content in. Within section 9.1 which describes the loaded image protocol the exact > > format of the content of "LoadOptions" is not described. I can see an interpretation > > it should be the full descriptor or that it can be the OptionalData. > > LoadImage() is a general library function, it can be called by the > firmware boot manager, but also by any other EFI application during boot > services. In this context, LoadOptions is arbitrary data to be filled in > by the caller of LoadImage() if it wants to. > > Section 3.1 is what describes how the Boot#### variables are to be > handled by the boot manager, and that is explicit about what gets > passed by the boot manager to the loaded image. > > > > > And actually if you looks in history from EDK code, you can see that it's been done that way there too at least at one time: > > https://github.com/tianocore/edk2/blob/b8d06293caa122f9c3bd2ae32a6c3f790a054e03/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c#L2433 > > > > I don't think that's passing the whole Boot#### variable. "Option" there > is a BDS_COMMON_OPTION, and that is created from the Boot#### variable > using BdsLibVariableToOption, which copies just the OptionalData into > BDS_COMMON_OPTION.LoadOptions. > > https://github.com/tianocore/edk2/blob/b8d06293caa122f9c3bd2ae32a6c3f790a054e03/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsMisc.c#L598 > > > > > Jacobo, > > > > Can you provide some more details on your system that is reproducing > > this? Model number, FW version would be useful. > > The links provided earlier on are on pretty old stuff, so knowing that this > > is a problem on something more current would be good. Hi Mario, thanks for joining the conversation. The system in which I'm testing is a Precision T3620 with the very last firmware 2.15.0 (published in mid 2020). It seems that this is affecting a lot of Dell's BIOSes: [1]: https://www.dell.com/community/Linux-Developer-Systems/Linux-EFISTUB/td-p/4586378 [2]: https://www.dell.com/community/Laptops-General-Read-Only/Dell-UEFI-implementation-does-not-support-Linux-Kernel-EFISTUB/td-p/5185272 [3]: https://bbs.archlinux.org/viewtopic.php?pid=1753169#p1753169 [4]: https://github.com/xdever/arch-efiboot > > > > I guess the other option if Ard chooses not to adopt a quirk for this > > described behavior is to use shim to load the kernel efistub, and let > > it do the split for you. We can circumvent this bug in several ways (using GRUB, packing kernel plus initrd into a single EFI file, etc.) but honestly, I'd like to have the same loading scheme in all my machines. As Arvin stated, and I share his statement, section 3.1.3 of the UEFI standard is clear: "The remaining bytes in the load option descriptor are a binary data buffer that is passed to the loaded image. If the field is zero bytes long, a NULL pointer is passed to the loaded image. The number of bytes in OptionalData can be computed by subtracting the starting offset of OptionalData from total size in bytes of the EFI_LOAD_OPTION".
