Re: [PATCH v2 0/3] Add ability to use SEV provisioned secrets for disk decryption

[Glenn Washburn]

On Fri, 13 Nov 2020 14:25:07 -0800
James Bottomley <jejb@linux.ibm.com> wrote:

> v2: update geli.c to use conditional prompt and add callback for
>     variable message printing and secret destruction
> 
> To achieve encrypted disk images in the AMD SEV encrypted virtual
> machine, we need to add the ability for grub to retrieve the disk
> passphrase from the SEV launch secret.  To do this, we've modified
> OVMF to set aside an area for the injected secret and pass up a
> configuration table for it:
> 
> https://edk2.groups.io/g/devel/topic/78198617#67339
> 
> The patches in this series modify grub to look for the disk passphrase
> in the secret configuration table and use it to decrypt any disks in
> the system if they are found.  This is so an encrypted image with a
> properly injected password will boot without any user intervention.
> 
> The three patches firstly modify the cryptodisk consumers to allow
> arbitrary password getters instead of the current console based one.

I like this idea in general.

> The next patch adds a '-s' option to cryptodisk to allow it to use a
> saved password and the final one adds a sevsecret command to check for
> the secrets configuration table and provision the disk passphrase from
> it if an entry is found.

I'm not in favor of this approach.  This feels like a special case of
providing a key file to cryptomount.  We have working (and I believe
merge worthy) patches for adding key file support. Unfortunately, due
to the current position in the grub development cycle, they have not
been merged.  As a side note, it might be interesting to re-work the
key file patch series to use the arbitrary password getter mechanism
you've created.

What I would prefer, because it feels more generic, is to have the
sevsecret module create a procfs entry (perhaps (proc)/sevsecret),
which outputs the secret data when read (or NULL string if some error
in finding the secret).  Then to cryptomount all devices that accept
the sev secret do:

  cryptomount -a -k (proc)/sevsecret

In this case you could re-use most of the code in
grub_efi_sevsecret_find and creating the procfs entry would be trivial
(see bottom of cryptodisk.c for an example on how to do this).

One potential issue could be getting error messages from
grub_efi_sevsecret_find back to the user and a solution could be to
replace the grub_error with grub_dprintf("sev", ...) statements and set
debug=sev unconditionally.  In most cases no output would be
generated, but some debug log messages should be generated on error.

Also, if this series does end up adding an option to cryptomount, a
documentation patch should be added.  I think we should start
documenting procfs paths as well.

Also, out of curiosity, is it possible that there are multiple
GRUB_EFI_DISKPASSWD_GUID entries defined?  You only get the first one,
but I'm wondering if the spec allows for more.

> With all this in place, the sequence to boot
> an encrypted volume without user intervention is:
> 
> sevsecret
> cryptomount -s
> source (crypto0)/boot/grub.cfg
> 
> Assuming there's a standard Linux root partition.
> 
> James

Glenn