* [PATCH 0/4] EFI envblk
@ 2023-07-26 20:59 Glenn Washburn
2023-07-26 20:59 ` [PATCH 1/4] efi: Load env block from GRUB_ENV EFI variable in early init Glenn Washburn
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Glenn Washburn @ 2023-07-26 20:59 UTC (permalink / raw)
To: grub-devel, Daniel Kiper; +Cc: Peter Jones, Glenn Washburn
This patch series (for me) was motivated by the "gdb: Add more support for
debugging on EFI platforms" patch series, which allowed printing in early
EFI init of the GDB command for properly loading symbols. That approach of
having the functionality be included at compile time was ultimately
rejected. During the discussion of that series, Robbie suggested[1] using
patches by Peter and in the Redhat downstream repo which uses an EFI
variable to store a GRUB env block. Using this, a user could store a
variable in the env block stored in the EFI variable and then have GRUB
load that env block in early init as a way to enable the printing of the
GDB command.
I've taken the original patches by Peter, made more suitable for including
in GRUB, fixed some bugs, and added a minor feature. The first patch, adds
env block loading in early EFI init from the GRUB_ENV EFI variable. The
second patch is included to provide tools for GRUB to set this EFI env
block itself, as opposed to needing to use the method suggested by
Robbie[1], which requires GNU/Linux and the user-space grub2-editenv
utility. Of course, if GRUB is crashing before one can run the
efi-export-env command, then other ways of creating and setting the EFI
envblk variable are necessary. The third patch adds a test which checks the
usability of the commands and the early init loading. And the last patch,
whichvmotivated this series, prints the GDB command string if the GRUB
variable named "enable_earlyinit_gdbinfo" is present and is set to "1", after
the env block is loaded from the GRUB_ENV EFI variable. We might want a
different name for the GRUB variable, I don't really have a preference.
Glenn
[1] https://mail.gnu.org/archive/html/grub-devel/2023-03/msg00072.html
Glenn Washburn (2):
tests: Add efienv_test for testing efi-export-env and efi-load-env
efi: Print GDB command for loading symbols if variable exists
Peter Jones (2):
efi: Load env block from GRUB_ENV EFI variable in early init
efi: Add efi-export-env and efi-load-env commands
Makefile.util.def | 6 ++
docs/grub.texi | 24 +++++
grub-core/Makefile.core.def | 7 ++
grub-core/commands/efi/env.c | 182 +++++++++++++++++++++++++++++++++++
grub-core/kern/efi/efi.c | 3 +
grub-core/kern/efi/init.c | 37 +++++++
grub-core/lib/envblk.c | 43 +++++++++
include/grub/efi/efi.h | 5 +
include/grub/lib/envblk.h | 3 +
tests/efienv_test.in | 57 +++++++++++
10 files changed, 367 insertions(+)
create mode 100644 grub-core/commands/efi/env.c
create mode 100644 tests/efienv_test.in
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/4] efi: Load env block from GRUB_ENV EFI variable in early init 2023-07-26 20:59 [PATCH 0/4] EFI envblk Glenn Washburn @ 2023-07-26 20:59 ` Glenn Washburn 2023-07-26 21:00 ` [PATCH 2/4] efi: Add efi-export-env and efi-load-env commands Glenn Washburn ` (4 subsequent siblings) 5 siblings, 0 replies; 10+ messages in thread From: Glenn Washburn @ 2023-07-26 20:59 UTC (permalink / raw) To: grub-devel, Daniel Kiper; +Cc: Peter Jones, Glenn Washburn From: Peter Jones <pjones@redhat.com> This allows setting GRUB variables before any user interaction or GRUB script can run. It is primarily intended to be used to turn on early debugging. Signed-off-by: Peter Jones <pjones@redhat.com> Replace grub_efi_guid_t -> grub_guid_t Signed-off-by: Glenn Washburn <development@efficientek.com> --- grub-core/Makefile.core.def | 1 + grub-core/kern/efi/init.c | 31 +++++++++++++++++++++++++++++++ include/grub/efi/efi.h | 5 +++++ 3 files changed, 37 insertions(+) diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def index d2cf29584a1a..0b447dd6dc67 100644 --- a/grub-core/Makefile.core.def +++ b/grub-core/Makefile.core.def @@ -219,6 +219,7 @@ kernel = { efi = kern/efi/acpi.c; efi = kern/efi/sb.c; efi = kern/lockdown.c; + efi = lib/envblk.c; i386_coreboot = kern/i386/pc/acpi.c; i386_multiboot = kern/i386/pc/acpi.c; i386_coreboot = kern/acpi.c; diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c index 9cdda3f5ca05..18329b1eacd7 100644 --- a/grub-core/kern/efi/init.c +++ b/grub-core/kern/efi/init.c @@ -28,8 +28,11 @@ #include <grub/env.h> #include <grub/mm.h> #include <grub/kernel.h> + #include <grub/stack_protector.h> +#include <grub/lib/envblk.h> + #ifdef GRUB_STACK_PROTECTOR static grub_efi_char16_t stack_chk_fail_msg[] = @@ -111,6 +114,31 @@ stack_protector_init (void) grub_addr_t grub_modbase; +/* Helper for grub_efi_env_init */ +static int +set_var (const char *name, const char *value, + void *whitelist __attribute__((__unused__))) +{ + grub_env_set (name, value); + return 0; +} + +static void +grub_efi_env_init (void) +{ + grub_guid_t efi_grub_guid = GRUB_EFI_GRUB_VARIABLE_GUID; + struct grub_envblk envblk_s = { NULL, 0 }; + grub_envblk_t envblk = &envblk_s; + + grub_efi_get_variable ("GRUB_ENV", &efi_grub_guid, &envblk_s.size, + (void **) &envblk_s.buf); + if (envblk_s.buf == NULL || envblk_s.size < 1) + return; + + grub_envblk_iterate (envblk, NULL, set_var); + grub_free (envblk_s.buf); +} + void grub_efi_init (void) { @@ -124,6 +152,9 @@ grub_efi_init (void) /* Initialize the memory management system. */ grub_efi_mm_init (); + /* Populate environment with variables from EFI envblk, if it exists. */ + grub_efi_env_init (); + /* * Lockdown the GRUB and register the shim_lock verifier * if the UEFI Secure Boot is enabled. diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h index 564c2f62045b..aa6a360a4999 100644 --- a/include/grub/efi/efi.h +++ b/include/grub/efi/efi.h @@ -25,6 +25,11 @@ #include <grub/efi/api.h> #include <grub/efi/pe32.h> +#define GRUB_EFI_GRUB_VARIABLE_GUID \ + { 0x91376aff, 0xcba6, 0x42be, \ + { 0x94, 0x9d, 0x06, 0xfd, 0xe8, 0x11, 0x28, 0xe8 } \ + } + #define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818 struct linux_arch_kernel_header { -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] efi: Add efi-export-env and efi-load-env commands 2023-07-26 20:59 [PATCH 0/4] EFI envblk Glenn Washburn 2023-07-26 20:59 ` [PATCH 1/4] efi: Load env block from GRUB_ENV EFI variable in early init Glenn Washburn @ 2023-07-26 21:00 ` Glenn Washburn 2023-07-26 21:00 ` [PATCH 3/4] tests: Add efienv_test for testing efi-export-env and efi-load-env Glenn Washburn ` (3 subsequent siblings) 5 siblings, 0 replies; 10+ messages in thread From: Glenn Washburn @ 2023-07-26 21:00 UTC (permalink / raw) To: grub-devel, Daniel Kiper; +Cc: Peter Jones, Glenn Washburn From: Peter Jones <pjones@redhat.com> This adds "efi-export-env VARIABLES" and "efi-load-env", which manipulate the environment block stored in the EFI variable GRUB_ENV-91376aff-cba6-42be-949d-06fde81128e8. Signed-off-by: Peter Jones <pjones@redhat.com> Fix rebase, cherry-pick issues, grub_efi_guid_t -> grub_guid_t, module name bug, missing return bug, memory leaks, formatting Add support for specifying multiple variables at once for efi-export-env. Signed-off-by: Glenn Washburn <development@efficientek.com> --- docs/grub.texi | 24 +++++ grub-core/Makefile.core.def | 6 ++ grub-core/commands/efi/env.c | 182 +++++++++++++++++++++++++++++++++++ grub-core/kern/efi/efi.c | 3 + grub-core/lib/envblk.c | 43 +++++++++ include/grub/lib/envblk.h | 3 + 6 files changed, 261 insertions(+) create mode 100644 grub-core/commands/efi/env.c diff --git a/docs/grub.texi b/docs/grub.texi index b39b72230c6f..730e8c8d75a4 100644 --- a/docs/grub.texi +++ b/docs/grub.texi @@ -4324,6 +4324,8 @@ you forget a command, you can run the command @command{help} * distrust:: Remove a pubkey from trusted keys * drivemap:: Map a drive to another * echo:: Display a line of text +* efi-export-env:: Export environment variable to UEFI +* efi-load-env:: Load the grub environment from UEFI * efitextmode:: Set/Get text output mode resolution * eval:: Evaluate agruments as GRUB commands * export:: Export an environment variable @@ -4784,6 +4786,28 @@ character will print that character. @end deffn +@node efi-export-env +@subsection efi-export-env + +@deffn Command efi-export-env varname [varname @dots{}] +Export the given GRUB variables to the GRUB_ENV UEFI variable as a GRUB +environment block. The contents of an existing environment block in GRUB_ENV +will be merged. So variables existing in GRUB_ENV but not given as arguements +will be unaffected by this command. +@end deffn + + +@node efi-load-env +@subsection efi-load-env + +@deffn Command efi-load-env +Load the grub environment from the GRUB_ENV UEFI variable. This will overwrite +existing variables if they exist in the environment block present in the UEFI +variable. Other existing GRUB variables will be unchanged. This can allow for +the persistence of state across boots. +@end deffn + + @node efitextmode @subsection efitextmode diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def index 0b447dd6dc67..a6e6b8cb135a 100644 --- a/grub-core/Makefile.core.def +++ b/grub-core/Makefile.core.def @@ -825,6 +825,12 @@ module = { enable = efi; }; +module = { + name = efienv; + common = commands/efi/env.c; + enable = efi; +}; + module = { name = efifwsetup; efi = commands/efi/efifwsetup.c; diff --git a/grub-core/commands/efi/env.c b/grub-core/commands/efi/env.c new file mode 100644 index 000000000000..b5775bcbd4bc --- /dev/null +++ b/grub-core/commands/efi/env.c @@ -0,0 +1,182 @@ +/* env.c - commands to load/export environment block to EFI variable */ +/* + * GRUB -- GRand Unified Bootloader + * Copyright (C) 2023 Free Software Foundation, 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/dl.h> +#include <grub/mm.h> +#include <grub/misc.h> +#include <grub/types.h> +#include <grub/mm.h> +#include <grub/misc.h> +#include <grub/efi/api.h> +#include <grub/efi/efi.h> +#include <grub/env.h> +#include <grub/lib/envblk.h> +#include <grub/command.h> + +GRUB_MOD_LICENSE ("GPLv3+"); + +static const grub_guid_t grub_env_guid = GRUB_EFI_GRUB_VARIABLE_GUID; + +static grub_err_t +grub_efi_export_env (grub_command_t cmd __attribute__ ((unused)), + int argc, char *argv[]) +{ + const char *value; + char *old_value; + struct grub_envblk envblk_s = { NULL, 0 }; + grub_envblk_t envblk = &envblk_s; + grub_err_t err = GRUB_ERR_NONE; + int i, changed = 1; + grub_efi_status_t status; + + grub_dprintf ("efienv", "argc:%d\n", argc); + for (i = 0; i < argc; i++) + grub_dprintf ("efienv", "argv[%d]: %s\n", i, argv[i]); + + if (argc < 1) + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("variable names expected")); + + status = grub_efi_get_variable ("GRUB_ENV", &grub_env_guid, &envblk_s.size, + (void **) &envblk_s.buf); + if (envblk_s.buf == NULL || envblk_s.size < 1 || status != GRUB_EFI_SUCCESS) + { + char *buf = grub_malloc (DEFAULT_ENVBLK_SIZE + 1); + if (!buf) + return grub_errno; + + grub_memcpy (buf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1); + grub_memset (buf + sizeof (GRUB_ENVBLK_SIGNATURE) - 1, '#', + DEFAULT_ENVBLK_SIZE - sizeof (GRUB_ENVBLK_SIGNATURE) + 1); + buf[DEFAULT_ENVBLK_SIZE] = '\0'; + + envblk_s.buf = buf; + envblk_s.size = DEFAULT_ENVBLK_SIZE; + } + else + { + char *buf = grub_realloc (envblk_s.buf, envblk_s.size + 1); + if (buf == NULL) + { + err = grub_errno; + goto cleanup; + } + + envblk_s.buf = buf; + envblk_s.buf[envblk_s.size] = '\0'; + } + + for (i = 0; i < argc; i++) + { + err = grub_envblk_get (envblk, argv[i], &old_value); + if (err != GRUB_ERR_NONE) + { + grub_dprintf ("efienv", "grub_envblk_get returned %d\n", err); + goto cleanup; + } + + value = grub_env_get (argv[i]); + if ((value == NULL && old_value == NULL) || + (value && old_value && !grub_strcmp (old_value, value))) + changed = 0; + + if (old_value != NULL) + grub_free (old_value); + + if (changed == 0) + { + grub_dprintf ("efienv", "No changes necessary for \"%s\"\n", argv[i]); + continue; + } + + if (value != NULL) + { + grub_dprintf ("efienv", "setting \"%s\" to \"%s\"\n", argv[i], value); + grub_envblk_set (envblk, argv[i], value); + } + else + { + grub_dprintf ("efienv", "deleting \"%s\" from envblk\n", argv[i]); + grub_envblk_delete (envblk, argv[0]); + } + + grub_dprintf ("efienv", "envblk is %" PRIuGRUB_SIZE " bytes:\n\"%s\"\n", + envblk_s.size, envblk_s.buf); + } + + grub_dprintf ("efienv", "removing GRUB_ENV\n"); + status = grub_efi_set_variable ("GRUB_ENV", &grub_env_guid, NULL, 0); + if (status != GRUB_EFI_SUCCESS) + grub_dprintf ("efienv", "removal returned 0x%" PRIxGRUB_EFI_UINTN_T "\n", status); + + grub_dprintf ("efienv", "setting GRUB_ENV\n"); + status = grub_efi_set_variable ("GRUB_ENV", &grub_env_guid, + envblk_s.buf, envblk_s.size); + if (status != GRUB_EFI_SUCCESS) + grub_dprintf ("efienv", "setting GRUB_ENV returned %" PRIxGRUB_EFI_UINTN_T "\n", status); + + cleanup: + grub_free (envblk_s.buf); + + return err; +} + +static int +set_var (const char *name, const char *value, + void *whitelist __attribute__((__unused__))) +{ + grub_env_set (name, value); + return 0; +} + +static grub_err_t +grub_efi_load_env (grub_command_t cmd __attribute__ ((unused)), + int argc, char *argv[] __attribute__((__unused__))) +{ + struct grub_envblk envblk_s = { NULL, 0 }; + grub_envblk_t envblk = &envblk_s; + + if (argc > 0) + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unexpected argument")); + + grub_efi_get_variable ("GRUB_ENV", &grub_env_guid, &envblk_s.size, + (void **) &envblk_s.buf); + if (envblk_s.buf == NULL || envblk_s.size < 1) + return 0; + + grub_envblk_iterate (envblk, NULL, set_var); + grub_free (envblk_s.buf); + + return GRUB_ERR_NONE; +} + +static grub_command_t export_cmd, loadenv_cmd; + +GRUB_MOD_INIT(efienv) +{ + export_cmd = grub_register_command ("efi-export-env", grub_efi_export_env, + N_("VARIABLE_NAME [VARIABLE_NAME ...]"), + N_("Export environment variable to UEFI.")); + loadenv_cmd = grub_register_command ("efi-load-env", grub_efi_load_env, + NULL, N_("Load the grub environment from UEFI.")); +} + +GRUB_MOD_FINI(efienv) +{ + grub_unregister_command (export_cmd); + grub_unregister_command (loadenv_cmd); +} diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c index 4b60495333e0..2b6bd7e2413d 100644 --- a/grub-core/kern/efi/efi.c +++ b/grub-core/kern/efi/efi.c @@ -224,6 +224,9 @@ grub_efi_set_variable_with_attributes (const char *var, const grub_guid_t *guid, if (status == GRUB_EFI_SUCCESS) return GRUB_ERR_NONE; + if (status == GRUB_EFI_NOT_FOUND && datasize == 0) + return GRUB_ERR_NONE; + return grub_error (GRUB_ERR_IO, "could not set EFI variable `%s'", var); } diff --git a/grub-core/lib/envblk.c b/grub-core/lib/envblk.c index ce55ad1f8118..443297779839 100644 --- a/grub-core/lib/envblk.c +++ b/grub-core/lib/envblk.c @@ -214,6 +214,49 @@ grub_envblk_delete (grub_envblk_t envblk, const char *name) } } +struct get_var_state { + const char * const name; + char * value; + int found; +}; + +static int +get_var (const char * const name, const char * const value, void *statep) +{ + struct get_var_state *state = (struct get_var_state *) statep; + + if (!grub_strcmp (state->name, name)) + { + state->found = 1; + state->value = grub_strdup (value); + if (state->value == NULL) + grub_errno = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")); + + return 1; + } + + return 0; +} + +grub_err_t +grub_envblk_get (grub_envblk_t envblk, const char * const name, char ** const value) +{ + struct get_var_state state = { + .name = name, + .value = NULL, + .found = 0, + }; + + grub_envblk_iterate (envblk, (void *) &state, get_var); + + *value = state.value; + + if (state.found && state.value == NULL) + return grub_errno; + + return GRUB_ERR_NONE; +} + void grub_envblk_iterate (grub_envblk_t envblk, void *hook_data, diff --git a/include/grub/lib/envblk.h b/include/grub/lib/envblk.h index c3e655921709..ab969af24612 100644 --- a/include/grub/lib/envblk.h +++ b/include/grub/lib/envblk.h @@ -22,6 +22,8 @@ #define GRUB_ENVBLK_SIGNATURE "# GRUB Environment Block\n" #define GRUB_ENVBLK_DEFCFG "grubenv" +#define DEFAULT_ENVBLK_SIZE 1024 + #ifndef ASM_FILE struct grub_envblk @@ -33,6 +35,7 @@ typedef struct grub_envblk *grub_envblk_t; grub_envblk_t grub_envblk_open (char *buf, grub_size_t size); int grub_envblk_set (grub_envblk_t envblk, const char *name, const char *value); +grub_err_t grub_envblk_get (grub_envblk_t envblk, const char * const name, char ** const value); void grub_envblk_delete (grub_envblk_t envblk, const char *name); void grub_envblk_iterate (grub_envblk_t envblk, void *hook_data, -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] tests: Add efienv_test for testing efi-export-env and efi-load-env 2023-07-26 20:59 [PATCH 0/4] EFI envblk Glenn Washburn 2023-07-26 20:59 ` [PATCH 1/4] efi: Load env block from GRUB_ENV EFI variable in early init Glenn Washburn 2023-07-26 21:00 ` [PATCH 2/4] efi: Add efi-export-env and efi-load-env commands Glenn Washburn @ 2023-07-26 21:00 ` Glenn Washburn 2023-07-26 21:00 ` [PATCH 4/4] efi: Print GDB command for loading symbols if variable exists Glenn Washburn ` (2 subsequent siblings) 5 siblings, 0 replies; 10+ messages in thread From: Glenn Washburn @ 2023-07-26 21:00 UTC (permalink / raw) To: grub-devel, Daniel Kiper; +Cc: Peter Jones, Glenn Washburn Signed-off-by: Glenn Washburn <development@efficientek.com> --- Makefile.util.def | 6 +++++ tests/efienv_test.in | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 tests/efienv_test.in diff --git a/Makefile.util.def b/Makefile.util.def index 1e9a13d3e348..a7cf53111221 100644 --- a/Makefile.util.def +++ b/Makefile.util.def @@ -1172,6 +1172,12 @@ script = { common = tests/help_test.in; }; +script = { + testcase = nonnative; + name = efienv_test; + common = tests/efienv_test.in; +}; + script = { testcase = nonnative; name = grub_script_gettext; diff --git a/tests/efienv_test.in b/tests/efienv_test.in new file mode 100644 index 000000000000..8ae8c2893d53 --- /dev/null +++ b/tests/efienv_test.in @@ -0,0 +1,57 @@ +#! @BUILD_SHEBANG@ +# Copyright (C) 2023 Free Software Foundation, 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/>. + +set -e +grubshell=@builddir@/grub-shell + +. "@builddir@/grub-core/modinfo.sh" + +case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in + *-efi) + ;; + # Skip Non-EFI platforms, as this test does not apply + *) + exit 77;; +esac + +test_script() { + cat <<'EOF' + efienv_test_var_val="#1 some \\$test value." + + if [ "$efienv_test_var" == "$efienv_test_var_val" ]; then + echo TEST PASSED + else + efienv_test_var="XXX${efienv_test_var_val}" + efi-export-env efienv_test_var + efienv_test_var=erased + efi-load-env + + if [ "$efienv_test_var" != "XXX${efienv_test_var_val}" ]; then + echo TEST FAILED + halt + fi + + efienv_test_var="${efienv_test_var_val}" + efi-export-env efienv_test_var + reboot + fi +EOF +} + +output=$(test_script | "${grubshell}" --modules="efienv,halt" | tail -n1) +if [ "$output" != "TEST PASSED" ]; then + exit 1 +fi -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] efi: Print GDB command for loading symbols if variable exists 2023-07-26 20:59 [PATCH 0/4] EFI envblk Glenn Washburn ` (2 preceding siblings ...) 2023-07-26 21:00 ` [PATCH 3/4] tests: Add efienv_test for testing efi-export-env and efi-load-env Glenn Washburn @ 2023-07-26 21:00 ` Glenn Washburn 2023-07-26 21:49 ` [PATCH 0/4] EFI envblk Vladimir 'phcoder' Serbinenko 2023-09-14 16:47 ` Daniel Kiper 5 siblings, 0 replies; 10+ messages in thread From: Glenn Washburn @ 2023-07-26 21:00 UTC (permalink / raw) To: grub-devel, Daniel Kiper; +Cc: Peter Jones, Glenn Washburn If the variable "enable_earlyinit_gdbinfo" exists, print the GDB command needed to properly load symbols in an attached GDB session. This variable must come from the environment block stored in the GRUB_ENV EFI variable, which allows this to get printed as early as possible. Signed-off-by: Glenn Washburn <development@efficientek.com> --- grub-core/kern/efi/init.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c index 18329b1eacd7..3dce0308cce8 100644 --- a/grub-core/kern/efi/init.c +++ b/grub-core/kern/efi/init.c @@ -142,6 +142,8 @@ grub_efi_env_init (void) void grub_efi_init (void) { + const char *value; + grub_modbase = grub_efi_section_addr ("mods"); /* First of all, initialize the console so that GRUB can display messages. */ @@ -155,6 +157,10 @@ grub_efi_init (void) /* Populate environment with variables from EFI envblk, if it exists. */ grub_efi_env_init (); + value = grub_env_get ("enable_earlyinit_gdbinfo"); + if (value != NULL && !grub_strcmp (value, "1")) + grub_efi_print_gdb_info (); + /* * Lockdown the GRUB and register the shim_lock verifier * if the UEFI Secure Boot is enabled. -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] EFI envblk 2023-07-26 20:59 [PATCH 0/4] EFI envblk Glenn Washburn ` (3 preceding siblings ...) 2023-07-26 21:00 ` [PATCH 4/4] efi: Print GDB command for loading symbols if variable exists Glenn Washburn @ 2023-07-26 21:49 ` Vladimir 'phcoder' Serbinenko 2023-07-28 20:56 ` Glenn Washburn 2023-07-28 23:14 ` Dimitri John Ledkov 2023-09-14 16:47 ` Daniel Kiper 5 siblings, 2 replies; 10+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2023-07-26 21:49 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 3150 bytes --] Have you considered that some firmwares brick if EFI storage is over 50% full? Why not having a file on ESP instead? Le jeu. 27 juil. 2023, 00:09, Glenn Washburn <development@efficientek.com> a écrit : > This patch series (for me) was motivated by the "gdb: Add more support for > debugging on EFI platforms" patch series, which allowed printing in early > EFI init of the GDB command for properly loading symbols. That approach of > having the functionality be included at compile time was ultimately > rejected. During the discussion of that series, Robbie suggested[1] using > patches by Peter and in the Redhat downstream repo which uses an EFI > variable to store a GRUB env block. Using this, a user could store a > variable in the env block stored in the EFI variable and then have GRUB > load that env block in early init as a way to enable the printing of the > GDB command. > > I've taken the original patches by Peter, made more suitable for including > in GRUB, fixed some bugs, and added a minor feature. The first patch, adds > env block loading in early EFI init from the GRUB_ENV EFI variable. The > second patch is included to provide tools for GRUB to set this EFI env > block itself, as opposed to needing to use the method suggested by > Robbie[1], which requires GNU/Linux and the user-space grub2-editenv > utility. Of course, if GRUB is crashing before one can run the > efi-export-env command, then other ways of creating and setting the EFI > envblk variable are necessary. The third patch adds a test which checks the > usability of the commands and the early init loading. And the last patch, > whichvmotivated this series, prints the GDB command string if the GRUB > variable named "enable_earlyinit_gdbinfo" is present and is set to "1", > after > the env block is loaded from the GRUB_ENV EFI variable. We might want a > different name for the GRUB variable, I don't really have a preference. > > Glenn > > [1] https://mail.gnu.org/archive/html/grub-devel/2023-03/msg00072.html > > Glenn Washburn (2): > tests: Add efienv_test for testing efi-export-env and efi-load-env > efi: Print GDB command for loading symbols if variable exists > > Peter Jones (2): > efi: Load env block from GRUB_ENV EFI variable in early init > efi: Add efi-export-env and efi-load-env commands > > Makefile.util.def | 6 ++ > docs/grub.texi | 24 +++++ > grub-core/Makefile.core.def | 7 ++ > grub-core/commands/efi/env.c | 182 +++++++++++++++++++++++++++++++++++ > grub-core/kern/efi/efi.c | 3 + > grub-core/kern/efi/init.c | 37 +++++++ > grub-core/lib/envblk.c | 43 +++++++++ > include/grub/efi/efi.h | 5 + > include/grub/lib/envblk.h | 3 + > tests/efienv_test.in | 57 +++++++++++ > 10 files changed, 367 insertions(+) > create mode 100644 grub-core/commands/efi/env.c > create mode 100644 tests/efienv_test.in > > -- > 2.34.1 > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > [-- Attachment #2: Type: text/html, Size: 4108 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] EFI envblk 2023-07-26 21:49 ` [PATCH 0/4] EFI envblk Vladimir 'phcoder' Serbinenko @ 2023-07-28 20:56 ` Glenn Washburn 2023-07-28 23:14 ` Dimitri John Ledkov 1 sibling, 0 replies; 10+ messages in thread From: Glenn Washburn @ 2023-07-28 20:56 UTC (permalink / raw) To: Vladimir 'phcoder' Serbinenko Cc: The development of GNU GRUB, Peter Jones On Thu, 27 Jul 2023 00:49:06 +0300 "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote: > Have you considered that some firmwares brick if EFI storage is over 50% > full? Why not having a file on ESP instead? What if the ESP is readonly? What if there is no ESP? I take your point about crappy firmwares. As mentioned these patches come from RedHat's downstream GRUB and I believe they are using them in production. So apparently this hasn't been an issue for their users. Now I suspect its probably almost never getting used, so perhaps its a low probability that the issue you mention is seen. Should there be giant warnings in the usage, documentation, or command execution (with confirmation)? How big of a problem is it really (considering the likelihood that this might be used on one of those machines)? Glenn > > Le jeu. 27 juil. 2023, 00:09, Glenn Washburn <development@efficientek.com> > a écrit : > > > This patch series (for me) was motivated by the "gdb: Add more support for > > debugging on EFI platforms" patch series, which allowed printing in early > > EFI init of the GDB command for properly loading symbols. That approach of > > having the functionality be included at compile time was ultimately > > rejected. During the discussion of that series, Robbie suggested[1] using > > patches by Peter and in the Redhat downstream repo which uses an EFI > > variable to store a GRUB env block. Using this, a user could store a > > variable in the env block stored in the EFI variable and then have GRUB > > load that env block in early init as a way to enable the printing of the > > GDB command. > > > > I've taken the original patches by Peter, made more suitable for including > > in GRUB, fixed some bugs, and added a minor feature. The first patch, adds > > env block loading in early EFI init from the GRUB_ENV EFI variable. The > > second patch is included to provide tools for GRUB to set this EFI env > > block itself, as opposed to needing to use the method suggested by > > Robbie[1], which requires GNU/Linux and the user-space grub2-editenv > > utility. Of course, if GRUB is crashing before one can run the > > efi-export-env command, then other ways of creating and setting the EFI > > envblk variable are necessary. The third patch adds a test which checks the > > usability of the commands and the early init loading. And the last patch, > > whichvmotivated this series, prints the GDB command string if the GRUB > > variable named "enable_earlyinit_gdbinfo" is present and is set to "1", > > after > > the env block is loaded from the GRUB_ENV EFI variable. We might want a > > different name for the GRUB variable, I don't really have a preference. > > > > Glenn > > > > [1] https://mail.gnu.org/archive/html/grub-devel/2023-03/msg00072.html > > > > Glenn Washburn (2): > > tests: Add efienv_test for testing efi-export-env and efi-load-env > > efi: Print GDB command for loading symbols if variable exists > > > > Peter Jones (2): > > efi: Load env block from GRUB_ENV EFI variable in early init > > efi: Add efi-export-env and efi-load-env commands > > > > Makefile.util.def | 6 ++ > > docs/grub.texi | 24 +++++ > > grub-core/Makefile.core.def | 7 ++ > > grub-core/commands/efi/env.c | 182 +++++++++++++++++++++++++++++++++++ > > grub-core/kern/efi/efi.c | 3 + > > grub-core/kern/efi/init.c | 37 +++++++ > > grub-core/lib/envblk.c | 43 +++++++++ > > include/grub/efi/efi.h | 5 + > > include/grub/lib/envblk.h | 3 + > > tests/efienv_test.in | 57 +++++++++++ > > 10 files changed, 367 insertions(+) > > create mode 100644 grub-core/commands/efi/env.c > > create mode 100644 tests/efienv_test.in > > > > -- > > 2.34.1 > > > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] EFI envblk 2023-07-26 21:49 ` [PATCH 0/4] EFI envblk Vladimir 'phcoder' Serbinenko 2023-07-28 20:56 ` Glenn Washburn @ 2023-07-28 23:14 ` Dimitri John Ledkov 1 sibling, 0 replies; 10+ messages in thread From: Dimitri John Ledkov @ 2023-07-28 23:14 UTC (permalink / raw) To: The development of GNU GRUB [-- Attachment #1: Type: text/plain, Size: 3977 bytes --] On Wed, 26 Jul 2023, 23:03 Vladimir 'phcoder' Serbinenko, <phcoder@gmail.com> wrote: > Have you considered that some firmwares brick if EFI storage is over 50% > full? Why not having a file on ESP instead? > Yes bricking did happen before. It was triggered by excessive runtime updates of bootloader settings. It did surface bugs of reclaiming variable storage which got fixed and released by most manufacturers. Due to recent revocations EFI storage is now routinely excercises meaning it is now reliable to use. EFI storage is used by kernel pstore too. Unlike read-only ESP EFI storage can be ephemeral r/w through the lifecycle of VM across reboots without a shutdown. Which is better than ESP. > Le jeu. 27 juil. 2023, 00:09, Glenn Washburn <development@efficientek.com> > a écrit : > >> This patch series (for me) was motivated by the "gdb: Add more support for >> debugging on EFI platforms" patch series, which allowed printing in early >> EFI init of the GDB command for properly loading symbols. That approach of >> having the functionality be included at compile time was ultimately >> rejected. During the discussion of that series, Robbie suggested[1] using >> patches by Peter and in the Redhat downstream repo which uses an EFI >> variable to store a GRUB env block. Using this, a user could store a >> variable in the env block stored in the EFI variable and then have GRUB >> load that env block in early init as a way to enable the printing of the >> GDB command. >> >> I've taken the original patches by Peter, made more suitable for including >> in GRUB, fixed some bugs, and added a minor feature. The first patch, adds >> env block loading in early EFI init from the GRUB_ENV EFI variable. The >> second patch is included to provide tools for GRUB to set this EFI env >> block itself, as opposed to needing to use the method suggested by >> Robbie[1], which requires GNU/Linux and the user-space grub2-editenv >> utility. Of course, if GRUB is crashing before one can run the >> efi-export-env command, then other ways of creating and setting the EFI >> envblk variable are necessary. The third patch adds a test which checks >> the >> usability of the commands and the early init loading. And the last patch, >> whichvmotivated this series, prints the GDB command string if the GRUB >> variable named "enable_earlyinit_gdbinfo" is present and is set to "1", >> after >> the env block is loaded from the GRUB_ENV EFI variable. We might want a >> different name for the GRUB variable, I don't really have a preference. >> >> Glenn >> >> [1] https://mail.gnu.org/archive/html/grub-devel/2023-03/msg00072.html >> >> Glenn Washburn (2): >> tests: Add efienv_test for testing efi-export-env and efi-load-env >> efi: Print GDB command for loading symbols if variable exists >> >> Peter Jones (2): >> efi: Load env block from GRUB_ENV EFI variable in early init >> efi: Add efi-export-env and efi-load-env commands >> >> Makefile.util.def | 6 ++ >> docs/grub.texi | 24 +++++ >> grub-core/Makefile.core.def | 7 ++ >> grub-core/commands/efi/env.c | 182 +++++++++++++++++++++++++++++++++++ >> grub-core/kern/efi/efi.c | 3 + >> grub-core/kern/efi/init.c | 37 +++++++ >> grub-core/lib/envblk.c | 43 +++++++++ >> include/grub/efi/efi.h | 5 + >> include/grub/lib/envblk.h | 3 + >> tests/efienv_test.in | 57 +++++++++++ >> 10 files changed, 367 insertions(+) >> create mode 100644 grub-core/commands/efi/env.c >> create mode 100644 tests/efienv_test.in >> >> -- >> 2.34.1 >> >> >> _______________________________________________ >> 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 > [-- Attachment #2: Type: text/html, Size: 5666 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] EFI envblk 2023-07-26 20:59 [PATCH 0/4] EFI envblk Glenn Washburn ` (4 preceding siblings ...) 2023-07-26 21:49 ` [PATCH 0/4] EFI envblk Vladimir 'phcoder' Serbinenko @ 2023-09-14 16:47 ` Daniel Kiper 2023-09-14 21:13 ` Glenn Washburn 5 siblings, 1 reply; 10+ messages in thread From: Daniel Kiper @ 2023-09-14 16:47 UTC (permalink / raw) To: Glenn Washburn; +Cc: grub-devel, Peter Jones, dimitri.ledkov On Wed, Jul 26, 2023 at 03:59:58PM -0500, Glenn Washburn wrote: > This patch series (for me) was motivated by the "gdb: Add more support for > debugging on EFI platforms" patch series, which allowed printing in early > EFI init of the GDB command for properly loading symbols. That approach of > having the functionality be included at compile time was ultimately > rejected. During the discussion of that series, Robbie suggested[1] using > patches by Peter and in the Redhat downstream repo which uses an EFI > variable to store a GRUB env block. Using this, a user could store a > variable in the env block stored in the EFI variable and then have GRUB > load that env block in early init as a way to enable the printing of the > GDB command. > > I've taken the original patches by Peter, made more suitable for including > in GRUB, fixed some bugs, and added a minor feature. The first patch, adds > env block loading in early EFI init from the GRUB_ENV EFI variable. The > second patch is included to provide tools for GRUB to set this EFI env > block itself, as opposed to needing to use the method suggested by > Robbie[1], which requires GNU/Linux and the user-space grub2-editenv > utility. Of course, if GRUB is crashing before one can run the > efi-export-env command, then other ways of creating and setting the EFI > envblk variable are necessary. The third patch adds a test which checks the > usability of the commands and the early init loading. And the last patch, > whichvmotivated this series, prints the GDB command string if the GRUB > variable named "enable_earlyinit_gdbinfo" is present and is set to "1", after > the env block is loaded from the GRUB_ENV EFI variable. We might want a > different name for the GRUB variable, I don't really have a preference. I am torn apart. On one hand it is nice feature. On the other hand it requires a lot of changes. So, I would prefer to defer merging this thing after the release. Until you convince me I should do otherwise... Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] EFI envblk 2023-09-14 16:47 ` Daniel Kiper @ 2023-09-14 21:13 ` Glenn Washburn 0 siblings, 0 replies; 10+ messages in thread From: Glenn Washburn @ 2023-09-14 21:13 UTC (permalink / raw) To: Daniel Kiper Cc: grub-devel, Peter Jones, dimitri.ledkov, Gerd Hoffmann, Oliver Steffen, Javier Martinez Canillas, Julian Andres Klode Adding a few others in case interested... On Thu, 14 Sep 2023 18:47:13 +0200 Daniel Kiper <dkiper@net-space.pl> wrote: > On Wed, Jul 26, 2023 at 03:59:58PM -0500, Glenn Washburn wrote: > > This patch series (for me) was motivated by the "gdb: Add more support for > > debugging on EFI platforms" patch series, which allowed printing in early > > EFI init of the GDB command for properly loading symbols. That approach of > > having the functionality be included at compile time was ultimately > > rejected. During the discussion of that series, Robbie suggested[1] using > > patches by Peter and in the Redhat downstream repo which uses an EFI > > variable to store a GRUB env block. Using this, a user could store a > > variable in the env block stored in the EFI variable and then have GRUB > > load that env block in early init as a way to enable the printing of the > > GDB command. > > > > I've taken the original patches by Peter, made more suitable for including > > in GRUB, fixed some bugs, and added a minor feature. The first patch, adds > > env block loading in early EFI init from the GRUB_ENV EFI variable. The > > second patch is included to provide tools for GRUB to set this EFI env > > block itself, as opposed to needing to use the method suggested by > > Robbie[1], which requires GNU/Linux and the user-space grub2-editenv > > utility. Of course, if GRUB is crashing before one can run the > > efi-export-env command, then other ways of creating and setting the EFI > > envblk variable are necessary. The third patch adds a test which checks the > > usability of the commands and the early init loading. And the last patch, > > whichvmotivated this series, prints the GDB command string if the GRUB > > variable named "enable_earlyinit_gdbinfo" is present and is set to "1", after > > the env block is loaded from the GRUB_ENV EFI variable. We might want a > > different name for the GRUB variable, I don't really have a preference. > > I am torn apart. On one hand it is nice feature. On the other hand > it requires a lot of changes. So, I would prefer to defer merging this > thing after the release. Until you convince me I should do otherwise... I would contend that there's only few small changes that matter. Most of the changes are only executed if the user explicitly does something (ie set an EFI variable), so its opt-in mostly. And it would be nearly impossible to accidentally opt-in (especially considering there's no documentation on the name of the special variable "enable_earlyinit_gdbinfo", which I'm still not convinced is the right name). And the changes that do matter are in old code that I'd have expected to shown an issue by now. Here's my analysis of those. Patch 1: * grub_efi_set_variable() is added to grub_efi_init(). This looks scary, but all its doing is calling grub_efi_get_variable() and returning (assuming the user has not done the manual opt in previously). grub_efi_get_variable() is small and well-used. I would say this is low risk. Patch 2: * grub_efi_set_variable_with_attributes() is changed to have a different return value (ie to succeed) when the datasize is 0 and the variable does not exist. There are very few code paths to this function. I've checked the existing ones and none present a problem. Patch 3: * No changes that matter. Patch 4: * grub_env_get() is called in grub_efi_init(). The initial envblk is setup at load time to have no vars (in the BSS), so this call will fail quickly if using the initial envblk, which should be the case, unless the user has explicitly already run the "efi-export-env" command (which means there wasn't a failure on a previous run up to this point with these changes). The "if" block after will never be executed nor will with grub_strcmp(), unless there was explicit interaction as stated above. So the vast majority of new/"untested" code (though I have tested with the test in patch 3), never gets run unless the user opts in. In that case, we do not want to make the situation worse, but its already in a state if they are wanting to enable this feature (ie can't get to shell to run gdbinfo). Supposing that there is a bug in the "opt-in" code paths that cause GRUB to not reach the shell, the user then needs to run either the EFI shell or some other bootloader that can delete UEFI environment variables. While this could be a pain, I would expect this to be a reasonable ask when GRUB is already in a very messed up state (and a very low probability of this situation ever even occurring). These don't appear to be the kind of changes that would have issue with non-x86 architectures (for which it hasn't been tested on _baremetal_, with QEMU it has). And considering that UEFI variable getting/setting has been around a long time, I'd expect that to be well tested and not a source of risk here. I'll admit that including these changes, will probably be low impact as this is for really bad situations. However, that's precisely when you want these kinds of changes. And I think its worth considering that there's a large base of users using similar code via RedHat's patched GRUB. So I think its a pretty low risk overall. Since this is something I expect to be used more by the distros (I doubt we'll see anyone needing this kind of help here), I wonder if they have any thoughts on this issue? Glenn _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-09-14 21:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-26 20:59 [PATCH 0/4] EFI envblk Glenn Washburn 2023-07-26 20:59 ` [PATCH 1/4] efi: Load env block from GRUB_ENV EFI variable in early init Glenn Washburn 2023-07-26 21:00 ` [PATCH 2/4] efi: Add efi-export-env and efi-load-env commands Glenn Washburn 2023-07-26 21:00 ` [PATCH 3/4] tests: Add efienv_test for testing efi-export-env and efi-load-env Glenn Washburn 2023-07-26 21:00 ` [PATCH 4/4] efi: Print GDB command for loading symbols if variable exists Glenn Washburn 2023-07-26 21:49 ` [PATCH 0/4] EFI envblk Vladimir 'phcoder' Serbinenko 2023-07-28 20:56 ` Glenn Washburn 2023-07-28 23:14 ` Dimitri John Ledkov 2023-09-14 16:47 ` Daniel Kiper 2023-09-14 21:13 ` Glenn Washburn
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.