[PATCH v3 0/1] INI format as alternative to BGENV.DAT

[Tobias Schmidl]

Changes to v2:
- Removed the external library, done both parsing and writing by hand.
Parsing is done even without libc.

Changes to v1:
- Added the library as single files instead of external dependency
- The distinction between in BGENV.DAT and BGENV.INI is now the other
way around - if the file name is BGENV.INI, it is parsed with as INI
file, in every other case the old behavior is used.
- Squashed some commits that belonged together

Tobias Schmidl (1):
INI file: Added parsing

BGENV.INI.example | 9 ++
Makefile.am | 2 +
env/env_ini_buffer.c | 310 +++++++++++++++++++++++++++++++++++++++
env/env_ini_buffer_efi.c | 1 +
env/fatvars.c | 19 ++-
include/env_ini_buffer.h | 24 +++
include/envdata.h | 13 ++
include/syspart.h | 5 +
tools/bg_envtools.c | 24 ++-
tools/bg_setenv.c | 55 +++++--
10 files changed, 447 insertions(+), 15 deletions(-)
create mode 100644 BGENV.INI.example
create mode 100644 env/env_ini_buffer.c
create mode 120000 env/env_ini_buffer_efi.c
create mode 100644 include/env_ini_buffer.h

--
2.37.2

[Tobias Schmidl]

Added the possibility of using an INI file instead of BGENV.DAT.

- Reading is possible from both `bg_printenv` and `efibootguard`
- Writing is at the moment only possible from `bg_setenv` (or via text
editor).

Signed-off-by: Tobias Schmidl <tobias...@siemens.com>
---

BGENV.INI.example | 9 ++
Makefile.am | 2 +
env/env_ini_buffer.c | 310 +++++++++++++++++++++++++++++++++++++++
env/env_ini_buffer_efi.c | 1 +
env/fatvars.c | 19 ++-
include/env_ini_buffer.h | 24 +++
include/envdata.h | 13 ++
include/syspart.h | 5 +
tools/bg_envtools.c | 24 ++-
tools/bg_setenv.c | 55 +++++--
10 files changed, 447 insertions(+), 15 deletions(-)
create mode 100644 BGENV.INI.example
create mode 100644 env/env_ini_buffer.c
create mode 120000 env/env_ini_buffer_efi.c
create mode 100644 include/env_ini_buffer.h

diff --git a/BGENV.INI.example b/BGENV.INI.example
new file mode 100644
index 0000000..7a1db34
--- /dev/null
+++ b/BGENV.INI.example
@@ -0,0 +1,9 @@
+# This file is by design formatted badly, to test for various cases.
+[bg_envdata]
+kernelfile = zImage
+kernelparams =
+in_progress = 0
+ustate =0
+watchdog_timeout_sec= 0
+revision=0x00000000
+
diff --git a/Makefile.am b/Makefile.am
index 48c560f..58d9684 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -89,6 +89,7 @@ libebgenv_a_SOURCES = \
env/env_config_file.c \
env/env_config_partitions.c \
env/env_disk_utils.c \
+ env/env_ini_buffer.c \
env/uservars.c \
tools/ebgpart.c

@@ -175,6 +176,7 @@ efi_sources = \
drivers/watchdog/init_array_start.S \
$(efi_sources_watchdogs) \
drivers/watchdog/init_array_end.S \
+ env/env_ini_buffer_efi.c \
env/syspart.c \
env/fatvars.c \
utils.c \
diff --git a/env/env_ini_buffer.c b/env/env_ini_buffer.c
new file mode 100644
index 0000000..b75171d
--- /dev/null
+++ b/env/env_ini_buffer.c
@@ -0,0 +1,310 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (c) Siemens AG, 2022
+ *
+ * Authors:
+ * Tobias Schmidl <tobias...@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include "env_ini_buffer.h"
+#include "envdata.h"
+
+static const char *my_strchr(const char *input, char search)
+{
+ for (;; ++input) {
+ if (*input == search) return input;
+ if (*input == '\0') return NULL;
+ }
+}
+static size_t my_strlen(const char *input)
+{
+ const char *iter = input;
+ while (*iter++ != '\0')
+ ;
+ return input - iter;
+}
+
+static int my_strncmp(const char *first_input, const char *second_input,
+ size_t count)
+{
+ for (; count != 0 && *first_input != '\0' && *second_input != '\0';
+ ++first_input, ++second_input, --count) {
+ if (*first_input != *second_input)
+ return *first_input - *second_input;
+ }
+ return 0;
+}
+
+static uint64_t my_atoi_dec(const char *input, size_t count)
+{
+ uint64_t return_value = 0;
+
+ if (count == 0 || input == NULL) return UINT64_MAX;
+
+ for (; count != 0 && *input != '\0'; ++input, --count) {
+ if (*input < '0' || *input > '9' ||
+ return_value >= INT_MAX / 10)
+ return UINT64_MAX;
+
+ return_value = return_value * 10 + *input - '0';
+ }
+ return return_value;
+}
+
+static uint64_t my_atoi_hex(const char *input, size_t count)
+{
+ uint64_t return_value = 0;
+
+ for (; count != 0 && *input != '\0'; ++input, --count) {
+ return_value <<= 4;
+ return_value +=
+ (*input < 'A') ? *input & 0xF : (*input & 0x7) + 9;
+ }
+
+ return return_value;
+}
+
+static uint64_t my_atoi(const char *input, size_t count)
+{
+ if (count > 2 && my_strncmp(input, "0x", count) == 0)
+ return my_atoi_hex(input + 2, count - 2);
+ else
+ return my_atoi_dec(input, count);
+}
+
+static inline bool my_isblank(char a)
+{
+ return (a == ' ') || (a == '\t');
+}
+static inline size_t MAX(size_t a, size_t b)
+{
+ return ((a) > (b) ? a : b);
+}
+static inline size_t MIN(size_t a, size_t b)
+{
+ return ((a) < (b) ? a : b);
+}
+
+static inline char16_t *str8to16_n(char16_t *buffer, const char *src,
+ size_t length)
+{
+ if (!src || !buffer) {
+ return NULL;
+ }
+ char16_t *tmp = buffer;
+ for (; *src && length--; *buffer++ = (char16_t)*src++)
+ ;
+ *buffer = 0;
+ return tmp;
+}
+
+static inline const char *skip_whitespace(const char *read_pointer,
+ size_t length)
+{
+ while (read_pointer != NULL && read_pointer < read_pointer + length &&
+ *read_pointer && my_isblank(*read_pointer++))
+ ;
+ return read_pointer;
+}
+
+bool env_ini_buffer_read(const char *read_buffer, BG_ENVDATA *data)
+{
+ bool return_value = true;
+ for (const char *read_pointer = read_buffer, *eol;
+ NULL != (eol = my_strchr(read_pointer, '\n'));
+ read_pointer = eol + 1) {
+ const char sep = '=';
+ const char *value_begin = NULL, *value_end = NULL,
+ *sep_finder = read_pointer;
+ for (; sep_finder < eol && *sep_finder != sep; ++sep_finder)
+ ;
+ if (sep_finder == eol || *sep_finder != sep) continue;
+ const char *key_begin = NULL, *key_end = NULL,
+ *key_finder = read_pointer;
+ for (; key_finder < sep_finder; ++key_finder) {
+ if (key_begin == NULL && !my_isblank(*key_finder)) {
+ key_begin = key_finder;
+ key_end = key_begin;
+ }
+ if (key_end == key_begin && my_isblank(*key_finder)) {
+ key_end = key_finder;
+ }
+ }
+ if (key_begin == NULL || key_end == NULL) continue;
+ if (key_end == key_begin && key_finder == sep_finder)
+ key_end = sep_finder;
+ const char *value_finder = sep_finder + 1;
+ for (; value_finder < eol; ++value_finder) {
+ if (value_begin == NULL && !my_isblank(*value_finder)) {
+ value_begin = value_finder;
+ value_end = value_begin;
+ }
+ if (value_end == value_begin &&
+ my_isblank(*value_finder))
+ value_end = value_finder;
+ }
+ if (value_end == value_begin && value_finder == eol)
+ value_end = eol;
+ if (value_begin == NULL || value_end == NULL ||
+ value_end == value_begin)
+ continue;
+
+ size_t key_length = key_end - key_begin,
+ value_length = value_end - value_begin;
+ if (my_strncmp(key_begin, BG_ENV_KERNELFILE_STRING,
+ MIN(my_strlen(BG_ENV_KERNELFILE_STRING),
+ key_length)) == 0) {
+ return_value &=
+ str8to16_n(data->kernelfile, value_begin,
+ value_length) != NULL;
+ printf_debug("kernelfile: %.*s\n", value_length,
+ value_begin);
+ } else if (my_strncmp(key_begin, BG_ENV_KERNELPARAMS_STRING,
+ MIN(my_strlen(BG_ENV_KERNELPARAMS_STRING),
+ key_length)) == 0) {
+ return_value &=
+ str8to16_n(data->kernelparams, value_begin,
+ value_length) != NULL;
+ printf_debug("kernelparams: %.*s\n", value_length,
+ value_begin);
+ } else if (my_strncmp(key_begin, BG_ENV_IN_PROGRESS_STRING,
+ MIN(my_strlen(BG_ENV_IN_PROGRESS_STRING),
+ key_length)) == 0) {
+ uint64_t test_value =
+ my_atoi(value_begin, value_length);
+ if (test_value != UINT64_MAX) {
+ data->in_progress = (uint8_t)test_value;
+ printf_debug("in_progress: %hhu\n",
+ (uint8_t)test_value);
+ } else
+ return_value = false;
+ } else if (my_strncmp(key_begin, BG_ENV_USTATE_STRING,
+ MIN(my_strlen(BG_ENV_USTATE_STRING),
+ key_length)) == 0) {
+ uint64_t test_value =
+ my_atoi(value_begin, value_length);
+ if (test_value != UINT64_MAX) {
+ data->ustate = (uint8_t)test_value;
+ printf_debug("ustate: %hhu\n",
+ (uint8_t)test_value);
+ } else
+ return_value = false;
+ } else if (
+ my_strncmp(
+ key_begin, BG_ENV_WATCHDOG_TIMEOUT_SEC_STRING,
+ MIN(my_strlen(
+ BG_ENV_WATCHDOG_TIMEOUT_SEC_STRING),
+ key_length)) == 0) {
+ uint64_t test_value =
+ my_atoi(value_begin, value_length);
+ if (test_value != UINT64_MAX) {
+ data->watchdog_timeout_sec =
+ (uint16_t)test_value;
+ printf_debug("watchdog_timeout_sec: %hu\n",
+ (uint16_t)test_value);
+ } else
+ return_value = false;
+ } else if (my_strncmp(key_begin, BG_ENV_REVISION_STRING,
+ MIN(my_strlen(BG_ENV_REVISION_STRING),
+ key_length)) == 0) {
+ uint64_t test_value =
+ my_atoi(value_begin, value_length);
+ if (test_value != UINT64_MAX) {
+ data->revision = (uint32_t)test_value;
+ printf_debug("revision: %u\n",
+ (uint32_t)test_value);
+ } else
+ return_value = false;
+ } else if (my_strncmp(key_begin, BG_ENV_CRC32_STRING,
+ MIN(my_strlen(BG_ENV_KERNELPARAMS_STRING),
+ key_length)) == 0) {
+ uint64_t test_value =
+ my_atoi(value_begin, value_length);
+ if (test_value != UINT64_MAX) {
+ data->crc32 = (uint32_t)test_value;
+ printf_debug("crc32: %u\n",
+ (uint32_t)test_value);
+ } else
+ return_value = false;
+ } else {
+ printf_debug("<unknown key>: %.*s\n", value_length,
+ value_begin);
+ }
+ }
+ return return_value;
+}
+#if !defined(_GNU_EFI) && !defined(GNU_EFI_USE_MS_ABI)
+int env_ini_buffer_write(char *write_buffer, size_t buffer_length,
+ const BG_ENVDATA *data)
+{
+ char *begin_buffer = write_buffer;
+ int bytes_written = 0;
+ size_t bytes_to_write =
+ MIN(buffer_length, strlen(BG_ENVDATA_STRING) + 3);
+ if ((bytes_written = snprintf(write_buffer, bytes_to_write + 1,
+ "[%s]\n", BG_ENVDATA_STRING)) <
+ (int)bytes_to_write)
+ return errno;
+ buffer_length -= bytes_written;
+ write_buffer += bytes_written;
+ char *value_buffer = alloca(BUFFER_SIZE);
+ if (value_buffer == NULL) return EFAULT;
+
+ if (str16to8(value_buffer, data->kernelfile) == NULL) return EFAULT;
+ bytes_to_write =
+ MIN(buffer_length, strnlen(value_buffer, BUFFER_SIZE) + 1);
+ if ((bytes_written = snprintf(write_buffer, bytes_to_write + 1,
+ "%s = %s\n", BG_ENV_KERNELFILE_STRING,
+ value_buffer)) < (int)bytes_to_write)
+ return errno;
+
+ buffer_length -= bytes_written;
+ write_buffer += bytes_written;
+
+ if (str16to8(value_buffer, data->kernelparams) == NULL) return EFAULT;
+ bytes_to_write =
+ MIN(buffer_length, strnlen(value_buffer, BUFFER_SIZE) + 1);
+ if ((bytes_written = snprintf(write_buffer, bytes_to_write + 1,
+ "%s = %s\n", BG_ENV_KERNELPARAMS_STRING,
+ value_buffer)) < (int)bytes_to_write)
+ return errno;
+ buffer_length -= bytes_written;
+ write_buffer += bytes_written;
+
+ if ((bytes_written = snprintf(write_buffer, buffer_length,
+ "%s = %hhu\n", BG_ENV_IN_PROGRESS_STRING,
+ data->in_progress)) < 0)
+ return errno;
+ buffer_length -= bytes_written;
+ write_buffer += bytes_written;
+
+ if ((bytes_written =
+ snprintf(write_buffer, buffer_length, "%s = %hhu\n",
+ BG_ENV_USTATE_STRING, data->ustate)) < 0)
+ return errno;
+ buffer_length -= bytes_written;
+ write_buffer += bytes_written;
+
+ if ((bytes_written = snprintf(write_buffer, buffer_length, "%s = %hu\n",
+ BG_ENV_WATCHDOG_TIMEOUT_SEC_STRING,
+ data->watchdog_timeout_sec)) < 0)
+ return errno;
+ buffer_length -= bytes_written;
+ write_buffer += bytes_written;
+
+ if ((bytes_written =
+ snprintf(write_buffer, buffer_length, "%s = 0x%08x\n",
+ BG_ENV_REVISION_STRING, data->revision)) < 0)
+ return errno;
+ buffer_length -= bytes_written;
+ write_buffer += bytes_written;
+
+ return 0;
+}
+#endif
diff --git a/env/env_ini_buffer_efi.c b/env/env_ini_buffer_efi.c
new file mode 120000
index 0000000..36510b7
--- /dev/null
+++ b/env/env_ini_buffer_efi.c
@@ -0,0 +1 @@
+env_ini_buffer.c
\ No newline at end of file
diff --git a/env/fatvars.c b/env/fatvars.c
index 1365f0d..6704425 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -133,7 +133,24 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
for (i = 0; i < numHandles; i++) {
EFI_FILE_HANDLE fh = NULL;
VOLUME_DESC *v = &volumes[config_volumes[i]];
- if (EFI_ERROR(open_cfg_file(v->root, &fh,
+ if (EFI_SUCCESS == open_ini_file(v->root, &fh, EFI_FILE_MODE_READ))
+ {
+ INFO(L"Found a INI file for the config partition %d\n", i);
+ UINTN buffer_length = 16384;
+ char* buffer = (char*)AllocateZeroPool(buffer_length);
+ if (buffer == NULL)
+ {
+ ERROR(L"Cannot allocate buffer for INI file.\n");
+ continue;
+ }
+ if (EFI_SUCCESS == read_cfg_file(fh, &buffer_length, (void*)buffer))
+ {
+ //env_ini_buffer_read(buffer, &env[i]);
+ }
+ FreePool(buffer);
+ }
+
+ else if (EFI_ERROR(open_cfg_file(v->root, &fh,
EFI_FILE_MODE_READ))) {
WARNING(L"Could not open environment file on config partition %d\n",
i);
diff --git a/include/env_ini_buffer.h b/include/env_ini_buffer.h
new file mode 100644
index 0000000..ddf437e
--- /dev/null
+++ b/include/env_ini_buffer.h
@@ -0,0 +1,24 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (c) Siemens AG, 2022
+ *
+ * Authors:
+ * Tobias Schmidl <tobias...@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#pragma once
+
+#include "env_api.h"
+
+extern bool env_ini_buffer_read(const char *read_buffer, BG_ENVDATA *data);
+
+#if !defined(_GNU_EFI) && !defined(GNU_EFI_USE_MS_ABI)
+extern int env_ini_buffer_write(char *write_buffer, size_t buffer_length,
+ const BG_ENVDATA *data);
+#endif
diff --git a/include/envdata.h b/include/envdata.h
index 9c4ad44..6f68212 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -18,6 +18,8 @@

#define ENV_FILE_NAME L"BGENV.DAT"
#define FAT_ENV_FILENAME "BGENV.DAT"
+#define ENV_INI_FILE_NAME L"BGENV.INI"
+#define FAT_ENV_INI_FILENAME "BGENV.INI"
#define ENV_STRING_LENGTH 255

#define USTATE_OK 0
@@ -45,4 +47,15 @@ struct _BG_ENVDATA {
};
#pragma pack(pop)

+#define BG_ENVDATA_STRING "bg_envdata"
+#define BG_ENV_KERNELFILE_STRING "kernelfile"
+#define BG_ENV_KERNELPARAMS_STRING "kernelparams"
+#define BG_ENV_IN_PROGRESS_STRING "in_progress"
+#define BG_ENV_USTATE_STRING "ustate"
+#define BG_ENV_WATCHDOG_TIMEOUT_SEC_STRING "watchdog_timeout_sec"
+#define BG_ENV_REVISION_STRING "revision"
+#define BG_ENV_CRC32_STRING "crc32"
+
+#define BUFFER_SIZE 16384
+
typedef struct _BG_ENVDATA BG_ENVDATA;
diff --git a/include/syspart.h b/include/syspart.h
index d725e23..e4520c3 100644
--- a/include/syspart.h
+++ b/include/syspart.h
@@ -25,6 +25,11 @@
(root), (file), ENV_FILE_NAME, (mode), \
EFI_FILE_ARCHIVE | EFI_FILE_HIDDEN | EFI_FILE_SYSTEM)

+#define open_ini_file(root, file, mode) \
+ (root)->Open( \
+ (root), (file), ENV_INI_FILE_NAME, (mode), \
+ EFI_FILE_ARCHIVE | EFI_FILE_HIDDEN | EFI_FILE_SYSTEM)
+
#define close_cfg_file(root, file) \
(root)->Close(file)

diff --git a/tools/bg_envtools.c b/tools/bg_envtools.c
index b81ffed..2eeac2d 100644
--- a/tools/bg_envtools.c
+++ b/tools/bg_envtools.c
@@ -15,6 +15,7 @@

#include <sys/stat.h>
#include "env_config_file.h"
+#include "env_ini_buffer.h"
#include "version.h"

#include "bg_envtools.h"
@@ -141,7 +142,28 @@ bool get_env(char *configfilepath, BG_ENVDATA *data)
return false;
}

- if (!(fread(data, sizeof(BG_ENVDATA), 1, config) == 1)) {
+ const char *file_name = strrchr(configfilepath, '/');
+ file_name = (file_name != NULL) ? file_name + 1 : configfilepath;
+
+ if (strcasecmp(file_name, FAT_ENV_INI_FILENAME) == 0)
+ {
+ size_t buffer_size = BUFFER_SIZE;
+ char * buffer = calloc(buffer_size, 1);
+ if (buffer == NULL)
+ {
+ VERBOSE(stderr, "Cannot allocate a buffer for INI file %s\n", configfilepath);
+ return false;
+ }
+ buffer_size = fread(buffer, 1, buffer_size, config);
+ if (ferror(config))
+ {
+ VERBOSE(stderr, "There was an error during reading the config file %s\n", configfilepath);
+ result = false;
+ }
+ result = env_ini_buffer_read(buffer, data);
+ free(buffer);
+ }
+ else if (!(fread(data, sizeof(BG_ENVDATA), 1, config) == 1)) {
VERBOSE(stderr, "Error reading environment data from %s\n",
configfilepath);
if (feof(config)) {
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index d685412..84df978 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -21,6 +21,7 @@
#include "bg_envtools.h"
#include "bg_setenv.h"
#include "bg_printenv.h"
+#include "env_ini_buffer.h"

static char tool_doc[] =
"bg_setenv - Environment tool for the EFI Boot Guard";
@@ -351,26 +352,54 @@ static int dumpenv_to_file(char *envfilepath, bool verbosity, bool preserve_env)
if (verbosity) {
dump_env(env.data, &ALL_FIELDS, false);
}
+
+ /* compare the filename. if we operate on bgenv.dat, use the old way,
+ * otherwise try the INI file approach. */
+ const char *file_name = strrchr(envfilepath, '/');
+ file_name = (file_name != NULL) ? file_name + 1 : envfilepath;
+
FILE *of = fopen(envfilepath, "wb");
- if (of) {
- if (fwrite(&data, sizeof(BG_ENVDATA), 1, of) != 1) {
+ if (of == NULL) {
+ result = errno;
+ fprintf(stderr, "Error opening output file %s (%s).\n",
+ envfilepath, strerror(result));
+ return result;
+ }
+ if (strcasecmp(file_name, FAT_ENV_INI_FILENAME) == 0) {
+ char *write_buffer = alloca(BUFFER_SIZE);
+ if (write_buffer == NULL ||
+ memset(write_buffer, '\0', BUFFER_SIZE) == NULL) {
+ fprintf(stderr, "Cannot allocate write buffer.\n");
+ result = ENOMEM;
+ } else if (0 != (result = env_ini_buffer_write(
+ write_buffer, BUFFER_SIZE, &data))) {
fprintf(stderr,
- "Error writing to output file: %s\n",
- strerror(errno));
- result = 1;
+ "Cannot convert env environment to INI: %s\n",
+ strerror(result));
+ } else if (fwrite(write_buffer, 1,
+ strnlen(write_buffer, BUFFER_SIZE - 1) + 1,
+ of) == 0) {
+ result = errno;
+ fprintf(stderr, "Error writing the INI file: %s\n",
+ strerror(result));
} else {
- fprintf(stdout, "Output written to %s.\n", envfilepath);
+ fprintf(stdout, "INI file written to %s.\n",
+ envfilepath);
}
- if (fclose(of)) {
- fprintf(stderr, "Error closing output file.\n");
- result = 1;
- };
+ } else if (fwrite(&data, sizeof(BG_ENVDATA), 1, of) != 1) {
+ result = errno;
+ fprintf(stderr, "Error writing to output file: %s\n",
+ strerror(result));
} else {
- fprintf(stderr, "Error opening output file %s (%s).\n",
- envfilepath, strerror(errno));
- result = 1;
+ fprintf(stdout, "Output written to %s.\n", envfilepath);
}

+ if (0 != fclose(of)) {
+ result = errno;
+ fprintf(stderr, "Error closing output file: %s\n",
+ strerror(result));
+ };
+
return result;
}

--
2.37.2

[Schmidl, Tobias]

Hi all,

are there any impediments for this patch?

Kind regards,

Tobias

[Jan Kiszka]

On 10.10.22 09:18, Schmidl, Tobias wrote:
> Hi all,
>
> are there any impediments for this patch?
>

I didn't look into details yet as I was interpreting the limitations as
if there is more work needed to make it functional.

Being now self-contained is a step forward. But we may need to
synchronize this with Christian's findings and refactorings. Not sure
where we stand there, though.

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux