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

[Robbie Harwood <rharwood@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4454 bytes --]

Javier Martinez Canillas <javierm@redhat.com> writes:

> 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

While we could revert the entire --is-supported logic as well, since
this is upstream pre-release code, it's probably easier for the
downstreams that pulled this change if we don't, so:

Reviewed-by: Robbie Harwood <rharwood@redhat.com>.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]