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.v...@criteo.com>
> Signed-off-by: Arthur Mesh <am...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
