Stop most overwriting in the ESP #204

alois31 · 2023-06-21T18:11:11Z

Motivation

Currently, whenever installation is performed, the stubs for all generations are re-generated with the newest stub version and written to the esp. If a newer stub causes breakage on a specific machine (e.g. by bad interactions with buggy firmware), this leads to all generations becoming unbootable. Since at least one generation was bootable in the past, this problem can be avoided by avoiding the re-writing of already existing stubs. (Of course, this problem can still happen with a bad systemd-boot update, but we should not introduce a needless second single point of failure).

Similarly, whenever installation is performed, all kernels and initrds are currently collected in a temporary directory, compared and written to the ESP if they don't already exist or mismatch. This can similarly lead to degraded usability of old generations in case initrd secrets get messed up. Furthermore, the approach is generally wasteful because it leads to significant (proportional in the number of generations) RAM (tmpfs) and CPU (for hashing) usage in the install process.

Approach

This PR consists of three basic steps, organized as separate commits. The first two should be mostly independent of each other, but the last one critically depends on both.

Currently, all files (kernels, initrds and stubs) are prepared for each generation in temporary storage. In a second step, all of them are installed to the ESP (with the optimization that identical files are not re-written). By making the kernels and initrds content-addressed, it is already possible to install all generations separately from each other, which leads to a significant simplification of the code and reduction in memory usage during installation.
The stub paths on the ESP are made fully input-addressed. They now depend on the toplevel and the contents of the public key used for signing.
Only in the last step, the important benefits are fully realized. Prior to installation of a generation, it is checked in the ESP whether that generation is already properly installed, and it is not subsequently reinstalled in this case. Due to the content-addressing added in step 1 and the input-addressing in step 2, this does not cause corruption. Since the data read from the ESP can only be used to influence skipping an installation or marking a file as live to the garbage collection, no security issue is introduced.

Benefits

More reliable rollbacks: the stub is no longer a single point of failure (except in the case of key rotation), and the initrd secrets of a generation don't change after the fact (unless the initrd is manually deleted and then re-generated).
Faster and less memory-intensive installation: bootloader installation now uses memory, I/O and CPU resources roughly proportional to the number of newly installed generations (usually 1), instead of proportional to the total number of generations. (There's formally still a proportionality to the latter, but the heavy operations are avoided.)

Drawbacks

The first bootloader installation after these changes re-writes all kernels, initrds and stubs on the ESP, because they now have different paths. This leads to roughly doubled ESP disk usage during this installation.
If multiple generations share a bare initrd, but use different secrets scripts, the final initrds will be separated. This leads to increased ESP usage. However, this situation should be rare (I have not actually observed it in my tests with the dummy secrets script that's always there), and the previous behavior was somewhat incorrect anyway.
If files on the ESP are corrupted, this will not be automatically fixed by re-running bootloader installation. Since the files are written atomically, the only case where this happens should be file system corruption. In this case, it is questionable whether overwriting the files fixes the problem anyway.

nikstur · 2023-06-23T22:59:52Z

Thank you for the PR @alois31. It looks really interesting! Its quite a substantial change so it'll take some time to review.

rust/tool/src/esp.rs

rust/tool/src/install.rs

rust/tool/tests/common/mod.rs

RaitoBezarius

Apologies for the delay, I think the approach is good IMHO.

alois31 · 2023-07-29T13:51:37Z

The fallout should be fixed now.

nikstur

Thank you for your patience. I like the overall idea. There are just a few design issues (mostly on the language level) that need to be sorted out.

rust/tool/src/install.rs

rust/tool/src/esp.rs

rust/tool/src/install.rs

rust/tool/src/esp.rs

rust/tool/tests/common/mod.rs

rust/tool/tests/install.rs

alois31 · 2023-08-13T17:19:24Z

The fix for the security issue turned out to be almost a complete rewrite of this PR. I have tried to follow all review suggestions in spirit, and have closed all reviews that I think I have addressed, or were specific to the code that does not exist any more.

alois31 · 2023-09-23T13:01:55Z

All merge conflicts are resolved, and I'm running this in production again.

RaitoBezarius · 2023-09-23T13:02:40Z

OK, now I will focus on it.

RaitoBezarius

So far so good, how do you envision the migration process?

alois31 · 2023-09-23T13:16:14Z

What do you mean by "migration process"? Apart from potential disk space issues caused by the doubled ESP usage during the initial run, everything should happen automatically. I can document that if you want.

RaitoBezarius · 2023-09-23T13:41:12Z

What do you mean by "migration process"? Apart from potential disk space issues caused by the doubled ESP usage during the initial run, everything should happen automatically. I can document that if you want.

We probably want to have a guideline on how to deal with that doubled ESP usage which can cause "out of disk space" issues and cause the whole process to fail.

RaitoBezarius · 2023-09-24T15:06:05Z

I am also running this in production now, without too much issues from what I am seeing.

RaitoBezarius · 2023-09-24T15:09:27Z

I think what is really left is the migration UX:

we could have a way to statically determine if the move from non-CA to CA will fail because of not enough space
we could have a way to move our own files first in a backup area, install the CA version, rollback to the previous and fail if there's any issue

This way, this would be invisible for our users.

RaitoBezarius · 2023-09-29T13:38:24Z

@alois31 Can I bother you with the conflicts? I am inclined to merge, we can always revert later on if we figure out this may contain unforeseen issues, this has waited long enough and I think this is the good direction we should go in.

RaitoBezarius · 2023-09-29T18:01:22Z

vm-test-run-lanzaboote-exports-efi-variables is broken with that PR because we assume something about the initrd name in the test script, can you fix it? (nix/tests/lanzaboote.nix)

I know that tests evaluation is broken, I will push a PR on master to fix this.

RaitoBezarius · 2023-09-29T22:05:14Z

The modified X doesn't boot with SecureBoot are not passing anymore, they are booting :D.

alois31 · 2023-10-01T09:15:33Z

nix-build -A checks.x86_64-linux succeeds now (I only did cargo test before, sorry for not catching the failures, and it took a while because bootloader tests need that huge disk image). The root cause was the same in most cases (the test suite didn't refer to the correct files after the name change). The only interesting thing was the sorting issue, which is now fixed by explicitly marking unspecialised generations as such (which works since "unspecialised" comes after "specialisation" in the alphabet).

alois31 · 2023-10-02T08:18:39Z

FAT32 is case-insensitive, so base64 does not seem like a smart idea. I will change to nix-base32 instead.

nikstur

This looks mostly good to go. Let's merge this quickly after you have addressed my minor comments.

docs/TROUBLESHOOTING.md

rust/tool/systemd/Cargo.toml

rust/tool/systemd/src/install.rs

rust/tool/systemd/tests/gc.rs

rust/tool/systemd/tests/common/mod.rs

rust/tool/systemd/src/install.rs

alois31 · 2023-10-03T17:11:57Z

Also it seems that one of my previous rebases messed up the commit structure, and I have to untangle this again.

alois31 · 2023-10-03T19:31:11Z

Most review suggestions have been implemented now. The docs I will rebase if/when that PR is merged, and for the other two ones I will wait for a reply on my comment to know how to proceed best.

alois31 · 2023-10-03T20:05:42Z

I didn't miss that. The docs don't make sense without the ones from #233. I will rebase if/when that is merged, as already stated.

nikstur · 2023-10-03T20:06:59Z

Yes sorry overread that part. Merged the other PR. Rebase and then we can merge this one too.

Kernels and initrds on the ESP are now content-addressed. By definition, it is impossible for two different kernels or initrds to ever end up at the same place, even in the presence of changing initrd secrets or other unreproducibility. The basic advantage of this is that installing the kernel or initrd for a generation can never break another generation. In turn, this enables the following two improvements: * All generations can be installed independently. In particular, the installation can be performed in one pass, one generation at a time. As a result, the code is significantly simplified, and memory usage (due to the temporary files) does not grow with the number of generations any more. * Generations that already have their files in place on the ESP do not need to be reinstalled. This will be taken advantage of in a subsequent commit.

The stubs on the ESP are now input-addressed, where the inputs are the system toplevel and the public key used for signature. This way, it is guaranteed that any stub at a given path will boot the desired system, even in the presence of one of the two edge-cases where it was not previously guaranteed: * The latest generation was deleted at one point, and its generation number was reused by a different system configuration. This is detected because the toplevel will change. * The secure boot signing key was rotated, so old stubs would not boot at all any more. This is detected because the public key will change. Avoiding these two cases will allow to skip reinstallation of stubs that are already in place at the correct path.

nikstur · 2023-10-03T20:38:53Z

I tested locally and it all passes. I'll merge when you give me the thumbs up @alois31

Since most files (stubs, kernels and initrds) on the ESP are properly input-addressed or content-addressed now, there is no point in overwriting them any more. Hence we detect what generations are already properly installed, and don't reinstall them any more. This approach leads to two distinct improvements: * Rollbacks are more reliable, because initrd secrets and stubs do not change any more for existing generations (with the necessary exception of stubs in case of signature key rotation). In particular, the risk of a newer stub breaking (for example, because of bad interactions with certain firmware) old and previously working generations is avoided. * Kernels and initrds that are not going to be (re)installed anyway are not read and hashed any more. This significantly reduces the I/O and CPU time required for the installation process, particularly when there is a large number of generations. The following drawbacks are noted: * The first time installation is performed after these changes, most of the ESP is re-written at a different path; as a result, the disk usage increases to roughly the double until the GC is performed. * If multiple generations share a bare initrd, but have different secrets scripts, the final initrds will now be separated, leading to increased disk usage. However, this situation should be rare, and the previous behavior was arguably incorrect anyway. * If the files on the ESP are corrupted, running the installation again will not overwrite them with the correct versions. Since the files are written atomically, this situation should not happen except in case of file system corruption, and it is questionable whether overwriting really fixes the problem in this case.

Atomic write works by first writing a temporary file, then syncing that temporary file to ensure it is fully on disk before the program can continue, and in the last step renaming the temporary file to the target. The middle step was missing, which is likely to lead to a truncated target file being present after power loss. Add this step. Furthermore, even with this fix, atomicity is not fully guaranteed, because FAT32 can become corrupted after power loss due to its design shortcomings. Even though we cannot really do anything about this case, adjust the comment to at least acknowledge the situation.

Due to the temporarily doubled ESP space usage, it is now easier to run into the out of space issue (once). Document how to proceed in this case without having to delete any generations. Furthermore, recovery in case of ESP corruption is now slightly more involved, because not all files are rewritten all the time. Adjust the documentation accordingly.

alois31 · 2023-10-04T06:22:39Z

I removed a stale comment caused by incorrectly resolving merge conflicts. From my point of view, this should be good to go now.

nikstur · 2023-10-04T09:08:00Z

Thank you for enduring the review process. Now it's finally merged!

blitz requested a review from nikstur June 21, 2023 22:35

nikstur mentioned this pull request Jul 25, 2023

stub: split up into a linux-bootloader crate #215

Merged