The tree is clean now, keep it that way. Run the whole argument-comments
suite like the clang-tidy one. Checking only the lines a pull request
changed misses comments that go stale when a parameter is renamed,
since no caller's line changes then. On four CPUs the suite takes about
a minute and a half.
meson test reports a test that exits with 77 as skipped, so make sure
the libclang bindings are there first. Otherwise a tools tree without
them would pass the step without checking anything.
While at it, add the script to the mypy and tab checks.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
tools/check-argument-comments.py only takes the /* name= */ form that
docs/CODING_STYLE.md shows as naming a parameter, as clang-tidy's
bugprone-argument-comment does. Add the '=' these comments are missing.
Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
While at it, add the '=' that some of the existing comments are missing.
By hand, drop the /* uncompressed */ in dns_packet_read_rr() which only
repeats the new comment, and wrap the get_mempolicy() call in
numa-util.c that grew to 141 columns.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
tools/check-argument-comments.py only takes the /* name= */ form that
docs/CODING_STYLE.md shows as naming a parameter, as clang-tidy's
bugprone-argument-comment does. Add the '=' these comments are missing.
Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
While at it, add the '=' that some of the existing comments are missing.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
docs/CODING_STYLE.md asks for a /* name= */ comment whenever NULL or
another value meaning "unset" is passed to a function. Add it wherever
a bare NULL, 0, false or true is passed for a parameter with a telling
name. Mechanical, generated by tools/check-argument-comments.py --fix.
The fsmount() shim took its parameter names from glibc which calls the
last one ms_flags. That's misleading as it takes MOUNT_ATTR_* flags and
not MS_* flags. The kernel and fsmount(2) call it attr_flags. Use the
names from fsmount(2) and fix up the two comments in namespace.c that
called it mount_attrs and flags.
Run tools/check-argument-comments.py over the sources that are compiled,
one test per top-level source directory, in a suite that a plain "meson
test" leaves out like the other linters. The NSS, PAM and cryptsetup
token modules don't end up in sources, so collect theirs as well. The
source directory is passed explicitly, so the suite works without git,
from a tarball say. The script exits with meson's skip code where the
libclang python bindings are missing.
CODING_STYLE asks for a /* name= */ comment whenever NULL or another
value meaning "unset" is passed to a function. Nothing enforces it.
clang-tidy's bugprone-argument-comment only validates comments that
exist, and its option to demand them for literals never sees NULL
behind its cast in C and flags every size and mode along the way.
Add tools/check-argument-comments.py. It parses each file with
libclang and the flags from compile_commands.json, with every set of
them for a file that is built more than once, takes the parameter
names from the first declaration the caller sees, the one clang-tidy
compares comments against, and reports arguments spelled as a bare
NULL, 0, false or true, in the file and in the in-tree headers it
includes.
The arguments are read from the text of the call. So a literal with a
comment after it is still bare, and a comment at the end of the
previous line belongs to the previous argument. Like clang-tidy, only
a /* name= */ comment names a parameter, and case and leading or
trailing underscores don't matter. A /* name */ that lost its '=' is
reported too.
Calls into libc are skipped, and so are calls produced by macro bodies
since the caller never spelled their arguments. A call written inside
a macro argument is checked, as is a call through a macro that only
renames the function, the shims in src/include/override for example,
and a call through the sym_* pointers of the dlopen() wrappers.
--diff REV limits the report to lines that differ from a revision, and
--fix inserts the comments.
tools/check-argument-comments.py --diff "$(git merge-base main HEAD)" src
The builtin headers, stddef.h and friends, are located by asking the
clang binary of the same major version. libclang derives their
location from its own, and in the Fedora rawhide tools tree that
comes out as the relative ../lib/clang/22, so every file fails to
parse with 'stddef.h' file not found.
Needs python3-clang and libclang 19 or newer. Older versions return
the file location from clang_getSpellingLocation(), so calls produced
by macro bodies would be checked as if the caller had spelled them.
Let's use idmap-aware inode LSM hooks on Linux 7.4 and newer to check
the
ownership recorded on the actual backing filesystem. This permits safe
OverlayFS uppers while rejecting persistent transient IDs, including
when a
mount idmap translates stable caller IDs into transient IDs. Older
kernels
continue to use the path policy.
Let's preserve the policy maps and replace pinned program links
atomically
on restart, so failed replacement keeps existing namespaces protected.
Tests
cover OverlayFS, mount idmaps, links, ownership changes, POSIX queue
creation
and namespace lifetimes, and failed pin replacement.
An older SetTimezone request may fail after a newer lease has changed the cached timezone.
Its error handler then clears the new value, so the current timezone is not retried until another lease update.
Keep the old request until its replacement has been queued successfully,
then cancel it. Release the completed request slot in the reply handler.
The review agents rely on memory when checking UAPI specifications. Let’s
clone the specifications alongside the kernel and libc sources and tell the
agents to consult them when a finding depends on a specification.
io_uring operations bypass several sandboxing settings today.
IORING_OP_SOCKET ignores RestrictAddressFamilies=, IORING_OP_OPENAT can
create SUID files despite RestrictSUIDSGID=, and SystemCallFilter=
doesn't
cover any operation that can do the work of a system call it denies.
Since Linux 7.0, a task can register classic BPF filters for io_uring
operations, and every ring that the task or its children create
afterwards
inherits them. Let's use them in the executor, so that these three
settings
also apply to the equivalent io_uring operations. We also add
IOUringFilter=, which takes an allow or deny list of io_uring
operations,
for operations like IORING_OP_URING_CMD that have no system call of
their
own.
"systemd-analyze io-uring-filter" lists the operations and the system
calls that SystemCallFilter= maps them to.
Once the executor registers io_uring filters, the kernel refuses
task-wide IORING_REGISTER_RESTRICTIONS for the service with EPERM. The
man
pages document that. On older kernels, the existing settings keep their
current behavior, and IOUringFilter= denies io_uring_setup() with
ENOSYS,
so that programs fall back to regular system calls.
The path policy rejects persistent OverlayFS uppers even for clients whose
identity mappings record only stable IDs. Let's choose inode hooks when
the kernel passes them mount idmaps (Linux 7.4 and newer), so we can check
ownership on the actual upper filesystem for each operation. We keep the
path policy for kernels without those signatures.
Let's preserve the policy maps and retire the old path attachments only
after the inode policy is active. We classify mapped IDs rather than the
caller's IDs, since a stable ID can map into a transient range.
PR #44620 restricts O_TMPFILE, RENAME_WHITEOUT and Btrfs subvolume creation
outside the path creation hooks. Let's use the inode policy's ownership
checks in those hooks too. We can't use inode_rename() for whiteouts,
because it gets neither the rename flags nor the mount idmap.
A link adds a name for an existing inode, so the inode policy checks the
owner of that inode instead of the caller's fsuid. Otherwise a client can
switch its fsuid to another mapped ID and link a file owned by its
transient ID onto persistent storage.
nsresourced reuses the program links it pinned earlier, so a restart after
an upgrade keeps the old programs attached until the next reboot. Let's
attach replacement programs before publishing their pins together, so a
failed upgrade leaves the previous policy attached.
The regression tests cover identity and idmapped OverlayFS uppers, unsafe
work directories, unnamed files, whiteouts, links, ownership changes, POSIX
queue lifetimes and failed pin replacement. Like nsresourced, the tests
remove the map entries of a namespace when they're done with it. Otherwise
a later namespace can reuse the inode number and match a stale entry.
POSIX queue creation bypasses the path hooks. Its resource accounting must
keep the creator's user namespace alive while the queue exists, or
nsresourced could recycle IDs still recorded in the queue's inode.
Let's check that no namespace-free notification arrives after the creator
exits and the namespace descriptors are closed, then require the
notification after unlinking the queue.
Btrfs subvolume creation bypasses the path and inode creation hooks. A
managed user namespace can therefore leave a subvolume root owned by its
transient range on a file system that outlives the namespace.
Let's apply the existing creation restrictions to the native and compat
ioctl calls for subvolume creation. The tests check persistent and idmapped
mounts, nested user namespaces, unrelated ioctls and other file systems.
The compat ioctl hook is absent on older kernels. Let's preserve the
native policy when that hook isn't available.
Distributions install .wants/, .requires/, and .upholds/ symlinks below
/usr/lib/systemd/system/, and PID 1 starts those units at boot.
systemctl
doesn't know about them: is-enabled reports the units as disabled, and
disable
can't turn them off.
Let's report such units as enabled, but only for a symlink that enable
would
create, i.e. one in a directory that WantedBy=, RequiredBy=, or
UpheldBy= in the
[Install] section names. Any other vendor symlink is a static
dependency, and
we leave it alone. remote-cryptsetup.target has
WantedBy=multi-user.target, and
systemd installs a symlink to it in initrd-root-device.target.wants/.
is-enabled now exits 0 for these units where it used to exit 1, which
packaging
scripts may notice.
Disable can't remove the vendor symlinks, since /usr/ is often read-only
and an
image update would bring them back. So we mask them with a symlink to
/dev/null
in /etc/ instead, and enable removes those masks again. We don't mask
where
something else already exists in /etc/, since it already shadows the
vendor
symlink. The changes list gains "mask-dependency" and
"unmask-dependency", so
that callers can tell a mask from an unlink.
Disable also stops removing masks. Today, disabling a template removes
the
masks of its instances, which unmasks them.
A preset policy of "disable" leaves vendor enablement alone. Masking it
would
stop units from starting at boot on every system whose preset files
don't name
the units the vendor enabled. #43319 builds on this and adds systemctl
--vendor, which applies presets to /usr/ at image build time.
Fixes: #43908
`systemd-firstboot --image=` customizes an image by writing into its
/etc. That doesn't work for images whose root file system is signed,
which is exactly the kind of image you'd want to hand out and customize
per deployment. Everything firstboot configures is also picked up as
credentials at boot though, so the settings can just as well travel in a
systemd-stub addon on the ESP, which isn't covered by the signature.
This adds `--addon=NAME` for that. Instead of mounting the image, the
settings are turned into the credentials of the same meaning (plus
`system.machine_id` for the machine ID), packed into a cpio in the
`.initrd` section of a addon, a kernel command line goes into
`.cmdline`, and the result ends up as `loader/addons/NAME.addon.efi` in
the ESP. The image is never mounted for that, the partition table is
dissected to find the ESP and mtools does the writing.
`--key=[NAME=]PATH` adds custom keys, say a certificate for verifying
extension images, as `keyring.NAME` credentials. The name on the
consuming side isn't settled yet, so treat that as a placeholder.
The addon is unsigned, whether the stub picks it up is up to the Secure
Boot policy, see #44362 for the stub side of this.
Follow-up for 6cadc7f86b.
Cover the -ENOTCONN path added for sd_bus_negotiate_timestamp() and
sd_bus_negotiate_creds(). This would have caught a regression to the
old assert_return() abort.
The restriction tests don't cover unnamed files or whiteout creation.
Let's check that O_TMPFILE and RENAME_WHITEOUT follow the named-file
restrictions on host, private and idmapped file systems, including
O_TMPFILE with O_EXCL.
Let's probe O_TMPFILE support before checking the host directory and use
link_fd() so the positive link checks also work on kernels before 6.10.
renameat2() with RENAME_WHITEOUT leaves a whiteout in place of the
source. That's a character device owned by the caller's fsuid and fsgid
which the file system creates from its ->rename() method, so it never
passes the path_mknod hook. Since Linux 5.8 this doesn't require any
privileges either.
So a managed user namespace that can rename an entry on a file system
that outlives it, e.g., in a world-writable directory the user prepared
from the outside, can leave a whiteout owned by its transient range
behind.
The only hook that sees this is path_rename, which gets the flags.
Refuse RENAME_WHITEOUT there whenever validate_mount() would refuse a
mknod in the source directory.
O_TMPFILE creates an inode without calling the path creation hooks, so a
managed user namespace can leave a file owned by its transient range on a
file system that outlives it. O_EXCL prevents linking the inode, but a host
process can reopen it through /proc/self/fd without retaining credentials
that keep the creator's user namespace alive.
Let's apply the named-file restrictions from the file_open hook to every
O_TMPFILE, including O_EXCL, so an unnamed file can't outlive its IDs.
IOUringFilter= takes the names of io_uring operations, and
SystemCallFilter= denies an operation if it denies one of the system calls
that the operation can do the work of. Users can only find the operation
names and the system calls of each operation in the source code.
Let's add "systemd-analyze io-uring-filter", which lists the operations
that systemd knows, together with the system calls of each operation,
like "systemd-analyze syscall-filter" does for the system call groups.
SystemCallFilter= can allow or deny io_uring as a whole, but not
individual io_uring operations. It only denies an operation together with
the system calls that the operation can do the work of. Some operations,
such as IORING_OP_URING_CMD, which passes commands to drivers, or the
zero-copy network operations, can thus only be denied together with
ioctl() or the socket calls.
Let's add IOUringFilter=, which takes an allow or deny list of io_uring
operations, like SystemCallFilter= does for system calls. On kernels
without io_uring BPF filters, we can't enforce IOUringFilter=, so we deny
io_uring_setup() with ENOSYS instead. Programs fall back to regular
system calls when io_uring_setup() fails with ENOSYS.
Many io_uring operations can do the work of a system call:
IORING_OP_OPENAT opens a file like openat(), IORING_OP_SOCKET creates a
socket like socket(), and so on. SystemCallFilter= doesn't apply to
io_uring operations. A service that can use io_uring can thus do things
that its system call filter denies.
On kernels with io_uring BPF filters, let's map each operation to the
system calls that it can do the work of, and deny the operation if the
filter denies any of them. IORING_OP_URING_CMD maps to ioctl() and also to
setsockopt(), getsockopt(), and getsockname(), because it runs those
socket calls on sockets.
We don't deny operations that we don't know about, even for an allow list.
A denied request fails with EACCES, while the kernel keeps reporting the
operation as supported, so programs couldn't fall back. Our seccomp allow
lists return ENOSYS for unknown system calls for the same reason.
We skip system calls that are denied with an error number of 0. A denied
io_uring request fails with EACCES and can't pretend to succeed. We also
don't register any io_uring filters if the system call filter denies
io_uring_setup(), because the service can't create rings then.
RestrictSUIDSGID= uses seccomp to check the mode argument of open(),
openat(), and creat(). The IORING_OP_OPENAT and IORING_OP_OPENAT2
operations of io_uring don't go through those system calls, so they can
still create SUID and SGID files.
On kernels with io_uring BPF filters, let's refuse IORING_OP_OPENAT and
IORING_OP_OPENAT2 requests that create a file with the SUID or SGID bit
set. The io_uring filter can read the flags and mode of IORING_OP_OPENAT2,
so we don't have to deny IORING_OP_OPENAT2 entirely, like we do for
openat2().
RestrictAddressFamilies= only filters the socket() system call. A service
can still create sockets of any address family with the IORING_OP_SOCKET
operation of io_uring, and the man page says so.
On kernels with io_uring BPF filters, let's register a filter that checks
the address family of IORING_OP_SOCKET requests against the same list.
Unlike the seccomp filter, the io_uring filter works on all architectures,
including the ones that create sockets with socketcall().
Once a task has io_uring filters, the kernel refuses task-wide
IORING_REGISTER_RESTRICTIONS for it with EPERM. The service can still
restrict its own io_uring instances and add more BPF filters. We document
the EPERM in the man page.
Since Linux 7.0, a task can register classic BPF filters for io_uring
opcodes with io_uring_register(), and every ring that the task or its
children create afterwards inherits the filters. Let's add helpers that
deny opcodes, restrict the address families of IORING_OP_SOCKET, and
refuse IORING_OP_OPENAT and IORING_OP_OPENAT2 requests that create SUID or
SGID files. We'll use them to apply the sandboxing settings of services to
io_uring.
An allow list of opcodes also denies the opcodes that the kernel
implements but we don't know about. We register a deny filter for each
unknown opcode rather than use IO_URING_BPF_FILTER_DENY_REST. That flag
only applies to opcodes without a filter. A filter that the service
manager inherited would otherwise exempt its opcode from the allow list.
We probe for support in a child process. A nested service manager can run
under a seccomp filter that kills the process on io_uring_register(). A
probe in the executor itself would then kill the executor, and the
service would fail to start. We treat a killed child as missing support.
The child marks itself as not dumpable. The kernel then writes no core
dump when the seccomp filter kills it.
With `systemd.imds.network=locked`, imdsd cannot reach IMDS at all. Our
sysctl.d/50-default.conf enables IPv4 reverse path filtering for all
links. Locked mode adds a "prohibit" route for the IMDS address to the
main table. imdsd's marked sockets use their own table, so its
_requests_ get out. But IMDS's _replies_ carry no mark: the reverse path
filter checks their source against the main table, finds the prohibit
route, and drops them.
To fix this, turn off reverse path filtering on the configured links in
locked mode. The prohibit route still blocks direct access for everybody
else.
Caveat: This does not help where `net.ipv4.conf.all.rp_filter` is on, as
the kernel applies the maximum of that and the link's setting. Ubuntu
does that by default, so for now this only works out of the box on
Debian, Fedora, etc. Cover that case in the tests, and expect a
timeout/failure with `all.rp_filter > 0`.
Fixes#44602
Replace open-coded "Failed to parse ..." log_syntax() calls in config
parser callbacks with the log_syntax_parse_error() /
log_syntax_parse_error_full() helper, which emits a uniform message and
handles the return value and ENOMEM consistently. As the helper already
treats -ENOMEM as critical and routes it through log_oom(), the now
redundant "if (r == -ENOMEM) return log_oom();" guards right before it are
dropped.
Settings that carry secret values (WakeOnLanPassword=, the MACsec key and
the WireGuard keys) log a "(secret)" placeholder instead of the actual
value.
Addresses https://github.com/systemd/systemd/pull/44629#discussion_r4463122734.
Christian Brauner