[PATCH 0/2] Improve logic to check for fwsetup support

[PATCH 0/2] Improve logic to check for fwsetup support

[Javier Martinez Canillas]

The fwsetup command can be used for rebooting to the firmware setup UI but
this can only be used on machines that have support for this feature.

Curently, this is achieved by checking in the 30_uefi-firmware template if
OsIndicationsSupported is present and has EFI_OS_INDICATIONS_BOOT_TO_FW_UI
set. This is again checked in the efifwsetup module init function and the
fwsetup command is only registered if that condition is meet.

But this logic has a comple of problems, since it assumes that the config
file is always generated in the same machine than the one that's booted.

If that's not the case, any of the following situations could happen:

* A config file generated on a non-EFI machine, will not have the fwsetup
  menu entry even when booting with EFI.
* A config file generated on an EFI machine, will have a fwsetup even when
  is not booted with EFI.
* A config file generated on an EFI machine that supports rebooting to the
  firmware setup, will have the fwsetup even if booting on an EFI machine
  that does not support that feature.

To prevent these, let's change when the check is done. Instead of doing it
at config generation time, do it at runtime with grub_platform == "efi" and
when the command is executed. That way, the 30_uefi-firmware template won't
make any assumption about the firmware support of the machine that's booted.

Best regards,
Javier


Javier Martinez Canillas (2):
  templates: Check for EFI at runtime instead of config generation time
  efi: Print an error if boot to firmware setup is not supported

 grub-core/commands/efi/efifwsetup.c | 43 +++++++++++++++--------------
 util/grub.d/30_uefi-firmware.in     | 21 ++++++--------
 2 files changed, 31 insertions(+), 33 deletions(-)

-- 
2.31.1

[PATCH 1/2] templates: Check for EFI at runtime instead of config generation time

[Javier Martinez Canillas]

The 30_uefi-firmware template checks if an OsIndicationsSupported UEFI var
exists and EFI_OS_INDICATIONS_BOOT_TO_FW_UI bit is set, to decide whether
a "fwsetup" menu entry would be added or not to the GRUB menu.

But this has the problem that it will only work if the configuration file
was created on an UEFI machine that supports booting to a firmware UI.

This for example doesn't support creating GRUB config files when executing
on systems that support both UEFI and legacy BIOS booting. Since creating
the config file from legacy BIOS wouldn't allow to access the firmware UI.

To prevent this, make the template to unconditionally create the grub.cfg
snippet but check at runtime if was booted through UEFI to decide if this
entry should be added. That way it won't be added when booting with BIOS.

There's no need to check if EFI_OS_INDICATIONS_BOOT_TO_FW_UI bit is set,
since that's already done by the "fwsetup" command when is executed.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 util/grub.d/30_uefi-firmware.in | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
index d344d3883d7..b6041b55e2a 100644
--- a/util/grub.d/30_uefi-firmware.in
+++ b/util/grub.d/30_uefi-firmware.in
@@ -26,19 +26,14 @@ export TEXTDOMAINDIR="@localedir@"
 
 . "$pkgdatadir/grub-mkconfig_lib"
 
-EFI_VARS_DIR=/sys/firmware/efi/efivars
-EFI_GLOBAL_VARIABLE=8be4df61-93ca-11d2-aa0d-00e098032b8c
-OS_INDICATIONS="$EFI_VARS_DIR/OsIndicationsSupported-$EFI_GLOBAL_VARIABLE"
+LABEL="UEFI Firmware Settings"
 
-if [ -e "$OS_INDICATIONS" ] && \
-   [ "$(( $(printf 0x%x \'"$(cat $OS_INDICATIONS | cut -b5)"\') & 1 ))" = 1 ]; then
-  LABEL="UEFI Firmware Settings"
+gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
 
-  gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
-
-  cat << EOF
-menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
-	fwsetup
-}
-EOF
+cat << EOF
+if [ "\$grub_platform" = "efi" ]; then
+	menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
+		fwsetup
+	}
 fi
+EOF
-- 
2.31.1

[PATCH 2/2] efi: Print an error if boot to firmware setup is not supported

[Javier Martinez Canillas]

The "fwsetup" command is only registered if the firmware supports booting
to the firmware setup UI. But it could be possible that the GRUB config
already contains a "fwsetup" entry, because it was generated in a machine
that has support for this feature.

To prevent users getting a "can't find command `fwsetup`" error if it is
not supported by the firmware, let's just always register the command but
print a more accurate message if the firmware doesn't support this option.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 grub-core/commands/efi/efifwsetup.c | 43 +++++++++++++++--------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/grub-core/commands/efi/efifwsetup.c b/grub-core/commands/efi/efifwsetup.c
index eaca0328388..328c45e82e0 100644
--- a/grub-core/commands/efi/efifwsetup.c
+++ b/grub-core/commands/efi/efifwsetup.c
@@ -27,6 +27,25 @@
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
+static grub_efi_boolean_t
+efifwsetup_is_supported (void)
+{
+  grub_efi_uint64_t *os_indications_supported = NULL;
+  grub_size_t oi_size = 0;
+  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
+
+  grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
+			 (void **) &os_indications_supported);
+
+  if (!os_indications_supported)
+    return 0;
+
+  if (*os_indications_supported & GRUB_EFI_OS_INDICATIONS_BOOT_TO_FW_UI)
+    return 1;
+
+  return 0;
+}
+
 static grub_err_t
 grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
 		  int argc __attribute__ ((unused)),
@@ -38,6 +57,10 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
   grub_size_t oi_size;
   grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
 
+  if (!efifwsetup_is_supported ())
+	  return grub_error (GRUB_ERR_INVALID_COMMAND,
+			     N_("Reboot to firmware setup is not supported"));
+
   grub_efi_get_variable ("OsIndications", &global, &oi_size,
 			 (void **) &old_os_indications);
 
@@ -56,28 +79,8 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
 
 static grub_command_t cmd = NULL;
 
-static grub_efi_boolean_t
-efifwsetup_is_supported (void)
-{
-  grub_efi_uint64_t *os_indications_supported = NULL;
-  grub_size_t oi_size = 0;
-  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
-
-  grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
-			 (void **) &os_indications_supported);
-
-  if (!os_indications_supported)
-    return 0;
-
-  if (*os_indications_supported & GRUB_EFI_OS_INDICATIONS_BOOT_TO_FW_UI)
-    return 1;
-
-  return 0;
-}
-
 GRUB_MOD_INIT (efifwsetup)
 {
-  if (efifwsetup_is_supported ())
     cmd = grub_register_command ("fwsetup", grub_cmd_fwsetup, NULL,
 				 N_("Reboot into firmware setup menu."));
 
-- 
2.31.1

Re: [PATCH 1/2] templates: Check for EFI at runtime instead of config generation time

[Daniel Kiper]

On Tue, Jul 06, 2021 at 11:02:14AM +0200, Javier Martinez Canillas wrote:
> The 30_uefi-firmware template checks if an OsIndicationsSupported UEFI var
> exists and EFI_OS_INDICATIONS_BOOT_TO_FW_UI bit is set, to decide whether
> a "fwsetup" menu entry would be added or not to the GRUB menu.
>
> But this has the problem that it will only work if the configuration file
> was created on an UEFI machine that supports booting to a firmware UI.
>
> This for example doesn't support creating GRUB config files when executing
> on systems that support both UEFI and legacy BIOS booting. Since creating
> the config file from legacy BIOS wouldn't allow to access the firmware UI.
>
> To prevent this, make the template to unconditionally create the grub.cfg
> snippet but check at runtime if was booted through UEFI to decide if this
> entry should be added. That way it won't be added when booting with BIOS.
>
> There's no need to check if EFI_OS_INDICATIONS_BOOT_TO_FW_UI bit is set,
> since that's already done by the "fwsetup" command when is executed.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  util/grub.d/30_uefi-firmware.in | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/util/grub.d/30_uefi-firmware.in b/util/grub.d/30_uefi-firmware.in
> index d344d3883d7..b6041b55e2a 100644
> --- a/util/grub.d/30_uefi-firmware.in
> +++ b/util/grub.d/30_uefi-firmware.in
> @@ -26,19 +26,14 @@ export TEXTDOMAINDIR="@localedir@"
>
>  . "$pkgdatadir/grub-mkconfig_lib"
>
> -EFI_VARS_DIR=/sys/firmware/efi/efivars
> -EFI_GLOBAL_VARIABLE=8be4df61-93ca-11d2-aa0d-00e098032b8c
> -OS_INDICATIONS="$EFI_VARS_DIR/OsIndicationsSupported-$EFI_GLOBAL_VARIABLE"
> +LABEL="UEFI Firmware Settings"
>
> -if [ -e "$OS_INDICATIONS" ] && \
> -   [ "$(( $(printf 0x%x \'"$(cat $OS_INDICATIONS | cut -b5)"\') & 1 ))" = 1 ]; then
> -  LABEL="UEFI Firmware Settings"
> +gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
>
> -  gettext_printf "Adding boot menu entry for UEFI Firmware Settings ...\n" >&2
> -
> -  cat << EOF
> -menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
> -	fwsetup
> -}
> -EOF
> +cat << EOF
> +if [ "\$grub_platform" = "efi" ]; then
> +	menuentry '$LABEL' \$menuentry_id_option 'uefi-firmware' {
> +		fwsetup
> +	}

Should not we go further and enable this menu entry only if (UEFI)
firmware settings are available? Of course this requires exporting
relevant information to the GRUB shell.

Daniel

Re: [PATCH 2/2] efi: Print an error if boot to firmware setup is not supported

[Daniel Kiper]

On Tue, Jul 06, 2021 at 11:02:15AM +0200, Javier Martinez Canillas wrote:
> The "fwsetup" command is only registered if the firmware supports booting
> to the firmware setup UI. But it could be possible that the GRUB config
> already contains a "fwsetup" entry, because it was generated in a machine
> that has support for this feature.
>
> To prevent users getting a "can't find command `fwsetup`" error if it is
> not supported by the firmware, let's just always register the command but
> print a more accurate message if the firmware doesn't support this option.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  grub-core/commands/efi/efifwsetup.c | 43 +++++++++++++++--------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/grub-core/commands/efi/efifwsetup.c b/grub-core/commands/efi/efifwsetup.c
> index eaca0328388..328c45e82e0 100644
> --- a/grub-core/commands/efi/efifwsetup.c
> +++ b/grub-core/commands/efi/efifwsetup.c
> @@ -27,6 +27,25 @@
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> +static grub_efi_boolean_t
> +efifwsetup_is_supported (void)
> +{
> +  grub_efi_uint64_t *os_indications_supported = NULL;
> +  grub_size_t oi_size = 0;
> +  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;

static grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;

AFAICT this should generate smaller code. Hmmm... I think we should add
a preparatory patch which changes all GUID assignments like that one to
"static". Could you do that?

> +  grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
> +			 (void **) &os_indications_supported);
> +
> +  if (!os_indications_supported)
> +    return 0;
> +
> +  if (*os_indications_supported & GRUB_EFI_OS_INDICATIONS_BOOT_TO_FW_UI)
> +    return 1;
> +

grub_free(os_indications_supported) call is missing in this function.
So, we need an additional patch which fixes this up front.

> +  return 0;
> +}
> +
>  static grub_err_t
>  grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
>  		  int argc __attribute__ ((unused)),
> @@ -38,6 +57,10 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
>    grub_size_t oi_size;
>    grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
>
> +  if (!efifwsetup_is_supported ())
> +	  return grub_error (GRUB_ERR_INVALID_COMMAND,
> +			     N_("Reboot to firmware setup is not supported"));

Most error messages start with lower case. Please fix this.

Daniel

Re: [PATCH 2/2] efi: Print an error if boot to firmware setup is not supported

[Paul Menzel]

Dear Javier,


Thank you for this patch.


Am 06.07.21 um 11:02 schrieb Javier Martinez Canillas:
> The "fwsetup" command is only registered if the firmware supports booting
> to the firmware setup UI. But it could be possible that the GRUB config
> already contains a "fwsetup" entry, because it was generated in a machine
> that has support for this feature.
> 
> To prevent users getting a "can't find command `fwsetup`" error if it is
> not supported by the firmware, let's just always register the command but
> print a more accurate message if the firmware doesn't support this option.

(I’d always paste the message in the commit message too.)

> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>   grub-core/commands/efi/efifwsetup.c | 43 +++++++++++++++--------------
>   1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/grub-core/commands/efi/efifwsetup.c b/grub-core/commands/efi/efifwsetup.c
> index eaca0328388..328c45e82e0 100644
> --- a/grub-core/commands/efi/efifwsetup.c
> +++ b/grub-core/commands/efi/efifwsetup.c
> @@ -27,6 +27,25 @@
>   
>   GRUB_MOD_LICENSE ("GPLv3+");
>   
> +static grub_efi_boolean_t
> +efifwsetup_is_supported (void)
> +{
> +  grub_efi_uint64_t *os_indications_supported = NULL;
> +  grub_size_t oi_size = 0;
> +  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
> +
> +  grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
> +			 (void **) &os_indications_supported);
> +
> +  if (!os_indications_supported)
> +    return 0;
> +
> +  if (*os_indications_supported & GRUB_EFI_OS_INDICATIONS_BOOT_TO_FW_UI)
> +    return 1;
> +
> +  return 0;
> +}
> +
>   static grub_err_t
>   grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
>   		  int argc __attribute__ ((unused)),
> @@ -38,6 +57,10 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
>     grub_size_t oi_size;
>     grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
>   
> +  if (!efifwsetup_is_supported ())
> +	  return grub_error (GRUB_ERR_INVALID_COMMAND,
> +			     N_("Reboot to firmware setup is not supported"));
> +

I’d add: “… by the current firmware”, so it’s clear where to look.


Kind regards,

Paul


>     grub_efi_get_variable ("OsIndications", &global, &oi_size,
>   			 (void **) &old_os_indications);
>   
> @@ -56,28 +79,8 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
>   
>   static grub_command_t cmd = NULL;
>   
> -static grub_efi_boolean_t
> -efifwsetup_is_supported (void)
> -{
> -  grub_efi_uint64_t *os_indications_supported = NULL;
> -  grub_size_t oi_size = 0;
> -  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
> -
> -  grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
> -			 (void **) &os_indications_supported);
> -
> -  if (!os_indications_supported)
> -    return 0;
> -
> -  if (*os_indications_supported & GRUB_EFI_OS_INDICATIONS_BOOT_TO_FW_UI)
> -    return 1;
> -
> -  return 0;
> -}
> -
>   GRUB_MOD_INIT (efifwsetup)
>   {
> -  if (efifwsetup_is_supported ())
>       cmd = grub_register_command ("fwsetup", grub_cmd_fwsetup, NULL,
>   				 N_("Reboot into firmware setup menu."));
>   
>