I promised in https://github.com/systemd/systemd/pull/41426 that its the
final update for coccinelle pointer deref checks. However it turned out
there is this coccinelle/parsing_hacks.h that I wasn't aware of. The
file missed some important things like _cleanup_(x) that prevented
coccinelle to check a bunch of functions.
This PR adds some missing defines to the parsing_hacks.h and fixes the
missing asserts(). I apologize that its a bit long (and frankly boring)
and that I missed this earlier.
The last commit contains one small behavior change (ret in
sd_varlink_idl_parse() is now really optional) but the big one is very
mechanical.
We have code like `size_t max_size = sizeof(*private)` in three
places. This is evaluated at compile time so its safe to use. However
the new pointer-deref checker in coccinelle is not smart enough to know
this and will flag those as errors. To avoid these false positives
we have some options:
1. Reorder so that we do:
```C
size_t max_size = 0;
assert(private);
max_size = sizeof(*private);
```
2. Use something like `size_t max_size = sizeof(*ASSERT_PTR(private));`
3. Place the assert before the declaration
4. Workaround coccinelle via SIZEOF(*private) that we can then hide
via parsing_hacks.h
5. Fix coccinelle (OCaml, hard)
6. ... somehting I missed?
None of these is very appealing. I went for (4) but happy about
suggestions.
Sometimes we want need to diff two unsigned numbers, which is awkward
because we need to cast them to something with a sign first, if we want
to use abs(). Let's add a helper that avoids the function call
altogether.
Also drop unnecessary parens arounds args which are delimited by commas.
This is similar to DEFINE_POINTER_ARRAY_FREE_FUNC, but one
pointer chase less. The name of the outer and inner functions are
specified separately. The inner function does not free, so it'll
be generally something like 'foo_done', but the outer function
does free, so it can be called 'foo_array_free'.
As mentioned in the grandfather commit, I want to use the _many
suffix for freeing of the contents of an array, so the functions
to free the array to get the suffix _array.
This is a helper macro that defines a function to drop elements of an
array but not the array itself. I used the "_many" suffix because it
most closely matches what happens here: we are calling the cleanup
function a bunch of times.
Move the push-based streaming compression API from import-compress.c
into compress.c and delete import-compress.c/h. This consolidates all
compression code in one place and makes all compression libraries
(liblzma, liblz4, libzstd, libz, libbz2) runtime-loaded via dlopen
instead of directly linked.
Introduce opaque Compressor/Decompressor types backed by a heap-
allocated struct defined only in compress.c, keeping all third-party
library headers out of compress.h.
Rewrite the per-codec fd-to-fd stream functions as thin wrappers around
the push API via generic compress_stream()/decompress_stream() taking a
Compression type parameter. Integrate LZ4 into this framework using the
LZ4 Frame API, eliminating all LZ4 special-casing.
Extend the Compression enum with COMPRESSION_GZIP and COMPRESSION_BZIP2
and add the corresponding blob, startswith, and stream functions for
both.
Rename the ImportCompress types and functions: ImportCompressType becomes
the existing Compression enum, ImportCompress becomes Compressor (with
Decompressor typedef), and all import_compress_*/import_uncompress_*
become compressor_*/decompressor_*. Rename dlopen_lzma() to dlopen_xz()
for consistency. Make compression_to_string() return lowercase by
default.
Add INT_MAX/UINT_MAX overflow checks for LZ4, zlib, and bzip2 blob
functions where the codec API uses narrower integer types than our
uint64_t parameters.
Migrate test-compress.c and test-compress-benchmark.c to the TEST()
macro framework, new assertion macros, and codec-generic loops instead
of per-codec duplication.
Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com>
In some builds (package builds, so with optimization and lto, but I
haven't been able to pin down the exact combination on options that
matters), we end up with items in the verbs array reordered. The order
matters (because of groups, but also because we have some specific order
for display), so this reordering is something that we don't want.
From what I was able to read, the compiler + linker generally keep the
order within a single translation unit, but this is more of a convention
and implementation choice than a guarantee. Add this attribute [1]. It
seems to have the desired effect in CI.
[1] https://gcc.gnu.org/onlinedocs/gcc-15.2.0/gcc/Common-Function-Attributes.html#index-no_005freorder-function-attribute
After having some more experience with how this works, I think some
changes are in order. So there are a handful of preparatory patches and
then conversion of a few progs that make use of the new functionality.
The fix in 8f1751a111 made me wonder if we could automatically detect
when pointers are accessed but when this might not be safe. Systemd
is already using a lot of `assert(dst)` and this change now forces
us to use them.
So this commit (ab)uses coccinelle to flag any pointer parameter
dereference not preceded by assert(param), ASSERT_PTR(param), or an
explicit NULL check. It adds integration into meson as a new "coccinelle"
test suite (just like clang-tidy) and is run in CI. The check is not
perfect but seems a reasonable heuristic.
For this RFC commit it is scoped to a subset, it excludes 25 dirs right
now and includes around 100. About 300 warnings left. Busywork that I am
happy to do if there is agreement that it is worth it.
With this in place we would have caught the bug from 8f1751a111 in CI:
```
FAIL: check-pointer-deref.cocci found issues in systemd/src/boot:
diff -u -p systemd/src/boot/measure.c /tmp/nothing/measure.c
--- systemd/src/boot/measure.c
+++ /tmp/nothing/measure.c
@@ -312,7 +312,6 @@ EFI_STATUS tpm_log_tagged_event(
if (err != EFI_SUCCESS)
return err;
- *ret_measured = true;
return EFI_SUCCESS;
}
```
This also adds a new POINTER_MAY_BE_NULL() for the cases when the
called function will do the NULL check (like `iovec_is_set()`).
This mirrors the idea and implementation for options.
Previously, the Verb struct was named as verb_func_data_nn, but
with the group marker entry, we don't have 'verb_func', so let's
just call the item verb_data_nn. Using the verb here is a complication
that is not needed.
If length is exactly 16, the loop would finish with length == 0,
but we'd carry on to the memcmp() check, where the 'p + 16' passed
would be invalid memory. memcmp() demands valid pointers even if
size is specified to 0, hence let's catch this ourselves.
With the uintptr_t data parameter, it is actually quite nice to have
VERB(do_impl, "name-a", …)
VERB(do_impl, "name-b", …)
int do_impl(…) { … }
To make this work, the do_impl_data struct needs to have a unique name and
we also need to suppress the warning about the forward declaration for
do_impl being repeated. I think it's fine to suppress the warning, it's
not needed for anything. If somebody declares the function with the same
name by mistake, the implementations are going to conflict too.
This attribute was introduced in gcc 11, and our baseline is currently
8.4. So let's allow using _retain_ everywhere, but make it into a noop
if not supported.
Using __has_attribute was suggested, but with gcc-11.5.0-14.el9.x86_64,
__has__attribute(__retain__) is true, but we get a warning when the
attribute is actually used.
Motivation:
Currently, when setting the default boot pattern, boot assessment status
is not taken into account. This means that with boot assessment enabled,
when an explicit boot entry is configured as the default entry using an
EFI var, as is common for instance in A/B boot schemes, the configured
entry will be booted indefinitly, regardless of the entry's boot
assessment status.
In order to allow for this use case in combination with boot assessment,
we introduce a new `preferred` keyword, both in the config file and in the
bootctl CLI, that acts very similar to the existing `default` keyword but
takes boot assessment into account and never selects any entries that
have been marked as bad.
If the preferred pattern does not resolve to any bootable entry, and a
default pattern is also specified, then the default pattern will be
considered next, and we may then still select a known-bad entry to be
booted.
Fixes: https://github.com/systemd/systemd/issues/31215
Fixes: https://github.com/systemd/systemd/issues/40192
Let's introduce these for the following two reasons:
- Using them makes sure the parameter names match between declaration
and definition (if both use the corresponding macros).
- They make sure developers can't forget the _const_ and _pure_
attributes for the declarations.
This commit also includes some include sorting fixes
Latest glibc uses _Generic to have strstr() and other functions return
const char* or char* based on whether the input is a const char* or a
char*. This causes build failures as we previously always expected a char*.
Let's fix the compilation failures and add our own macros similar to glibc's
to have string functions that return a mutable or const pointer depending on
the input.
shim is often used as part of the EFI boot chain with Linux kernels.
shim has an option to disable all verification of binaries it loads.
This can be performed by end-user using mokutil / mokmanager EFI app,
which set BootServices only variable MokSBState. shim honors that, and
mirrors it as readonly MokSBStateRT for the post-ExitBootService access.
Thus presense of MokSBStateRT is an indicator that shim was used during
boot chain.
Some OEM vendors are known to set MokSBState variable, without user
having done so.
When verification is disabled, one should assume secureboot is insecure,
because any EFI binary was allowed to run, including but not limited to
unsigned or revoked:
- grub
- systemd-boot
- UKI
- linux kernel
Linux kernel also has code to check this variable, and correctly report
that Secure Boot is disabled, see:
-
https://github.com/torvalds/linux/blob/3a8660878839faadb4f1a6dd72c3179c1df56787/drivers/firmware/efi/libstub/secureboot.c#L57
With this change bootctl output changes like this:
```diff
System:
Firmware: n/a (n/a)
Firmware Arch: x64
- Secure Boot: enabled (user)
+ Secure Boot: disabled (insecure)
TPM2 Support: yes
Measured UKI: no
Boot into FW: supported
```
This implementation is trying to mimic mokutil behaviour like this one:
```
$ mokutil --sb-state
SecureBoot enabled
SecureBoot validation is disabled in shim
```
As well as the linux kernel behavior of:
```
$ journalctl -b | grep 'Secure boot disabled'
kernel: Secure boot disabled
```
Note that MokSBState is extended into PCR7 as well as also into PCR14.
For more details see https://github.com/rhboot/shim/blob/main/README.tpm
Previously, ONCE is false for the first time, and true for later times,
hence log_once() and log_once_errno() suppress logging in the first call,
rather than later calls.
Fortunately, ONCE macro is only used in log_once() and log_once_errno(),
hence this only fixes spurious logging.
Fedora's kernels now ship with a .sbat section:
kernel,1,Red Hat,kernel-core,6.17.0-0.rc1.250814g0cc53520e68b.20.fc44.x86_64,mailto:secalert@redhat.com
kernel.fedora,1,Red Hat,kernel-core,6.17.0-0.rc1.250814g0cc53520e68b.20.fc44.x86_64,mailto:secalert@redhat.com
This pushes the combined .sbat section just over its pre-allocated size of 512 bytes:
File "/usr/bin/ukify", line 1048, in pe_add_sections
raise PEError(f'Not enough space in existing section {section.name} to append new data')
PEError: Not enough space in existing section .sbat to append new data
PE sections need to align to 512 bytes, so to make it all fit we pad the .sbat
section with zeros to 1k. Various tools already should strip trailing zeros when
using sbat sections, since ukify always inserts a trailing NUL.
The defines are moved to sbat.h, they are used only in sd-stub and sd-boot.
It turns out checking sysfs is not 100% reliable to figure out whether
the firmware had TPM2 support enabled or not. For example with EDK2 arm64, the
default upstream build config bundles TPM2 support with SecureBoot support,
so if the latter is disabled, TPM2 is also unavailable. But still, the ACPI
TPM2 table is created just as if it was enabled. So /sys/firmware/acpi/tables/TPM2
exists and looks correct, but there are no measurements, neither the firmware
nor the loader/stub can do them, and /sys/kernel/security/tpm0/binary_bios_measurements
does not exist.
The loader can use the apposite UEFI protocol to check, which is a more
definitive answer. Given userspace can also make use of this information, export
the bitmask with the list of active banks as-is. If it's not 0, then we can be
sure a working TPM2 was available in EFI mode.
Partially fixes https://github.com/systemd/systemd/issues/38071
It declares STDC_VERSION 202311L but still doesn't support this feature:
https://en.cppreference.com/w/c/compiler_support/23
The EDG frontend is used by intellisense, which is the default
engine in VSCode, so parsing is currently broken and tons of
spurious errors are shown. Skip this feature when this
compiler frontend is used.
Follow-up for 92100f14c3
Instead of using run-clang-tidy.py with its own scheduling, let's
just gather a list of source files ourselves and then use that to
add a unit test for each source file that runs clang-tidy on the
source file.
We also add a bit of logic to run clang-tidy on most header files
as well for extra coverage. This uncovered various header files that
were not standalone so this commit also includes fixes to make sure
the clang-tidy tests are all green.
We can also use this in a later commit to run clang-include-cleaner
on each source file in the same way.
In preparation for adopting forward declarations to reduce unnecessary
transitive includes across the tree, let's introduce a forward.h header
with forward declarations for all libc, libsystemd, basic and shared types.
Additionally, this header exports all basic integer types and errno constants,
as well as all macros including assertions macros. These header files contain
types often used in headers and are always included in every source file one
way or another anyway.
To avoid having to include memory-util.h and alloc-util.h in forward.h, we
split off the parts we need from both into cleanup-util.h and only include
cleanup-util.h in forward.h.
To keep this commit self-contained, we include cleanup-fundamental.h and
cleanup-util.h from the headers that originally contained the same macros.
We'll remove these again in a later commit that optimizes the includes in
src/basic and src/fundamental.
Split out of #37364
Zbigniew Jędrzejewski-Szmek
Michael Vogt
Daan De Meyer
Lennart Poettering
Mike Yuan
Yu Watanabe
r-vdp
Daan De Meyer
Lennart Poettering
Dimitri John Ledkov
Zbigniew Jędrzejewski-Szmek
Luca Boccassi