This is a followup for https://github.com/systemd/systemd/pull/41096
that makes more subsystems pass the new `check-pointer-deref` coccinelle
checks. See the individual commits.
My plan is to do a few more of these PRs until we have it all covered. I
could also do it in a single very big PR but I'm worried about a)
conflicts b) that its just too big/annoying to review. Only 7 dirs left
but some (like src/basic) are quite big (~50k loc) so those PRs will be
a bit bigger.
A malformed image can cause a division by zero, check that
the parameters are not zero.
Reported on yeswehackl.com as YWH-PGM9780-173
Follow-up for 0fa2cac4f0
Our coccinelle/check-pointer-deref.cocci checker has a special
case for `assert(pidref_is_set(param))`. It turns out we can
generalize this and catch the following:
- iovec_is_set
- sd_dhcp_duid_is_set
- sd_dhcp_client_id_is_set
This corrects sorting of menu entries regarding profile numbers:
1. If the profile number is unset, let's treat this identical to profile
0, when ordering stuff, because an item with no profile is
conceptually the same as an item with only a profile 0.
2. Let's take the profile number into account also if sort keys are
used. This was makes profiles work sensibly in type 1 entries, via
the recently added "profile" stanza.
Follow-up for: 5fb90fa319
When showing the list of menu entries via "p", the "internal call:"
field was showing nonsense, since
fb6cf4bbb7.
Fix that by adding a proper entry type for "internal" menu items such as
reboot/firmware/poweroff, and then check for that.
With this in place all entries now have a loader type that makes sense
and describes precisely what an entry is about.
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()`).
A malformed bmp with 8bits depth but smaller color
map would cause out of bounds reads. This is not a real
problem as the image is signed, but better to be safe.
Reported on yeswehack.com as:
YWH-PGM9780-135
Follow-up for 0fa2cac4f0
Previously we'd never write the ESP random seed file (or initialize the
random seed EFI table) if it didn't already exist. Let's adjust this a
bit, and also create it fresh if we have a "good" random source, i.e. if
the EFI table already existed or if the RNG protocol is implemented by
EFI.
This is useful as it increases the chance the random seed table is
valid, and we can use it as source for randomness in later stages.
In one of the reviews one of the LLMs noticed that the pragma is set but
never unset, so it remains in effect for the rest of the translation
unit. From the comment, it's not clear how old those "old compilers"
were, so let's try if things work without this workaround.
Also, in general we prefer variables that are always defined over
checking with #ifdef, so #if defined(HAVE_NO_STACK_PROTECTOR_ATTRIBUTE)
is something that we want to avoid.
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.
tpm_log_tagged_event() only measures the event to the TPM while
tpm_log_ipl_event() measures the event both to the TPM and CC. Fix the
inconsistency.
Note, this is a potentially breaking change for TDX guests as systemd will
now measure more stuff to the MRTD/RTMRs, reference values for attestation may
need to be adjusted.
Found by Claude Code Review.
In a project I'm working on I recently observed a boot failure
with the most recent version of systemd. It seems it is triggered
by bb19b61049 which fixed a bug that now leads to the function
being excuted differently. The code is missing a check if
`*ret_version` is actually valid in the `ret_measured = true`
case.
pe_section_table_from_file already checks with SECTION_TABLE_BYTES_MAX,
do the same in pe_section_table_from_base() just in case.
Originally reported on yeswehack.com as:
YWH-PGM9780-117
Follow-up for f4e081051d
Suppress warnings like the following from clang tidy:
```
../src/boot/addon.c:11:19: error: function 'efi_main' can be made static to enforce internal linkage [misc-use-internal-linkage,-warnings-as-errors]
11 | EFIAPI EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *system_table);
| ^
```
Some warnings are suppressed simply by setting comments to ignore the warning,
some are by making global variables static, or include a suitable header.
With distributions like Ubuntu and Fedora using systemd-stub to auto load
DTB's on Windows on ARM laptops, the CHID DTB match failing is expected
when that same UKI is instead booted on an ARM SystemReady system where
no DTB is necessary.
In the ARM SystemReady case showing a big red error message is undesirable
and leads to confused users and bug-reports. Lower the message to debug
level when the status is EFI_NOT_FOUND to avoid these false positive error
messages.
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2444759
These are meant to ship with ukify, in the same package,
so install them when ukify is enabled. Given they are
hardware-specific, use a per-arch subdirectory to make it
easier to enable them by default in ukify.
Add hwids json files for Snapdragon devices that use of this
feature since they don't ship with devie trees embedded in firmware.
The json files were originally generated for Ubuntu stubble and
are kown to work since they ship in all Ubuntu arm64 ISOs by default.
Some of the devices might not be supported in mainline Linux yet
and use preliminary compatible strings.
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
The current code incorrectly subtracts ImageBase from section
VirtualAddress values when loading sections into memory. This is based
on a misunderstanding of the PE specification.
VirtualAddress in section headers is the address of the first byte of
the section relative to the image base when the section is loaded into
memory. In other words, VirtualAddress is already an RVA measured from
the image base, it is definitely NOT an absolute address that needs to
be adjusted.
So when loading a PE image into a newly allocated buffer, sections
should be copied to buffer + VirtualAddress, regardless of what
ImageBase says. The ImageBase field merely indicates the *preferred*
load address, it does not affect how section RVAs are interpreted.
This happens to not cause issues when ImageBase was 0 (since
VirtualAddress - 0 = VirtualAddress), which is why this bug went
undetected on modern kernels. However, it fails with kernels that have
non-zero ImageBase values.
So let's remove the nonsensical VirtualAddress < ImageBase check, and
remove the ImageBase subtractions from section loading offsets. This
lets all kernel UKIs work properly again.
Fixes: #40342
UEFI Block I/O Protocol has `Media->IoAlign` field dictating the minimum
alignment for I/O buffer. It's quite surprising this has been lingering
here unnoticed for years, seems like most UEFI implementations have
small or no alignment requirements. U-Boot is not the case here, and
requires at least 512 byte alignment, hence attempt to read GPT
partition table fail and in effect systemd-boot can not find XBOOTLDR
partition.
These patches allow to boot from XBOOTLDR partition on U-Boot - tested
with latest systemd revision and U-Boot master
(`8de6e8f8a076d2c9b6d38d8563db135c167077ec`) on x64 and ARM32, of which
both are failing without the patch.
Also fixes Bitlocker probing logic, which is the only other place where
raw block I/O is used, however this is untested.
This code is used to read data copied from /etc/os-release. According to
the spec[1], values can be enclosed in single quotes or double quotes.
Not handling single quotes results in the quotes appearing in the
systemd-boot menu, e.g. 'Gentoo Linux'.
[1] https://www.freedesktop.org/software/systemd/man/latest/os-release.html
Signed-off-by: James Le Cuirot <jlecuirot@microsoft.com>
The current code incorrectly subtracts ImageBase from section
VirtualAddress values when loading sections into memory. This is based
on a misunderstanding of the PE specification.
VirtualAddress in section headers is the address of the first byte of
the section relative to the image base when the section is loaded into
memory. In other words, VirtualAddress is already an RVA measured from
the image base, it is definitely NOT an absolute address that needs to
be adjusted.
So when loading a PE image into a newly allocated buffer, sections
should be copied to buffer + VirtualAddress, regardless of what
ImageBase says. The ImageBase field merely indicates the *preferred*
load address, it does not affect how section RVAs are interpreted.
This happens to not cause issues when ImageBase was 0 (since
VirtualAddress - 0 = VirtualAddress), which is why this bug went
undetected on modern kernels. However, it fails with kernels that have
non-zero ImageBase values.
So let's remove the nonsensical VirtualAddress < ImageBase check, and
remove the ImageBase subtractions from section loading offsets. This
lets all kernel UKIs work properly again.
Fixes: #40342
This is a small followup for
https://github.com/systemd/systemd/pull/40344#discussion_r2695031041
(thanks Daan and Lennart!).
There are quite a few places in the code that currently log a
`log_error()` when a `log_warning()` is more appropriate because
the error is ignored. This commit now shows those as warnings.
Allow configuring the menu timeout used by sd-boot by setting
`io.systemd.boot.timeout=<value>` as SMBIOS type 11 string.
This takes precedence over the setting from the config file
and the EFI var.
Useful for e.g. integration tests that want to control that
a generic image boots faster.
When n_all_initrds == 0, then all_initrds is unmodified from its initial
value of:
_cleanup_free_ struct iovec *all_initrds = NULL;
and in the else block of the "if (n_all_initrds > 1)" the NULL is
dereferenced:
final_initrd = all_initrds[0];
Leading to the stub crashing due to a NULL pointer deref.
Fix this by initializing final_initrd to all 0s and only
running the else block if (n_all_initrds == 1).
Luca Boccassi
Zbigniew Jędrzejewski-Szmek
Michael Vogt
Lennart Poettering
Mike Yuan
Vitaly Kuznetsov
Yu Watanabe
Hans de Goede
Tobias Heider
r-vdp
Artur Kowalski
James Le Cuirot
cdown
Michael Vogt
Lennart Poettering
DaanDeMeyer