bg_setenv --confirm sets all configuration ustate to OK

[Christopher Obbard]

Hi!

I have come across an oddity in my efibootguard implementation, with EFI Boot
Guard v0.13 from Debian Bookworm. I haven't tried this with the latest
version.

We have two boot configurations and shipping with config1 disabled (e.g ustate
set to FAILED).

efibootguard boots from config0 and on first boot, I run `bg_setenv --confirm`
which I am assuming should just set config0 to OK, but it seems to sets the
ustate of all partitions to OK.


To reproduce this, I have an initial environment with:

bg_printenv[356]: ----------------------------
bg_printenv[356]: Config Partition #0 Values:
bg_printenv[356]: in_progress: no
bg_printenv[356]: revision: 2
bg_printenv[356]: kernel: C:BOOT0:linux.efi
bg_printenv[356]: kernelargs:
bg_printenv[356]: watchdog timeout: 30 seconds
bg_printenv[356]: ustate: 0 (OK)
bg_printenv[356]: user variables:
bg_printenv[356]: ----------------------------
bg_printenv[356]: Config Partition #1 Values:
bg_printenv[356]: in_progress: no
bg_printenv[356]: revision: 1
bg_printenv[356]: kernel:
bg_printenv[356]: kernelargs:
bg_printenv[356]: watchdog timeout: 30 seconds
bg_printenv[356]: ustate: 3 (FAILED)
bg_printenv[356]: user variables:


Running `bg_setenv --confirm` results in:

bg_setenv[405]: Processing journal...
bg_setenv[405]: Task = SET, key = ustate, type = 0, val = 0
bg_setenv[405]: Probing config file at /tmp/mnt-ydqsvs/BGENV.DAT.
bg_setenv[405]: New environment data:
bg_setenv[405]: ---------------------
bg_setenv[405]: Values:
bg_setenv[405]: in_progress: no
bg_setenv[405]: revision: 2
bg_setenv[405]: kernel: C:BOOT0:linux.efi
bg_setenv[405]: kernelargs:
bg_setenv[405]: watchdog timeout: 30 seconds
bg_setenv[405]: ustate: 0 (OK)
bg_setenv[405]: user variables:
bg_setenv[405]: Probing config file at /tmp/mnt-QXVipB/BGENV.DAT.
bg_setenv[405]: Environment update was successful.


and the environment ends up being:

bg_printenv[472]: ----------------------------
bg_printenv[472]: Config Partition #0 Values:
bg_printenv[472]: in_progress: no
bg_printenv[472]: revision: 2
bg_printenv[472]: kernel: C:BOOT0:linux.efi
bg_printenv[472]: kernelargs:
bg_printenv[472]: watchdog timeout: 30 seconds
bg_printenv[472]: ustate: 0 (OK)
bg_printenv[472]: user variables:
bg_printenv[472]: ----------------------------
bg_printenv[472]: Config Partition #1 Values:
bg_printenv[472]: in_progress: no
bg_printenv[472]: revision: 1
bg_printenv[472]: kernel:
bg_printenv[472]: kernelargs:
bg_printenv[472]: watchdog timeout: 30 seconds
bg_printenv[472]: ustate: 0 (OK)
bg_printenv[472]: user variables:


I am wondering, if `bg_setenv --confirm` should set only the booted
configuration to OK or if the case which I have here, where all configs are
set to OK, even though one is FAILED ?



Thanks!

Christopher Obbard

[Michael Adler]

Hi Christopher,

> I haven't tried this with the latest version.

I checked the code, and this is indeed intended behavior:
`journal_process_action` has some special logic in place (see [1]), which
results in calling `ebg_env_setglobalstate` if `ustate` is to be updated. This
function sets the `ustate` of *all* environments to zero, as per documentation
[2].

> I am wondering, if `bg_setenv --confirm` should set only the booted
> configuration to OK or if the case which I have here, where all configs are
> set to OK, even though one is FAILED ?

Good question. I guess there are reasons for the current behavior :) Jan
Kiszka might remember the reason. I can only guess; maybe it is meant to
prepare the standby BGENV for the next update (starting with a clean slate)
since you are not supposed to roll back anymore.

Kind Regards,
Michael

[1] https://github.com/siemens/efibootguard/blob/b9f2d0354f77a8630a2c5cfbf426fd5e76ec2938/tools/bg_setenv.c#L115
[2] https://github.com/siemens/efibootguard/blob/b9f2d0354f77a8630a2c5cfbf426fd5e76ec2938/include/ebgenv.h#L144

--
Michael Adler

Siemens AG
Technology
Connectivity & Edge
Smart Embedded Systems
T CED OES-DE
Otto-Hahn-Ring 6
81739 Munich, Germany

Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Jim Hagemann
Snabe; Managing Board: Roland Busch, Chairman, President and Chief Executive
Officer; Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese;
Registered offices: Berlin and Munich, Germany; Commercial registries:
Berlin-Charlottenburg, HRB 12300, Munich, HRB 6684; WEEE-Reg.-No. DE 23691322

[Jan Kiszka]

On 22.03.24 17:30, 'Michael Adler' via EFI Boot Guard wrote:
> Hi Christopher,
>
>> I haven't tried this with the latest version.
>
> I checked the code, and this is indeed intended behavior:
> `journal_process_action` has some special logic in place (see [1]), which
> results in calling `ebg_env_setglobalstate` if `ustate` is to be updated. This
> function sets the `ustate` of *all* environments to zero, as per documentation
> [2].
>
>> I am wondering, if `bg_setenv --confirm` should set only the booted
>> configuration to OK or if the case which I have here, where all configs are
>> set to OK, even though one is FAILED ?
>
> Good question. I guess there are reasons for the current behavior :) Jan
> Kiszka might remember the reason. I can only guess; maybe it is meant to
> prepare the standby BGENV for the next update (starting with a clean slate)
> since you are not supposed to roll back anymore.
>

I don't remember this detail anymore either. It dates back to the early
days of our code base, and the commit introducing this was not properly
motivate - my bad having accepted it back then.

Christian, as this behavior is also part of the library API, can you
remember when this global setting of ustate(s) to OK would be needed?

The current state machine is definitely convoluted and not well
maintainable anymore. But as we have an official API, it's also not easy
to change things. We are already discussing internally for a while that
it would need a "version 2" to reconstruct the actually needed features
and behavior without having to deal with the legacy.

Jan

--
Siemens AG, Technology
Linux Expert Center

[Storm, Christian]

Hi,

>>> I haven't tried this with the latest version.
>>
>> I checked the code, and this is indeed intended behavior:
>> `journal_process_action` has some special logic in place (see [1]), which
>> results in calling `ebg_env_setglobalstate` if `ustate` is to be updated. This
>> function sets the `ustate` of *all* environments to zero, as per documentation
>> [2].
>>
>>> I am wondering, if `bg_setenv --confirm` should set only the booted
>>> configuration to OK or if the case which I have here, where all configs are
>>> set to OK, even though one is FAILED ?
>>
>> Good question. I guess there are reasons for the current behavior :) Jan
>> Kiszka might remember the reason. I can only guess; maybe it is meant to
>> prepare the standby BGENV for the next update (starting with a clean slate)
>> since you are not supposed to roll back anymore.
>>
>
> I don't remember this detail anymore either. It dates back to the early
> days of our code base, and the commit introducing this was not properly
> motivate - my bad having accepted it back then.
>
> Christian, as this behavior is also part of the library API, can you
> remember when this global setting of ustate(s) to OK would be needed?

Nope, my best guess is that it was not a technical necessity but rather a semantically motivated implementation decision: When you confirm the current state to be OK, then there's probably little reason (semantically) to leave an environment in FAILED state as you just confirmed everything to be OK and the former FAILED-marked environment will not be considered for being booted anyway ― at least as long as the more recent current environment is not corrupt.

That said, just because a boot path is marked as FAILED doesn't prevent the environment selection logics from considering it for being booted, see https://github.com/siemens/efibootguard/blob/next/env/fatvars.c#L207-L226.
That, though, could be considered a "feature" as it tries very hard to bring the machine up, even in light of some FAILED-marked trickery like yours :) However, it only tries hard for the latest two environments, not considering further if you happen to have them...

The bigger problem with setting all environments to OK on an `bg_setenv ―confirm` is, however, that all environments are written to. In SWUpdate, we're going to great lengths to avoid this, see https://github.com/sbabic/swupdate/blob/master/bootloader/ebg.c#L37-L93.



Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T CED OES-DE
Otto-Hahn-Ring 6, 81739 Munich, Germany