Comment corrected in my local repo.

" " includes should be lower then <>.

for (i =0;

Missing blank space.

Tabs.

Wrong tab.

Style:
void*

Style.
{

memset()

}

We don't assaign default values with declaration.

Is this assign help in something?

Style:

caddr_t kmdp;

Style.

keybuf_t *
get_keybuf(void)
{

        return (keybuf);
}

Whole this file is incorrect in case of style(9), so i would lave the style exacly how all file is:

#define<space>MODINFOMD_KEYBUF[tab]0x000d

nsaved_keys is unsigned; this should be too.

What ensures the passed keybuf has enough room for nsaved_keys entries?

explicit_bzero

shouldn't this sort after sys/ headers?

explicit_bzero

explicit_bzero

NBBY instead of 8?

Why use typedef for new structs? Generally, a bare struct is preferred in new code.

Additionally, _t suffix is strongly discouraged for any non-standard and especially non-scalar type.

This uses a distinct constant from G_ELI_DATAIVKEYLEN and no validation is performed that they are compatible in geli_fill_keybuf.

The sizeof() seems mismatched.

I prefer:

if (...ke_type != KEYBUF_TYPE_GELI)
    continue;

To avoid the extra level of indentation in this already heavily indented area of code. But this is fine.

ke_data and key have different constant sizes. Either they need to share a constant or this needs better validation.

Maybe:

if (found_intake)
    goto have_key;

To avoid all of this churn.

have_key: would go here.

why?

These unrelated whitespace changes are gratuitous.

explicit_bzero isn't available in the boot-time libraries presently. If it's to be added, I would suggest that be done in a separate patch.

See above.

Sure, that's fine.

save_key won't increment it past GELI_MAX_KEYS

Note that the check that MAX_KEY_BYTES is greater than G_ELI_DATAIVKEYLEN is performed in g_eli.h. This is due to complications with the BIOS GELI implementation that prevent including geom/eli/g_eli.h in this file.

I'd rather leave it as is. The continue thing could trip someone up in the future.

The key intake mechanism is designed to support different kinds of keys without a great deal of modification, so ke_data is bigger than a GELI key. It's designed with 4096-bit RSA keys in mind (ie. the largest commonly-used crypto keys). GELI keys are going to be probably 256-bits at most.

If exposed as a symbol, data exfiltration attacks become utterly trivial. Of course, these attacks should be defeated by zeroing out the keys, but there's no sense in leaving potential attack vectors lying around.

My emacs config auto-deletes trailing whitespace. I can delete them from the diff manually.

The check in g_eli.h is slightly different. It is comparing against a G_ELI_DATAKEYLEN (no "IV").

What is the distinction in constant sizes here?

It shouldn't be a problem. It's a common pattern.

If we aren't zeroing the key memory we have big problems. Leaving it as a public symbol makes that easier to validate and makes the code more straightforward. This is not a potential attack vector.

Note: this isn't my code change.

USERKEYLEN is the maximum length of a password, which is hashed to produce the mkey. DATAIVKEYLEN is 64 bytes, I believe.

I don't see any benefit to making it a public symbol that isn't realized by the existing access method. Anyway, it's moot; the API is already designed to provide access through get_keybuf and the code already uses it.

This is wrong.

in sys/geom/eli/g_eli.c you expect the keybuf to contain the decryption key for the master key, not the already-decrypted master key.

You want to save_key(key) a few lines above, before the call to g_eli_mkey_decrypt()

I strongly think we should pass around the decryption key for the master key, rather than the master key itself. If it is somehow leaked, you can easily change the decryption key using geli rekey, but if the master key itself it leaked, then the entire volume must be destroyed and rewritten with a different master key.

This was a straight-up mistake. It is supposed to be the hashed password (the decryption key), not the decrypted master key.

this was marked as done, but does not appear to be done

now that you have context in the diff, it is easier to show you why this is still wrong.

You are saving the key on line 230, but it was bzero'd on like 218.

You should move save_key up to after like 217.

I've fixed in my local repo, but I won't post the patch here until I've been able to test it (or unless there's enough other fixes to warrant doing so).

explicit_bzero isn't available in this context. As discussed elsewhere, it ought to be handled separately.

this is likely a merge conflict, but you are zeroing the key before we can call save_key.

I'll post an update with some other fixes shortly.