On Fri, Nov 13, 2015 at 11:34 AM, Ignat Korchagin <ig...@cloudflare.com> wrote:
> On Wed, Nov 11, 2015 at 10:09 AM, Andrei Borzenkov <arvidj...@gmail.com>
> 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?
Checked again the UEFI spec. For globally defined variables which are
strings they specify ASCII. So if we leave this option, ascii is the
best name.
>>
>>> + 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 <sevenbitst...@gmail.com>
>>> wrote:
>>>>
>>>> On Wednesday, November 4, 2015, Andrei Borzenkov <arvidj...@gmail.com>
>>>> 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 <sevenbitst...@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> On Tuesday, November 3, 2015, Mat Troi <mattrois...@gmail.com> 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
>>>>>>> Grub-devel@gnu.org
>>>>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Grub-devel mailing list
>>>>>> Grub-devel@gnu.org
>>>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Grub-devel mailing list
>>>>> Grub-devel@gnu.org
>>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
