On 05.09.25 18:48, 'Storm, Christian' via EFI Boot Guard wrote: > Hi Jan, > >> Unfortunately, it turned out that we are creating holes in the final EFI >> binaries by using sections that are not know > > s/know/known/ > >> to the gnu-efi linker >> script. These wholes > > s/wholes/holes/g >
I only found this one case - will correct both findings in next. Thanks! Jan >> cause troubles when signing and specifically later >> on verifying the signatures. >> >> Avoid this by moving to a classic driver registration pattern where >> constructor functions call a central services which adds the passed >> driver descriptor to a list. This list is then later on walked for the >> actual probing. >> >> That works nicely for gnu-efi 3.0.16 and later after it added support >> for calling constructors during init. For older versions, we have to do >> that ourselves, though. We are using part of the old pattern for it >> which annotated start and end of the init_array and manually walked it, >> calling the listed functions. >> >> Signed-off-by: Jan Kiszka <[email protected]> >> --- >> Makefile.am | 2 +- >> configure.ac | 1 + >> drivers/watchdog/wdfuncs_end.c | 10 ++++++--- >> drivers/watchdog/wdfuncs_start.c | 10 ++++++--- >> include/utils.h | 19 ++++++++++++----- >> main.c | 35 ++++++++++++++++++++++++-------- >> scripts/cppcheck.sh | 5 +++-- >> 7 files changed, 60 insertions(+), 22 deletions(-) >> >> diff --git a/Makefile.am b/Makefile.am >> index 35eb08c..9d5aa28 100644 >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -360,7 +360,7 @@ $(efi_solib): $(efi_objects) >> nm -D -u $@ | grep ' U ' && exit 1 || : >> >> $(efi_loadername): $(efi_solib) >> - $(AM_V_GEN) $(OBJCOPY) $(efi_sections) -j .wdfuncs \ >> + $(AM_V_GEN) $(OBJCOPY) $(efi_sections) -j .init_array \ >> $(objcopy_format) $< $@ >> >> $(kernel_stub_solib): $(kernel_stub_objects) >> diff --git a/configure.ac b/configure.ac >> index 59f9e03..038930b 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -235,6 +235,7 @@ fi >> # GNU_EFI_VERSION resolves to gnu-efi's version without dots, e.g., >> GNU_EFI_VERSION=3016 >> # gnu-efi versions < 3.0.16 resolve to GNU_EFI_VERSION=0 >> AC_SUBST([GNU_EFI_VERSION], [$(($PKG_CONFIG --modversion "gnu-efi" >> 2>/dev/null || echo 0) | $TR -d '.' )]) >> +AC_DEFINE_UNQUOTED([GNU_EFI_VERSION], [${GNU_EFI_VERSION}], [gnu-efi >> version]) >> >> AC_SUBST([OBJCOPY_HAS_EFI_APP_TARGET], [$($OBJCOPY --info | $GREP -q pei- >> 2>/dev/null && echo "true")]) >> >> diff --git a/drivers/watchdog/wdfuncs_end.c b/drivers/watchdog/wdfuncs_end.c >> index 792cbdf..d19a0ab 100644 >> --- a/drivers/watchdog/wdfuncs_end.c >> +++ b/drivers/watchdog/wdfuncs_end.c >> @@ -1,7 +1,7 @@ >> /* >> * EFI Boot Guard >> * >> - * Copyright (c) Siemens AG, 2024 >> + * Copyright (c) Siemens AG, 2024-2025 >> * >> * Authors: >> * Christian Storm <[email protected]> >> @@ -12,10 +12,14 @@ >> * SPDX-License-Identifier: GPL-2.0-only >> */ >> >> +#if GNU_EFI_VERSION < 3016 >> + >> #include <efi.h> >> #include "utils.h" >> >> -/* Section .wdfunc's end address for watchdog probing function pointers >> +/* Section .init_array's end address for watchdog probing function pointers >> * preceding this marker, if any. */ >> -WATCHDOG_PROBE wdfuncs_end __attribute__((used, section(".wdfuncs"))) = >> +WATCHDOG_PROBE wdfuncs_end __attribute__((used, section(".init_array"))) = >> (WATCHDOG_PROBE)0x4353; >> + >> +#endif >> diff --git a/drivers/watchdog/wdfuncs_start.c >> b/drivers/watchdog/wdfuncs_start.c >> index e327ac9..bdf47f5 100644 >> --- a/drivers/watchdog/wdfuncs_start.c >> +++ b/drivers/watchdog/wdfuncs_start.c >> @@ -1,7 +1,7 @@ >> /* >> * EFI Boot Guard >> * >> - * Copyright (c) Siemens AG, 2024 >> + * Copyright (c) Siemens AG, 2024-2025 >> * >> * Authors: >> * Christian Storm <[email protected]> >> @@ -12,10 +12,14 @@ >> * SPDX-License-Identifier: GPL-2.0-only >> */ >> >> +#if GNU_EFI_VERSION < 3016 >> + >> #include <efi.h> >> #include "utils.h" >> >> -/* Section .wdfunc's sentinel value and start address for watchdog probing >> +/* Section .init_array's sentinel value and start address for watchdog >> probing >> * function pointers following this marker, if any. */ >> -WATCHDOG_PROBE wdfuncs_start __attribute__((used, section(".wdfuncs"))) = >> +WATCHDOG_PROBE wdfuncs_start __attribute__((used, section(".init_array"))) = >> (WATCHDOG_PROBE)0x5343; >> + >> +#endif >> diff --git a/include/utils.h b/include/utils.h >> index 101e30b..54b9a42 100644 >> --- a/include/utils.h >> +++ b/include/utils.h >> @@ -1,7 +1,7 @@ >> /* >> * EFI Boot Guard >> * >> - * Copyright (c) Siemens AG, 2017-2021 >> + * Copyright (c) Siemens AG, 2017-2025 >> * >> * Authors: >> * Jan Kiszka <[email protected]> >> @@ -42,11 +42,20 @@ EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE >> device, >> CHAR16 *GetBootMediumPath(const CHAR16 *input); >> >> typedef EFI_STATUS (*WATCHDOG_PROBE)(EFI_PCI_IO *, UINT16, UINT16, UINTN); >> -#define _CONCAT(prefix, func) prefix ## func >> -#define CONCAT(prefix, func) _CONCAT(prefix, func) >> + >> +typedef struct _WATCHDOG_DRIVER { >> + WATCHDOG_PROBE probe; >> + struct _WATCHDOG_DRIVER *next; >> +} WATCHDOG_DRIVER; >> + >> +VOID register_watchdog(WATCHDOG_DRIVER *driver); >> + >> #define WATCHDOG_REGISTER(_func) >> \ >> - __attribute__((used, section(".wdfuncs"))) static WATCHDOG_PROBE \ >> - CONCAT(wdfuncs##_, _func) = (WATCHDOG_PROBE)_func; >> + static WATCHDOG_DRIVER this_driver = {.probe = _func}; \ >> + static void __attribute__((constructor)) register_driver(void) \ >> + { \ >> + register_watchdog(&this_driver); \ >> + } >> >> VOID PrintC(const UINT8 color, const CHAR16 *fmt, ...); >> #define ERROR(fmt, ...) >> \ >> diff --git a/main.c b/main.c >> index fdd5e93..5d61385 100644 >> --- a/main.c >> +++ b/main.c >> @@ -1,7 +1,7 @@ >> /* >> * EFI Boot Guard >> * >> - * Copyright (c) Siemens AG, 2017 >> + * Copyright (c) Siemens AG, 2017-2025 >> * >> * Authors: >> * Jan Kiszka <[email protected]> >> @@ -31,10 +31,27 @@ extern CHAR16 *boot_medium_path; >> #define PCI_GET_VENDOR_ID(id) (UINT16)(id) >> #define PCI_GET_PRODUCT_ID(id) (UINT16)((id) >> 16) >> >> +static WATCHDOG_DRIVER *watchdog_drivers; >> +static WATCHDOG_DRIVER *last_watchdog_driver; >> + >> +VOID register_watchdog(WATCHDOG_DRIVER *driver) >> +{ >> + if (last_watchdog_driver != NULL) >> + last_watchdog_driver->next = driver; >> + else >> + watchdog_drivers = driver; >> + last_watchdog_driver = driver; >> +} >> >> static EFI_STATUS probe_watchdogs(UINTN timeout) >> { >> - if (wdfuncs_end - wdfuncs_start - 1 == 0) { >> +#if GNU_EFI_VERSION < 3016 >> + const unsigned long *entry = wdfuncs_start; >> + for (entry++; entry < wdfuncs_end; entry++) { >> + ((void (*)(void))*entry)(); >> + } >> +#endif >> + if (watchdog_drivers == NULL) { >> if (timeout > 0) { >> ERROR(L"No watchdog drivers registered, but timeout is non-zero.\n"); >> return EFI_UNSUPPORTED; >> @@ -83,14 +100,16 @@ static EFI_STATUS probe_watchdogs(UINTN timeout) >> continue; >> } >> >> - const unsigned long *entry = wdfuncs_start; >> - for (entry++; entry < wdfuncs_end; entry++) { >> - WATCHDOG_PROBE probe = (WATCHDOG_PROBE) *entry; >> - if ((status = probe(pci_io, PCI_GET_VENDOR_ID(value), >> - PCI_GET_PRODUCT_ID(value), >> - timeout)) == EFI_SUCCESS) { >> + WATCHDOG_DRIVER *driver = watchdog_drivers; >> + while (driver) { >> + status = driver->probe(pci_io, >> + PCI_GET_VENDOR_ID(value), >> + PCI_GET_PRODUCT_ID(value), >> + timeout); >> + if (status == EFI_SUCCESS) { >> break; >> } >> + driver = driver->next; >> } >> >> (VOID) BS->CloseProtocol(handle_buffer[index], &PciIoProtocol, >> diff --git a/scripts/cppcheck.sh b/scripts/cppcheck.sh >> index 0bd1310..d6e4aba 100755 >> --- a/scripts/cppcheck.sh >> +++ b/scripts/cppcheck.sh >> @@ -45,8 +45,9 @@ suppress+=" >> --suppress=nullPointerRedundantCheck:kernel-stub/main.c" >> suppress+=" --suppress=unusedStructMember:kernel-stub/main.c" >> suppress+=" --suppress=unusedStructMember:kernel-stub/fdt.c" >> # Not applicable because of API requirements >> -suppress+=" >> --suppress=constParameterCallback:drivers/watchdog/ipc4x7e_wdt.c" >> -suppress+=" >> --suppress=constParameterCallback:drivers/watchdog/w83627hf_wdt.c" >> +suppress+=" --suppress=constParameterPointer:drivers/watchdog/ipc4x7e_wdt.c" >> +suppress+=" >> --suppress=constParameterPointer:drivers/watchdog/w83627hf_wdt.c" >> +# Not applicable because of API requirements >> suppress+=" --suppress=constParameterCallback:kernel-stub/initrd.c" >> >> enable="--enable=warning \ >> -- >> 2.43.0 >> > > > > Kind regards, > Christian > -- Siemens AG, Foundational Technologies Linux Expert Center -- You received this message because you are subscribed to the Google Groups "EFI Boot Guard" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion visit https://groups.google.com/d/msgid/efibootguard-dev/4fc5ddb6-6c96-4def-83d6-d09caf8bf608%40siemens.com.
