Re: [PATCH 0/4] EFI envblk

[Glenn Washburn]

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