Fix GELIBoot support if GELI sector size is > 512

[allanjude]

Summary

Add support for 4k sector GELI encrypted partitions to the bootloader
This is the default created by the installer

Because the IV is different for each sector, and the XTS tweak carries forward you can not decrypt a partial sector if the starting offset is not 0

Make boot2 and the loader read in 4k aligned chunks

Test Plan

Testing on a laptop and in virtualbox with 512 and 4096 byte GELI sectors
Tested with ZFS with ashift=9, which resulted in unaligned reads that required additional code to get working

[ed]

Hi there,

Thanks for working on this! I've written some comments for you, but also have a small number of general remarks:

• Casts: Your code seems to have an above average number of casts in it. Casts should only be used in the rare occasion that they're actually needed. They may also be the source of subtle bugs. For example, in your code you use ~(unsigned int)x in a couple of places. This can be disastrous when promoted to 64 bits. uint64_t x = ~(unsigned int)1; does not equal 0xfffffffffffffffe, but 0xfffffffe.
• Comments: style(9) requires that comments have a full stop at the end. Also make sure that multi-line comments are properly formatted.
• Operators: style(9) requires that unary operators (casts, logical/bitwise negation) have no trailing whitespace. If a statements needs to be broken up across multiple lines, make sure that the operator is written at the end of the first line.

sys/boot/geli/geliboot.c

80–83

The name of the variable somewhat contradicts its use. My initial thought would be that this computes the byte offset of the disk sector inside of the GELI sector (the remainder), but if I take a look at the arithmetic, it rounds the offset of the disk sector down to its GELI sector. Maybe it should be called alignsector?

88

Instead of copying, you could also just turn buf into a pointer, pointing to the last sector, right? Maybe you could just eliminate buf and call eli_metadata_decode() directly with the right offset?

sys/boot/geli/geliboot.h

59

I'm personally not too happy with naming like this. The value that it has is already encoded in the name. I would be amazed if this definition would have any value other than 4096.

I think that the intent behind this definition is that it corresponds to GELI's block size, right? Maybe it should be called DEV_GELI_BSIZE? Most of the comments in the code below also mention 512 and 4096 explicitly, which I'm not a fan of.

sys/boot/i386/zfsboot/zfsboot.c

258

s/leadingg/leading/

[allanjude]

In D5820#124356, @ed wrote:

Hi there,

Thanks for working on this! I've written some comments for you, but also have a small number of general remarks:

• Casts: Your code seems to have an above average number of casts in it. Casts should only be used in the rare occasion that they're actually needed. They may also be the source of subtle bugs. For example, in your code you use ~(unsigned int)x in a couple of places. This can be disastrous when promoted to 64 bits. uint64_t x = ~(unsigned int)1; does not equal 0xfffffffffffffffe, but 0xfffffffe.

Yes, I was concerned about this, this is why I am seeking review.
Currently, the explicit casts all immediately proceed the variable that we are taking the complement of, to ensure the correct value is used
The casts all match the destination variable.

• Comments: style(9) requires that comments have a full stop at the end. Also make sure that multi-line comments are properly formatted.

I think I got all of these

• Operators: style(9) requires that unary operators (casts, logical/bitwise negation) have no trailing whitespace. If a statements needs to be broken up across multiple lines, make sure that the operator is written at the end of the first line.

Fixed

[allanjude]

Address feedback from Ed

Remove all magic numbers

[allanjude]

sys/boot/geli/geliboot.h

59

GELI blocks can be 512 bytes or 4096 bytes. The code just assumes 4096 because that way it works in either case.

I guess I'll call it DEV_GELIBOOT_BSIZE since it is not specific to GELI

[ed]

In D5820#124449, @allanjude wrote:

Currently, the explicit casts all immediately proceed the variable that we are taking the complement of, to ensure the correct value is used

But in some cases these casts provide absolutely no value. For example, take the expression below:

(int)((DEV_GELIBOOT_BSIZE / BIOSDISK_SECSIZE) - 1)

It would work equally well without the cast. We're just doing simple arithmetic on some integer literals here.

The casts all match the destination variable.

Sure, but if anyone ever refactors this code, this may easily become no longer the case.

[oshogbo]

Only some style nits.

sys/boot/geli/geliboot.c

263

Over 80.

sys/boot/i386/libi386/biosdisk.c

725

Style.
I would go with:
alignlba = dblk &

~(daddr_t)((DEV_GELIBOOT_BSIZE / BIOSDISK_SECSIZE) - 1);

732

Up.

sys/boot/i386/zfsboot/zfsboot.c

248

Over 80.

[allanjude]

In D5820#124493, @ed wrote:

In D5820#124449, @allanjude wrote:

Currently, the explicit casts all immediately proceed the variable that we are taking the complement of, to ensure the correct value is used

But in some cases these casts provide absolutely no value. For example, take the expression below:

(int)((DEV_GELIBOOT_BSIZE / BIOSDISK_SECSIZE) - 1)

Maybe it is the bad line wrap, but we are taking the compliment of that value, hence the cast.

It would work equally well without the cast. We're just doing simple arithmetic on some integer literals here.

The casts all match the destination variable.

Sure, but if anyone ever refactors this code, this may easily become no longer the case.

[allanjude]

Fixes suggested by Ed and Marius

[saper_saper.info]

It seems to me that this change cause

ZFS: i/o error - all block copies unavailable

after some reboots (always after a crash).

I am crashing a Xen kernel to reproduce this:

https://lists.freebsd.org/pipermail/freebsd-virtualization/2016-April/004362.html

https://lists.freebsd.org/pipermail/freebsd-current/2016-April/060949.html

I have used a simple bisecting recompiling /usr/src/lib/libstand and /usr/src/sys/boot to find the broken commit.

[allanjude]

In D5820#131246, @saper_saper.info wrote:

It seems to me that this change cause

ZFS: i/o error - all block copies unavailable

after some reboots (always after a crash).

Can you compile sys/boot with the make flag: -DBOOT_BIOSDISK_DEBUG

It should give some insight into what is going wrong

[saper_saper.info]

Adding -DBOOT_BIOSDISK_DEBUG does not seem to influence gptzfsboot very much (I am even getting the same binary back).

I have managed to reproduce this easily using a FreeBSD 11 r298793 image using the installer, with partition sizes adjusted and the "Force 4K sector size" option off.

Here is the zfs stream of the zvol containing the whole device: http://marcincieslak.com/tmp/freebsdX.snapshot.xz (root password is "tester"). This is a pristine version right after the installer was done, therefore it always boots. I have used Xen to boot this one but it might work on bhyve as well (but one really needs console).

However, sometimes just a plain shutdown -r now causes the gptzfsboot abort. I think it depends on what kind of ZFS activity was done prior to the reboot.

But saying sysctl debug.kdb.panic=1 and rebooting is a 100% way to reproduce this.