Re: [PATCH 1/3] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules

[Patrick Steinhardt <ps@pks.im>]

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

On Thu, Aug 26, 2021 at 12:08:50AM -0500, Glenn Washburn wrote:
> As an example, passing a password as a cryptomount argument is implemented.
> However, the backends are not implemented, so testing this will return a not
> implemented error.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 31 ++++++++++++++++++++++---------
>  grub-core/disk/geli.c       |  4 ++++
>  grub-core/disk/luks.c       |  4 ++++
>  grub-core/disk/luks2.c      |  4 ++++
>  include/grub/cryptodisk.h   |  8 ++++++++
>  5 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 90f82b2d3..b966b19ab 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},
> +    {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
>  }
>  
>  static grub_err_t
> -grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> +grub_cryptodisk_scan_device_real (const char *name,
> +				  grub_disk_t source,
> +				  grub_cryptomount_args_t cargs)
>  {
>    grub_err_t err;
>    grub_cryptodisk_t dev;
> @@ -1015,7 +1018,9 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
>      if (!dev)
>        continue;
>      
> +    *dev->cargs = *cargs;
>      err = cr->recover_key (source, dev);
> +    *dev->cargs = NULL;
>      if (err)
>      {
>        cryptodisk_close (dev);

Is there any specific reason why you choose to set `cargs` as a struct
field? Seeing uses like this makes me wonder whether it wouldn't be
better to pass in `cargs` as explicit arguments whenever required. This
would also automatically answer any lifetime questions which arise.

[snip]
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 13103ea6a..e2a4a3bf5 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -165,6 +165,10 @@ luks_recover_key (grub_disk_t source,
>    grub_size_t max_stripes = 1;
>    char *tmp;
>  
> +  /* Keyfiles are not implemented yet */
> +  if (dev->cargs->key_data || dev->cargs->key_len)
> +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +
>    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>    if (err)
>      return err;

Hum. This makes me wonder whether we'll have to adjust all backends
whenever we add a new parameter to `cargs` to return `NOT_IMPLEMENTED`.
I fear that it won't scale nicely, and that it is a recipe for passing
unsupported arguments to backends even though they don't know to handle
them.

Not sure about a nice alternative though. The only idea I have right now
is something like a capabilities bitfield exposed by backends: only if a
specific bit is set will we pass the corresponding field to such a
backend. This would allow us to easily centralize the logic into a
single function which only receives both the capabilities bitset and the
`cargs` struct.

Other than that I really like where this is going.

Patrick

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