[PATCH 0/8] Update cppcheck, skip watchdog code in absence of drivers

[Jan Kiszka]

First of all, this refreshes our cppcheck call, dropping no longer
needed suppressions, enhancing detection, addressing findings, and
accelerating the execution.

Then comes an optimization of the code base in case a target does not
provide any watchdog drivers. Currently, this is the case for anything
!= x86 (32 and 64 bit).

The last two patches were originally developed to enable compile-time
relocation of the watchdog drivers from the bootloader into the UKI.
This plan is now dropped in favor of [1], but the changes still appear
valuable for the existing EBG codebase.

Jan

[1] https://groups.google.com/g/efibootguard-dev/c/pCzKDkbuNjQ

Jan Kiszka (8):
drivers/utils: Constify parameter of smbios_find_struct
kernel-stub: Privatize align_addr
utils: Privatize get_volume_[custom_]label
tools/tests: Privatize remove_fake_partitions
ci: Renovate cppcheck
ci: Catch also cppcheck warnings as errors
Move watchdog infrastructure into separate source file
Configure out watchdog infrastructure during build

.github/workflows/main.yaml | 2 +-
Makefile.am | 33 +++++----
configure.ac | 6 ++
drivers/utils/smbios.c | 2 +-
drivers/watchdog/amdfch_wdt.c | 1 +
drivers/watchdog/atom-quark.c | 1 +
drivers/watchdog/eiois200_wdt.c | 1 +
drivers/watchdog/hpwdt.c | 1 +
drivers/watchdog/i6300esb.c | 1 +
drivers/watchdog/ipc4x7e_wdt.c | 1 +
drivers/watchdog/ipcbx21a.c | 1 +
drivers/watchdog/ipmi_wdt.c | 1 +
drivers/watchdog/itco.c | 1 +
drivers/watchdog/w83627hf_wdt.c | 1 +
drivers/watchdog/wdat.c | 1 +
drivers/watchdog/wdfuncs_end.c | 4 +-
drivers/watchdog/wdfuncs_start.c | 4 +-
include/smbios.h | 2 +-
include/utils.h | 17 -----
include/{utils.h => watchdog.h} | 29 +-------
kernel-stub/main.c | 4 +-
main.c | 111 +++-------------------------
scripts/cppcheck.sh | 49 +++++++++----
tools/tests/fake_devices.c | 2 +-
tools/tests/fake_devices.h | 1 -
utils.c | 4 +-
watchdog.c | 121 +++++++++++++++++++++++++++++++
27 files changed, 214 insertions(+), 188 deletions(-)
copy include/{utils.h => watchdog.h} (53%)
create mode 100644 watchdog.c

--
2.47.3

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

Certain messages like style issues are not causing the error code to be
non-zero, thus can easily be missed in CI.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
scripts/cppcheck.sh | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/cppcheck.sh b/scripts/cppcheck.sh
index 1a7c333..8fbaf23 100755
--- a/scripts/cppcheck.sh
+++ b/scripts/cppcheck.sh
@@ -80,8 +80,12 @@ build_tmp=$(mktemp -d)
# files are provided. Compare 'cppcheck --help'.
cppcheck -f -q --error-exitcode=2 \
-j $(ncpus) --cppcheck-build-dir="$build_tmp" \
- $enable $suppress $ignore $cpp_conf $includes $path "$@"
+ $enable $suppress $ignore $cpp_conf $includes $path "$@" \
+ 2>&1 | tee "$build_tmp/output.log"
res=$?
+if [ $res -eq 0 ]; then
+ [ $(stat -c %s "$build_tmp/output.log") -eq 0 ] || res=1
+fi

rm -r "$build_tmp"
exit $res
--
2.47.3

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

Finding by cppcheck 2.17.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

kernel-stub/main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel-stub/main.c b/kernel-stub/main.c
index 9d77dcf..bfa655c 100644
--- a/kernel-stub/main.c
+++ b/kernel-stub/main.c
@@ -58,8 +58,8 @@ typedef struct {

static EFI_LOADED_IMAGE kernel_image;

-EFI_PHYSICAL_ADDRESS align_addr(EFI_PHYSICAL_ADDRESS ptr,
- EFI_PHYSICAL_ADDRESS align)
+static EFI_PHYSICAL_ADDRESS align_addr(EFI_PHYSICAL_ADDRESS ptr,
+ EFI_PHYSICAL_ADDRESS align)
{
return (ptr + align - 1) & ~(align - 1);
}
--
2.47.3

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

Finding by cppcheck 2.17.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

include/utils.h | 1 -
utils.c | 4 ++--
2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 2860258..261581c 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -34,7 +34,6 @@ extern CHAR16 *boot_medium_path;

typedef enum { DOSFSLABEL, CUSTOMLABEL, NOLABEL } LABELMODE;

-CHAR16 *get_volume_label(EFI_FILE_HANDLE fh);
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,
diff --git a/utils.c b/utils.c
index 99813be..a40e402 100644
--- a/utils.c
+++ b/utils.c
@@ -40,7 +40,7 @@ static BOOLEAN IsOnBootMedium(EFI_DEVICE_PATH *dp)
return result;
}

-CHAR16 *get_volume_label(EFI_FILE_HANDLE fh)
+static CHAR16 *get_volume_label(EFI_FILE_HANDLE fh)
{
EFI_FILE_SYSTEM_INFO *fsi;
EFI_GUID fsiGuid = EFI_FILE_SYSTEM_INFO_ID;
@@ -60,7 +60,7 @@ CHAR16 *get_volume_label(EFI_FILE_HANDLE fh)
return fsi->VolumeLabel;
}

-CHAR16 *get_volume_custom_label(EFI_FILE_HANDLE fh)
+static CHAR16 *get_volume_custom_label(EFI_FILE_HANDLE fh)
{
EFI_STATUS status;
EFI_FILE_HANDLE tmp;
--
2.47.3

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

Finding by cppcheck 2.17.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

tools/tests/fake_devices.c | 2 +-
tools/tests/fake_devices.h | 1 -

2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/tests/fake_devices.c b/tools/tests/fake_devices.c
index ad991b4..f5d2931 100644
--- a/tools/tests/fake_devices.c
+++ b/tools/tests/fake_devices.c
@@ -72,7 +72,7 @@ allocate_fake_part_error:
exit(1);
}

-void remove_fake_partitions(int n)
+static void remove_fake_partitions(int n)
{
PedPartition *pp = fake_devices[n].part_list;
PedPartition *next;
diff --git a/tools/tests/fake_devices.h b/tools/tests/fake_devices.h
index 895b5e5..7b0db5f 100644
--- a/tools/tests/fake_devices.h
+++ b/tools/tests/fake_devices.h
@@ -21,7 +21,6 @@ extern int num_fake_devices;

void allocate_fake_devices(int n);
void add_fake_partition(int devnum);
-void remove_fake_partitions(int n);
void free_fake_devices(void);

PedDevice *ped_device_get_next_custom_fake(const PedDevice *dev);
--
2.47.3

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

Introduce a build-time switch that signals whether there are watchdog
drivers available for a target. If not set, we can drop more bits from
the build and print more precise output.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

Makefile.am | 34 +++++++++++++++++++---------------
configure.ac | 6 ++++++
main.c | 11 +++++++++++
scripts/cppcheck.sh | 2 +-
4 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index fe1b07c..9025241 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -207,6 +207,20 @@ efi_loadername = efibootguard$(MACHINE_TYPE_NAME).efi

if BOOTLOADER

+efi_sources = \
+ env/syspart.c \
+ env/fatvars.c \
+ print.c \
+ utils.c \
+ loader_interface.c \
+ main.c
+
+watchdog_sources = \
+ drivers/watchdog/wdfuncs_start.c \
+ $(efi_sources_watchdogs) \
+ drivers/watchdog/wdfuncs_end.c \
+ watchdog.c
+
# NOTE: wdat.c is placed first so it is tried before any other drivers
# NOTE: ipcbx21a.c must be *before* itco.c
# NOTE: ipc4x7e_wdt.c must be *before* itco.c
@@ -226,22 +240,12 @@ watchdog_sources_x86 = \
drivers/utils/simatic.c \
drivers/utils/smbios.c
if ARCH_IS_X86
-efi_sources_watchdogs = $(watchdog_sources_x86)
-else
-efi_sources_watchdogs =
+watchdog_sources += $(watchdog_sources_x86)
endif

-efi_sources = \
- drivers/watchdog/wdfuncs_start.c \
- $(efi_sources_watchdogs) \
- drivers/watchdog/wdfuncs_end.c \
- watchdog.c \
- env/syspart.c \
- env/fatvars.c \
- print.c \
- utils.c \
- loader_interface.c \
- main.c
+if HAVE_WATCHDOGS
+efi_sources += $(watchdog_sources)
+endif

kernel_stub_name = kernel-stub$(MACHINE_TYPE_NAME).efi

@@ -331,7 +335,7 @@ kernel_stub_solib = $(top_builddir)/kernel-stub/kernel-stub$(MACHINE_TYPE_NAME).
efibootguard_DATA = $(efi_loadername) $(kernel_stub_name)
CLEANFILES += $(efi_objects) $(efi_solib) $(efi_loadername)
CLEANFILES += $(kernel_stub_objects) $(kernel_stub_solib) $(kernel_stub_name)
-EXTRA_DIST += $(efi_sources) $(watchdog_sources_x86) $(kernel_stub_sources)
+EXTRA_DIST += $(efi_sources) $(watchdog_sources) $(watchdog_sources_x86) $(kernel_stub_sources)

define gnuefi_compile
$(AM_V_CC) $(MKDIR_P) $(shell dirname $@)/; \
diff --git a/configure.ac b/configure.ac
index 9f41b30..1779661 100644
--- a/configure.ac
+++ b/configure.ac
@@ -225,6 +225,12 @@ if test "x$silent_boot" != "xno"; then
AC_DEFINE([SILENT_BOOT], [] , [Silent Boot])
fi

+# Signal to build whether there are watchdog drivers available
+AM_CONDITIONAL([HAVE_WATCHDOGS], [test -z "$ARCH_IS_X86_TRUE"])
+if test -z "$HAVE_WATCHDOGS_TRUE"; then
+ AC_DEFINE([HAVE_WATCHDOGS], [] , [Target provides watchdog drivers])
+fi
+
dnl pkg-config
PKG_PROG_PKG_CONFIG()
if test "x$PKG_CONFIG" = "xno"; then
diff --git a/main.c b/main.c
index 988efa5..b3fe340 100644
--- a/main.c
+++ b/main.c
@@ -97,10 +97,17 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
WARNING(L"Cannot close volumes.\n", status);
}

+#if defined(HAVE_WATCHDOGS)
status = probe_watchdogs(bg_loader_params.timeout);
if (EFI_ERROR(status)) {
error_exit(L"Cannot probe watchdog", status);
}
+#else
+ if (bg_loader_params.timeout > 0) {
+ error_exit(L"No watchdog drivers available, but timeout is non-zero",
+ EFI_UNSUPPORTED);
+ }
+#endif

/* Load and start image */
status = BS->LoadImage(FALSE, this_image, payload_dev_path, NULL, 0,
@@ -152,8 +159,12 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
loaded_image->LoadOptionsSize =
(StrLen(bg_loader_params.payload_options) + 1) * sizeof(CHAR16);

+#if defined(HAVE_WATCHDOGS)
INFO(L"Starting %s with watchdog set to %d seconds ...\n",
bg_loader_params.payload_path, bg_loader_params.timeout);
+#else
+ INFO(L"Starting %s ...\n", bg_loader_params.payload_path);
+#endif

BS->Stall(1000 * 1000 * ENV_BOOT_DELAY);

diff --git a/scripts/cppcheck.sh b/scripts/cppcheck.sh
index 8fbaf23..93298a2 100755
--- a/scripts/cppcheck.sh
+++ b/scripts/cppcheck.sh
@@ -71,7 +71,7 @@ includes="-I . \
-I /usr/include/x86_64-linux-gnu \
-I /usr/lib/gcc/x86_64-linux-gnu/9/include"

-cpp_conf="-U__WINT_TYPE__ -U__GNUC__"
+cpp_conf="-U__WINT_TYPE__ -U__GNUC__ -DHAVE_WATCHDOGS"
path=${1-.}

build_tmp=$(mktemp -d)
--
2.47.3