Make bootloader updates on UEFI-based systems work #299
Conversation
|
I don't like that I have the bootloader ID hardcoded to |
please don't. we should figure out how to get the debian packages to do the right thing, without grml-debootstrap starting grub commands. |
|
Which grub command are you referring to? The I think SystemBuildTools are supposed to run that command.
Therefore, I am pretty sure only the system build tool is responsible for setting up the bootloader, which requires running bootloader installation commands. |
The postinst used by grub-efi-amd64 autodetects the bootloader ID from the I'm not quite sure how |
|
Based on my research, you can only install GRUB on legacy BIOS systems without running grub-install manually if you have grub-pc installed. That isn't possible with this PR since you can't have grub-pc and grub-efi-ARCH co-installed, so my PR will have to leave the grub-install commands that install the legacy BIOS bootloader. However, I think I can get the EFI bootloader to be handled automatically by the Debian package, without having to explicitly call grub-install. So I'll do that. (I should also be looking into how to allow grub-pc and grub-efi-ARCH to be co-installed, but that's a potentially large task that has to be handled in Debian upstream.) |
|
Alright, this works on my system. I didn't touch the |
|
Please use, review the following simplification, if sane |
|
Avoid repetitive |
|
@zeha Looking through the code, I can't figure out why |
|
@adrelanos Some of the installation commands are order-dependent in the code, so I'm a bit leery of changing them. There's only three lines that are really repetitive, and deduplicating them would result in more code than is already there. I think for now leaving them as-is and working with them in a future PR would be a good idea. I looked long and hard at the Also got the simplification @adrelanos recommended integrated. |
|
So this is the history prior to this pull request: First only grub-pc was supported. Then EFI support was added. Then VMEFI support was added. And finally, ARM64 (EFI only) support was added. This had resulted in a bit of overly complex code with the Now with the latest commit, after this refactoring, it looks much cleaner. |
|
Ready for review. Could you kindly review please? |
|
I'll try to take a look later |
grml-debootstrap
Outdated
| # packages are uploaded. Doing this means that the BIOS bootloader won't | ||
| # be automatically updated, which stinks, however the BIOS bootloader | ||
| # doesn't have the same security concerns as the EFI bootloader (there's | ||
| # no Secure Boot to grapple with when using legacy BIOS boot) so it's |
There was a problem hiding this comment.
having both is just unsupportable and should imo go. not updating grub means it will fail to boot by chance
There was a problem hiding this comment.
Legacy BIOS + EFI dual boot compatibility already works great for the Linux distributions we're maintaining.
This feature is also in progress of being contributed to calamares:
- Prototype implementation of BIOS+UEFI boot support calamares/calamares#2422
- Debian for grub-pc with grub-efi co-install-ability feature request Allow concurrent installation of grub-pc and grub-efi-amd64 2 might also get implemented.
It has to be clarified what isn't being updated. "Most" is getting updated. This includes /etc/grub.d, /etc/default/grub.d, grub.cfg, packaged files. The only part not getting updated is the legacy BIOS bootloader inside the MBR.
That however, shouldn't be much of an issue. The MBR legacy BIOS bootloader isn't a terribly complex program, it's development is completed, MBR space is limited. No updates for it are to be expected because legacy BIOS bootloader development is completed and EFI is the new thing. And even if there are minor updates, these won't be security-related.
Updates for the EFI bootloader are more important as these might be security relevant.
There was a problem hiding this comment.
Most of the point of this update was to make hybrid BIOS+UEFI boot work better. With Debian Stable, the chances of "fail to boot by chance" I believe are extremely small since a change that would result in failure to boot without an updated GRUB probably wouldn't be allowed in a stable release. With Testing or Sid, then yeah, you have a potential problem. Would it be sufficient to simply refuse hybrid BIOS+UEFI installation on Testing or Sid, at least until the packages are co-installable? (I'm actually intending on doing some work upstream to make the packages co-installable.)
There was a problem hiding this comment.
these won't be security-related.
doesn't matter if they are security related, update must work or booting will break.
With Debian Stable, the chances of "fail to boot by chance" I believe are extremely small since a change that would result in failure to boot without an updated GRUB probably wouldn't be allowed in a stable release
This happened in the past and it will happen again.
There was a problem hiding this comment.
If grml-debootstrap has to remove this support until grub-efi-ARCH and grub-pc are co-installable, Kicksecure will probably need to fork grml-debootstrap from the commit immediately before the feature removal, since that feature is currently integral to a feature we try to offer.
Perhaps an alternate solution - allow installing both, but let the user choose which one gets automatically updated via a parameter? I'd be willing to implement that, and then hopefully once grub-efi-ARCH and grub-pc are co-installable it can be dropped later on.
grml-debootstrap
Outdated
| fi | ||
|
|
||
| mkdir -p "${MNTPOINT}"/boot/efi/EFI/BOOT | ||
| mkdir -p "${MNTPOINT}"/boot/efi/EFI/"${EFI_ID}" |
There was a problem hiding this comment.
can we just skip this? i've never needed to call mkdir on these directories, and i don't see why grml-debootstrap should need to do that.
There was a problem hiding this comment.
We cannot. If those directories do not exist, the grub-efi-ARCH package will not install the bootloader at package installation time (at least not into the missing location). This is coded into the postinst script's logic. Creating these dirs allows the package to auto-install (and more importantly, auto-update) the bootloader.
There was a problem hiding this comment.
could you please point me to the code for this (both install and update case) in the grub2 package? I'm not seeing it in https://sources.debian.org/src/grub2/2.12-5/debian/postinst.in/
There was a problem hiding this comment.
I'll try to find it again, I'm certain I saw it and even verified that the mkdir commands worked to fix the bug, but now I'm having trouble finding it too. It might be only a Bookworm thing.
There was a problem hiding this comment.
nvm, found it, it looks like this was fixed in >Bookworm but is a problem in Bookworm specifically. https://sources.debian.org/src/grub2/2.06-13%2Bdeb12u1/debian/postinst.in/#L720
There was a problem hiding this comment.
ok, i understand the code path in the postinst for updates.
but this will be done correctly by grub-install --target=x86_64-efi / grub-install --target=arm64-efi - no?
don't get me wrong, i don't want to be annoying about this. I just really don't want to have code in grml-debootstrap dealing with internals of the grub packaging, and very much not code that the grub maintainers do not know about and do not include in their testing of upgrade paths.
There was a problem hiding this comment.
Erm, I thought you had explicitly asked me to remove the grub-install commands: #299 (comment) This PR currently removes them in favor of making the Debian packaging do the right thing automatically. If I misunderstood you, I'm happy to undo that.
There was a problem hiding this comment.
Quote grub2 package, Debian changelog, Colin Watson Nov 2020
- Don't call grub-install on fresh install of grub-pc. It's the job of
installers to do that after a fresh install.-- Colin Watson cjwatson@debian.org Sun, 08 Nov 2020 16:26:08 +0000
There was a problem hiding this comment.
Quote grub2 package, Debian changelog, Colin Watson Nov 2020
- Don't call grub-install on fresh install of grub-pc. It's the job of
installers to do that after a fresh install.-- Colin Watson cjwatson@debian.org Sun, 08 Nov 2020 16:26:08 +0000
|
I added the efi-id stuff because it's necessary for Kicksecure's use case (we can work around it if it doesn't exist, but it's preferable if it does exist). Are there problems with it existing? Would there be a different way of implementing it that would make it acceptable? |
So you're saying your usecase includes setting a different GRUB_DISTRIBUTOR, without setting a different os-release vendor name? |
Indeed it does. |
Uff, that sounds very use case specific and like something that should be handled from within custom hooks/scripts instead? We're trying to strip down on command line options rather than adding further ones. We need to keep grml-debootstrap and its code-base support- and maintainable, which also means that we should focus on having command line options that don't conflict with each other or one needs to figure out with lots of trial-and-error. Common use cases should all work out by default. (We'll try to document and verbalize our plans and vision in #311 BTW.) I didn't manage to go through all the code change yet, but I think one approach to support BIOS/legacy next to EFI systems at the same time is to install We should rework/simplify/unify all the efi + arm related code path, this seems to get more and more complex :) |
Yeah, that's an option. I can remove that bit if it's desirable, and then our custom scripts can work with it. I didn't know about the grub-cloud stuff, I'll take a look at it. |
That might sound like it but it's really not that special. It's just the "simple" use case for a Debian derivative. Changing (For this reason, Qubes also decided not to touch Changing GRUB_DISTRIBUTOR however makes sense to avoid pretending to be Debian, while actually being a derivative, to avoid breaking multi-booting. |
|
Quote
Quote list of files:
AMD64 Could package
The serial console related issues I've run into ~ 5 years ago when I was also considering "why not enable a serial console by default inside VM images". If going for a package
grub-cloud seems to support only a limited amount of architectures. (Intel/AMD64 and ARM64 at time of writing.) Depending on your plans for multiple architecture support (Debian is the universal operating system), it might be too limited. |
|
Analysis:
In conclusion,
|
Are you sure? Should be as simple as Am I missing something? 🤔 |
|
If you look at Real GRUB Real GRUB This means, only if It does a lot of complex stuff. Some examples... Lots of debconf... Line 444 to line 711. But all of that happens only if
For package |
|
It's worth noting some of the old grub-pc installation code had to do with things like migrating from grub-legacy to grub2, fighting with RAID devices, multi-OS support, Wubi handling (Wubi was an old way of running Ubuntu as a full operating system from a file on a Windows machine), etc., etc. Lots of legacy and edge case handling. A VM doesn't have pretty much any of those edge cases, so a much simpler solution may be perfectly fine there. One thing interesting to note is that the grub-cloud package claims |
|
@mika, @zeha, @adrelanos Alright, see what you think of that. I've tested this and was able to build amd64, i386, and arm64 VMs both with and without UEFI support where possible, all of them build and boot with the appropriate firmware in QEMU. One downside to using grub-cloud-amd64 on amd64 is that boot is significantly slowed down, probably due to the serial terminal settings I would guess. Not sure how to resolve that, or if it's worth resolving. |
I am sure this is caused by the serial console settings that the The relevant
Pretty sure it needs to be resolved. Here are some possible solutions: Option A) Not using
|
Given this seems to be a bug in Debian, it needs to be fixed in Debian. grml-debootstrap aims to install Debian, not a custom distro. option B,C,D,E are no-gos IMO. |
|
Ultimately grub-cloud shouldn't be shipping /etc/default/grub. If it wants to use a postinst script to write it at installation time, that's fine, but shipping it directly is a step too far since it forbids installers (like grml-debootstrap) from generating it directly without running into problems. This same issue is causing other problems too: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=950079 This is technically a different bug, but I think it's from a same or similar cause. I'll file the bug report and see if I can convince the maintainer to resolve the issue. |
|
In the short term, can we just live with the slowdown? Downstreams can override it if they dislike it this much (that's what Kicksecure is currently planning to do). |
I guess so. How much of a slow down is there? quick thought on alternatives: we could build our own equivalent package from src:grml-debootstrap. Obviously that'd only work for trixie and newer. ftpmasters permitting. |
otoh: I think in a server VM it's reasonable to enable serial console. Is it known what the actual slowdown is and which part causes it? |
Possible in theory. I am surprised by that suggestion. Then more grml-specific things would be invented. It wouldn't be a pure Debian, and this exact way of using GRUB would have fewer users. It's on the Kicksecure roadmap to implement Debian for grub-pc + grub-efi co-install-ability feature request Allow concurrent installation of grub-pc and grub-efi-amd64. This I would consider fixing the real root cause. Once/if this can be fixed upstream in Debian, the downstream implementation in grml-debootstrap will become a lot simpler. It might not be in time for Trixie.
That's also why I didn't bring up such solutions. There would be conflicts with grml-debootstrap goals, as far as I understand them.
As a result, there would be two code paths in grml-debootstrap:
That's the next hurdle. They could tell us to fix the root issues instead of inventing yet another GRUB package.
It could also be a security issue.
No. This would require a larger investigation. There might even be multiple bugs.
We have this on the roadmap too, but it will be done at a later time. Using a (virtual) serial console is a niche use case nowadays, mostly only used by developers and advanced users. And then, on top of that, enabling a (virtual) serial console in GRUB/kernel parameters while not actually adding a (virtual) serial console to the system is an even more niche use case. |
It would be a package in Debian, which would need to have autopkgtests. Then the usual things for packages in Debian with autopkgtests follow. I think this is roughly equivalently good or bad to using grub-cloud, but obviously it woud have fewer users. But yes, all of that is cementing in a workaround as a solution that one would hope to go away, as time moves on. |
Login gettys on serial consoles are more or less automatic and pervasive and I've never seen them being classified as security issues. |
Only if you are wearing x86-desktop-colored glasses. |
@adrelanos On my Kubuntu 24.04 system at least, all of /dev/ttyS* is owned by |
|
IRC log snippet from discussion in #debian-devel: It looks like writing an /etc/default/grub.d file to override the serial console stuff isn't a departure from Debian, it's perfectly normal behavior and even the intended way of using the system. GRUB packages own /etc/default/grub, if we don't like how they're configured we're supposed to override it with config snippets. |
|
related Debian |
|
Given the above IRC chat, and given that Debian policy doesn't consider packages (such as Is |
I still think we should roll with the defaults for now. What you do in your downstream usage, is left to you. |
|
@zeha, @mika Is there more that needs done on this? I believe the consensus is that using |
Yes, the The Please also squash your commits/changes once you're done, provide a good and verbose commit message which clarifies the changes and the overall picture, thanks. :) |
|
Oh and @ArrayBolt3 please be aware that there's also grub magic in |
To be clear, it would be desirable to merge the entirety of the grub configuration/installation block from chroot-script into grml-debootstrap itself, ensuring the |
|
I think I probably misunderstood the above - the two different grub_install functions in grml-debootstrap and chroot-script are pretty different, and while I'm not scared to merge them, I think that's a substantial enough change it should wait for a future PR. For now I'll make the code in chroot-script also able to install the correct bootloaders, so I don't end up overloading this PR. |
ArrayBolt3
force-pushed
the
arraybolt3/better-grub-efi
branch
from
51825d5 to
c407ca7
Compare
|
Alright, there's the squashed commit. There's one possible problem left here, which is that for some reason when doing an arm64 EFI install onto a physical drive on an amd64 BIOS system, shim isn't installed into the ESP. I don't know why yet. |
Previously, it was possible to install both BIOS and UEFI bootloaders at the same time when creating VM images with grml-debootstrap. In this setup however, the UEFI bootloader would not be automatically updated, due to the absence of a grub-efi-ARCH package on the installed system. It also was not possible to install both bootloaders using grml-debootstrap on a physical disk, and arm64 VM images could not be built with full UEFI support as enabled by the --vmefi argument (these builds would fail). To rectify these issues: * Use grub-cloud-amd64 to install both BIOS and UEFI bootloaders and manage updates. * Add support for installing BIOS and UEFI bootloaders simultaneously on physical disk installations. * Get rid of the ARM_EFI_TARGET variable and make ARM64 VM builds use VMEFI=1 by default. Fixes grml#297, grml#257
ArrayBolt3
force-pushed
the
arraybolt3/better-grub-efi
branch
from
c407ca7 to
34b42c1
Compare
|
Fixed the UEFI bootloader on arm64 physical disk installs, and fixed a couple of other issues too (I had a known-buggy line of code in one spot that needed replaced, and I had a placeholder left around). |
|
fyi, I believe this is ready for review again, whenever someone wants to take a look at its current state. |
Fixes #297. Quick summary of changes:
grub-installcalls,--bootloader-id=debianand--force-extra-removableare used. This ensures the bootloader is installed both to the distro-specific path (/boot/efi/EFI/debian) and to the removable media path.--removableis removed from these calls as it is no longer needed and in fact prevents the normal bootloader location from being installed to. Its job is done by--force-extra-removablenow.--vmefiis enabled,grub-pc-binis installed rather thangrub-pc, allowing the BIOS bootloader to be installed but not allowing it to be automatically updated. This is to allow installinggrub-efi-ARCH, which allows automatically updating the EFI bootloader. (Sadly the two cannot be installed at the same time, which is why this change is necessary. If they could be installed at the same time, that would be best, but Debian isn't capable of that at the moment.)grub-efi-ARCHis installed along withgrub-efi-ARCH-signed.grub2/force_efi_extra_removable, is documented at https://wiki.debian.org/UEFI#Force_grub-efi_installation_to_the_removable_media_path.