-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
shim: another attempt to fix load options handling #379
Conversation
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
I am running with this patch, and it works for me. I'll submit a shim with it for signing for Ubuntu soon, and then we can test it IRL by real users other than me |
I like this patch, but I'm not going to merge it without making this code more testable. To that end, and considering #381 (comment) , I'm going to keep working on https://github.com/vathpela/mallory/tree/test-load-options , which is most of the way to where we need it to be, with eyes on pulling those test cases along with this branch. |
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:
the OptionalData if it is.
current shim path and ignore it if it is (the UEFI shell case).
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: