Re: [PATCH v2 2/3] cryptodisk: add OS provided secret support

[Glenn Washburn]

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

> Make use of the new OS provided secrets API so that if the new '-s'
> option is passed in we try to extract the secret from the API rather
> than prompting for it.
> 
> The primary consumer of this is AMD SEV, which has been programmed to
> provide an injectable secret to the encrypted virtual machine.  OVMF
> provides the secret area and passes it into the EFI Configuration
> Tables.  The grub EFI layer pulls the secret out and primes the
> secrets API with it.  The upshot of all of this is that a SEV
> protected VM can do an encrypted boot with a protected boot secret.
> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> 
> ---
> 
> v2: add callback for after secret use
> ---
>  grub-core/disk/cryptodisk.c | 61
> ++++++++++++++++++++++++++++++++++--- include/grub/cryptodisk.h   |
> 5 +++ 2 files changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 682f5a55d..508b25450 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
>      /* TRANSLATORS: It's still restricted to cryptodisks only.  */
>      {"all", 'a', 0, N_("Mount all."), 0, 0},
>      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."),
> 0, 0},
> +    {"secret", 's', 0, N_("Get OS provisioned secret and mount all
> volumes encrypted with that secret"), 0, 0}, {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -967,6 +968,12 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>  
>  static int check_boot, have_it;
>  static char *search_uuid;
> +static char *os_passwd;
> +
> +/* variable to hold the passed in secret area. */
> +static char *os_secret_area;
> +static grub_secret_cb *os_secret_cb;
> +static void *os_secret_arg;
>  
>  static void
>  cryptodisk_close (grub_cryptodisk_t dev)
> @@ -977,6 +984,17 @@ cryptodisk_close (grub_cryptodisk_t dev)
>    grub_free (dev);
>  }
>  
> +static int
> +os_password_get(char buf[], unsigned len)
> +{
> +  /* os_passwd should be null terminated, so just copy everything */
> +  grub_strncpy(buf, os_passwd, len);
> +  /* and add a terminator just in case */
> +  buf[len - 1] = 0;
> +
> +  return 1;

Shouldn't this return 0 for success?

> +}
> +
>  static grub_err_t
>  grub_cryptodisk_scan_device_real (const char *name, grub_disk_t
> source) {
> @@ -996,8 +1014,17 @@ grub_cryptodisk_scan_device_real (const char
> *name, grub_disk_t source) return grub_errno;
>      if (!dev)
>        continue;
> -    
> -    err = cr->recover_key (source, dev, grub_password_get);
> +
> +    if (os_passwd)
> +      {
> +     err = cr->recover_key (source, dev, os_password_get);
> +     if (err)
> +       /* if the key doesn't work ignore the access denied error
> */
> +       grub_error_pop();
> +      }
> +    else
> +      err = cr->recover_key (source, dev, grub_password_get);
> +

I think a cleaner approach might be to leave this essentially as it was,
just change grub_password_get to a global variable (maybe
grub_password_get_cb).  Then in grub_cmd_cryptomount set
grub_password_get_cb to grub_password_get by default, and to
os_password_get in the -s option handling.

>      if (err)
>      {
>        cryptodisk_close (dev);
> @@ -1013,6 +1040,16 @@ grub_cryptodisk_scan_device_real (const char
> *name, grub_disk_t source) return GRUB_ERR_NONE;
>  }
>  
> +grub_err_t
> +grub_cryptodisk_set_secret (char *secret, grub_secret_cb *cb, void
> *arg) +{
> +  os_secret_area = secret;

This looks like its asking to introduce memory leak bugs if something
else decides to use this. Its not in this patch series because secret
points to global memory, but for anyone else using this generic API it
might not be. Perhaps grub_cryptodisk_set_secret should take ownership
of the memory, and thus free os_secret_area if its non-NULL here.

> +  os_secret_cb = cb;
> +  os_secret_arg = arg;
> +
> +  return GRUB_ERR_NONE;
> +}
> +
>  #ifdef GRUB_UTIL
>  #include <grub/util/misc.h>
>  grub_err_t
> @@ -1089,7 +1126,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> ctxt, int argc, char **args) {
>    struct grub_arg_list *state = ctxt->state;
>  
> -  if (argc < 1 && !state[1].set && !state[2].set)
> +  if (argc < 1 && !state[1].set && !state[2].set && !state[3].set)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name
> required"); 
>    have_it = 0;
> @@ -1107,6 +1144,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> ctxt, int argc, char **args) 
>        check_boot = state[2].set;
>        search_uuid = args[0];
> +      os_passwd = NULL;
>        grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
>        search_uuid = NULL;
>  
> @@ -1117,11 +1155,25 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> ctxt, int argc, char **args) else if (state[1].set || (argc == 0 &&
> state[2].set)) {
>        search_uuid = NULL;
> +      os_passwd = NULL;
>        check_boot = state[2].set;
>        grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
>        search_uuid = NULL;
>        return GRUB_ERR_NONE;
>      }
> +  else if (state[3].set)
> +    {
> +      /* do we have a secret? */
> +      if (os_secret_area == NULL)
> +     return grub_error (GRUB_ERR_INVALID_COMMAND, "No OS secret
> is provisioned"); +
> +      os_passwd = os_secret_area;
> +      search_uuid = NULL;
> +      grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
> +      os_passwd = NULL;
> +
> +      return os_secret_cb (have_it, os_secret_arg);
> +    }
>    else
>      {
>        grub_err_t err;
> @@ -1132,6 +1184,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> ctxt, int argc, char **args) grub_size_t len;
>  
>        search_uuid = NULL;
> +      os_passwd = NULL;
>        check_boot = state[2].set;
>        diskname = args[0];
>        len = grub_strlen (diskname);
> @@ -1299,7 +1352,7 @@ GRUB_MOD_INIT (cryptodisk)
>  {
>    grub_disk_dev_register (&grub_cryptodisk_dev);
>    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> -                           N_("SOURCE|-u UUID|-a|-b"),
> +                           N_("SOURCE|-u UUID|-a|-b|-s"),
>                             N_("Mount a crypto device."), options);
>    grub_procfs_register ("luks_script", &luks_script);
>  }
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 45dae5483..8236b5933 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -163,4 +163,9 @@ grub_util_get_geli_uuid (const char *dev);
>  grub_cryptodisk_t grub_cryptodisk_get_by_uuid (const char *uuid);
>  grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t
> disk); 
> +typedef grub_err_t (grub_secret_cb)(int have_it, void *arg);
> +
> +grub_err_t grub_cryptodisk_set_secret(char *secret, grub_secret_cb
> *cb,
> +                                   void *arg);
> +
>  #endif