Re: [Regression] efi: Don't display a uefi-firmware entry if it's not supported

[Javier Martinez Canillas]

On 8/31/22 00:07, Philip Müller wrote:
> On 30.08.22 23:34, Javier Martinez Canillas wrote:
>>> You could add a feature flag, which causes grub-core to set an
>>> environment variable when a new feature is supported. See the features
>>> array in grub-core/normal/main.c.
>>>
>>> You would then check for this feature flag in the grub.d snippet
>>> before calling fwsetup --is-supported.
>>>
>> Please don't. As mentioned, I think we should aim to simplify the grub.cfg
>> instead of making it more complicated.
> 
> Well I think it would be the best approach to add backward compatibility 
> as most users don't even know on how to install grub via grub-install.
> That is done via the graphical installer Calamares on most Arch-based 
> Distributions. Updating the grub menu is common if you install multiple 
> kernels or use snapshots via BTRFS.
> 
> Simply calling 'fwsetup' is a big NO-NO to me and others. The old 
> version runs into the EFI firware or simply turn off the PC during boot, 
> which creates boot loops for some or unbootable systems.
>

I'm OK to not call fwsetup at all, that's what we originally had in the
series posted by Robbie, for example in v2:

https://lists.gnu.org/archive/html/grub-devel/2022-08/msg00169.html

Then they added the fwsetup --is-supported option as mentioned because
other developer asked for that. That patch was included in v3 onward:

https://lists.gnu.org/archive/html/grub-devel/2022-08/msg00196.html
 
> I checked on my end with an older grub in /boot and the updated menu.cfg 
> script. Only when removing the snippet of 30_uefi-firmware the system is 
> bootable again.
>

That's fair. Then probably what we should do is to partially revert
commit 26031d3b1016 ("efi: Don't display a uefi-firmware entry if
it's not supported"). Something like the following patch perhaps ?

>From c3edc64687686d9a5a54f769ec03101b2c4cdef1 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Wed, 31 Aug 2022 00:30:31 +0200
Subject: [PATCH] templates: Don't execute fwsetup unconditionally
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit 26031d3b1016 ("efi: Don't display a uefi-firmware entry if it's not
supported") added a new `fwsetup --is-supported` option that could be used
to check if the firmware allows to enter into a firmware user interface.

That option was then used by /etc/grub.d/30_uefi-firmware script to figure
out whether a `fwsetup` entry should be included or not in the boot menu.

But unfortunately, old GRUB images will fail to boot if are booted with an
updated GRUB config file. Since the `fwsetup --is-supported` call would be
taken as a plan `fwsetup` with no options.

This could either lead to a crash (i.e: on legacy BIOS systems where that
is not supported) or to the machine entering into the firmware settings.

To prevent that, let's partially revert the mentioned commit and keep the
old logic that didn't execute the `fwsetup` command and just included an
entry for it if GRUB is executed in an EFI platform.

Fixes: 26031d3b1016 ("efi: Don't display a uefi-firmware entry if it's not 
supported")
Reported-by: Philip Müller <philm@manjaro.org>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 util/grub.d/30_uefi-firmware.in | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
index c1731e5bbbb3..b6041b55e2a0 100644
--- a/util/grub.d/30_uefi-firmware.in
+++ b/util/grub.d/30_uefi-firmware.in
@@ -31,8 +31,7 @@ LABEL="UEFI Firmware Settings"
 gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
 
 cat << EOF
-fwsetup --is-supported
-if [ "\$grub_platform" = "efi" -a "\$?" = 0 ]; then
+if [ "\$grub_platform" = "efi" ]; then
        menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
                fwsetup
        }
-- 
2.37.1

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat