Re: Grub get and set efi variables

[Andrei Borzenkov]

06.11.2015 05:00, Ignat Korchagin пишет:
> Actually, I submitted similar patch recently as well. It provides read
> function for variables and accepts a hint on how to process them. The
> original patch is here:
> https://lists.gnu.org/archive/html/grub-devel/2015-09/msg00043.html.

Yes, I was intending to comment on it, sorry.

I am still unsure whether printf-like format specifier could be more 
flexible, but otherwise I like it in that it provides for future 
extensions. see comments below.

> Probably, I forgot to enable plain-text mode, so it got there as a
> binary attachment. I will repeat it here for convenience.
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 9764cd2..49fa3ec 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -728,6 +728,12 @@ module = {
>   };
>
>   module = {
> +  name = efivar;
> +  efi = commands/efi/efivar.c;
> +  enable = efi;
> +};
> +
> +module = {
>     name = blocklist;
>     common = commands/blocklist.c;
>   };
> diff --git a/grub-core/commands/efi/efivar.c b/grub-core/commands/efi/efivar.c
> new file mode 100644
> index 0000000..ca206eb
> --- /dev/null
> +++ b/grub-core/commands/efi/efivar.c
> @@ -0,0 +1,146 @@
> +/* efivar.c - Read EFI global variables. */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2015 Free Software Foundation, Inc.
> + *  Copyright (C) 2015 CloudFlare, Inc.
> + *
> + *  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/types.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +#include <grub/efi/api.h>
> +#include <grub/efi/efi.h>
> +#include <grub/extcmd.h>
> +#include <grub/env.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static const struct grub_arg_option options[] = {
> +  {"type", 't', GRUB_ARG_OPTION_OPTIONAL, N_("Parse EFI_VAR as
> specific type (hex, uint8, string). Default: hex."), N_("TYPE"),
> ARG_TYPE_STRING},
> +  {0, 0, 0, 0, 0, 0}
> +};
> +
> +enum efi_var_type
> +  {
> +    EFI_VAR_STRING = 0,
> +    EFI_VAR_UINT8,
> +    EFI_VAR_HEX,
> +    EFI_VAR_INVALID = -1
> +  };
> +
> +static enum efi_var_type
> +parse_efi_var_type (const char *type)
> +{
> +  if (!grub_strncmp (type, "string", sizeof("string")))
> +    return EFI_VAR_STRING;
> +

I think this should be "ascii" or "utf8". "string" is too ambiguous in 
UEFI environment, it can also mean sequence of UCS-2 characters.

> +  if (!grub_strncmp (type, "uint8", sizeof("uint8")))
> +    return EFI_VAR_UINT8;
> +
> +  if (!grub_strncmp (type, "hex", sizeof("hex")))
> +    return EFI_VAR_HEX;
> +
> +  return EFI_VAR_INVALID;
> +}
> +
> +static grub_err_t
> +grub_cmd_get_efi_var (struct grub_extcmd_context *ctxt,
> +  int argc, char **args)
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +  grub_err_t status;
> +  void *efi_var = NULL;
> +  grub_size_t efi_var_size = 0;
> +  enum efi_var_type efi_type = EFI_VAR_HEX;
> +  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
> +  char *env_var = NULL;
> +  grub_size_t i;
> +
> +  if (2 != argc)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
> +

May be follow the suite and use "--set var". It could be useful to 
simply display variable on screen, like

efivar OsIndicators

or even

efivar --all

> +  if (state[0].set)
> +    efi_type = parse_efi_var_type (state[0].arg);
> +
> +  if (EFI_VAR_INVALID == efi_type)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid EFI variable type"));
> +
> +  efi_var = grub_efi_get_variable (args[0], &global, &efi_var_size);
> +  if (!efi_var || !efi_var_size)
> +    status = grub_error (GRUB_ERR_READ_ERROR, N_("cannot read variable"));
> +

Should it distinguish between non-existent variable and failure to read 
variable? Is non-existent variable an error?

> +  switch (efi_type)
> +  {
> +    case EFI_VAR_STRING:
> +      env_var = grub_malloc (efi_var_size + 1);
> +      if (!env_var)
> +        {
> +          status = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> +          break;
> +        }
> +      grub_memcpy(env_var, efi_var, efi_var_size);
> +      env_var[efi_var_size] = '\0';
> +      break;
> +
> +    case EFI_VAR_UINT8:
> +      env_var = grub_malloc (4);
> +      if (!env_var)
> +        {
> +          status = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> +          break;
> +        }
> +      grub_snprintf (env_var, 4, "%u", *((grub_uint8_t *)efi_var));
> +      break;
> +
> +    case EFI_VAR_HEX:
> +      env_var = grub_malloc (efi_var_size * 2 + 1);
> +      if (!env_var)
> +        {
> +          status = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> +          break;
> +        }
> +      for (i = 0; i < efi_var_size; i++)
> +        grub_snprintf (env_var + (i * 2), 3, "%02x", ((grub_uint8_t
> *)efi_var)[i]);
> +      break;
> +
> +    default:
> +      status = grub_error (GRUB_ERR_BUG, N_("should not happen (bug
> in module?)"));
> +  }
> +
> +  status = grub_env_set (args[1], env_var);
> +
> +  if (env_var)
> +    grub_free (env_var);
> +
> +  if (efi_var)
> +    grub_free (efi_var);
> +
> +  return status;
> +}
> +
> +static grub_extcmd_t cmd = NULL;
> +
> +GRUB_MOD_INIT (efivar)
> +{
> +  cmd = grub_register_extcmd ("get_efivar", grub_cmd_get_efi_var, 0,
> N_("[-t TYPE] EFI_VAR ENV_VAR"),
> + N_("Read EFI variable and put its contents in environment
> variable."), options);
> +}
> +
> +GRUB_MOD_FINI (efivar)
> +{
> +  if (cmd)
> +    grub_unregister_extcmd (cmd);
> +}
>
>
> On Thu, Nov 5, 2015 at 10:25 AM, SevenBits <address@hidden> wrote:
> > On Wednesday, November 4, 2015, Andrei Borzenkov <address@hidden>
> > wrote:
> > > 04.11.2015 02:05, Mat Troi пишет:
> > > > Hi SevenBits,
> > > >
> > > > Thanks for letting me know.  Out of curiosity, any particular reason why
> > > > this patch did not get merged?  It looks to be a useful feature.
> > >
> > > First, this patch was reverse patch :)
> >
> >
> > Yeah, I think I had accidentally passed the arguments to the diff command in
> > the wrong order.
> >
> > > I am not convinced making it easy to set EFI variable from within GRUB is
> > > good thing, because there are known reports about systems rendered
> > > unbootable by writing too much into EFI flash. What is your use case that
> > > absolutely requires setting EFI variables? How are you going to implement it
> > > on other platforms?
> >
> >
> > I should probably note that I wrote this patch for a specific project of
> > mine which required the ability to read UEFI variables. I added in write
> > functionality for good measure because I could. But I agree, this would only
> > encourage tinkering and users messing with their systems and potentially
> > bricking it, which would of course be blamed on GRUB.
> >
> > > Reading does not harm and may be useful, but then it should provide
> > > generic interface to access arbitrary vendor namespace, not only EFI global
> > > variables, and handle arbitrary binary data, even if initial implementation
> > > handles only subset of them. Once someone starts to use it changing it will
> > > be much more difficult.
> > >
> > > Maybe it should take hints how to interpret variable values, or have
> > > format option akin to printf.
> >
> >
> > I would be happy to resume work on this, and debase it on the current code,
> > if GRUB has a clear need for such functionality. I would prefer to have it
> > be clear what the patch should consist of, though.
> >
> > A key question would be how to e.g. handle arbitrary data stored in
> > variables if it is not something easily represent able like a string. Right
> > now, the patch stores it's value into an environment variable specified by
> > the user. Unless the powers that be think otherwise, I think this is the
> > best way to go.
> >
> > > > Thanks,
> > > > Mat
> > > >
> > > > On Tue, Nov 3, 2015 at 12:12 PM, SevenBits <address@hidden>
> > > > wrote:
> > > >
> > > > > On Tuesday, November 3, 2015, Mat Troi <address@hidden> wrote:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Searching through google, I see there was an email thread to add a
> > > > > > patch
> > > > > > for getting and setting efi variable in GRUB2.
> > > > > > https://lists.gnu.org/archive/html/grub-devel/2013-11/msg00328.html
> > > > > >
> > > > > > However, looking at the tree, it doesn't look like this patch was
> > > > > > added,
> > > > > > am I missing something?  Does anyone know if we have command to get/set
> > > > > > efi
> > > > > > variables in GRUB2?
> > > > > >
> > > > > >
> > > > > >
> > > > > > http://git.savannah.gnu.org/gitweb/?p=grub.git;a=tree;f=grub-core/commands/efi;h=005fd2efc9a2eede2a1eb1cab8081c360219107b;hb=HEAD
> > > > > I'm the author of that patch. No, it was never merged into the tree. As
> > > > > far as I know, there is no equivalent functionality; GRUB does not
> > > > > support
> > > > > this feature.
> > > > >
> > > > >
> > > > > > Thanks,
> > > > > > Mat
> > > > >
> > > > > _______________________________________________
> > > > > Grub-devel mailing list
> > > > > address@hidden
> > > > > https://lists.gnu.org/mailman/listinfo/grub-devel
> > > >
> > > >
> > > >
> > > > _______________________________________________
> > > > Grub-devel mailing list
> > > > address@hidden
> > > > https://lists.gnu.org/mailman/listinfo/grub-devel
> > >
> > >
> > > _______________________________________________
> > > Grub-devel mailing list
> > > address@hidden
> > > https://lists.gnu.org/mailman/listinfo/grub-devel
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel