[PATCH] kern/efi: Adding efi-watchdog command

[PATCH] kern/efi: Adding efi-watchdog command

[Erwan Velu]

This patch got written by Arthur Mesh from Juniper (now at Apple Sec team).
It was extracted from https://lists.gnu.org/archive/html/grub-devel/2015-09/msg00065.html

Since this email, the this patch was :
- rebased against the current tree
- added a default timeout value

This commit adds a new command efi-watchdog to manage efi watchdogs.

On server platforms, this allow grub to reset the host automatically
if it wasn't able to succeed at booting in a given timeframe.
This usually covers the following issues :
- net boot is too slow and never ends
- grub is unable to find a proper configuration and fails

If EFI_WATCHDOG_MINUTES is set a compile time, this enable the watchdog
behavior at the early init of GRUB meaning that even if grub is not able
to load its configuration file, the watchdog will be triggered
automatically.

Please note that watchdog only covers GRUB itself.
Additional hardware watchdog are required if some want to protect the early
operating system loading phase.

By default grub disable the watchdog and so this patch.
Therefore, this commit have no impact on grub's behavior.

This patch is used in production for month on a 50K server platform with success.

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 docs/grub.texi            | 15 ++++++++++
 grub-core/kern/efi/init.c | 63 ++++++++++++++++++++++++++++++++++++++-
 include/grub/efi/efi.h    |  2 ++
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index f8b4b3b21a7f..b52161a19b3b 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3991,6 +3991,7 @@ 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-watchdog::                Manipulate EFI watchdog
 * eval::                        Evaluate agruments as GRUB commands
 * export::                      Export an environment variable
 * false::                       Do nothing, unsuccessfully
@@ -4442,6 +4443,20 @@ When interpreting backslash escapes, backslash followed by any other
 character will print that character.
 @end deffn
 
+@node efi-watchdog
+@subsection efi-watchdog
+
+@deffn Command efi-watchdog @option{enable}|@option{disable} code timeout
+Enable or disable the system's watchdog timer.
+Only available in EFI targeted GRUB.
+
+If @option{enable} is used, the @var{code} is logged upon
+watchdog timeout event. The UEFI BIOS reserves codes 0x0000 to 0xFFFF.
+The @var{timeout} represents number of seconds to set the watchdog timeout to.
+When the watchdog exceed the timeout, the system is reset.
+
+if @option{disable} is used, the EFI watchdog is disarmed.
+@end deffn
 
 @node eval
 @subsection eval
diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
index 7facacf09c7b..c8cda3854ce7 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -28,6 +28,8 @@
 #include <grub/mm.h>
 #include <grub/kernel.h>
 #include <grub/stack_protector.h>
+#include <grub/extcmd.h>
+#include <grub/command.h>
 
 #ifdef GRUB_STACK_PROTECTOR
 
@@ -82,6 +84,60 @@ stack_protector_init (void)
 
 grub_addr_t grub_modbase;
 
+static grub_command_t cmd_list;
+
+static grub_err_t
+grub_cmd_efi_watchdog (grub_command_t cmd  __attribute__ ((unused)),
+                      int argc, char **args)
+{
+    long input;
+    grub_efi_status_t status;
+    grub_efi_uintn_t timeout;
+    grub_efi_uint64_t code;
+
+    if (argc < 1)
+       return grub_error (GRUB_ERR_BAD_ARGUMENT,
+           N_("usage: efi-watchdog (enable|disable) <code> <timeout>"));
+
+    if (grub_strcasecmp (args[0], "enable") == 0) {
+
+       if (argc != 3)
+           return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                              N_("usage: efi-watchdog enable <code> <timeout>"));
+
+       input = grub_strtol (args[1], 0, 0);
+
+       if (input >= 0) {
+           code = input;
+       } else {
+           return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                              N_("<code> must be non-negative"));
+       }
+
+    } else if (grub_strcasecmp (args[0], "disable") == 0) {
+
+       if (argc != 1)
+           return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                              N_("usage: efi-watchdog disable"));
+       timeout = 0;
+       code = 0;
+
+    } else {
+       return grub_error (GRUB_ERR_BAD_ARGUMENT,
+           N_("usage: efi-watchdog (enable|disable) <code> <timeout>"));
+    }
+
+    status = efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
+                        timeout, code, sizeof(L"GRUB"), L"GRUB");
+
+    if (status != GRUB_EFI_SUCCESS)
+       return grub_error (GRUB_ERR_BUG,
+                          N_("Unexpected UEFI SetWatchdogTimer() error"));
+    else
+       return GRUB_ERR_NONE;
+}
+
+
 void
 grub_efi_init (void)
 {
@@ -105,10 +161,14 @@ grub_efi_init (void)
       grub_shim_lock_verifier_setup ();
     }
 
+  grub_printf("grub %s: Arming EFI watchdog at %d minutes\n", PACKAGE_VERSION, EFI_WATCHDOG_MINUTES);
   efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
-	      0, 0, 0, NULL);
+              60*EFI_WATCHDOG_MINUTES, 0, sizeof(L"GRUB"), L"GRUB");
 
   grub_efidisk_init ();
+
+  cmd_list = grub_register_command ("efi-watchdog", grub_cmd_efi_watchdog, 0,
+     N_("Enable/Disable system's watchdog timer."));
 }
 
 void (*grub_efi_net_config) (grub_efi_handle_t hnd, 
@@ -146,4 +206,5 @@ grub_efi_fini (void)
 {
   grub_efidisk_fini ();
   grub_console_fini ();
+  grub_unregister_command (cmd_list);
 }
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 83d958f9945e..372f995b74f4 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -124,4 +124,6 @@ struct grub_net_card;
 grub_efi_handle_t
 grub_efinet_get_device_handle (struct grub_net_card *card);
 
+/* EFI Watchdog armed by grub, in minutes */
+#define EFI_WATCHDOG_MINUTES 0
 #endif /* ! GRUB_EFI_EFI_HEADER */
-- 
2.25.1

Re: [PATCH] kern/efi: Adding efi-watchdog command

[Daniel Kiper]

On Mon, Aug 30, 2021 at 04:08:07PM +0200, Erwan Velu wrote:
> This patch got written by Arthur Mesh from Juniper (now at Apple Sec team).
> It was extracted from https://lists.gnu.org/archive/html/grub-devel/2015-09/msg00065.html
>
> Since this email, the this patch was :
> - rebased against the current tree
> - added a default timeout value
>
> This commit adds a new command efi-watchdog to manage efi watchdogs.
>
> On server platforms, this allow grub to reset the host automatically

s/allow grub/allows the GRUB/g

> if it wasn't able to succeed at booting in a given timeframe.
> This usually covers the following issues :
> - net boot is too slow and never ends
> - grub is unable to find a proper configuration and fails

s/grub/the GRUB/

> If EFI_WATCHDOG_MINUTES is set a compile time, this enable the watchdog
> behavior at the early init of GRUB meaning that even if grub is not able
> to load its configuration file, the watchdog will be triggered
> automatically.

This should not happen at compile time. Please take a look at commit
968de8c23 (shim_lock: Only skip loading shim_lock verifier with explicit
consent). It is good example how to do such things during the GRUB image
generation. Though it does not solve a problem how to enable watchdog
early for UEFI Secure Boot signed images...

> Please note that watchdog only covers GRUB itself.
> Additional hardware watchdog are required if some want to protect the early
> operating system loading phase.

Is watchdog disabled when ExitBootServices() is called?

> By default grub disable the watchdog and so this patch.
> Therefore, this commit have no impact on grub's behavior.
>
> This patch is used in production for month on a 50K server platform with success.
>

Here you should add Signed-off-by of original author.

> Signed-off-by: Erwan Velu <e.velu@criteo.com>
> ---
>  docs/grub.texi            | 15 ++++++++++
>  grub-core/kern/efi/init.c | 63 ++++++++++++++++++++++++++++++++++++++-
>  include/grub/efi/efi.h    |  2 ++
>  3 files changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index f8b4b3b21a7f..b52161a19b3b 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3991,6 +3991,7 @@ 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-watchdog::                Manipulate EFI watchdog
>  * eval::                        Evaluate agruments as GRUB commands
>  * export::                      Export an environment variable
>  * false::                       Do nothing, unsuccessfully
> @@ -4442,6 +4443,20 @@ When interpreting backslash escapes, backslash followed by any other
>  character will print that character.
>  @end deffn
>
> +@node efi-watchdog
> +@subsection efi-watchdog
> +
> +@deffn Command efi-watchdog @option{enable}|@option{disable} code timeout

Could you use -e|-d instead of enable/disable. Additionally, "code" and
"timeout" are only valid for enable. So, I think you should put square
brackets around them.

And it seems to me you should reverse order of "code" and "timeout".

Could not we assume defaults for "code" and "timeout" if they are not
given? Or at least suggest defaults in the documentation?

> +Enable or disable the system's watchdog timer.
> +Only available in EFI targeted GRUB.
> +
> +If @option{enable} is used, the @var{code} is logged upon
> +watchdog timeout event. The UEFI BIOS reserves codes 0x0000 to 0xFFFF.

s/UEFI BIOS/UEFI/

And "reserves" for what? Cannot you use them? Or should you use these
codes because they are reserved for you? Anyway, this requires
clarification. And it would be nice to know where these codes come from?

> +The @var{timeout} represents number of seconds to set the watchdog timeout to.
> +When the watchdog exceed the timeout, the system is reset.

Are there any limitations on timeout value? If yes they should be
specified here.

> +if @option{disable} is used, the EFI watchdog is disarmed.
> +@end deffn
>
>  @node eval
>  @subsection eval
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index 7facacf09c7b..c8cda3854ce7 100644
> --- a/grub-core/kern/efi/init.c
> +++ b/grub-core/kern/efi/init.c
> @@ -28,6 +28,8 @@
>  #include <grub/mm.h>
>  #include <grub/kernel.h>
>  #include <grub/stack_protector.h>
> +#include <grub/extcmd.h>
> +#include <grub/command.h>
>
>  #ifdef GRUB_STACK_PROTECTOR
>
> @@ -82,6 +84,60 @@ stack_protector_init (void)
>
>  grub_addr_t grub_modbase;
>
> +static grub_command_t cmd_list;
> +
> +static grub_err_t
> +grub_cmd_efi_watchdog (grub_command_t cmd  __attribute__ ((unused)),
> +                      int argc, char **args)
> +{
> +    long input;

s/long/unsigned long/

> +    grub_efi_status_t status;
> +    grub_efi_uintn_t timeout;
> +    grub_efi_uint64_t code;

Improper indention here and below. Please take a look at
grub-core/kern/efi/sb.c. It contains good examples how code
should be formatted.

> +    if (argc < 1)
> +       return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +           N_("usage: efi-watchdog (enable|disable) <code> <timeout>"));
> +    if (grub_strcasecmp (args[0], "enable") == 0) {

Would not be simpler to use extcmd interface instead here?
Please take a look at grub-core/term/terminfo.c:grub_cmd_terminfo().
And now I think you should add "-c" option for code and "-t" for timeout.
Then you even do not need "-d". The "-t 0" should disable watchdog instead.

> +       if (argc != 3)
> +           return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                              N_("usage: efi-watchdog enable <code> <timeout>"));
> +
> +       input = grub_strtol (args[1], 0, 0);

I think this should be grub_strtoul(). And please catch all errors from
this function. It means you cannot ignore endptr value returned.

Additionally, IMO base should be explicitly set to 16 here.

> +       if (input >= 0) {
> +           code = input;
> +       } else {
> +           return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                              N_("<code> must be non-negative"));
> +       }

Hmmm... Where do you parse and set timeout?

> +    } else if (grub_strcasecmp (args[0], "disable") == 0) {
> +
> +       if (argc != 1)
> +           return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                              N_("usage: efi-watchdog disable"));
> +       timeout = 0;
> +       code = 0;
> +
> +    } else {
> +       return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +           N_("usage: efi-watchdog (enable|disable) <code> <timeout>"));
> +    }
> +
> +    status = efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
> +                        timeout, code, sizeof(L"GRUB"), L"GRUB");
> +
> +    if (status != GRUB_EFI_SUCCESS)
> +       return grub_error (GRUB_ERR_BUG,
> +                          N_("Unexpected UEFI SetWatchdogTimer() error"));
> +    else
> +       return GRUB_ERR_NONE;
> +}
> +
> +
>  void
>  grub_efi_init (void)
>  {
> @@ -105,10 +161,14 @@ grub_efi_init (void)
>        grub_shim_lock_verifier_setup ();
>      }
>
> +  grub_printf("grub %s: Arming EFI watchdog at %d minutes\n", PACKAGE_VERSION, EFI_WATCHDOG_MINUTES);
>    efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
> -	      0, 0, 0, NULL);
> +              60*EFI_WATCHDOG_MINUTES, 0, sizeof(L"GRUB"), L"GRUB");

Why do you use minutes here if you use seconds above? I think you should
use seconds everywhere.

Daniel

Re: [PATCH] kern/efi: Adding efi-watchdog command

[Erwan Velu]

Le 01/09/2021 à 18:24, Daniel Kiper a écrit :

First of all, thanks for your extensive review, I'll fix all this in a V2.

[...]
> Here you should add Signed-off-by of original author.

Yeah, I was wondering that but the only email I have was from his 
previous company.

I have no idea what email I should use so its useful.


[....]

Re: [PATCH] kern/efi: Adding efi-watchdog command

[Daniel Kiper]

On Thu, Sep 02, 2021 at 11:08:47AM +0200, Erwan Velu wrote:
> Le 01/09/2021 à 18:24, Daniel Kiper a écrit :
>
> First of all, thanks for your extensive review, I'll fix all this in a V2.

Cool!

> [...]
> > Here you should add Signed-off-by of original author.
>
> Yeah, I was wondering that but the only email I have was from his previous
> company.
>
> I have no idea what email I should use so its useful.

I thought you know where he works. So, maybe you will be able to find
his current email address too. If you find it please ask him which
address should be used in this patch. If you are not able to get his
current email address please use one from original patch.

Daniel

Re: [PATCH] kern/efi: Adding efi-watchdog command

[Erwan Velu]

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

[...]
The ext_cmd change is cool except the linker isn't happy as it cannot find
the associated symbols.
I asked a question on the IRC channel on this point.
I'm not familiar with the dependencies definition in this project, where
should I work to get ext_cmd being linked with kern/efi ?
I tried inside grub-core/Makefile.core.def without much luck for now. I
even wonder if we can linked modules with kern.
If you can point me in the right direction for this. I checked some other
commits around ext_cmd but none changes the makefiles.

I also forgot to mention, your review was high quality for a newcomer. You
point to the sample commits and that's very useful to see the expectations.
Thanks for that.


>
> > [...]
> > > Here you should add Signed-off-by of original author.
> >
> > Yeah, I was wondering that but the only email I have was from his
> previous
> > company.
> >
> > I have no idea what email I should use so its useful.
>
> I thought you know where he works. So, maybe you will be able to find
> his current email address too. If you find it please ask him which
> address should be used in this patch. If you are not able to get his
> current email address please use one from original patch.
>
> I just have his linked profile but locked as I'm not premium.
I'll use the one I found.

[-- Attachment #2: Type: text/html, Size: 1702 bytes --]

Re: [PATCH] kern/efi: Adding efi-watchdog command

[Daniel Kiper]

On Thu, Sep 02, 2021 at 02:32:48PM +0200, Erwan Velu wrote:
> [...]
> The ext_cmd change is cool except the linker isn't happy as it cannot find the
> associated symbols.
> I asked a question on the IRC channel on this point.
> I'm not familiar with the dependencies definition in this project, where should
> I work to get ext_cmd being linked with kern/efi ?
> I tried inside grub-core/Makefile.core.def without much luck for now. I even
> wonder if we can linked modules with kern.
> If you can point me in the right direction for this. I checked some other
> commits around ext_cmd but none changes the makefiles.

Ugh... Sorry, I forgot extcmd interface is not available in the GRUB kernel.
So, please ignore this comment.

> I also forgot to mention, your review was high quality for a newcomer. You
> point to the sample commits and that's very useful to see the expectations.
> Thanks for that.

Nice to hear that. Thank you!

Daniel

Re: [PATCH] kern/efi: Adding efi-watchdog command

[Erwan Velu]

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

Le jeu. 2 sept. 2021 à 11:08, Erwan Velu <e.velu@criteo.com> a écrit :

> Le 01/09/2021 à 18:24, Daniel Kiper a écrit :
>
> First of all, thanks for your extensive review, I'll fix all this in a V2.
>
> [...]
Just sent the V2, hope you'll enjoy this version of the patch.
I learned many things on some GRUB internals and also simplified a lot the
interface.

[-- Attachment #2: Type: text/html, Size: 733 bytes --]