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