[PATCH v3 1/5] Add function to remove last node from device path

39 views
Skip to first unread message

Andreas J. Reichel

unread,
Sep 3, 2018, 10:15:53 AM9/3/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

To compare device paths it is necessary to be able to split off
sub-paths from the right hand side. Implement a simple function
for that purpose.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
include/utils.h | 1 +
utils.c | 25 +++++++++++++++++++++++++
2 files changed, 26 insertions(+)

diff --git a/include/utils.h b/include/utils.h
index b8eddd1..846e8be 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -41,5 +41,6 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count);
EFI_STATUS close_volumes(VOLUME_DESC *volumes, UINTN count);
EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE device,
CHAR16 *payloadpath);
+CHAR16 *dirname(CHAR16 *input);

#endif // __H_UTILS__
diff --git a/utils.c b/utils.c
index 5aff01b..94226fb 100644
--- a/utils.c
+++ b/utils.c
@@ -271,3 +271,28 @@ EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE device,

return appendeddevpath;
}
+
+CHAR16 *dirname(CHAR16 *input)
+{
+ CHAR16 *dst;
+ UINTN len;
+
+ len = StrLen(input);
+
+ dst = mmalloc((len + 1) * sizeof(CHAR16));
+ if (!dst) {
+ return NULL;
+ }
+
+ StrCpy(dst, input);
+
+ for (UINTN i = len; i > 0; i--)
+ {
+ if (dst[i] == L'/') {
+ dst[i] = L'\0';
+ break;
+ }
+ }
+
+ return dst;
+}
--
2.18.0

Andreas J. Reichel

unread,
Sep 3, 2018, 10:15:53 AM9/3/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

During environment enumeration, notify if it is on the boot device

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
utils.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/utils.c b/utils.c
index ad779f4..dfed669 100644
--- a/utils.c
+++ b/utils.c
@@ -181,6 +181,10 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
(*volumes)[rootCount].fscustomlabel);

mfree(devpathstr);
+
+ if (IsOnBootDevice(devpath)) {
+ Print(L"(On the boot device)\n");
+ }
rootCount++;
}
*count = rootCount;
--
2.18.0

Andreas J. Reichel

unread,
Sep 3, 2018, 10:15:53 AM9/3/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

If one environment is found on the boot device, only use environments
from there and ignore all other devices. This way, one can boot from a
memory stick without messing around with already installed efibootguard
configs on the harddrive for example.

Also simplify error handling in config loading and writing.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
env/fatvars.c | 90 +++++++++++++++++++++++++++--------------------
env/syspart.c | 48 +++++++++++++++++++++++--
include/syspart.h | 3 +-
3 files changed, 98 insertions(+), 43 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index d51956c..d1e5ecb 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -21,23 +21,26 @@ static BG_ENVDATA env[ENV_NUM_CONFIG_PARTS];

BG_STATUS save_current_config(void)
{
- BG_STATUS result = BG_SUCCESS;
+ BG_STATUS result = BG_CONFIG_ERROR;
EFI_STATUS efistatus;
UINTN numHandles = volume_count;
- EFI_FILE_HANDLE *roots;
+ UINTN *config_volumes;

- roots = (EFI_FILE_HANDLE *)mmalloc(sizeof(EFI_FILE_HANDLE) *
- volume_count);
- if (!roots) {
+ config_volumes = (UINTN *)mmalloc(sizeof(UINTN) * volume_count);
+ if (!config_volumes) {
Print(L"Error, could not allocate memory for config partition "
- L"handles.\n");
- return BG_CONFIG_ERROR;
+ L"mapping.\n");
+ return result;
}

- if (EFI_ERROR(enumerate_cfg_parts(roots, &numHandles))) {
- Print(L"Error, could not enumerate config partitions.");
- mfree(roots);
- return BG_CONFIG_ERROR;
+ if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {
+ Print(L"Error, could not enumerate config partitions.\n");
+ goto scc_cleanup;
+ }
+
+ if (EFI_ERROR(filter_cfg_parts(&config_volumes, &numHandles))) {
+ Print(L"Error, could not filter config partitions.\n");
+ goto scc_cleanup;
}

if (numHandles != ENV_NUM_CONFIG_PARTS) {
@@ -46,19 +49,18 @@ BG_STATUS save_current_config(void)
numHandles, ENV_NUM_CONFIG_PARTS);
/* In case of saving, this must be treated as error, to not
* overwrite another partition's config file. */
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto scc_cleanup;
}

+ VOLUME_DESC *v = &volumes[config_volumes[current_partition]];
EFI_FILE_HANDLE fh = NULL;
- efistatus = open_cfg_file(roots[current_partition], &fh,
- EFI_FILE_MODE_WRITE | EFI_FILE_MODE_READ);
+ efistatus = open_cfg_file(v->root, &fh, EFI_FILE_MODE_WRITE |
+ EFI_FILE_MODE_READ);
if (EFI_ERROR(efistatus)) {
Print(L"Error, could not open environment file on system "
L"partition %d: %r\n",
current_partition, efistatus);
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto scc_cleanup;
}

UINTN writelen = sizeof(BG_ENVDATA);
@@ -70,43 +72,49 @@ BG_STATUS save_current_config(void)
(VOID *)&env[current_partition]);
if (EFI_ERROR(efistatus)) {
Print(L"Error writing environment to file: %r\n", efistatus);
- result = BG_CONFIG_ERROR;
+ (VOID) close_cfg_file(v->root, fh);
+ goto scc_cleanup;
}

- if (EFI_ERROR(close_cfg_file(roots[current_partition], fh))) {
+ if (EFI_ERROR(close_cfg_file(v->root, fh))) {
Print(L"Error, could not close environment config file.\n");
- result = BG_CONFIG_ERROR;
+ goto scc_cleanup;
}
- mfree(roots);
+
+ result = BG_SUCCESS;
+scc_cleanup:
+ mfree(config_volumes);
return result;
}

BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
{
- BG_STATUS result = BG_SUCCESS;
+ BG_STATUS result = BG_CONFIG_ERROR;
UINTN numHandles = volume_count;
- EFI_FILE_HANDLE *roots;
+ UINTN *config_volumes;
UINTN i;
int env_invalid[ENV_NUM_CONFIG_PARTS] = {0};

- roots = (EFI_FILE_HANDLE *)mmalloc(sizeof(EFI_FILE_HANDLE) *
- volume_count);
- if (!roots) {
+ config_volumes = (UINTN *)mmalloc(sizeof(UINTN) * volume_count);
+ if (!config_volumes) {
Print(L"Error, could not allocate memory for config partition "
- L"handles.\n");
- return BG_CONFIG_ERROR;
+ L"mapping.\n");
+ return result;
}

- if (EFI_ERROR(enumerate_cfg_parts(roots, &numHandles))) {
- Print(L"Error, could not enumerate config partitions.");
- mfree(roots);
- return BG_CONFIG_ERROR;
+ if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {
+ Print(L"Error, could not enumerate config partitions.\n");
+ goto lc_cleanup;
+ }
+
+ if (EFI_ERROR(filter_cfg_parts(&config_volumes, &numHandles))) {
+ Print(L"Error, could not filter config partitions.\n");
+ goto lc_cleanup;
}

if (numHandles > ENV_NUM_CONFIG_PARTS) {
Print(L"Error, too many config partitions found. Aborting.\n");
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto lc_cleanup;
}

if (numHandles < ENV_NUM_CONFIG_PARTS) {
@@ -121,8 +129,9 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
/* Load all config data */
for (i = 0; i < numHandles; i++) {
EFI_FILE_HANDLE fh = NULL;
- if (EFI_ERROR(open_cfg_file(roots[i], &fh,
- EFI_FILE_MODE_READ))) {
+ VOLUME_DESC *v = &volumes[config_volumes[i]];
+ if (EFI_ERROR(open_cfg_file(v->root, &fh,
+ EFI_FILE_MODE_READ))) {
Print(L"Warning, could not open environment file on "
L"config partition %d\n",
i);
@@ -135,7 +144,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
L"partition %d.\n",
i);
env_invalid[i] = 1;
- if (EFI_ERROR(close_cfg_file(roots[i], fh))) {
+ if (EFI_ERROR(close_cfg_file(v->root, fh))) {
Print(L"Error, could not close environment "
L"config file.\n");
}
@@ -158,7 +167,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
result = BG_CONFIG_PARTIALLY_CORRUPTED;
}

- if (EFI_ERROR(uefi_call_wrapper(fh->Close, 1, fh))) {
+ if (EFI_ERROR(close_cfg_file(v->root, fh))) {
Print(L"Error, could not close environment config "
L"file.\n");
/* Don't abort, so we may still be able to boot a
@@ -223,7 +232,10 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
Print(L" kernel: %s\n", bglp->payload_path);
Print(L" args: %s\n", bglp->payload_options);
Print(L" timeout: %d seconds\n", bglp->timeout);
- mfree(roots);
+
+ result = BG_SUCCESS;
+lc_cleanup:
+ mfree(config_volumes);
return result;
}

diff --git a/env/syspart.c b/env/syspart.c
index 29124dd..8a792df 100644
--- a/env/syspart.c
+++ b/env/syspart.c
@@ -14,15 +14,16 @@

#include <syspart.h>
#include <utils.h>
+#include <envdata.h>

#define MAX_INFO_SIZE 1024

-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
+EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *numHandles)
{
EFI_STATUS status;
UINTN rootCount = 0;

- if (!roots || !numHandles) {
+ if (!config_volumes || !numHandles) {
Print(L"Invalid parameter in system partition enumeration.\n");
return EFI_INVALID_PARAMETER;
}
@@ -36,7 +37,7 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
EFI_FILE_MODE_READ);
if (status == EFI_SUCCESS) {
Print(L"Config file found on volume %d.\n", index);
- roots[rootCount] = volumes[index].root;
+ config_volumes[rootCount] = index;
rootCount++;
status = close_cfg_file(volumes[index].root, fh);
if (EFI_ERROR(status)) {
@@ -50,3 +51,44 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
Print(L"%d config partitions detected.\n", rootCount);
return EFI_SUCCESS;
}
+
+EFI_STATUS filter_cfg_parts(UINTN **config_volumes, UINTN *numHandles)
+{
+ BOOLEAN use_envs_on_bootdevice_only = FALSE;
+ UINTN *filtered_volumes;
+
+ Print(L"Config filter: \n");
+ for (UINTN index = 0; index < *numHandles; index++) {
+ VOLUME_DESC *v = &volumes[(*config_volumes)[index]];
+
+ if (IsOnBootDevice(v->devpath)) {
+ use_envs_on_bootdevice_only = TRUE;
+ };
+ }
+
+ if (!use_envs_on_bootdevice_only) {
+ // nothing to do
+ return EFI_SUCCESS;
+ }
+
+ filtered_volumes = mmalloc(sizeof(UINTN) * *numHandles);
+
+ Print(L"Booting with environments from boot device only.\n");
+ UINTN index = 0;
+ for (UINTN j = 0; j < *numHandles; j++) {
+ UINTN cvi = (*config_volumes)[j];
+ VOLUME_DESC *v = &volumes[cvi];
+
+ if (IsOnBootDevice(v->devpath)) {
+ filtered_volumes[index++] = (*config_volumes)[j];
+ } else {
+ Print(L"Filtered out config on volume #%d\n", cvi);
+ }
+ }
+ *numHandles = index;
+
+ mfree(*config_volumes);
+ *config_volumes = filtered_volumes;
+ Print(L"Remaining Config Partitions: %d\n", *numHandles);
+ return EFI_SUCCESS;
+}
diff --git a/include/syspart.h b/include/syspart.h
index 502d1f1..308668c 100644
--- a/include/syspart.h
+++ b/include/syspart.h
@@ -32,6 +32,7 @@
#define read_cfg_file(file, len, buffer) \
uefi_call_wrapper((file)->Read, 3, (file), (len), (buffer))

-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *maxHandles);
+EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *maxHandles);
+EFI_STATUS filter_cfg_parts(UINTN **config_volumes, UINTN *maxHandles);

#endif // __H_SYSPART__
--
2.18.0

Andreas J. Reichel

unread,
Sep 3, 2018, 10:15:53 AM9/3/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

diff to v2:
* Renamed failsafe-mode to recovery-mode
* Filter operates in O(n) instead of O(n^2)
* Fix some wrong memory allocation
* Get rid of complicated failsafe-mode-flag and simplify things

This patch series implements the new recovery mode to provide a way
to override on-device environments via other boot sources and hence
enable the user to repair or install systems with efibootguard on
e.g. a memory stick.

If an environment is detected on the boot device, only environments
from this device will be used.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>

Andreas Reichel (5):
Add function to remove last node from device path
Add function to get boot device and compare it to others
Print information if environment is on boot dev
Implement recovery mode as config filter
Update documentation

bootguard.c | 1 +
docs/RECOVERY.md | 9 +++++
docs/UPDATE.md | 5 ++-
env/fatvars.c | 90 +++++++++++++++++++++++++++--------------------
env/syspart.c | 48 +++++++++++++++++++++++--
include/syspart.h | 3 +-
include/utils.h | 2 ++
main.c | 8 +++++
utils.c | 47 +++++++++++++++++++++++++
9 files changed, 167 insertions(+), 46 deletions(-)
create mode 100644 docs/RECOVERY.md

--
2.18.0

Andreas J. Reichel

unread,
Sep 3, 2018, 10:15:53 AM9/3/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

* Update UPDATE.md
* Add new RECOVERY.md to explain recovery mode

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
docs/RECOVERY.md | 9 +++++++++
docs/UPDATE.md | 5 ++---
2 files changed, 11 insertions(+), 3 deletions(-)
create mode 100644 docs/RECOVERY.md

diff --git a/docs/RECOVERY.md b/docs/RECOVERY.md
new file mode 100644
index 0000000..98e118f
--- /dev/null
+++ b/docs/RECOVERY.md
@@ -0,0 +1,9 @@
+# Recovery Mode #
+
+If more than the expected number of environments is detected during boot, the
+system stops booting. This can be a problem if the user wants to boot the
+system with a memory stick to update a broken installation.
+
+In order to allow external boot devices with other environment configurations,
+the Recovery Mode was introduced. If any environment is found on the boot
+device, the boot loader will only use environments from this device.
diff --git a/docs/UPDATE.md b/docs/UPDATE.md
index 19c2f39..419a006 100644
--- a/docs/UPDATE.md
+++ b/docs/UPDATE.md
@@ -11,7 +11,7 @@ The structure of the environment data is as follows:
struct _BG_ENVDATA {
uint16_t kernelfile[ENV_STRING_LENGTH];
uint16_t kernelparams[ENV_STRING_LENGTH];
- uint8_t padding;
+ uint8_t in_progress;
uint8_t ustate;
uint16_t watchdog_timeout_sec;
uint32_t revision;
@@ -23,8 +23,7 @@ struct _BG_ENVDATA {
The fields have the following meaning:
* `kernelfile`: Path to the kernel image, utf-16 encoded
* `kernelparams`: Arguments to the kernel, utf-16 encoded
-* `padding`: Padding byte to stay compatible with the offsets of the previous
- version.
+* `in_progress`: This stores if an update is in progress.
* `ustate`: Update status (`0` OK, `1` INSTALLED, `2` TESTING, `3`: FAILED)
* `watchdog_timeout_sec`: Number of seconds, the watchdog times out after
* `revision`: The revision number explained above
--
2.18.0

Andreas J. Reichel

unread,
Sep 3, 2018, 10:15:53 AM9/3/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

Get the boot device out of the loaded image's handle and add a
function to compare a device path with the boot device's one.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
bootguard.c | 1 +
include/utils.h | 1 +
main.c | 8 ++++++++
utils.c | 18 ++++++++++++++++++
4 files changed, 28 insertions(+)

diff --git a/bootguard.c b/bootguard.c
index 0aa95b4..9eea350 100644
--- a/bootguard.c
+++ b/bootguard.c
@@ -18,3 +18,4 @@ EFI_HANDLE this_image;

VOLUME_DESC *volumes = NULL;
UINTN volume_count = 128;
+CHAR16 *boot_device_path;
diff --git a/include/utils.h b/include/utils.h
index 846e8be..3d1a318 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -42,5 +42,6 @@ EFI_STATUS close_volumes(VOLUME_DESC *volumes, UINTN count);
EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE device,
CHAR16 *payloadpath);
CHAR16 *dirname(CHAR16 *input);
+BOOLEAN IsOnBootDevice(EFI_DEVICE_PATH *dp);

#endif // __H_UTILS__
diff --git a/main.c b/main.c
index 2f8e151..986bb57 100644
--- a/main.c
+++ b/main.c
@@ -24,6 +24,7 @@

extern const unsigned long init_array_start[];
extern const unsigned long init_array_end[];
+extern CHAR16 *boot_device_path;

static EFI_STATUS probe_watchdog(EFI_LOADED_IMAGE *loaded_image,
EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
@@ -105,6 +106,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
EFI_STATUS status;
BG_STATUS bg_status;
BG_LOADER_PARAMS bg_loader_params;
+ CHAR16 *tmp;

ZeroMem(&bg_loader_params, sizeof(bg_loader_params));

@@ -123,6 +125,11 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
status);
}

+ tmp = DevicePathToStr(DevicePathFromHandle(loaded_image->DeviceHandle));
+ boot_device_path = dirname(tmp);
+ mfree(tmp);
+ Print(L"Boot device: %s\n", boot_device_path);
+
status = get_volumes(&volumes, &volume_count);
if (EFI_ERROR(status)) {
error_exit(L"Could not get volumes installed on system.\n",
@@ -176,6 +183,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
}

mfree(payload_dev_path);
+ mfree(boot_device_path);

status =
uefi_call_wrapper(BS->OpenProtocol, 6, payload_handle,
diff --git a/utils.c b/utils.c
index 94226fb..ad779f4 100644
--- a/utils.c
+++ b/utils.c
@@ -16,6 +16,24 @@
#include <bootguard.h>
#include <utils.h>

+BOOLEAN IsOnBootDevice(EFI_DEVICE_PATH *dp)
+{
+ extern CHAR16 *boot_device_path;
+ CHAR16 *device_path, *tmp;
+ BOOLEAN result = FALSE;
+
+ tmp = DevicePathToStr(dp);
+ device_path = dirname(tmp);
+ mfree(tmp);
+
+ if (StrCmp(device_path, boot_device_path) == 0) {
+ result = TRUE;
+ }
+ mfree(device_path);
+
+ return result;
+}
+
uint32_t calc_crc32(void *data, int32_t size)
{
uint32_t crc;
--
2.18.0

Jan Kiszka

unread,
Sep 4, 2018, 11:44:03 AM9/4/18
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
There are two corner cases that the POSIX dirname addresses specially
while this one does not:

- dirname("/dir/") = "/" => you return "/dir"
- dirname("") and dirname("file") = "." => you return "" and "file",
respectively

And there is another, more major difference: dirname() works with static
result buffers or modifies the input.

Given these deviations, we should either adjust to POSIX or at least
call that thing slightly differently (GetDirname?).

Jan

Jan Kiszka

unread,
Sep 4, 2018, 11:44:09 AM9/4/18
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
On 2018-09-03 16:09, [ext] Andreas J. Reichel wrote:
For my understanding: What is actually cut off there, ie. who does that
path look like so that we only to cut off the last element, and there is
not filesystem directory element remaining in it?
Looks good, /me just needs to understand the path structure, and maybe
that should then also be documented in the code.

Jan

Jan Kiszka

unread,
Sep 4, 2018, 11:49:23 AM9/4/18
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
On 2018-09-03 16:09, [ext] Andreas J. Reichel wrote:
Shouldn't that be appended to the "Volume X:..." line, rather than
leaving it alone on a new one?

Jan

Jan Kiszka

unread,
Sep 4, 2018, 12:06:45 PM9/4/18
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
On 2018-09-03 16:09, [ext] Andreas J. Reichel wrote:
Missing NULL check. That will actually give the function an error path.

Alternatively, you could sort all volumes you want to keep to the
beginning of the existing array and only adjust *numHandles. Then you
could actually return that value because there will be no error code
anymore, and that would simplify also the caller.

> +
> + Print(L"Booting with environments from boot device only.\n");
> + UINTN index = 0;
> + for (UINTN j = 0; j < *numHandles; j++) {
> + UINTN cvi = (*config_volumes)[j];
> + VOLUME_DESC *v = &volumes[cvi];
> +
> + if (IsOnBootDevice(v->devpath)) {
> + filtered_volumes[index++] = (*config_volumes)[j];
> + } else {
> + Print(L"Filtered out config on volume #%d\n", cvi);

Maybe "Ignoring config on..."?

> + }
> + }
> + *numHandles = index;
> +
> + mfree(*config_volumes);
> + *config_volumes = filtered_volumes;
> + Print(L"Remaining Config Partitions: %d\n", *numHandles);

Do we need that verbosity level? I don't see the value for the user yet.

> + return EFI_SUCCESS;
> +}
> diff --git a/include/syspart.h b/include/syspart.h
> index 502d1f1..308668c 100644
> --- a/include/syspart.h
> +++ b/include/syspart.h
> @@ -32,6 +32,7 @@
> #define read_cfg_file(file, len, buffer) \
> uefi_call_wrapper((file)->Read, 3, (file), (len), (buffer))
>
> -EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *maxHandles);
> +EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *maxHandles);
> +EFI_STATUS filter_cfg_parts(UINTN **config_volumes, UINTN *maxHandles);
>
> #endif // __H_SYSPART__
>

Jan

Jan Kiszka

unread,
Sep 4, 2018, 12:16:46 PM9/4/18
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
On 2018-09-03 16:09, [ext] Andreas J. Reichel wrote:
What is the exact semantic? "Non-zero if update is in progress"?

> * `ustate`: Update status (`0` OK, `1` INSTALLED, `2` TESTING, `3`: FAILED)
> * `watchdog_timeout_sec`: Number of seconds, the watchdog times out after
> * `revision`: The revision number explained above
>

Jan

Andreas Reichel

unread,
Sep 5, 2018, 7:10:39 AM9/5/18
to Jan Kiszka, efibootg...@googlegroups.com
Okay look, before, it was called "RemoveLastNodeFromDevicePath" or
something like that, which was exactly what it did.
Then you wanted to call that dirname... because the function did similar
things like dirname.
Now you tell me that this function behaves differently than dirname in
POSIX?

This is NOT dirname and I called it so because you wanted it.
Please tell me, what exactly do you want because somehow I don't
understand you.

Thank you.
Andreas

>
> Jan

--
Andreas Reichel
Dipl.-Phys. (Univ.)
Software Consultant

Andreas...@tngtech.com, +49-174-3180074
TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterfoehring
Geschaeftsfuehrer: Henrik Klagges, Dr. Robert Dahlke, Gerhard Mueller
Sitz: Unterfoehring * Amtsgericht Muenchen * HRB 135082

Jan Kiszka

unread,
Sep 5, 2018, 7:15:52 AM9/5/18
to Andreas Reichel, efibootg...@googlegroups.com
How would that function deal with the corner cases?

> something like that, which was exactly what it did.
> Then you wanted to call that dirname... because the function did similar
> things like dirname.
> Now you tell me that this function behaves differently than dirname in
> POSIX?
>
> This is NOT dirname and I called it so because you wanted it.
> Please tell me, what exactly do you want because somehow I don't
> understand you.

If you want "ExtractDevicePath" rather than something generic, call it
like that. But generally it is better to follow existing, understood
standards.

Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Andreas Reichel

unread,
Sep 5, 2018, 7:18:21 AM9/5/18
to Jan Kiszka, efibootg...@googlegroups.com
On Tue, Sep 04, 2018 at 05:44:07PM +0200, Jan Kiszka wrote:
> On 2018-09-03 16:09, [ext] Andreas J. Reichel wrote:
> > From: Andreas Reichel <andreas.r...@siemens.com>
> >
> > Get the boot device out of the loaded image's handle and add a
> > function to compare a device path with the boot device's one.
> >
> > status);
> > }
> > + tmp = DevicePathToStr(DevicePathFromHandle(loaded_image->DeviceHandle));
>
> For my understanding: What is actually cut off there, ie. who does that path
> look like so that we only to cut off the last element, and there is not
> filesystem directory element remaining in it?
>
The DevicePath is pointing to the device, not to the file. That was my
argumentation all along regarding dirname/RemoveLastNodeFromDevicePath.
Since it is not just a directory.

Please read below.

> > +BOOLEAN IsOnBootDevice(EFI_DEVICE_PATH *dp)
> > +{
> > + extern CHAR16 *boot_device_path;
> > + CHAR16 *device_path, *tmp;
> > + BOOLEAN result = FALSE;
> > +
> > + tmp = DevicePathToStr(dp);
> > + device_path = dirname(tmp);
> > + mfree(tmp);
> > +
> > + if (StrCmp(device_path, boot_device_path) == 0) {
> > + result = TRUE;
> > + }
> > + mfree(device_path);
> > +
> > + return result;
> > +}
> > +
> > uint32_t calc_crc32(void *data, int32_t size)
> > {
> > uint32_t crc;
> >
>
> Looks good, /me just needs to understand the path structure, and maybe that
> should then also be documented in the code.
>
Agreed. We have something like

controller/bus/device/partition

in all cases.

Like

PciRoot(0)/Pci(0x1,0x1)/Ata(Primary,Master)/HD(Part2,Sig870E7D1B-3D9F-4E49-98BA-7283AB6AA632)

which gets cut to

PciRoot(0)/Pci(0x1,0x1)/Ata(Primary,Master)

I will add that to the documentation.

Andreas Reichel

unread,
Sep 5, 2018, 7:21:04 AM9/5/18
to Jan Kiszka, efibootg...@googlegroups.com
On Tue, Sep 04, 2018 at 06:06:44PM +0200, Jan Kiszka wrote:
> On 2018-09-03 16:09, [ext] Andreas J. Reichel wrote:
> > From: Andreas Reichel <andreas.r...@siemens.com>
> >
> > +
> > + filtered_volumes = mmalloc(sizeof(UINTN) * *numHandles);
>
> Missing NULL check. That will actually give the function an error path.
>
> Alternatively, you could sort all volumes you want to keep to the beginning
> of the existing array and only adjust *numHandles. Then you could actually
> return that value because there will be no error code anymore, and that
> would simplify also the caller.
>
Thanks. Good idea.
> > +
> > + Print(L"Booting with environments from boot device only.\n");
> > + UINTN index = 0;
> > + for (UINTN j = 0; j < *numHandles; j++) {
> > + UINTN cvi = (*config_volumes)[j];
> > + VOLUME_DESC *v = &volumes[cvi];
> > +
> > + if (IsOnBootDevice(v->devpath)) {
> > + filtered_volumes[index++] = (*config_volumes)[j];
> > + } else {
> > + Print(L"Filtered out config on volume #%d\n", cvi);
>
> Maybe "Ignoring config on..."?
>
> > + }
> > + }
> > + *numHandles = index;
> > +
> > + mfree(*config_volumes);
> > + *config_volumes = filtered_volumes;
> > + Print(L"Remaining Config Partitions: %d\n", *numHandles);
>
> Do we need that verbosity level? I don't see the value for the user yet.
>
I will better make that an optional debug output.

Andreas

Andreas Reichel

unread,
Sep 5, 2018, 7:21:40 AM9/5/18
to Jan Kiszka, efibootg...@googlegroups.com
Ok no problem.

Andreas

> Jan
>
> > + }
> > rootCount++;
> > }
> > *count = rootCount;
> >
>

Andreas Reichel

unread,
Sep 5, 2018, 7:50:24 AM9/5/18
to Jan Kiszka, efibootg...@googlegroups.com
There ARE no such corner cases... there is no leading slash and there
are no empty strings and there is no file... so we are talking about implementing
for theoretical scenarious which simply are not there with UEFI device.

Or am I wrong and you saw an implementation where this happens?

> > something like that, which was exactly what it did.
> > Then you wanted to call that dirname... because the function did similar
> > things like dirname.
> > Now you tell me that this function behaves differently than dirname in
> > POSIX?
> >
> > This is NOT dirname and I called it so because you wanted it.
> > Please tell me, what exactly do you want because somehow I don't
> > understand you.
>
> If you want "ExtractDevicePath" rather than something generic, call it like
> that. But generally it is better to follow existing, understood standards.
>
By that you imply "follow POSIX standard", but we have no POSIX within
UEFI. So why should we follow Posix standard. You cannot follow a
standard where it does not exist.

I already told you, the name was good as it was "RemoveLastNodeFrom..."
because it is not extract nor anything else... it already was a
devicepath. Because a partition is also a 'device' in UEFI obviously.
Maybe that is a point for confusion?

But I really don't like to argue about this function name anymore...

Please chose one of the following:
a) GetPathToDeviceFromDevicePath
b) GetPathToDevice
c) ExtractPathToDevice
d) ExtractDevicePathToDevice
e) RemoveLastNode
f) RemoveLastNodeFromDevicePath
g) TrimToSlashFromRight

But anyhow this will never be and need not be something generic. And by
no means POSIX here.
If you name me a breaking condition that happens because you have it,
I will fix it.

Andreas

> Jan
>
> --
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> Corporate Competence Center Embedded Linux

Jan Kiszka

unread,
Sep 5, 2018, 8:18:35 AM9/5/18
to Andreas Reichel, efibootg...@googlegroups.com
OK, so you are removing the disk/partition element from the boot device
path. Is it UEFI standard that this path will always have the above
structure?

It's probably the question what "device" is in this UEFI context, but
you are passing in a device according to the type (EFI_DEVICE_PATH), and
the extracted path is maybe rather the "boot medium".

As I said on patch 1: Make the extraction helper context specific in its
naming. Then you can do all the shortcuts you like, and we can settle
that topic.

Andreas Reichel

unread,
Sep 5, 2018, 9:28:21 AM9/5/18
to Jan Kiszka, efibootg...@googlegroups.com
Regarding the "standard" of this structure:

Node structure:
see
https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/DevicePath.h
about line 1235

there are a lot of different path conventions depending on the
boot device. If we were to boot from Fiber Channel or
from IPV6_DEVICE_PATH, then this will most probably also work
in some manner because we always go to the closest parent.

String representation:
All have in common the same string representation system as
defined in

i.e. UEFI Spec 2.6, paragraph 9.3.5

If it works or not in a useful sense depends on the partitioning and the
medium structure. For example, if we had a block device
inside a block device, and the file system was raw on the inner block
device then the outer block device would be the parent
node we compare against (hypothetic scenario).

If we had two partitions within each other, then the outer partition
would be the parent node we compare against.

But it's a good idea to call it boot medium instead of boot device to get
rid of the confusion. Wether it is a real medium or not depends on the
protocol AND the system setup we boot with.

So I will name the function GetBootMediumPath. Then the context
is clear.

Do you agree?

Andreas
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> Corporate Competence Center Embedded Linux

Andreas J. Reichel

unread,
Sep 5, 2018, 12:26:44 PM9/5/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

During environment enumeration, notify if it is on the boot device

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
utils.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/utils.c b/utils.c
index 8104349..cb65b88 100644
--- a/utils.c
+++ b/utils.c
@@ -176,11 +176,16 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
get_volume_label((*volumes)[rootCount].root);
(*volumes)[rootCount].fscustomlabel =
get_volume_custom_label((*volumes)[rootCount].root);
- Print(L"Volume %d: %s, LABEL=%s, CLABEL=%s\n", rootCount,
+ Print(L"Volume %d: ", rootCount);
+ if (IsOnBootDevice(devpath)) {
+ Print(L"(On boot medium) ");
+ }
+ Print(L"%s, LABEL=%s, CLABEL=%s\n",
devpathstr, (*volumes)[rootCount].fslabel,
(*volumes)[rootCount].fscustomlabel);

mfree(devpathstr);
+
rootCount++;
}
*count = rootCount;
--
2.18.0

Andreas J. Reichel

unread,
Sep 5, 2018, 12:26:44 PM9/5/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

If one environment is found on the boot device, only use environments
from there and ignore all other devices. This way, one can boot from a
memory stick without messing around with already installed efibootguard
configs on the harddrive for example.

Also simplify error handling in config loading and writing.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
env/fatvars.c | 84 +++++++++++++++++++++++++----------------------
env/syspart.c | 50 ++++++++++++++++++++++++++--
include/syspart.h | 3 +-
3 files changed, 94 insertions(+), 43 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index d51956c..99ac37e 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -21,44 +21,43 @@ static BG_ENVDATA env[ENV_NUM_CONFIG_PARTS];

BG_STATUS save_current_config(void)
{
- BG_STATUS result = BG_SUCCESS;
+ BG_STATUS result = BG_CONFIG_ERROR;
EFI_STATUS efistatus;
UINTN numHandles = volume_count;
- EFI_FILE_HANDLE *roots;
+ UINTN *config_volumes;

- roots = (EFI_FILE_HANDLE *)mmalloc(sizeof(EFI_FILE_HANDLE) *
- volume_count);
- if (!roots) {
+ config_volumes = (UINTN *)mmalloc(sizeof(UINTN) * volume_count);
+ if (!config_volumes) {
Print(L"Error, could not allocate memory for config partition "
- L"handles.\n");
- return BG_CONFIG_ERROR;
+ L"mapping.\n");
+ return result;
}

- if (EFI_ERROR(enumerate_cfg_parts(roots, &numHandles))) {
- Print(L"Error, could not enumerate config partitions.");
- mfree(roots);
- return BG_CONFIG_ERROR;
+ if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {
+ Print(L"Error, could not enumerate config partitions.\n");
+ goto scc_cleanup;
}

+ numHandles = filter_cfg_parts(config_volumes, numHandles);
+
if (numHandles != ENV_NUM_CONFIG_PARTS) {
Print(L"Error, unexpected number of config partitions: found "
L"%d, but expected %d.\n",
numHandles, ENV_NUM_CONFIG_PARTS);
/* In case of saving, this must be treated as error, to not
* overwrite another partition's config file. */
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto scc_cleanup;
}

+ VOLUME_DESC *v = &volumes[config_volumes[current_partition]];
EFI_FILE_HANDLE fh = NULL;
- efistatus = open_cfg_file(roots[current_partition], &fh,
- EFI_FILE_MODE_WRITE | EFI_FILE_MODE_READ);
+ efistatus = open_cfg_file(v->root, &fh, EFI_FILE_MODE_WRITE |
+ EFI_FILE_MODE_READ);
if (EFI_ERROR(efistatus)) {
Print(L"Error, could not open environment file on system "
L"partition %d: %r\n",
current_partition, efistatus);
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto scc_cleanup;
}

UINTN writelen = sizeof(BG_ENVDATA);
@@ -70,43 +69,46 @@ BG_STATUS save_current_config(void)
+ numHandles = filter_cfg_parts(config_volumes, numHandles);
+
if (numHandles > ENV_NUM_CONFIG_PARTS) {
Print(L"Error, too many config partitions found. Aborting.\n");
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto lc_cleanup;
}

if (numHandles < ENV_NUM_CONFIG_PARTS) {
@@ -121,8 +123,9 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
/* Load all config data */
for (i = 0; i < numHandles; i++) {
EFI_FILE_HANDLE fh = NULL;
- if (EFI_ERROR(open_cfg_file(roots[i], &fh,
- EFI_FILE_MODE_READ))) {
+ VOLUME_DESC *v = &volumes[config_volumes[i]];
+ if (EFI_ERROR(open_cfg_file(v->root, &fh,
+ EFI_FILE_MODE_READ))) {
Print(L"Warning, could not open environment file on "
L"config partition %d\n",
i);
@@ -135,7 +138,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
L"partition %d.\n",
i);
env_invalid[i] = 1;
- if (EFI_ERROR(close_cfg_file(roots[i], fh))) {
+ if (EFI_ERROR(close_cfg_file(v->root, fh))) {
Print(L"Error, could not close environment "
L"config file.\n");
}
@@ -158,7 +161,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
result = BG_CONFIG_PARTIALLY_CORRUPTED;
}

- if (EFI_ERROR(uefi_call_wrapper(fh->Close, 1, fh))) {
+ if (EFI_ERROR(close_cfg_file(v->root, fh))) {
Print(L"Error, could not close environment config "
L"file.\n");
/* Don't abort, so we may still be able to boot a
@@ -223,7 +226,10 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
Print(L" kernel: %s\n", bglp->payload_path);
Print(L" args: %s\n", bglp->payload_options);
Print(L" timeout: %d seconds\n", bglp->timeout);
- mfree(roots);
+
+ result = BG_SUCCESS;
+lc_cleanup:
+ mfree(config_volumes);
return result;
}

diff --git a/env/syspart.c b/env/syspart.c
index 29124dd..0cfeb79 100644
--- a/env/syspart.c
+++ b/env/syspart.c
@@ -14,15 +14,16 @@

#include <syspart.h>
#include <utils.h>
+#include <envdata.h>

#define MAX_INFO_SIZE 1024

-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
+EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *numHandles)
{
EFI_STATUS status;
UINTN rootCount = 0;

- if (!roots || !numHandles) {
+ if (!config_volumes || !numHandles) {
Print(L"Invalid parameter in system partition enumeration.\n");
return EFI_INVALID_PARAMETER;
}
@@ -36,7 +37,7 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
EFI_FILE_MODE_READ);
if (status == EFI_SUCCESS) {
Print(L"Config file found on volume %d.\n", index);
- roots[rootCount] = volumes[index].root;
+ config_volumes[rootCount] = index;
rootCount++;
status = close_cfg_file(volumes[index].root, fh);
if (EFI_ERROR(status)) {
@@ -50,3 +51,46 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
Print(L"%d config partitions detected.\n", rootCount);
return EFI_SUCCESS;
}
+
+static void swap_uintn(UINTN *a, UINTN *b)
+{
+ UINTN tmp;
+ tmp = *a;
+ *a = *b;
+ *b = tmp;
+}
+
+UINTN filter_cfg_parts(UINTN *config_volumes, UINTN numHandles)
+{
+ BOOLEAN use_envs_on_bootdevice_only = FALSE;
+
+ Print(L"Config filter: \n");
+ for (UINTN index = 0; index < numHandles; index++) {
+ VOLUME_DESC *v = &volumes[config_volumes[index]];
+
+ if (IsOnBootDevice(v->devpath)) {
+ use_envs_on_bootdevice_only = TRUE;
+ };
+ }
+
+ if (!use_envs_on_bootdevice_only) {
+ // nothing to do
+ return numHandles;
+ }
+
+ Print(L"Booting with environments from boot device only.\n");
+ UINTN num_sorted = 0;
+ for (UINTN j = 0; j < numHandles; j++) {
+ UINTN cvi = config_volumes[j];
+ VOLUME_DESC *v = &volumes[cvi];
+
+ if (IsOnBootDevice(v->devpath)) {
+ swap_uintn(&config_volumes[j],
+ &config_volumes[num_sorted++]);
+ } else {
+ Print(L"Ignoring config on volume #%d\n", cvi);
+ }
+ }
+
+ return num_sorted;
+}
diff --git a/include/syspart.h b/include/syspart.h
index 502d1f1..b22577b 100644
--- a/include/syspart.h
+++ b/include/syspart.h
@@ -32,6 +32,7 @@
#define read_cfg_file(file, len, buffer) \
uefi_call_wrapper((file)->Read, 3, (file), (len), (buffer))

-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *maxHandles);
+EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *maxHandles);
+UINTN filter_cfg_parts(UINTN *config_volumes, UINTN maxHandles);

Andreas J. Reichel

unread,
Sep 5, 2018, 12:26:44 PM9/5/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

Get the boot device out of the loaded image's handle and add a
function to compare a device path with the boot device's one.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
bootguard.c | 1 +
include/utils.h | 1 +
main.c | 8 ++++++++
utils.c | 18 ++++++++++++++++++
4 files changed, 28 insertions(+)

diff --git a/bootguard.c b/bootguard.c
index 0aa95b4..9eea350 100644
--- a/bootguard.c
+++ b/bootguard.c
@@ -18,3 +18,4 @@ EFI_HANDLE this_image;

VOLUME_DESC *volumes = NULL;
UINTN volume_count = 128;
+CHAR16 *boot_device_path;
diff --git a/include/utils.h b/include/utils.h
index e7c76fc..068f89c 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -42,5 +42,6 @@ EFI_STATUS close_volumes(VOLUME_DESC *volumes, UINTN count);
EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE device,
CHAR16 *payloadpath);
CHAR16 *GetBootMediumPath(CHAR16 *input);
+BOOLEAN IsOnBootDevice(EFI_DEVICE_PATH *dp);

#endif // __H_UTILS__
diff --git a/main.c b/main.c
index 2f8e151..d141720 100644
--- a/main.c
+++ b/main.c
@@ -24,6 +24,7 @@

extern const unsigned long init_array_start[];
extern const unsigned long init_array_end[];
+extern CHAR16 *boot_device_path;

static EFI_STATUS probe_watchdog(EFI_LOADED_IMAGE *loaded_image,
EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
@@ -105,6 +106,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
EFI_STATUS status;
BG_STATUS bg_status;
BG_LOADER_PARAMS bg_loader_params;
+ CHAR16 *tmp;

ZeroMem(&bg_loader_params, sizeof(bg_loader_params));

@@ -123,6 +125,11 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
status);
}

+ tmp = DevicePathToStr(DevicePathFromHandle(loaded_image->DeviceHandle));
+ boot_device_path = GetBootMediumPath(tmp);
+ mfree(tmp);
+ Print(L"Boot device: %s\n", boot_device_path);
+
status = get_volumes(&volumes, &volume_count);
if (EFI_ERROR(status)) {
error_exit(L"Could not get volumes installed on system.\n",
@@ -176,6 +183,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
}

mfree(payload_dev_path);
+ mfree(boot_device_path);

status =
uefi_call_wrapper(BS->OpenProtocol, 6, payload_handle,
diff --git a/utils.c b/utils.c
index c544eeb..8104349 100644
--- a/utils.c
+++ b/utils.c
@@ -16,6 +16,24 @@
#include <bootguard.h>
#include <utils.h>

+BOOLEAN IsOnBootDevice(EFI_DEVICE_PATH *dp)
+{
+ extern CHAR16 *boot_device_path;
+ CHAR16 *device_path, *tmp;
+ BOOLEAN result = FALSE;
+
+ tmp = DevicePathToStr(dp);
+ device_path = GetBootMediumPath(tmp);
+ mfree(tmp);
+
+ if (StrCmp(device_path, boot_device_path) == 0) {
+ result = TRUE;
+ }
+ mfree(device_path);
+
+ return result;
+}
+
uint32_t calc_crc32(void *data, int32_t size)
{
uint32_t crc;
--
2.18.0

Andreas J. Reichel

unread,
Sep 5, 2018, 12:26:45 PM9/5/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

* Update UPDATE.md
* Add new RECOVERY.md to explain recovery mode

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
docs/RECOVERY.md | 28 ++++++++++++++++++++++++++++
docs/UPDATE.md | 5 ++---
2 files changed, 30 insertions(+), 3 deletions(-)
create mode 100644 docs/RECOVERY.md

diff --git a/docs/RECOVERY.md b/docs/RECOVERY.md
new file mode 100644
index 0000000..caff130
--- /dev/null
+++ b/docs/RECOVERY.md
@@ -0,0 +1,28 @@
+# Recovery Mode #
+
+If more than the expected number of environments is detected during boot, the
+system stops booting. This can be a problem if the user wants to boot the
+system with a memory stick to update a broken installation.
+
+In order to allow external boot devices with other environment configurations,
+the Recovery Mode was introduced. If any environment is found on the boot
+device, the boot loader will only use environments from this device.
+
+## Config filter ##
+
+The config filter depends on the detection of the boot media. For currently
+allowed boot media and GPT partitioning, the device path as retrieved from the
+loaded image is something like
+
+```
+PciRoot(0)/Pci(0x1,0x1)/Ata(Primary,Master)/HD(Part1,Sig12345678-1234-1234-1234-123456789012)
+```
+
+The last node is taken off by the function GetBootMediumPath resulting in
+
+```
+PciRoot(0)/Pci(0x1,0x1)/Ata(Primary,Master)
+```
+
+After enumerating all config partitions, if recovery mode is active, all config
+partitions that don't share this common 'BootMediumpath' are sorted out.
diff --git a/docs/UPDATE.md b/docs/UPDATE.md
index 19c2f39..17dd40d 100644
--- a/docs/UPDATE.md
+++ b/docs/UPDATE.md
@@ -11,7 +11,7 @@ The structure of the environment data is as follows:
struct _BG_ENVDATA {
uint16_t kernelfile[ENV_STRING_LENGTH];
uint16_t kernelparams[ENV_STRING_LENGTH];
- uint8_t padding;
+ uint8_t in_progress;
uint8_t ustate;
uint16_t watchdog_timeout_sec;
uint32_t revision;
@@ -23,8 +23,7 @@ struct _BG_ENVDATA {
The fields have the following meaning:
* `kernelfile`: Path to the kernel image, utf-16 encoded
* `kernelparams`: Arguments to the kernel, utf-16 encoded
-* `padding`: Padding byte to stay compatible with the offsets of the previous
- version.
+* `in_progress`: This stores `1` if an update is in progress, `0` otherwise.
* `ustate`: Update status (`0` OK, `1` INSTALLED, `2` TESTING, `3`: FAILED)
* `watchdog_timeout_sec`: Number of seconds, the watchdog times out after
* `revision`: The revision number explained above
--
2.18.0

Andreas J. Reichel

unread,
Sep 5, 2018, 12:26:45 PM9/5/18
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

diff to v3:
* Filter sorts inside input array and does not alloc memory
* Better documentation
* Again rename dirname to GetBootMediumPath

This patch series implements the new recovery mode to provide a way
to override on-device environments via other boot sources and hence
enable the user to repair or install systems with efibootguard on
e.g. a memory stick.

If an environment is detected on the boot device, only environments
from this device will be used.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>

Andreas Reichel (5):
Add function to get device path to the boot medium
Add function to get boot device and compare it to others
Print information if environment is on boot dev
Implement recovery mode as config filter
Update documentation

bootguard.c | 1 +
docs/RECOVERY.md | 28 ++++++++++++++++
docs/UPDATE.md | 5 ++-
env/fatvars.c | 84 +++++++++++++++++++++++++----------------------
env/syspart.c | 50 ++++++++++++++++++++++++++--
include/syspart.h | 3 +-
include/utils.h | 2 ++
main.c | 8 +++++
utils.c | 50 +++++++++++++++++++++++++++-
9 files changed, 184 insertions(+), 47 deletions(-)
create mode 100644 docs/RECOVERY.md

--
2.18.0

Jan Kiszka

unread,
Sep 6, 2018, 9:35:15 AM9/6/18
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
Thanks, applied to next. The naming became a bit inconsistent now (boot
medium/device), I'll send some cleanup in a minute.
Reply all
Reply to author
Forward
0 new messages