[PATCH] Rollback in TESTING state when kernel image cannot load

[Storm, Christian]

From: Christian Storm <christi...@siemens.com>

In an update scenario, if ustate was just switched from
INSTALLED=1 to TESTING=2 but the kernel image to be tested
is not present, reboot to trigger a fallback into the
original boot path.

Signed-off-by: Christian Storm <christi...@siemens.com>
---
env/fatvars.c | 1 +
include/bootguard.h | 1 +
main.c | 14 ++++++++++++++
3 files changed, 16 insertions(+)

diff --git a/env/fatvars.c b/env/fatvars.c
index 675f6cc..07c9e85 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -198,6 +198,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
save_current_config(config_volumes, numHandles);
}

+ bglp->ustate = env[latest_idx].ustate;
bglp->payload_path = StrDuplicate(env[current_partition].kernelfile);
bglp->payload_options =
StrDuplicate(env[current_partition].kernelparams);
diff --git a/include/bootguard.h b/include/bootguard.h
index 93d415c..af0f54a 100644
--- a/include/bootguard.h
+++ b/include/bootguard.h
@@ -37,4 +37,5 @@ typedef struct _BG_LOADER_PARAMS {
CHAR16 *payload_path;
CHAR16 *payload_options;
UINTN timeout;
+ UINT8 ustate;
} BG_LOADER_PARAMS;
diff --git a/main.c b/main.c
index 3885754..8046ac1 100644
--- a/main.c
+++ b/main.c
@@ -184,6 +184,20 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
status = BS->LoadImage(TRUE, this_image, payload_dev_path, NULL, 0,
&payload_handle);
if (EFI_ERROR(status)) {
+ if (bg_loader_params.ustate == USTATE_TESTING) {
+ /*
+ * `ustate` was just switched from `1` (INSTALLED)
+ * to `2` (TESTING), but the kernel image to be
+ * tested is not present. Reboot to trigger a
+ * fallback into the original boot path.
+ */
+ ERROR(L"Failed to load kernel image %s (%r).\n",
+ bg_loader_params.payload_path, status);
+ ERROR(L"Triggering Rollback as ustate==2 (TESTING).\n");
+ (VOID) BS->Stall(3 * 1000 * 1000);
+ ST->RuntimeServices->ResetSystem(EfiResetCold,
+ EFI_SUCCESS, 0, NULL);
+ }
error_exit(L"Cannot load specified kernel image", status);
}

--
2.49.0

[Jan Kiszka]

On 26.03.25 15:21, 'Storm, Christian' via EFI Boot Guard wrote:
> From: Christian Storm <christi...@siemens.com>
>
> In an update scenario, if ustate was just switched from
> INSTALLED=1 to TESTING=2 but the kernel image to be tested
> is not present, reboot to trigger a fallback into the
> original boot path.

And why do we want or even need this?

Looks to me like an optimization that will accelerate the reset which
would otherwise have to wait for the watchdog to fire. But your vague
description may suggest you are actually fixing a critical bug.

Jan

Siemens AG, Foundational Technologies
Linux Expert Center

[Storm, Christian]

Hi Jan,

>> In an update scenario, if ustate was just switched from
>> INSTALLED=1 to TESTING=2 but the kernel image to be tested
>> is not present, reboot to trigger a fallback into the
>> original boot path.
>
> And why do we want or even need this?
>
> Looks to me like an optimization that will accelerate the reset which
> would otherwise have to wait for the watchdog to fire.

In a perfect world with perfect firmware shipped, this will just spare you a few seconds waiting for the watchdog to kick in, so a small optimization, you're totally right. However, the assumption of perfectness unfortunately doesn't hold :)

> But your vague description may suggest you are actually fixing a critical bug.

It's a bug, namely on aarch64 + U-Boot with flawed code+configuration where the watchdog is indefinitely fed by U-Boot, so never kicking in, and EFI Boot Guard's error_exit() calling BS→Exit() after having waited a bit results in being dropped to back to U-Boot.
This patch is a fix to not get dropped back to U-Boot in the particular case described in the commit message.
While it's not strictly needed, see your argument above, it does help in exactly this situation and spares you a few seconds on top.

Best regards,
Christian

--
Dr. Christian Storm
Siemens AG, FT RPD CED
Friedrich-Ludwig-Bauer-Str. 3, 85748 Garching, Germany

[Jan Kiszka]

On 26.03.25 21:38, 'Storm, Christian' via EFI Boot Guard wrote:
> Hi Jan,
>
>>> In an update scenario, if ustate was just switched from
>>> INSTALLED=1 to TESTING=2 but the kernel image to be tested
>>> is not present, reboot to trigger a fallback into the
>>> original boot path.
>>
>> And why do we want or even need this?
>>
>> Looks to me like an optimization that will accelerate the reset which
>> would otherwise have to wait for the watchdog to fire.
>
> In a perfect world with perfect firmware shipped, this will just spare you a few seconds waiting for the watchdog to kick in, so a small optimization, you're totally right. However, the assumption of perfectness unfortunately doesn't hold :)
>

Well, if your watchdog should not work, then a missing or otherwise not
loading kernel file is only one of the many issues that will leave the
device stuck in an error state.

>
>> But your vague description may suggest you are actually fixing a critical bug.
>
> It's a bug, namely on aarch64 + U-Boot with flawed code+configuration where the watchdog is indefinitely fed by U-Boot, so never kicking in, and EFI Boot Guard's error_exit() calling BS→Exit() after having waited a bit results in being dropped to back to U-Boot.

That is a bug - of your U-Boot configuration. U-Boot MUST NOT feed the
watchdog.

> This patch is a fix to not get dropped back to U-Boot in the particular case described in the commit message.
> While it's not strictly needed, see your argument above, it does help in exactly this situation and spares you a few seconds on top.

I can only recommend to study our meta-iot2050 for a working reference
for which your change is really what it is: an optimization ;)

Jan

[Storm, Christian]

Yes, understood, acknowledged, and has already been addressed. Anyway, I think we should consider this patch for merging as it is a (simple) optimization which is generally applicable, hence it is relevant to upstream EFI Boot Guard. Incidentally, it also fixes a specific bug which is not relevant for upstream EFI Boot Guard but may help in considering merging this patch.


Kind regards,

[Jan Kiszka]

Then please reword the description accordingly.

[Storm, Christian]

From: Christian Storm <christi...@siemens.com>

In an update scenario, if ustate was just switched from
INSTALLED=1 to TESTING=2 but the kernel image to be tested
is not present, reboot to trigger a fallback into the
original boot path.

This is a simple optimization to not wait until the watchdog
kicks in and reboots the system to the same effect.
Incidentally, this also fixes a bug on aarch64 where U-Boot
was configured to feed the watchdog indefinitely, i.e., no
reboot happens, ever.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

Changes in v2: Add rationale for patch.

2.49.0

[Jan Kiszka]

On 31.03.25 20:07, 'Storm, Christian' via EFI Boot Guard wrote:
> From: Christian Storm <christi...@siemens.com>
>
> In an update scenario, if ustate was just switched from
> INSTALLED=1 to TESTING=2 but the kernel image to be tested
> is not present, reboot to trigger a fallback into the
> original boot path.
>
> This is a simple optimization to not wait until the watchdog
> kicks in and reboots the system to the same effect.
> Incidentally, this also fixes a bug on aarch64 where U-Boot
> was configured to feed the watchdog indefinitely, i.e., no
> reboot happens, ever.

"Incidentally, this also help to fix downstream issues where U-Boot was
incorrectly configured to feed the watchdog indefinitely, i.e., no
reboot happens, ever."

Applied with this clarification.

Thanks,
Jan