[PATCH 1/3] bootloader: Add is_bootloader() to distinguish bootloaders

[Christian Storm]

Add bool is_bootloader(const char *name) to distinguish the
currently selected bootloader and add central bootloader
name #define's as well to the documentation to help in
maintaining the uniqueness of bootloader names.

Signed-off-by: Christian Storm <christi...@siemens.com>
---
bootloader/ebg.c | 2 +-
bootloader/grub.c | 2 +-
bootloader/none.c | 2 +-
bootloader/uboot.c | 2 +-
core/bootloader.c | 7 +++++++
doc/source/bootloader_interface.rst | 12 +++++++++++-
include/bootloader.h | 16 ++++++++++++++++
7 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/bootloader/ebg.c b/bootloader/ebg.c
index 00dc5c5..4194a38 100644
--- a/bootloader/ebg.c
+++ b/bootloader/ebg.c
@@ -236,5 +236,5 @@ static bootloader* probe(void)
__attribute__((constructor))
static void ebg_probe(void)
{
- (void)register_bootloader("ebg", probe());
+ (void)register_bootloader(BOOTLOADER_EBG, probe());
}
diff --git a/bootloader/grub.c b/bootloader/grub.c
index 21bb6ac..7bda742 100644
--- a/bootloader/grub.c
+++ b/bootloader/grub.c
@@ -353,5 +353,5 @@ static bootloader grub = {
__attribute__((constructor))
static void grub_probe(void)
{
- (void)register_bootloader("grub", &grub);
+ (void)register_bootloader(BOOTLOADER_GRUB, &grub);
}
diff --git a/bootloader/none.c b/bootloader/none.c
index d9d1c0b..d996da3 100644
--- a/bootloader/none.c
+++ b/bootloader/none.c
@@ -54,5 +54,5 @@ static bootloader none = {
__attribute__((constructor))
static void none_probe(void)
{
- (void)register_bootloader("none", &none);
+ (void)register_bootloader(BOOTLOADER_NONE, &none);
}
diff --git a/bootloader/uboot.c b/bootloader/uboot.c
index ed4c516..2720b1e 100644
--- a/bootloader/uboot.c
+++ b/bootloader/uboot.c
@@ -144,5 +144,5 @@ static bootloader* probe(void)
__attribute__((constructor))
static void uboot_probe(void)
{
- (void)register_bootloader("uboot", probe());
+ (void)register_bootloader(BOOTLOADER_UBOOT, probe());
}
diff --git a/core/bootloader.c b/core/bootloader.c
index b8678b8..c02b249 100644
--- a/core/bootloader.c
+++ b/core/bootloader.c
@@ -55,6 +55,13 @@ int set_bootloader(const char *name)
return -ENOENT;
}

+bool is_bootloader(const char *name) {
+ if (!name || !current) {
+ return false;
+ }
+ return strcmp(current->name, name) == 0;
+}
+
const char* get_bootloader(void)
{
return current ? current->name : NULL;
diff --git a/doc/source/bootloader_interface.rst b/doc/source/bootloader_interface.rst
index 9066c74..b8c0182 100644
--- a/doc/source/bootloader_interface.rst
+++ b/doc/source/bootloader_interface.rst
@@ -82,9 +82,19 @@ registers this bootloader to SWUpdate at run-time:
__attribute__((constructor))
static void trunk_probe(void)
{
- (void)register_bootloader("trunk", &trunk);
+ (void)register_bootloader(BOOTLOADER_TRUNK, &trunk);
}

+with
+
+.. code-block:: c
+
+ #define BOOTLOADER_TRUNK "trunk"
+
+added to ``include/bootloader.h`` as a single central "trunk" bootloader
+name definition aiding in maintaining the uniqueness of bootloader names.
+
+
.. attention:: Take care to uniquely name the bootloader.


diff --git a/include/bootloader.h b/include/bootloader.h
index 8315759..529ec12 100644
--- a/include/bootloader.h
+++ b/include/bootloader.h
@@ -6,6 +6,12 @@
*/

#pragma once
+#include <stdbool.h>
+
+#define BOOTLOADER_EBG "ebg"
+#define BOOTLOADER_NONE "none"
+#define BOOTLOADER_GRUB "grub"
+#define BOOTLOADER_UBOOT "uboot"

#define load_symbol(handle, container, fname) \
*(void**)(container) = dlsym(handle, fname); \
@@ -50,6 +56,16 @@ int set_bootloader(const char *name);
*/
const char* get_bootloader(void);

+/*
+ * is_bootloader - Test whether bootloader is currently selected
+ *
+ * @name : bootloader name to check if it's the currently selected one
+ *
+ * Return:
+ * true if name is currently selected bootloader, false otherwise
+ */
+bool is_bootloader(const char *name);
+
/*
* print_registered_bootloaders - print registered bootloaders
*/
--
2.38.1

[Christian Storm]

Expose SWUpdate's bootloader interface to suricatta Lua modules.

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

doc/source/bootloader_interface.rst | 4 +
suricatta/server_lua.c | 109 ++++++++++++++++++++++++++++
suricatta/suricatta.lua | 56 ++++++++++++++
3 files changed, 169 insertions(+)

diff --git a/doc/source/bootloader_interface.rst b/doc/source/bootloader_interface.rst
index b8c0182..8527aec 100644
--- a/doc/source/bootloader_interface.rst
+++ b/doc/source/bootloader_interface.rst
@@ -93,6 +93,10 @@ with

added to ``include/bootloader.h`` as a single central "trunk" bootloader

name definition aiding in maintaining the uniqueness of bootloader names.

+This new "trunk" bootloader should also be added to the Suricatta Lua
+Module interface specification's bootloader Table
+``suricatta.bootloader.bootloaders = { ... }`` in
+``suricatta/suricatta.lua``.

.. attention:: Take care to uniquely name the bootloader.

diff --git a/suricatta/server_lua.c b/suricatta/server_lua.c
index 6dcf938..3d4bf70 100644
--- a/suricatta/server_lua.c
+++ b/suricatta/server_lua.c
@@ -30,6 +30,7 @@
#include <swupdate.h>
#include <parselib.h>
#include <state.h>
+#include <bootloader.h>
#include <swupdate_settings.h>
#include <swupdate_dict.h>
#include "suricatta_private.h"
@@ -1388,6 +1389,85 @@ failure:
return 2;
}

+/**
+ * @brief Test whether a bootloader is currently set.
+ *
+ * @param [Lua] Name of bootloader to test for being currently set.
+ * @return [Lua] True if given name is the currently set bootloader, false otherwise.
+ */
+static int lua_bootloader_is(lua_State *L)
+{
+ lua_pushboolean(L, is_bootloader(luaL_checkstring(L, -1)));
+ return 1;
+}
+
+/**
+ * @brief Get currently set bootloader's name.
+ *
+ * @return [Lua] Name of currently set bootloader.
+ */
+static int lua_bootloader_get(lua_State *L)
+{
+ (void)lua_pushstring(L, get_bootloader());
+ return 1;
+}
+
+/**
+ * @brief Get value of a bootloader environment variable.
+ *
+ * @param [Lua] Name of the bootloader environment variable to get value of.
+ * @return [Lua] Value of the bootloader environment variable.
+ */
+static int lua_bootloader_env_get(lua_State *L)
+{
+ char* value = bootloader_env_get(luaL_checkstring(L, -1));
+ (void)lua_pushstring(L, value);
+ free(value);
+ return 1;
+}
+
+/**
+ * @brief Set value of a bootloader environment variable.
+ *
+ * @param [Lua] Name of the bootloader environment variable to set.
+ * @param [Lua] Value to set the bootloader environment variable to.
+ * @return [Lua] True, or, in case of error, nil.
+ */
+static int lua_bootloader_env_set(lua_State *L)
+{
+ bootloader_env_set(luaL_checkstring(L, -2), luaL_checkstring(L, -1)) == 0
+ ? lua_pushboolean(L, true)
+ : lua_pushnil(L);
+ return 1;
+}
+
+/**
+ * @brief Drop a bootloader environment variable.
+ *
+ * @param [Lua] Name of the bootloader environment variable to drop.
+ * @return [Lua] True, or, in case of error, nil.
+ */
+static int lua_bootloader_env_unset(lua_State *L)
+{
+ bootloader_env_unset(luaL_checkstring(L, -1)) == 0
+ ? lua_pushboolean(L, true)
+ : lua_pushnil(L);
+ return 1;
+}
+
+/**
+ * @brief Set multiple bootloader environment variables from local file.
+ *
+ * @param [Lua] Path to local file in format `<variable>=<value>`.
+ * @return [Lua] True, or, in case of error, nil.
+ */
+static int lua_bootloader_env_apply(lua_State *L)
+{
+ bootloader_apply_list(luaL_checkstring(L, -1)) == 0
+ ? lua_pushboolean(L, true)
+ : lua_pushnil(L);
+ return 1;
+}

/**
* @brief Get update state from persistent storage (bootloader).
@@ -1479,6 +1559,35 @@ static int suricatta_lua_module(lua_State *L)
#undef MAP
lua_settable(L, -3);

+ luaL_Reg lua_funcs_bootloader[] = {
+ { "is", lua_bootloader_is },
+ { "get", lua_bootloader_get },
+ { NULL, NULL }
+ };
+ luaL_Reg lua_funcs_bootloader_env[] = {
+ { "get", lua_bootloader_env_get },
+ { "set", lua_bootloader_env_set },
+ { "unset", lua_bootloader_env_unset },
+ { "apply", lua_bootloader_env_apply },
+ { NULL, NULL }
+ };
+ lua_pushstring(L, "bootloader");
+ lua_newtable(L);
+ luaL_setfuncs(L, lua_funcs_bootloader, 0);
+ lua_pushstring(L, "bootloaders");
+ lua_newtable(L);
+ push_to_table(L, "EBG", BOOTLOADER_EBG);
+ push_to_table(L, "NONE", BOOTLOADER_NONE);
+ push_to_table(L, "GRUB", BOOTLOADER_GRUB);
+ push_to_table(L, "UBOOT", BOOTLOADER_UBOOT);
+ lua_settable(L, -3);
+ lua_pushstring(L, "env");
+ lua_newtable(L);
+ luaL_setfuncs(L, lua_funcs_bootloader_env, 0);
+ lua_settable(L, -3);
+
+ lua_settable(L, -3);
+
lua_pushstring(L, "status");
lua_newtable(L);
push_to_table(L, "OK", SERVER_OK);
diff --git a/suricatta/suricatta.lua b/suricatta/suricatta.lua
index 81988d2..bfcb3be 100644
--- a/suricatta/suricatta.lua
+++ b/suricatta/suricatta.lua
@@ -58,6 +58,62 @@ suricatta.notify = {
}


+--- SWUpdate's bootloader interface as in `include/bootloader.h`.
+--
+--- @class suricatta.bootloader
+suricatta.bootloader = {
+ --- Bootloaders supported by SWUpdate.
+ --
+ --- @enum suricatta.bootloader.bootloaders
+ bootloaders = {
+ EBG = "ebg",
+ NONE = "none",
+ GRUB = "grub",
+ UBOOT = "uboot",
+ },
+ --- Operations on the currently set bootloader's environment.
+ --
+ --- @class suricatta.bootloader.env
+ env = {}
+}
+
+--- Get currently set bootloader's name.
+--
+--- @return suricatta.bootloader.bootloaders | nil # Name of currently set bootloader
+suricatta.bootloader.get = function() end
+
+--- Test whether bootloader `name` is currently set.
+--
+--- @param name suricatta.bootloader.bootloaders Name of bootloader to test for being currently selected
+--- @return boolean # True if `name` is currently set bootloader, false otherwise
+suricatta.bootloader.is = function(name) end
+
+--- Get value of a bootloader environment variable.
+--
+--- @param variable string Name of the bootloader environment variable to get value for
+--- @return string | nil # Value of the bootloader environment variable or nil if non-existent
+suricatta.bootloader.env.get = function(variable) end
+
+--- Set value of a bootloader environment variable.
+--
+--- @param variable string Name of the bootloader environment variable to set
+--- @param value string Value to set the bootloader environment variable `variable` to
+--- @return boolean | nil # True on success, nil on error
+suricatta.bootloader.env.set = function(variable, value) end
+
+--- Drop a bootloader environment variable.
+--
+--- @param variable string Name of the bootloader environment variable to drop
+--- @return boolean | nil # True on success, nil on error
+suricatta.bootloader.env.unset = function(variable) end
+
+--- Set multiple bootloader environment variables from local file.
+--
+--- @param filename string Path to local file in format `<variable>=<value>`
+--- @return boolean | nil # True on success, nil on error
+suricatta.bootloader.env.apply = function(filename) end
+
+
--- SWUpdate's persistent state IDs as in `include/state.h` and reverse-lookup.
--
--- @enum suricatta.pstate
--
2.38.1

[Christian Storm]

Completely rework the EFI Boot Guard Binding accommodating to
libebgenv's implicit assumptions and actions. To this end,
environment modification transactions are introduced.
The assumptions and rationale is documented and in addition
log messages are emitted for particular combinations of
bootloader_transaction_marker and bootloader_state_marker.

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

bootloader/ebg.c | 567 ++++++++++++++++++++++++++++++----------
core/stream_interface.c | 34 +++
2 files changed, 469 insertions(+), 132 deletions(-)

diff --git a/bootloader/ebg.c b/bootloader/ebg.c
index 4194a38..99ee40b 100644
--- a/bootloader/ebg.c
+++ b/bootloader/ebg.c
@@ -1,22 +1,22 @@
/*
* Author: Christian Storm
- * Author: Andreas Reichel
- * Copyright (C) 2018, Siemens AG
+ * Copyright (C) 2022, Siemens AG
*
* SPDX-License-Identifier: GPL-2.0-only
*/

+#include <stdint.h>
#include <unistd.h>
#include <stdlib.h>
#include <limits.h>
#include <stdbool.h>
#include <stdio.h>
#include <string.h>
-#include <util.h>
+#include <dlfcn.h>
#include <efibootguard/ebgenv.h>
-#include <generated/autoconf.h>
-#include <state.h>
-#include "dlfcn.h"
+#include "generated/autoconf.h"
+#include "util.h"
+#include "state.h"
#include "bootloader.h"

static struct {
@@ -25,184 +25,481 @@ static struct {
int (*env_open_current)(ebgenv_t *e);
int (*env_get)(ebgenv_t *e, char *key, char* buffer);
int (*env_set)(ebgenv_t *e, char *key, char *value);
- int (*env_set_ex)(ebgenv_t *e, char *key, uint64_t datatype, uint8_t *value, uint32_t datalen);
+ int (*env_set_ex)(ebgenv_t *e, char *key, uint64_t datatype,
+ uint8_t *value, uint32_t datalen);
uint16_t (*env_getglobalstate)(ebgenv_t *e);
int (*env_setglobalstate)(ebgenv_t *e, uint16_t ustate);
int (*env_close)(ebgenv_t *e);
int (*env_finalize_update)(ebgenv_t *e);
} libebg;

-static ebgenv_t ebgenv = {0};

-static int do_env_set(const char *name, const char *value)
+/*
+ * ----------------------------------------------------------------------------
+ * | Logics, Assumptions & Rationale |
+ * ----------------------------------------------------------------------------
+ *
+ * EFI Boot Guard boots the environment having `EBGENV_IN_PROGRESS == 0`
+ * and the highest revision number. If multiple environments have the highest
+ * revision number, environment probing order is decisive.
+ * This environment is called the *current boot path*.
+ * Sorted descending on revision numbers and arbitrated by probing order, the
+ * other environments are termed *alternative boot paths*.
+ *
+ * Environment modifications ― except blessing a successful update ― must not
+ * touch the current boot path. Instead, a new boot path is created by
+ * "upcycling" the least recent alternative boot path.
+ * More specifically, environment modifications are captured in a *transaction*:
+ * An in-memory working copy of the current boot path environment is created
+ * which has a by one incremented higher revision than the current boot path.
+ * Modifications are performed on this working copy environment.
+ * When committing the transaction, i.e., writing it to disk, the new current
+ * boot path is persisted and booted next.
+ *
+ * A transaction is started by setting
+ * `EBGENV_USTATE = STATE_IN_PROGRESS` or
+ * `BOOTVAR_TRANSACTION = STATE_IN_PROGRESS`
+ * which is idempotent. Then, `libebgenv` sets
+ * - `EBGENV_IN_PROGRESS = 1` and
+ * - `EBGENV_REVISION` to the current boot path's revision plus one, and
+ * - the transaction `inflight` marker is set to `true`.
+ *
+ * A transaction is committed when setting `EBGENV_USTATE = STATE_INSTALLED`.
+ * Then, `libebgenv` sets
+ * - `EBGENV_IN_PROGRESS = 0` and
+ * - `EBGENV_USTATE = USTATE_INSTALLED`,
+ * - the new current boot path is persisted to disk, and
+ * - the transaction `inflight` marker is reset to `false`.
+ * With this, the current boot path becomes the most recent alternative boot
+ * path serving as rollback boot path if the new current boot path fails to
+ * boot. In this case, the failed boot path is marked with
+ * - `EBGENV_USTATE = USTATE_FAILED` and
+ * - `EBGENV_REVISION = 0`
+ * by which the rollback boot path becomes the current boot path (again).
+ * If the new current boot path boots successfully, the /current/ boot path
+ * needs to be written to acknowledge the successful update via setting
+ * `EBGENV_USTATE = USTATE_OK`.
+ *
+ * Note: The modification of EFI Boot Guard's environment variable
+ * `EBGENV_IN_PROGRESS` cannot be disabled as it's hard-wired in EFI
+ * Boot Guard with particular semantics.
+ * Note: Successive calls to libebgenv's `env_open_current()` after the first
+ * invocation are no-ops and select the in-memory working copy's current
+ * environment.
+ * Note: libebgenv's `env_close()` first *writes* to the current environment and
+ * then closes it. There's currently no way to just close it, e.g., for re-
+ * loading the environment from disk.
+ *
+ */
+
+
+/* EFI Boot Guard hard-coded environment variable names. */
+#define EBGENV_IN_PROGRESS (char *)"in_progress"
+#define EBGENV_REVISION (char *)"revision"
+#define EBGENV_USTATE (char *)"ustate"
+
+static ebgenv_t ebgenv = { 0 };
+static bool inflight = false;
+
+static inline bool is(const char *s1, const char *s2)
{
- int ret;
+ return strcmp(s1, s2) == 0;
+}

- errno = 0;
- libebg.beverbose(&ebgenv, loglevel > INFOLEVEL ? true : false);
+static char *_env_get(const char *name)
+{
+ /*
+ * libebgenv's env_get() is two-staged: The first call yields the
+ * value's size in bytes, the second call, with an accordingly
+ * sized buffer, yields the actual value.
+ */
+ size_t size = libebg.env_get(&ebgenv, (char *)name, NULL);
+ if (size == 0) {
+ WARN("Cannot find key %s", name);
+ return NULL;
+ }

- if ((ret = libebg.env_open_current(&ebgenv)) != 0) {
- ERROR("Cannot open current bootloader environment: %s.", strerror(ret));
- return ret;
- }
-
- if (strncmp(name, BOOTVAR_TRANSACTION, strlen(name) + 1) == 0 &&
- strncmp(value, get_state_string(STATE_IN_PROGRESS),
- strlen(get_state_string(STATE_IN_PROGRESS)) + 1) == 0) {
- DEBUG("Setting %s=%s in bootloader environment", name, value);
- /* Open or create a new environment to reflect
- * EFI Boot Guard's representation of SWUpdate's
- * recovery_status=in_progress. */
- if ((ret = libebg.env_create_new(&ebgenv)) != 0) {
- ERROR("Cannot open/create new bootloader environment: %s.",
- strerror(ret));
- }
- } else if (strncmp(name, (char *)STATE_KEY, strlen((char *)STATE_KEY) + 1) == 0) {
- /* Map update_state_t to EFI Boot Guard's API. */
- switch (*value) {
- case STATE_IN_PROGRESS:
- case STATE_FAILED:
- case STATE_TESTING:
- /* Fall-through for update_state_t values destined either
- * for BOOTVAR_TRANSACTION or handled by EBG internally. */
- break;
- case STATE_OK:
- case STATE_INSTALLED:
- DEBUG("Setting %s=%s in bootloader environment", name, value);
- if ((ret = libebg.env_setglobalstate(&ebgenv, *value - '0')) != 0) {
- ERROR("Cannot set %s=%s in bootloader environment.", STATE_KEY, value);
- }
- break;
- default:
- ret = -EINVAL;
- ERROR("Unsupported bootloader environment assignment %s=%s.", STATE_KEY, value);
- }
- } else {
- /* A new environment is created if EFI Boot Guard's
- * representation of SWUpdate's recovery_status is
- * not in_progress. */
- DEBUG("Setting %s=%s in bootloader environment", name, value);
- if ((ret = libebg.env_create_new(&ebgenv)) != 0) {
- ERROR("Cannot open/create new bootloader environment: %s.",
- strerror(ret));
- return ret;
- }
- if ((ret = libebg.env_set(&ebgenv, (char *)name, (char *)value)) != 0) {
- ERROR("Cannot set %s=%s in bootloader environment: %s.",
- name, value, strerror(ret));
- }
+ char *value = malloc(size);
+ if (value == NULL) {
+ ERROR("Error allocating memory");
+ return NULL;
+ }
+
+ int result = libebg.env_get(&ebgenv, (char *)name, value);
+ if (result != 0) {
+ ERROR("Cannot get %s: %s", name, strerror(-result));
+ free(value);
+ return NULL;
+ }
+ return value;
+}
+
+/* Note: EFI Boot Guard Environment integers are at most uint32_t. */
+static inline uint32_t _env_to_uint32(char *value)
+{
+ if (!value) {
+ return UINT_MAX;
}
- (void)libebg.env_close(&ebgenv);
+ errno = 0;
+ uint32_t result = strtoul(value, NULL, 10);
+ free(value);
+ return errno != 0 ? UINT_MAX : result;
+}

- return ret;
+static inline uint8_t ascii_to_uint8(unsigned char value)
+{
+ return value - '0';
}

-static int do_env_unset(const char *name)
+static inline unsigned char uint8_to_ascii(uint8_t value)
{
- int ret;
+ return value + '0';
+}

+static char *do_env_get(const char *name)
+{
+ errno = 0;
libebg.beverbose(&ebgenv, loglevel > INFOLEVEL ? true : false);

- if ((ret = libebg.env_open_current(&ebgenv)) != 0) {
- ERROR("Cannot open current bootloader environment: %s.", strerror(ret));
- return ret;
+ int result = libebg.env_open_current(&ebgenv);
+ if (result != 0) {
+ ERROR("Cannot open bootloader environment: %s", strerror(result));
+ return NULL;
}

- DEBUG("Unsetting %s in bootloader environment", name);
- if (strncmp(name, BOOTVAR_TRANSACTION, strlen(name) + 1) == 0) {
- ret = libebg.env_finalize_update(&ebgenv);
- if (ret) {
- ERROR("Cannot unset %s in bootloader environment: %s.", BOOTVAR_TRANSACTION, strerror(ret));
+ if (!inflight && is(name, EBGENV_USTATE)) {
+ /*
+ * When not in an in-flight transaction, get the "global significant"
+ * EBGENV_USTATE value:
+ * If rolled-back and successfully booted, there's an alternative
+ * boot path that has
+ * EBGENV_REVISION == 0 and
+ * EBGENV_USTATE == STATE_FAILED
+ * which is how EFI Boot Guard encodes a rolled-back condition.
+ * To act on this condition, e.g., report and clear it, STATE_FAILED
+ * as the global significant EBGENV_USTATE value is returned.
+ *
+ * If not rolled-back, the current boot path's EBGENV_USTATE value
+ * is returned.
+ */
+ char *value = NULL;
+ if (asprintf(&value, "%u", libebg.env_getglobalstate(&ebgenv)) == -1) {
+ ERROR("Error allocating memory");
+ return NULL;
}
- } else if (strncmp(name, (char *)STATE_KEY, strlen((char *)STATE_KEY) + 1) == 0) {
- /* Unsetting STATE_KEY is semantically equivalent to setting it to STATE_OK. */
- if ((ret = libebg.env_setglobalstate(&ebgenv, STATE_OK - '0')) != 0) {
- ERROR("Cannot unset %s in bootloader environment.", STATE_KEY);
- }
- } else {
- ret = libebg.env_set_ex(&ebgenv, (char *)name, USERVAR_TYPE_DELETED, (uint8_t *)"", 1);
+ return value;
}
- (void)libebg.env_close(&ebgenv);

- return ret;
+ return _env_get(name);
}

-static char *do_env_get(const char *name)
+static int create_new_environment(void)
{
- char *value = NULL;
- size_t size;
+ uint32_t revision = _env_to_uint32(_env_get(EBGENV_REVISION));
+ uint8_t in_progress = (uint8_t)_env_to_uint32(_env_get(EBGENV_IN_PROGRESS));
+ if ((revision == UINT_MAX) || (in_progress == UINT8_MAX)) {
+ ERROR("Cannot get environment revision or in-progress marker");
+ return -EIO;
+ }
+ if (in_progress == 1) {
+ return 0;
+ }
+ int result = libebg.env_create_new(&ebgenv);
+ if (result != 0) {
+ ERROR("Cannot create new environment revision: %s", strerror(result));
+ return -result;
+ }
+ /*
+ * libebgenv has now set:
+ * EBG_ENVDATA->in_progress = 1
+ * EBG_ENVDATA->revision = <current boot path's revision>++
+ */
+ uint32_t new_revision = _env_to_uint32(_env_get(EBGENV_REVISION));
+ if (new_revision == UINT_MAX) {
+ return -EIO;
+ }
+ if (++revision != new_revision) {
+ ERROR("No new environment revision was created!");
+ return -ENOENT;
+ }
+ inflight = true;
+ DEBUG("Created new environment revision %d, starting transaction",
+ new_revision);
+ return 0;
+}

+static int do_env_set(const char *name, const char *value)
+{
errno = 0;
libebg.beverbose(&ebgenv, loglevel > INFOLEVEL ? true : false);

- int ret;
- if ((ret = libebg.env_open_current(&ebgenv)) != 0) {
- ERROR("Cannot open current bootloader environment: %s.",
- strerror(ret));
- return NULL;
+ if (!inflight) {
+ /*
+ * Without an in-flight transaction, only allow
+ * (1) starting a transaction or
+ * (2) acknowledging an update.
+ */
+ if (!is(name, BOOTVAR_TRANSACTION) && !is(name, EBGENV_USTATE)) {
+ ERROR("Not setting %s=%s w/o in-flight transaction",
+ name, value);
+ return -EINVAL;
+ }
+ if (is(name, BOOTVAR_TRANSACTION) &&
+ !is(value, get_state_string(STATE_IN_PROGRESS))) {
+ ERROR("Not setting %s=%s w/o in-flight transaction",
+ name, value);
+ return -EINVAL;
+ }
+
+ if (is(name, EBGENV_USTATE)) {
+ switch (*value) {
+ case STATE_OK: /* Acknowledging an update. */
+ case STATE_IN_PROGRESS: /* Starting a transaction. */
+ break;
+ default:
+ ERROR("Not setting %s=%s w/o in-flight transaction",
+ name, value);
+ return -EINVAL;
+ }
+ }
+ }
+
+ int result = libebg.env_open_current(&ebgenv);
+ if (result != 0) {
+ ERROR("Cannot open bootloader environment: %s", strerror(result));
+ return -result;
}

- if (strncmp(name, (char *)STATE_KEY, strlen((char *)STATE_KEY) + 1) == 0) {
- value = (char *)malloc(sizeof(char));
- *value = libebg.env_getglobalstate(&ebgenv);
- /* Map EFI Boot Guard's int return to update_state_t's char value */
- *value = *value + '0';
- } else {
- if ((size = libebg.env_get(&ebgenv, (char *)name, NULL)) != 0) {
- value = malloc(size);
- if (value) {
- if (libebg.env_get(&ebgenv, (char *)name, value) != 0) {
- value = NULL;
+ if (is(name, BOOTVAR_TRANSACTION)) {
+ /*
+ * Note: This gets called by core/stream_interface.c's
+ * update_transaction_state() with the update
+ * state's string representation.
+ */
+ if (is(value, get_state_string(STATE_IN_PROGRESS))) {
+ return create_new_environment();
+ }
+
+ if (is(value, get_state_string(STATE_FAILED)) ||
+ is(value, get_state_string(STATE_INSTALLED)) ||
+ is(value, get_state_string(STATE_OK))) {
+ /*
+ * Irrespective of the value, set EBGENV_IN_PROGRESS = 0,
+ * else EFI Boot Guard will *NOT* consider this environment
+ * for booting at all.
+ */
+ if ((result = libebg.env_set(&ebgenv, EBGENV_IN_PROGRESS,
+ (char *)"0")) != 0) {
+ ERROR("Error setting %s=0: %s", EBGENV_IN_PROGRESS,
+ strerror(-result));
+ return result;
+ }
+ return 0;
+ }
+
+ /* Fall-through for invalid EBGENV_IN_PROGRESS values. */
+ ERROR("Unsupported setting %s=%s", EBGENV_IN_PROGRESS, value);
+ return -EINVAL;
+ }
+
+ if (!is(name, EBGENV_USTATE)) {
+ if ((result = libebg.env_set(&ebgenv, (char *)name, (char *)value)) != 0) {
+ ERROR("Error setting %s=%s: %s", name, value, strerror(-result));
+ return result;
+ }
+ return 0;
+ }
+
+ switch (*value) {
+ case STATE_IN_PROGRESS:
+ return create_new_environment();
+ case STATE_OK:
+ if (inflight) {
+ /*
+ * Environment modification within the in-flight transaction,
+ * i.e., the in-memory working copy, just set it.
+ */
+ if ((result = libebg.env_set(&ebgenv, EBGENV_USTATE,
+ (char *)value)) != 0) {
+ ERROR("Error setting %s=%s: %s", EBGENV_USTATE,
+ get_state_string(STATE_OK),
+ strerror(-result));
+ return result;
+ }
+ return 0;
+ }
+
+ unsigned char global_ustate = uint8_to_ascii((uint8_t)
+ libebg.env_getglobalstate(&ebgenv));
+ if (global_ustate == STATE_NOT_AVAILABLE) {
+ ERROR("Cannot read global %s", EBGENV_USTATE);
+ return -EIO;
+ }
+ unsigned char current_ustate = uint8_to_ascii((uint8_t)
+ _env_to_uint32(
+ _env_get(EBGENV_USTATE)));
+ if (!is_valid_state(current_ustate)) {
+ ERROR("Cannot read current %s", EBGENV_USTATE);
+ return -EIO;
+ }
+
+ if (global_ustate == STATE_FAILED) {
+ TRACE("Found rolled-back condition, clearing marker");
+ /*
+ * Clear rolled-back condition by setting
+ * EBGENV_USTATE = STATE_OK on all alternative
+ * boot paths having EBGENV_USTATE != STATE_OK
+ * and write them to disk.
+ * Note: Does not write to the current boot path (to
+ * which was rolled-back to).
+ */
+ if ((result = libebg.env_setglobalstate(&ebgenv,
+ ascii_to_uint8(STATE_OK))) != 0) {
+ ERROR("Error resetting failure condition: %s",
+ strerror(-result));
+ return result;
+ }
+ /*
+ * Restore prior current boot path's EBGENV_USTATE value
+ * (as there's no way to reload it from disk).
+ * Should be STATE_OK anyway but better play it safe...
+ */
+ if (current_ustate != STATE_OK) {
+ if ((result = libebg.env_set(&ebgenv, EBGENV_USTATE,
+ (char[2]){ current_ustate, '\0' })) != 0) {
+ ERROR("Error restoring %s: %s", EBGENV_USTATE,
+ strerror(-result));
+ return result;
}
}
+ return 0;
+ }
+
+ if (current_ustate == STATE_TESTING) {
+ TRACE("Found successful update, blessing it");
+ /*
+ * Acknowledge, update the /current/ boot path on disk.
+ */
+ if ((result = libebg.env_set(&ebgenv, EBGENV_USTATE,
+ (char *)value)) != 0) {
+ ERROR("Error setting %s=%s: %s", EBGENV_USTATE,
+ value, strerror(-result));
+ return result;
+ }
+
+ if ((result = libebg.env_close(&ebgenv)) != 0) {
+ ERROR("Error persisting environment: %s",
+ strerror(result));
+ return -result;
+ }
+ return 0;
}
+
+ WARN("Unsupported state for setting %s=%s", EBGENV_USTATE,
+ get_state_string(*value));
+ return -EINVAL;
+ case STATE_INSTALLED:
+ if ((result = libebg.env_finalize_update(&ebgenv)) != 0) {
+ ERROR("Error finalizing environment: %s", strerror(result));
+ return -result;
+ }
+ /*
+ * libebgenv has now set:
+ * EBG_ENVDATA->in_progress = 0
+ * EBG_ENVDATA->ustate = USTATE_INSTALLED.
+ * Now, persist the in-memory working copy environment as new
+ * current boot path and terminate the transaction.
+ * Note: Does write to an alternative boot path "upcycled" to
+ * the new current boot path; Does *NOT* write to the current
+ * boot path.
+ */
+ if ((result = libebg.env_close(&ebgenv)) != 0) {
+ ERROR("Error persisting environment: %s", strerror(result));
+ return -result;
+ }
+ inflight = false;
+ return 0;
+ case STATE_FAILED:
+ /*
+ * SWUpdate sets this if the installation has failed. In this case,
+ * the transaction is simply not committed, so nothing to do.
+ */
+ return 0;
+ default:
+ break;
}

- (void)libebg.env_close(&ebgenv);
+ /*
+ * Fall-through for invalid or EBGENV_USTATE values handled
+ * by EFI Boot Guard internally.
+ */
+ WARN("Unsupported setting %s=%s", EBGENV_USTATE, get_state_string(*value));
+ return -EINVAL;
+}
+
+static int do_env_unset(const char *name)
+{
+ errno = 0;
+ libebg.beverbose(&ebgenv, loglevel > INFOLEVEL ? true : false);

- if (value == NULL) {
- ERROR("Cannot get %s from bootloader environment: %s",
- name, strerror(errno));
+ int result = libebg.env_open_current(&ebgenv);
+ if (result != 0) {
+ ERROR("Cannot open bootloader environment: %s", strerror(result));
+ return -result;
}

- return value;
+ if (is(name, EBGENV_USTATE)) {
+ /*
+ * Unsetting EBGENV_USTATE is semantically equivalent to
+ * setting EBGENV_USTATE = STATE_OK.
+ */
+ return do_env_set(EBGENV_USTATE, (char[2]){ STATE_OK, '\0' });
+ }
+
+ if (is(name, BOOTVAR_TRANSACTION)) {
+ /*
+ * Unsetting BOOTVAR_TRANSACTION is semantically equivalent to
+ * setting EBGENV_IN_PROGRESS = 0.
+ */
+ return do_env_set(EBGENV_IN_PROGRESS, (char *)"0");
+ }
+
+ if ((result = libebg.env_set_ex(&ebgenv, (char *)name, USERVAR_TYPE_DELETED,
+ (uint8_t *)"", 1)) != 0) {
+ ERROR("Error unsetting %s: %s", name, strerror(-result));
+ return result;
+ }
+ return 0;
}

static int do_apply_list(const char *filename)
{
- FILE *fp = NULL;
- char *line = NULL;
- char *key;
- char *value;
- size_t len = 0;
- int ret = 0;
-
errno = 0;
libebg.beverbose(&ebgenv, loglevel > INFOLEVEL ? true : false);

- if (!(fp = fopen(filename, "rb"))) {
- ERROR("Failed to open bootloader environment file %s: %s",
- filename, strerror(errno));
- return -1;
+ FILE *file = fopen(filename, "rb");
+ if (!file) {
+ ERROR("Cannot open bootloader environment source file %s: %s",
+ filename, strerror(errno));
+ return -EIO;
}

- while ((getline(&line, &len, fp)) != -1) {
- key = strtok(line, "=");
- value = strtok(NULL, "\t\n");
- if (value != NULL && key != NULL) {
- if ((ret = do_env_set(key, value)) != 0) {
+ char *line = NULL;
+ size_t length = 0;
+ int result = 0;
+ while ((getline(&line, &length, file)) != -1) {
+ char *key = strtok(line, "=");
+ char *value = strtok(NULL, "\t\n");
+ if (key != NULL && value != NULL) {
+ if ((result = do_env_set(key, value)) != 0) {
break;
}
}
}

- if (fp) {
- fclose(fp);
- }
- if (line) {
- free(line);
- }
- return ret;
+ fclose(file);
+ free(line);
+ return result;
}

static bootloader ebg = {
@@ -212,9 +509,15 @@ static bootloader ebg = {
.apply_list = &do_apply_list
};

-static bootloader* probe(void)
+static bootloader *probe(void)
{
- void* handle = dlopen("libebgenv.so.0", RTLD_NOW | RTLD_GLOBAL);
+ if (!is(STATE_KEY, EBGENV_USTATE)) {
+ ERROR("CONFIG_UPDATE_STATE_BOOTLOADER=%s is required for "
+ "EFI Boot Guard support", EBGENV_USTATE);
+ return NULL;
+ }
+
+ void *handle = dlopen("libebgenv.so.0", RTLD_NOW | RTLD_GLOBAL);
if (!handle) {
return NULL;
}
diff --git a/core/stream_interface.c b/core/stream_interface.c
index 81c26c3..9ba6332 100644
--- a/core/stream_interface.c
+++ b/core/stream_interface.c
@@ -611,6 +611,40 @@ void *network_initializer(void *data)
if (!(inst.fd < 0))
close(inst.fd);

+ if (!software->parms.dry_run && is_bootloader(BOOTLOADER_EBG)) {
+ if (!software->bootloader_transaction_marker) {
+ /*
+ * EFI Boot Guard's "in_progress" environment variable
+ * has special semantics hard-coded, hence the
+ * bootloader transaction marker cannot be disabled.
+ */
+ TRACE("Note: Setting EFI Boot Guard's 'in_progress' "
+ "environment variable cannot be disabled.");
+ }
+ if (!software->bootloader_state_marker) {
+ /*
+ * With a disabled update state marker, there's no
+ * transaction auto-commit via
+ * save_state(STATE_INSTALLED)
+ * which effectively calls
+ * bootloader_env_set(STATE_KEY, STATE_INSTALLED).
+ * Hence, manually calling save_state(STATE_INSTALLED)
+ * or equivalent is required to commit the transaction.
+ * This can be useful to, e.g., terminate the transaction
+ * from an according progress interface client or an
+ * SWUpdate suricatta module after it has received an
+ * update activation request from the remote server.
+ */
+ TRACE("Note: EFI Boot Guard environment transaction "
+ "will not be auto-committed.");
+ }
+ if (!software->bootloader_transaction_marker &&
+ !software->bootloader_state_marker) {
+ WARN("EFI Boot Guard environment modifications will "
+ "not be persisted.");
+ }
+ }
+
/* do carry out the installation (flash programming) */
if (ret == 0) {
TRACE("Valid image found: copying to FLASH");
--
2.38.1

[Stefano Babic]

Hi Christian,

CI fails, see:

https://source.denx.de/swupdate/swupdate/-/pipelines/14326

Reason is here due to the introduced check - our test in CI does not
foresee a valid environment. I do not see the ERROR as well, I think
because bootloader are loaded before initializing the tracing inside
SWUpdate. Anyway, with this code:

[TRACE] : SWUPDATE running : [print_registered_bootloaders] : ebg
shared lib not found.

Could you check ?

So all this code is just to track that variable is not stored persistently.

> /* do carry out the installation (flash programming) */
> if (ret == 0) {
> TRACE("Valid image found: copying to FLASH");

Best regards,
Stefano
--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Christian Storm]

Hi Stefano,

> CI fails, see:
>
> https://source.denx.de/swupdate/swupdate/-/pipelines/14326

Sorry, my bad, see below.

My bad, many thanks for analyzing!
I reproduced the error and will look into it...

The reason I introduced the check there is that I wanted to fail early
in case this required assumption does not hold (and to not clutter the
methods by having this check first in each of them).

The failure reason is indeed that those checks run with the default
bootloader which they don't find as it's not deployed. So, this should
fix it (I will test it and submit a patch) as it uses the default NONE
bootloader instead of a specific one:

--- a/scripts/acceptance-tests/CheckImage.mk
+++ b/scripts/acceptance-tests/CheckImage.mk
@@ -6,7 +6,7 @@
#
# test commands for --check command-line option
#
-SWU_CHECK_BASE = ./swupdate -l 5 -c $(if $(CONFIG_SIGALG_CMS),-k $(obj)/cacert.pem) $(if $(strip $(filter %.cfg, $^)), -f $(filter %.cfg, $^))
+SWU_CHECK_BASE = ./swupdate -B none -l 5 -c $(if $(CONFIG_SIGALG_CMS),-k $(obj)/cacert.pem) $(if $(strip $(filter %.cfg, $^)), -f $(filter %.cfg, $^))
SWU_CHECK = $(SWU_CHECK_BASE) $(if $(CONFIG_HW_COMPATIBILITY),-H test:1) $(if $(strip $(filter-out FORCE,$<)),-i $<) $(if $(strip $(KBUILD_VERBOSE:0=)),,>/dev/null 2>&1)

quiet_cmd_swu_check_assert_false = RUN $@

Yes, but that's just a means to an end: For one, it works around writing
intermediate boot configurations which are not/might not be complete and
hence may not boot correctly and second, it works around writing the
environment(s) too often including unnecessarily touching the current
boot path under some conditions. The whole idea is to capture modifications
in a "transaction" and only flush to disk once a configuration is
"complete". The same is true for acknowledging an update. This is what
I tried to explain in the "Logics, Assumptions & Rationale" Section...


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T CED SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany