Commits on Jun 7, 2021

1. shim: another attempt to fix load options handling …

   The load options handling is quite complicated and tries to accomodate
   several scenarios, but there are currently multiple issues:
   
   - If the supplied LoadOptions is an EFI_LOAD_OPTION structure,
   second_stage gets initialized to the entire contents of the OptionalData
   field and load_options is initialized to NULL, which means it isn't
   possible to pass additional options to the second stage loader (and it
   looks like the intention is for this to be supported).
   
   - If the supplied LoadOptions contains 2 or more strings, the code seems
   to assume that shim was executed from the UEFI shell and that the first
   argument is the path of the shim executable, so it's ignored. But this
   breaks the ability to pass additional options to the second stage loader
   from BDS on firmware implementations that initialize LoadOptions to just
   the OptionalData field of the EFI_LOAD_OPTION, which is what EDK2 seems
   to do.
   
   This is moot anyway because this case (strings == 2) doesn't actually seem
   to work, as nothing sets loader_len and therefore second_stage is not set
   to the custom loader path.
   
   - If the supplied LoadOptions contains a single string that isn't shim's
   path, nothing sets loader_len and therefore second_stage isn't set at the
   end of set_second_stage.
   
   - set_second_stage replaces L' ' characters with L'\0' - whilst this is
   useful to NULL terminate the path for the second stage, it doesn't seem
   quite right to do this for the remaining LoadOptions data. Grub's
   chainloader command supplies additional arguments as a NULL-terminated
   space-delimited string via LoadOptions. Making it NULL-delimited seems to
   be incompatible with the kernel's commandline handling, which wouldn't
   work for scenarios where you might want to direct-boot a kernel image
   (wrapped in systemd's EFI stub) from shim.
   
   - handle_image passes the original LoadOptions to the second stage if
   load_options is NULL, which means that the second stage currently always
   gets shim's load options.
   
   I've made an attempt to try to fix things. After the initial
   checks in set_second_stage, it now does this:
   
   - Tries to parse LoadOptions as an EFI_LOAD_OPTION in order to extract
   the OptionalData if it is.
   - If it's not an EFI_LOAD_OPTION, check if the first string is the
   current shim path and ignore it if it is (the UEFI shell case).
   - Split LoadOptions in to a single NULL terminated string (used to
   initialize second_stage) and the unmodified remaining data (used to
   initialize load_options and load_options_size).
   
   I've also modified handle_image to always set LoadOptions and
   LoadOptionsSize. If shim is executed with no options, or is only
   executed with a single option to override the second stage loader
   path, the second stage is executed with LoadOptions = NULL and
   LoadOptionsSize = 0 now.
   
   I've tested this on EDK2 and I can load a custom loader with extra
   options from both BDS and the UEFI shell:
   
   FS0:\> shimx64.efi test.efi
   LoadOptionsSize: 0
   LoadOptions: (null)
   FS0:\> shimx64.efi       test.efi
   LoadOptionsSize: 0
   LoadOptions: (null)
   FS0:\> shimx64.efi test.efi foo bar
   LoadOptionsSize: 16
   LoadOptions: foo bar

   

   chrisccoulson committed Jun 7, 2021
   Configuration menu

   • View commit details
   • Copy full SHA for 4a4d647
   • Browse repository at this point

   Copy the full SHA

   4a4d647 View commit details

   Browse the repository at this point in the history