Skip to content
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

Don't parse load options if invoked from removable media path #399

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

julian-klode
Copy link
Collaborator

No description provided.

@julian-klode julian-klode changed the title Disable removable load options Don't parse load options if invoked from removable media path Aug 4, 2021
@julian-klode julian-klode force-pushed the disable-removable-load-options branch 3 times, most recently from e20c174 to afe3f03 Compare August 5, 2021 10:55
@julian-klode julian-klode marked this pull request as ready for review August 5, 2021 16:20
@julian-klode
Copy link
Collaborator Author

There is a question whether is_removable_media_path() should move to load-options.c / become non-static and we then do the exit in parse_load_options() which would also allow us to test that bit, but shuffles code around a bit more.

Then we could also dump the load options even when ignoring it, and play around with machines and improve our parsing code for firmware generated entries.

@steve-mcintyre
Copy link
Collaborator

Hey @julian-klode

Just wondering about the effects of this change - won't this totally block anybody using the RM path from doing EFI capsule updates?

@julian-klode
Copy link
Collaborator Author

julian-klode commented Aug 11, 2021

So if you just install to the removable media path, you are busted and can't boot anything else via it. That's why you have to make sure to always install a shimx64.efi alongside a bootx64.efi.

My understanding of the situation is that in Debian and Ubuntu, we always install shimx64.efi to the vendor directory, and that's how fwupd is loaded, so things should just work. The code to fallback to using bootx64.efi in fwupd is disabled by default, but, yes, will be broken with that change enabled.

Other people might have other requirements, and it might not work for them, so they'd not enable this option and get hit with a 2s timeout and fallback from #393 if misparsing happened.

@steve-mcintyre
Copy link
Collaborator

So if you just install to the removable media path, you are busted and can't boot anything else via it. That's why you have to make sure to always install a shimx64.efi alongside a bootx64.efi.

My understanding of the situation is that in Debian and Ubuntu, we always install shimx64.efi to the vendor directory, and that's how fwupd is loaded, so things should just work. The code to fallback to using bootx64.efi in fwupd is disabled by default, but, yes, will be broken with that change enabled.

Right. I'm worried about the (too-large!) number of people who still have to rely on the RM path due to firmware or system setup issues.

@julian-klode
Copy link
Collaborator Author

So if you just install to the removable media path, you are busted and can't boot anything else via it. That's why you have to make sure to always install a shimx64.efi alongside a bootx64.efi.
My understanding of the situation is that in Debian and Ubuntu, we always install shimx64.efi to the vendor directory, and that's how fwupd is loaded, so things should just work. The code to fallback to using bootx64.efi in fwupd is disabled by default, but, yes, will be broken with that change enabled.

Right. I'm worried about the (too-large!) number of people who still have to rely on the RM path due to firmware or system setup issues.

You think they'll end up in a situation where they can set arguments for bootx64.efi, but not for shimx64.efi?

@haobinnan
Copy link

Exception occurred after using patches 393-1 / 393-2 / 399-1 / 399-2. And I got following messages after replacing \EFI\Microsoft\Boot\bootmgfw.efi with shimx64.efi:
Falled to open \EFI\Microsoft\Boot\S - Invalid Parameter
Failed to load image \EFI\Microsoft\Boot\S: Invalid Parameter
start_image() returned Invalid Parameter, falling back to default loader

@julian-klode

@TriMoon

This comment was marked as abuse.

@julian-klode
Copy link
Collaborator Author

@haobinnan Just to clarify - seems I missed this: Are you saying that if you just apply only 393 and 399, you see that issue, and it does not happen without those specific patches? I don't see them adding code that would cause parsing issues. What happens if you apply just 393 or 399?

Simple refactoring that extracts the path checking on the given
loaded image. This will be useful to check if we were booted via
removable media path in other places.
We see various reports of boot failures because the generated
boot entries contain garbage/tagging that we do not expect, and
that we then parse as a second stage boot loader.
@vathpela vathpela merged commit b437584 into rhboot:main Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants