Re: [PATCH 1/5] protectors: Add key protectors framework

[Glenn Washburn]

On Mon, 24 Jan 2022 06:12:14 -0800
Hernan Gatta <hegatta@linux.microsoft.com> wrote:

> From: Hernan Gatta <hegatta@microsoft.com>
> 
> A key protector encapsulates functionality to retrieve an unlocking key for a
> fully-encrypted disk from a specific source. A key protector module registers
> itself with the key protectors framework when it is loaded and unregisters 
> when
> unloaded. Additionally, a key protector may accept parameters that describe 
> how
> it should operate.
> 
> The key protectors framework, besides offering registration and unregistration
> functions, also offers a one-stop routine for finding and invoking a key
> protector. This method accepts a formatted string with the name of a key
> protector followed optionally by colon-separated, key protector-specific
> parameters. If a key protector with the specified name exists and if an
> unlocking key is successfully retrieved by the latter, the function returns to
> the caller the retrieved key and its length.
> 
> Signed-off-by: Hernan Gatta <hegatta@linux.microsoft.com>
> ---
>  grub-core/Makefile.am       |  1 +
>  grub-core/Makefile.core.def |  1 +
>  grub-core/kern/protectors.c | 98 
> +++++++++++++++++++++++++++++++++++++++++++++
>  include/grub/protector.h    | 55 +++++++++++++++++++++++++
>  4 files changed, 155 insertions(+)
>  create mode 100644 grub-core/kern/protectors.c
>  create mode 100644 include/grub/protector.h
> 
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index ee88e44..f78cd9d 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -90,6 +90,7 @@ endif
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/mm.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/parser.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/partition.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/protector.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/stack_protector.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/term.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/time.h
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8022e1c..e4ae78b 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -138,6 +138,7 @@ kernel = {
>    common = kern/misc.c;
>    common = kern/parser.c;
>    common = kern/partition.c;
> +  common = kern/protectors.c;
>    common = kern/rescue_parser.c;
>    common = kern/rescue_reader.c;
>    common = kern/term.c;
> diff --git a/grub-core/kern/protectors.c b/grub-core/kern/protectors.c
> new file mode 100644
> index 0000000..2df0c60
> --- /dev/null
> +++ b/grub-core/kern/protectors.c
> @@ -0,0 +1,98 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022 Microsoft Corporation
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/list.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/protector.h>
> +
> +struct grub_key_protector *grub_key_protectors = NULL;
> +
> +grub_err_t
> +grub_key_protector_register (struct grub_key_protector *protector)
> +{
> +  if (!protector || !protector->name || !grub_strlen(protector->name))
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  if (grub_key_protectors &&
> +      grub_named_list_find (GRUB_AS_NAMED_LIST (grub_key_protectors),
> +                            protector->name))
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  grub_list_push (GRUB_AS_LIST_P (&grub_key_protectors),
> +                  GRUB_AS_LIST (protector));
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_key_protector_unregister (struct grub_key_protector *protector)
> +{
> +  if (!protector)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  grub_list_remove (GRUB_AS_LIST (protector));
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_key_protector_recover_key (char *args, grub_uint8_t **key,
> +                                grub_size_t *key_size)
> +{
> +  char *first_separator = NULL;
> +  char *protector_args = NULL;
> +  struct grub_key_protector *protector = NULL;
> +
> +  if (!grub_key_protectors)
> +    return GRUB_ERR_OUT_OF_RANGE;
> +
> +  if (!args || !grub_strlen (args))
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  /* Find the position of the first parameter separator: the stuff before it 
> is
> +   * the name of the requested key protector and the stuff after it are the
> +   * parameters for said key protector, if any. */
> +  first_separator = grub_strchr (args, ':');
> +  if (first_separator)
> +    {
> +      /* Reject a separator at the very beginning. */
> +      if (first_separator == args)
> +        return GRUB_ERR_BAD_ARGUMENT;
> +
> +      /* Having a lone colon after the name of the key protector with no
> +       * further parameters thereafter does not make any sense. */
> +      if (*(first_separator + 1) == '\0')
> +        return GRUB_ERR_BAD_ARGUMENT;

This should be allowed, its an unnecessary restriction. For instance,
it prevents building this string without logic to determine if a colon
is needed or not. Instead of this if block, I suggest having...

> +
> +      /* Consume the colon, effectively splitting 'args' in two. */
> +      first_separator[0] = '\0';
> +

... here, an if with the opposite conditional.

if (*(first_separator + 1) != '\0')

> +      /* The protector-specific arguments are after the first colon. */
> +      protector_args = first_separator + 1;
> +    }
> +
> +  /* Try to find a key protector with the specified name. */
> +  protector = grub_named_list_find (GRUB_AS_NAMED_LIST (grub_key_protectors),
> +                                    args);
> +  if (!protector)
> +    return GRUB_ERR_OUT_OF_RANGE;
> +
> +  /* 'protector_args' may be NULL. */
> +  return protector->recover_key (protector_args, key, key_size);
> +}
> diff --git a/include/grub/protector.h b/include/grub/protector.h
> new file mode 100644
> index 0000000..d445a33
> --- /dev/null
> +++ b/include/grub/protector.h
> @@ -0,0 +1,55 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022 Microsoft Corporation
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef GRUB_PROTECTOR_HEADER
> +#define GRUB_PROTECTOR_HEADER 1
> +
> +#include <grub/err.h>
> +#include <grub/types.h>
> +
> +enum grub_key_protector_flags
> +  {
> +    GRUB_KEY_PROTECTOR_FLAG_NONE = 0,
> +    GRUB_KEY_PROTECTOR_FLAG_REPEATABLE = 1 << 0
> +  };
> +
> +struct grub_key_protector
> +{
> +  struct grub_key_protector *next;
> +  struct grub_key_protector **prev;
> +
> +  const char *name;
> +  enum grub_key_protector_flags flags;
> +
> +  grub_err_t (*recover_key) (char *args, grub_uint8_t **key,
> +                             grub_size_t *key_size);
> +};
> +
> +extern struct grub_key_protector *EXPORT_VAR (grub_key_protectors);
> +
> +grub_err_t
> +EXPORT_FUNC (grub_key_protector_register) (struct grub_key_protector 
> *protector);
> +
> +grub_err_t
> +EXPORT_FUNC (grub_key_protector_unregister) (struct grub_key_protector 
> *protector);
> +
> +grub_err_t
> +EXPORT_FUNC (grub_key_protector_recover_key) (char args[], grub_uint8_t 
> **key,
> +                                              grub_size_t *key_size);
> +
> +#endif /* ! GRUB_PROTECTOR_HEADER */

Glenn