Re: Grub get and set efi variables

[Ignat Korchagin]

On Wed, Nov 11, 2015 at 10:09 AM, Andrei Borzenkov <address@hidden> wrote:
> 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.
I'm still not sure how exactly GRUB + UEFI interprets "raw buffers"
when printing. Maybe, to avoid confusion, it might be better to
completely remove this option. Basically, you do not want to interpret
raw buffers as strings. For best compatibility "hex" mode should be
promoted, I guess. What do you think?
>
>> +  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
>
Yes. Agree.
>> +  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?
>
>
grub_efi_get_variable itself does not report the difference. It should
be modified to do that. From possible use-cases does it really matter
why you did not get the variable?
>> +  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
>>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel