GELI support for EFI loader

[eric_metricspace.net]

Summary

This patch adds drivers necessary to support GELI in the EFI loader. This patch is relative to the changes in https://reviews.freebsd.org/D10447.

The work has two main components: The first is an EFI KMS pseudo-device driver which serves as the method for passing keys between boot1 and loader. The second is the GELI driver itself, which uses the EFI driver interface. Another portion of the original work from this patch was committed as the keybufs patch; this work uses keybufs to send keys into the kernel.

This patch should correctly handle such cases as GPT schemes inside GELI volumes, nested GELI volumes, and others, assuming the underlying EFI implementation is fully-compliant. As such, it adds a freebsd-geli partition label (which also serves to avoid providing information about encrypted partition contents to attackers).

Finally, the patch creates a separate "boot_crypto" library under sys/boot. This is intentionally kept separate from the crypto code used by the i386 boot loader. This is due to the fact that the i386 loader is space-constrained, whereas EFI and modern firmware-based other loaders generally are not. The new boot_crypto library is intentionally designed to be extended to incorporate more ciphers (such as camellia and others supported by GELI), and quite possibly DSA algorithms for signature checking.

Note that a ZFS-related bug has been discovered in https://reviews.freebsd.org/D10447, which seems to affect this patch as well. However, the patch works for non-ZFS systems, and the ordinary code review can proceed while the bug is being investigated.

Test Plan

This code has been adapted from an implementation that has been tested on real hardware; however, it has not undergone such testing itself.

This code has been in use on real hardware, with a pure GELI+EFI boot setup hosting a ZFS pool spread across multiple devices since mid-May 2017.

[lifanov]

sys/boot/efi/boot1/Makefile

18 ↗

(On Diff #27757)

This block now appears twice.

[eric_metricspace.net]

Resolved issues with booting, tested with successful boot on QEMU. This should work now, but still needs to be tested on real hardware.

[lattera-gmail.com]

D10447, the patch this depends upon, needs to be updated to latest HEAD. I'd love to help test this patch out.

[eric_metricspace.net]

Rebase to HEAD

[allanjude]

sys/boot/efi/libefi/geli_driver.c

148 ↗

(On Diff #30196)

this should be removed:

https://svnweb.freebsd.org/base/head/sys/boot/geli/pwgets.c?r1=296963&r2=314213

[allanjude]

sys/boot/boot_crypto/boot_crypto_aes.c

99 ↗

(On Diff #30196)

sys/boot/boot_crypto/boot_crypto_aes.c:99:1: warning: control reaches end of non-void function [-Wreturn-type]

138 ↗

(On Diff #30196)

sys/boot/boot_crypto/boot_crypto_aes.c:138:1: warning: control reaches end of non-void function [-Wreturn-type]

sys/boot/efi/libefi/efipart.c

395 ↗

(On Diff #30196)

looks like you renamed 'node' to 'lastnode', but are checking 'node' for NULL, and it is never set.

sys/boot/efi/libefi/geli_driver.c

167 ↗

(On Diff #30196)

how is this not reached? it looks perfectly reachable.

[allanjude]

With this minor fix: https://github.com/allanjude/freebsd/commit/bc4280b8702e3f982cc8624d052125c5b5a3face.diff

This UEFI booted a GELI encrypted ZFS for me. There is a one-line fix to the installer to stop it making an unencrypted bootpool.

[eric_metricspace.net]

Rebased to head, addressed reviewer comments

[eric_metricspace.net]

Rebased to HEAD

[eric_metricspace.net]

This one is finally on deck. I am currently running a build/test cycle after merging from HEAD following the commit of boot1_refactor. I don't anticipate breakage, but it's best to be sure. Allan should run his tests once I get through mine, since he found some issues I didn't.

Once I've got everything tested, this could probably be broken up into three or four sub-commits:

• Add the freebsd-geli partition type
• Add the EFI key management and interaction with the intake buffer
• Revert changes to efipart that manually parse and handle partitions from the underlying disk, as opposed to using the EFI_HANDLES for the partitions (this obviously won't work with GELI at all)
• Add the GELI driver

I'm ok with either way, committing some or all of these separately or just putting the whole thing in at once. I'll leave it up to the committers.

[imp]

This change is way too big. You need to break it down into smaller bites (see my series of commits for the size of bytes that I want to see). There's been all kinds of issues that I've had to cleanup with the last round of patches, and I'm not going to do that again. Please submit a series of small patches. And please rebase often so that there's not regressions (I caught several in D10443, but a few slipped through I have to clean back up now). With smaller changes, it's a lot easier for me to mange, and it's a lot easier to rebase.I stopped about 1/4 the way through this review because there's so many things that need to be fixed.

sys/boot/boot_crypto/Makefile

6 ↗

(On Diff #31487)

this likely needs to be updated to use the refactored defs.mk.

17 ↗

(On Diff #31487)

This needs to be SYSDIR

22 ↗

(On Diff #31487)

This needs to be SRCTOP

26 ↗

(On Diff #31487)

This needs to be SYSDIR

27 ↗

(On Diff #31487)

This needs to be SYSDIR

29 ↗

(On Diff #31487)

Nope. Can't commit this. If the asserts are firing the code must be fixed.

sys/boot/boot_crypto/boot_crypto.h

34–39 ↗

(On Diff #31487)

That would be much better. Perhaps you should create a small change that does that instead of recreate the defines here.

sys/boot/efi/boot1/Makefile

64 ↗

(On Diff #31487)

This needs to be updated to use BOOTDIR

108 ↗

(On Diff #31487)

Move this to defs.mk and use ${OBJTOP}/sys/boot/boot_crypto/libboot_crytpo.a

115–116 ↗

(On Diff #31487)

Generally, LIBSTAND should be last. And this patch needs to be reworked to use the new LIBSA that I just committed.

sys/boot/efi/boot1/boot1.c

487 ↗

(On Diff #31487)

style changes should be separated out.

511 ↗

(On Diff #31487)

style change

528 ↗

(On Diff #31487)

style change

626 ↗

(On Diff #31487)

style change

633 ↗

(On Diff #31487)

debug change should be separate.

652 ↗

(On Diff #31487)

style change

666 ↗

(On Diff #31487)

don't use magic numbers, define a constant.

902–910 ↗

(On Diff #31487)

This change should be separate.

[eric_metricspace.net]

This one breaks up much easier, since it's mostly new code. Be aware, however, that the changes will introduce dead code until the GELI driver itself goes in.

[jmaloney_ixsystems.com]

In D10512#263482, @eric_metricspace.net wrote:

This one breaks up much easier, since it's mostly new code. Be aware, however, that the changes will introduce dead code until the GELI driver itself goes in.

Hi Eric. I would like to merge in this work as is into TrueOS if it still works. Are there any merges besides the 4 commits from 10-17-17 that are needed for us to merge as is? Is there a separate commit for the "GELI driver"?

[eric_metricspace.net]

In D10512#264865, @jmaloney_ixsystems.com wrote:

In D10512#263482, @eric_metricspace.net wrote:

This one breaks up much easier, since it's mostly new code. Be aware, however, that the changes will introduce dead code until the GELI driver itself goes in.

Hi Eric. I would like to merge in this work as is into TrueOS if it still works. Are there any merges besides the 4 commits from 10-17-17 that are needed for us to merge as is? Is there a separate commit for the "GELI driver"?

I'd say hang back for the moment. I split up the patch last week, now I need to test each bit. Plus, we're about to yank boot1, so things are kind of in a state of flux right now.

[imp]

I agree with Eric. Bringing it into TrueOS is a guaranteed set of conflicts in the future.

I'll try to get Erik's split stuff in as soon as I can...

[eric_metricspace.net]

The current state of things combines all the GELI precursors and the dual-purpose loader patch, then applies the GELI driver. This is placed here for testing.

Note that GELI only works with a loader-only installation.

[eric_metricspace.net]

Fix error that prevented ZFS preferred pool detection

[eric_metricspace.net]

This is now deployed on a laptop with a multi-device, all-GELI ZFS pool. It boots with loader.efi depolyed to the ESP (a no-boot1 configuration).

[eric_metricspace.net]

An alternate approach to GELI was merged.