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

[Daniel Kiper]

On Thu, Sep 02, 2021 at 06:50:35PM +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
> - reworked to be controlled via mkimage
> - reworked to simplify the command line

All lines above should go after SOB and "---". Good example what I mean
is here [1].

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

I do not see any reason to name the command "efi-watchdog". It should be
simply named as "watchdog". If in the future we will support more
watchdog devices we can add a command to switch between them.

> On server platforms, this allows GRUB to reset the host automatically

Please drop "On server platforms".

> 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
> - the GRUB is unable to find a proper configuration and fails
>
> Using --efi-watchdog-timeout option at mknetdir time allow users

s/efi-watchdog-timeout/watchdog-timeout/

And I am not sure what you mean by "Using ... option at mknetdir time"...

> controlling the default value of the timer.
> This set 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.

The GRUB not always calls ExitBootServices(). So, this is not entirely true.
E.g Xen calls ExitBootServices() instead of GRUB on UEFI platforms.

> As per the UEFI specification :
>       The watchdog timer is only used during boot services. On successful 
> completion of
>       EFI_BOOT_SERVICES.ExitBootServices() the watchdog timer is disabled.
>
> This implies this watchdog doesn't cover the the OS loading.
> If you want to cover this phase of the boot,
> an additional hardware watchdog is required (usually set in the BIOS).

Ditto. Please update these two paragraphs taking into account my comments above.

> On the command line, a single argument is enough to control the watchdog.
> This argument defines the timeout of the watchdog (in seconds).
>       - If 0 > argument < 0xFFFF, the watchdog is armed with the associate 
> timeout.
>       - If argument is set to 0, the watchdog is disarmed.
>
> By default GRUB disable the watchdog and so this patch.
> Therefore, this commit have no impact on the default GRUB's behavior.
>
> This patch is used in production for month on a 50K server platform with 
> success.

I think you are missing a description why this functionality must be in
the GRUB core.img.

> Signed-off-by: Erwan Velu <e.velu@criteo.com>
> Signed-off-by: Arthur Mesh <amesh@juniper.net>

Arthur Mesh SOB should be before yours.

> ---
>  docs/grub.texi              | 13 ++++++
>  grub-core/kern/efi/init.c   | 85 ++++++++++++++++++++++++++++++++++++-
>  include/grub/efi/efi.h      |  2 +
>  include/grub/kernel.h       |  3 +-
>  include/grub/util/install.h |  8 +++-
>  util/grub-install-common.c  | 12 +++++-
>  util/grub-mkimage.c         | 11 ++++-
>  util/mkimage.c              | 23 +++++++++-
>  8 files changed, 147 insertions(+), 10 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index f8b4b3b21a7f..f012e9537306 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,18 @@ When interpreting backslash escapes, backslash 
> followed by any other
>  character will print that character.
>  @end deffn
>
> +@node efi-watchdog
> +@subsection efi-watchdog

s/efi-watchdog/watchdog/ and below please...

> +@deffn Command efi-watchdog @var{timeout}
> +Control the EFI system's watchdog timer.
> +Only available in EFI targeted GRUB.

I do not see any reason to put this in two lines.

> +
> +When the watchdog exceed @var{timeout}, the system is reset.
> +
> +If @var{timeout} is set to 0, the watchdog is disarmed.
> +

Please drop this empty line.

> +@end deffn
>
>  @node eval
>  @subsection eval
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index 7facacf09c7b..532e8533426e 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>

I think this include is not needed.

> +#include <grub/command.h>
>
>  #ifdef GRUB_STACK_PROTECTOR
>
> @@ -82,6 +84,82 @@ stack_protector_init (void)
>
>  grub_addr_t grub_modbase;
>
> +static grub_command_t cmd_list;
> +
> +

Please drop this empty line.

> +static grub_err_t grub_efi_set_watchdog_timer(unsigned long timeout)

Please use grub_efi_uintn_t instead of unsigned long. However, please
remember that its size depends on architecture. So, it can be 32-bits or
64-bits...

> +{
> +  /* User defined code, set to BOOT (B007) for GRUB
> +  UEFI SPEC 2.6 reports :

Please refer to the latest UEFI spec.

> +    The numeric code to log on a watchdog timer timeout event.
> +    The firmware reserves codes 0x0000 to 0xFFFF.
> +    Loaders and operating systems may use other timeout codes.

This comment says you cannot use 0xb007. I would use 0xdeadb007deadb007
by default. However, I would give an option to change the default by user.

> +  */

Please format comments according to [2].

> +  grub_efi_uint64_t code = 0xB007;
> +  grub_efi_status_t status = 0;
> +
> +  if (timeout >= 0xFFFF)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("efi-watchdog: timeout must 
> be lower than 65535"));

Why do you artificially limit the timeout value? Please do not do that.

> +  if (timeout > 0)
> +    grub_printf("grub %s: Arming EFI watchdog at %lu seconds\n", 
> PACKAGE_VERSION, timeout);

If you want debug messages please use grub_dprintf() instead of grub_printf().

> +  else {
> +    grub_printf("grub %s: Disarming EFI watchdog\n", PACKAGE_VERSION);
> +    code = 0;
> +  }
> +
> +  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"));

Please print status value in case of an error, e.g. "UEFI SetWatchdogTimer() 
error: %..."

> +  else
> +    return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_efi_watchdog (grub_command_t cmd  __attribute__ ((unused)),
> +                      int argc, char **args)
> +{
> +  unsigned long timeout = 0;

s/unsigned long/grub_efi_uintn_t/ Please use types according to the UEFI
specification and convert properly between them.

> +
> +

Please drop this redundant empty line.

> +  if (argc < 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("usage: efi-watchdog 
> <timeout>"));

As I said earlier. I think "watchdog" command should take two arguments:
-t <timeout> and -c <code>.

> +  const char *ptr = args[0];
> +  timeout = grub_strtoul (ptr, &ptr, 0);

Please read strtoul() documentation and handle all errors properly.

> +  if (grub_errno)

if (grub_errno != GRUB_ERR_NONE)

> +    return grub_errno;
> +
> +  return grub_efi_set_watchdog_timer(timeout);

Please do not forget about spaces between function names and "(".

> +}
> +
> +static void
> +grub_efi_watchdog_timer_init(void)
> +{
> +  struct grub_module_header *header;
> +  unsigned long efi_watchdog_timeout = 0;

Again, please use correct UEFI type...

> +
> +  FOR_MODULES (header)

Missing "{}" for FOR_MODULES() and...

> +  if (header->type == OBJ_TYPE_EFI_WATCHDOG_TIMEOUT)

... incorrect "if" indention. Please take a look at
grub-core/kern/efi/sb.c for more details...

> +    {
> +      char *efi_wdt_orig_addr;
> +      static char *efi_wdt_addr;
> +      static grub_off_t efi_wdt_size = 0;
> +
> +      efi_wdt_orig_addr = (char *) header + sizeof (struct 
> grub_module_header);
> +      efi_wdt_size = header->size - sizeof (struct grub_module_header);
> +      efi_wdt_addr = grub_malloc (efi_wdt_size);

Please do not ignore grub_malloc() errors. In general you need proper
error handling for every function which may fail.

> +      grub_memmove (efi_wdt_addr, efi_wdt_orig_addr, efi_wdt_size);
> +      efi_watchdog_timeout = (unsigned long) *efi_wdt_addr;
> +      break;
> +    }
> +
> +  grub_efi_set_watchdog_timer(efi_watchdog_timeout);
> +}
> +
> +

Please drop this redundant empty line...

>  void
>  grub_efi_init (void)
>  {
> @@ -105,10 +183,12 @@ grub_efi_init (void)
>        grub_shim_lock_verifier_setup ();
>      }
>
> -  efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
> -           0, 0, 0, NULL);
> +  grub_efi_watchdog_timer_init();
>
>    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 +226,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 */
> diff --git a/include/grub/kernel.h b/include/grub/kernel.h
> index abbca5ea3359..172599c58b23 100644
> --- a/include/grub/kernel.h
> +++ b/include/grub/kernel.h
> @@ -30,7 +30,8 @@ enum
>    OBJ_TYPE_PREFIX,
>    OBJ_TYPE_PUBKEY,
>    OBJ_TYPE_DTB,
> -  OBJ_TYPE_DISABLE_SHIM_LOCK
> +  OBJ_TYPE_DISABLE_SHIM_LOCK,
> +  OBJ_TYPE_EFI_WATCHDOG_TIMEOUT,
>  };
>
>  /* The module header.  */
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index 7df3191f47ef..1276742d09e6 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -67,6 +67,8 @@
>        N_("SBAT metadata"), 0 },                                              
> \
>    { "disable-shim-lock", GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK, 0, 0,       
> \
>        N_("disable shim_lock verifier"), 0 },                         \
> +  { "efi-watchdog-timeout", GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT, 
> N_("EFI_WATCHDOG_TIMEOUT"), 0, \
> +      N_("arm efi watchdog at EFI_WATCHDOG_TIMEOUT seconds"), 0 }, \
>    { "verbose", 'v', 0, 0,                                            \
>      N_("print verbose messages."), 1 }
>
> @@ -128,7 +130,8 @@ enum grub_install_options {
>    GRUB_INSTALL_OPTIONS_INSTALL_CORE_COMPRESS,
>    GRUB_INSTALL_OPTIONS_DTB,
>    GRUB_INSTALL_OPTIONS_SBAT,
> -  GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK
> +  GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK,
> +  GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT
>  };
>
>  extern char *grub_install_source_directory;
> @@ -190,7 +193,8 @@ grub_install_generate_image (const char *dir, const char 
> *prefix,
>                            const struct grub_install_image_target_desc 
> *image_target,
>                            int note,
>                            grub_compression_t comp, const char *dtb_file,
> -                          const char *sbat_path, const int 
> disable_shim_lock);
> +                          const char *sbat_path, const int disable_shim_lock,
> +                          unsigned long efi_watchdog_timeout);
>
>  const struct grub_install_image_target_desc *
>  grub_install_get_image_target (const char *arg);
> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> index 4e212e690c52..dab94799a6c3 100644
> --- a/util/grub-install-common.c
> +++ b/util/grub-install-common.c
> @@ -460,11 +460,13 @@ static char **pubkeys;
>  static size_t npubkeys;
>  static char *sbat;
>  static int disable_shim_lock;
> +static unsigned long efi_watchdog_timeout;
>  static grub_compression_t compression;
>
>  int
>  grub_install_parse (int key, char *arg)
>  {
> +  const char *const_arg = arg;
>    switch (key)
>      {
>      case 'C':
> @@ -562,6 +564,11 @@ grub_install_parse (int key, char *arg)
>        grub_util_error (_("Unrecognized compression `%s'"), arg);
>      case GRUB_INSTALL_OPTIONS_GRUB_MKIMAGE:
>        return 1;
> +    case GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT:
> +      efi_watchdog_timeout = grub_strtoul (arg, &const_arg, 0);

Please do not ignore errors...

> +      if (grub_errno)
> +        grub_util_error (_("timeout argument must be an integer : %s"), arg);
> +      return 1;
>      default:
>        return 0;
>      }
> @@ -661,9 +668,10 @@ grub_install_make_image_wrap_file (const char *dir, 
> const char *prefix,
>                 " --output '%s' "
>                 " --dtb '%s' "
>                 "--sbat '%s' "
> +               "--efi-watchdog-timeout '%lu' "
>                 "--format '%s' --compression '%s' %s %s %s\n",
>                 dir, prefix,
> -               outname, dtb ? : "", sbat ? : "", mkimage_target,
> +               outname, dtb ? : "", sbat ? : "", efi_watchdog_timeout, 
> mkimage_target,
>                 compnames[compression], note ? "--note" : "",
>                 disable_shim_lock ? "--disable-shim-lock" : "", s);

Could you be more consistent and add your argument behind the
--disable-shim-lock one?

>    free (s);
> @@ -676,7 +684,7 @@ grub_install_make_image_wrap_file (const char *dir, const 
> char *prefix,
>                              modules.entries, memdisk_path,
>                              pubkeys, npubkeys, config_path, tgt,
>                              note, compression, dtb, sbat,
> -                            disable_shim_lock);
> +                            disable_shim_lock, efi_watchdog_timeout);
>    while (dc--)
>      grub_install_pop_module ();
>  }
> diff --git a/util/grub-mkimage.c b/util/grub-mkimage.c
> index c0d559937020..fce7941a58fe 100644
> --- a/util/grub-mkimage.c
> +++ b/util/grub-mkimage.c
> @@ -83,6 +83,7 @@ static struct argp_option options[] = {
>    {"compression",  'C', "(xz|none|auto)", 0, N_("choose the compression to 
> use for core image"), 0},
>    {"sbat", 's', N_("FILE"), 0, N_("SBAT metadata"), 0},
>    {"disable-shim-lock", GRUB_INSTALL_OPTIONS_DISABLE_SHIM_LOCK, 0, 0, 
> N_("disable shim_lock verifier"), 0},
> +  {"efi-watchdog-timeout", GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT, 0, 0, 
> N_("efi watchdog timeout"), 0},
>    {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
>    { 0, 0, 0, 0, 0, 0 }
>  };
> @@ -128,6 +129,7 @@ struct arguments
>    char *sbat;
>    int note;
>    int disable_shim_lock;
> +  unsigned long efi_watchdog_timeout;
>    const struct grub_install_image_target_desc *image_target;
>    grub_compression_t comp;
>  };
> @@ -138,6 +140,7 @@ argp_parser (int key, char *arg, struct argp_state *state)
>    /* Get the input argument from argp_parse, which we
>       know is a pointer to our arguments structure. */
>    struct arguments *arguments = state->input;
> +  const char *const_arg = arg;
>
>    switch (key)
>      {
> @@ -239,6 +242,12 @@ argp_parser (int key, char *arg, struct argp_state 
> *state)
>        arguments->disable_shim_lock = 1;
>        break;
>
> +    case GRUB_INSTALL_OPTIONS_EFI_WATCHDOG_TIMEOUT:
> +      arguments->efi_watchdog_timeout = grub_strtoul (arg, &const_arg, 0);
> +      if (grub_errno)
> +        grub_util_error (_("timeout argument must be an integer : %s"), arg);
> +      break;
> +
>      case 'v':
>        verbosity++;
>        break;
> @@ -325,7 +334,7 @@ main (int argc, char *argv[])
>                              arguments.npubkeys, arguments.config,
>                              arguments.image_target, arguments.note,
>                              arguments.comp, arguments.dtb,
> -                            arguments.sbat, arguments.disable_shim_lock);
> +                            arguments.sbat, arguments.disable_shim_lock, 
> arguments.efi_watchdog_timeout);
>
>    if (grub_util_file_sync (fp) < 0)
>      grub_util_error (_("cannot sync `%s': %s"), arguments.output ? : 
> "stdout",
> diff --git a/util/mkimage.c b/util/mkimage.c
> index a26cf76f72f2..91b03801e628 100644
> --- a/util/mkimage.c
> +++ b/util/mkimage.c
> @@ -870,12 +870,12 @@ grub_install_generate_image (const char *dir, const 
> char *prefix,
>                            size_t npubkeys, char *config_path,
>                            const struct grub_install_image_target_desc 
> *image_target,
>                            int note, grub_compression_t comp, const char 
> *dtb_path,
> -                          const char *sbat_path, int disable_shim_lock)
> +                          const char *sbat_path, int disable_shim_lock, 
> unsigned long efi_watchdog_timeout)
>  {
>    char *kernel_img, *core_img;
>    size_t total_module_size, core_size;
>    size_t memdisk_size = 0, config_size = 0;
> -  size_t prefix_size = 0, dtb_size = 0, sbat_size = 0;
> +  size_t prefix_size = 0, dtb_size = 0, sbat_size = 0, 
> efi_watchdog_timeout_size = 0;
>    char *kernel_path;
>    size_t offset;
>    struct grub_util_path_list *path_list, *p;
> @@ -929,6 +929,12 @@ grub_install_generate_image (const char *dir, const char 
> *prefix,
>    if (sbat_path != NULL && image_target->id != IMAGE_EFI)
>      grub_util_error (_(".sbat section can be embedded into EFI images 
> only"));
>
> +  if (efi_watchdog_timeout)
> +    {
> +      efi_watchdog_timeout_size = ALIGN_ADDR(sizeof (efi_watchdog_timeout));
> +      total_module_size += efi_watchdog_timeout_size + sizeof (struct 
> grub_module_header);
> +    }
> +
>    if (disable_shim_lock)
>      total_module_size += sizeof (struct grub_module_header);
>
> @@ -1078,6 +1084,19 @@ grub_install_generate_image (const char *dir, const 
> char *prefix,
>        offset += sizeof (*header);
>      }
>
> +  if (efi_watchdog_timeout)
> +    {
> +      struct grub_module_header *header;
> +
> +      header = (struct grub_module_header *) (kernel_img + offset);
> +      header->type = grub_host_to_target32 (OBJ_TYPE_EFI_WATCHDOG_TIMEOUT);
> +      header->size = grub_host_to_target32 (efi_watchdog_timeout_size + 
> sizeof (*header));

After looking at this it seems to me the timeout should be u32 thing
internally in the GRUB.

Daniel

[1] https://lists.gnu.org/archive/html/grub-devel/2021-06/msg00017.html
[2] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments