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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3997 bytes --]

On Fri, Nov 13, 2020 at 07:50:38PM -0600, Glenn Washburn wrote:
> 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.

Ah, should've read all messages first. I'm now repeating kind of the
same thing as a reply to patch 1/3.

> 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

Interesting approach. I think I tend to agree, but I'm not too sure
about potential security implications. Anyway, I still like the password
callback function per-se as it nicely deduplicates existing code
already. So regardless of where we end up here, I think that would be
nice to have anyway.

Patrick

> 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
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]