Page MenuHomeFreeBSD

GELI support for EFI loader
AbandonedPublic

Authored by eric_metricspace.net on Apr 26 2017, 5:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 27 2024, 4:20 AM
Unknown Object (File)
Dec 25 2023, 2:28 AM
Unknown Object (File)
Dec 20 2023, 1:15 AM
Unknown Object (File)
Dec 12 2023, 6:29 PM
Unknown Object (File)
Nov 25 2023, 4:37 AM
Unknown Object (File)
Nov 8 2023, 12:30 PM
Unknown Object (File)
Nov 7 2023, 7:22 PM
Unknown Object (File)
Nov 7 2023, 7:12 PM

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

lifanov added inline comments.
sys/boot/efi/boot1/Makefile
18

This block now appears twice.

eric_metricspace.net edited the test plan for this revision. (Show Details)

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

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

sys/boot/boot_crypto/boot_crypto_aes.c
100

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

139

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

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
168

how is this not reached? it looks perfectly reachable.

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 marked 5 inline comments as done.

Rebased to head, addressed reviewer comments

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.

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
7

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

18

This needs to be SYSDIR

23

This needs to be SRCTOP

27

This needs to be SYSDIR

28

This needs to be SYSDIR

30

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

sys/boot/boot_crypto/boot_crypto.h
35–40

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
67

This needs to be updated to use BOOTDIR

111

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

118–120

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
485

style changes should be separated out.

509

style change

526

style change

624

style change

631

debug change should be separate.

650

style change

664

don't use magic numbers, define a constant.

902–910

This change should be separate.

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.

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"?

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.

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...

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.

Fix error that prevented ZFS preferred pool detection

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).

An alternate approach to GELI was merged.