Newly allocated new_res variable was left unreferenced if memory
allocation failed in ndisc_option_add_encrypted_dns_internal().
Co-authored-by: Siteshwar Vashisht <svashisht@redhat.com>
`sd_dhcp_server_configure_pool()` calculated the maximum pool size
before checking whether `PoolOffset=` was inside the subnet.
Because the calculation used unsigned arithmetic, an offset beyond the
broadcast address could wrap around to a large value and pass validation.
For example, `PoolOffset=256` with a `/24` and `PoolSize=1` was accepted,
allowing the DHCP server to offer an address outside its configured subnet.
Reject offsets at or beyond the broadcast address before performing the
subtraction, and calculate the remaining pool size directly as
`broadcast_off - offset`.
According to RFC specifications
```
RFC 1542 section 4.1.1 states:
The relay agent MUST silently discard BOOTREQUEST messages whose 'hops'
field exceeds the value 16."
```
"Exceeds the value 16" means hops > 16, i.e. a message that arrives with
hops == 16 is still valid and must be relayed (after which its hops field
becomes 17). The code used ">= 16", which silently dropped a valid message
that had legitimately traversed exactly 16 relay agents, one hop too early.
This matches the wording of the adjacent comment, which already says
"exceeds the value 16".
NDisc packets received through the socket path are filtered before
parser dispatch, but parser entry points should still reject malformed
packet bytes instead of asserting on them. Return EBADMSG for non-zero
ICMPv6 code values in RA, NA, and Redirect messages.
Follow-up for c34cb1d645
Follow-up for 696eb2b8de
Follow-up for 44e8cf303b
We have two concepts: a list of files that should be extracted from an
intermediate build artifact to be used in a later build artifact, and for a
given artifact, a list "donors". The first list was called "extract", because
it is passed to the .extract_objects() meson function, and the second was
called "objects", because that is the meson parameter to specify pre-built
objects files (6350d2dbd9). But this naming is
confusing: we don't care about the 'extract' step, this is something internal
to the build machinery. And 'objects' is a very generic term.
Let's use 'export' for the stuff that is "exported" for other binaries to use,
and 'import' to say where to import from. Those terms are symmetrical and the
association between them should be intuitive. (If you think the terms are
actually assymetrical, there are precedents for confusing the import with the
import sources. E.g. in modern English, turkeys are called so because they were
imported from the Americas and guineafowls were imported from Africa via
Türkiye and all that foreign stuff is alike.)
dhcp6_option_parse_ia_pdprefix() validates the lifetimes but never the
prefixlen byte, so a delegated prefix with prefixlen == 0 or > 128 is
stored in the lease and handed over.
RFC 8415 defines the prefix length as 1 to 128, and the send-side
option_append_pd_prefix() already rejects 0, so reject the out-of-range
values on the receive path too.
Follow-up for f8ad4dd45d
If the user callback set via sd_lldp_rx_set_callback() drops the last
reference to the sd_lldp_rx object, trying to use it later does not go
well. Take a ref to keep the objects alive as long as they are needed.
dhcp_option_type_from_code() returns _DHCP_OPTION_TYPE_INVALID (-EINVAL)
for the PAD and END option codes, and dump_dhcp_option_one() uses the
returned value directly as an index into the functions[] table. Those
codes are excluded by an assert() at the top of the function, but
assert() compiles down to __builtin_unreachable() under NDEBUG, so a
negative array index read is reachable there (and trips static
analyzers). Bail out explicitly on the error return.
CID#1660105
Follow-up for 149adb2fdc
Four *_attach_event helpers swallow sd_event_default() failures and
return success while leaving obj->event NULL:
sd_dhcp_client_attach_event (sd-dhcp-client.c)
sd_dhcp6_client_attach_event (sd-dhcp6-client.c)
sd_ndisc_attach_event (sd-ndisc.c)
sd_radv_attach_event (sd-radv.c)
Each contains the same copy-pasted typo in the NULL-event branch:
r = sd_event_default(&obj->event);
if (r < 0)
return 0; /* swallows -ENOMEM / -ECHILD */
The caller is told the attach succeeded, but obj->event is still
NULL. A subsequent *_start() then trips assert_return(obj->event,
-EINVAL) and returns a spurious -EINVAL (or aborts on assert-abort
builds), and any consumer of *_get_event() dereferences NULL.
The sibling helper sd_dhcp_server_attach_event already uses the
correct pattern:
r = sd_event_default(&server->event);
if (r < 0)
return r;
Fix by returning r instead of 0 at each of the four sites so the
sd_event_default() errno is propagated to the caller and ownership
stays with the caller.
Assisted-by: kres (claude-opus-4-7)
Signed-off-by: Chris Mason <clm@meta.com>
sd_dhcp6_client_add_option() and sd_dhcp6_client_add_vendor_option()
store the caller's sd_dhcp6_option in an ordered container whose value
destructor is sd_dhcp6_option_unref, registered via
DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR in dhcp6_option_hash_ops. The
container therefore owns exactly one ref per stored slot.
Both helpers guarded the ref bump only on r < 0:
r = ordered_{hashmap,set}_ensure_put(..., v);
if (r < 0)
return r;
sd_dhcp6_option_ref(v);
ordered_hashmap_ensure_put() (and the ordered_set wrapper) forward the
underlying hashmap_put_boldly tri-state verbatim: 1 on fresh insert, 0
when the identical key+value pair is already stored (no new slot
allocated), <0 on error. The r == 0 path fell through and bumped the
refcount without a corresponding slot.
The extra ref is permanently stranded once the container's destructor
runs. In a long-running networkd that reapplies .network files on
reload this leaks one sd_dhcp6_option allocation per duplicate-add.
Fix by tightening the guard to r <= 0 in both helpers, so
sd_dhcp6_option_ref(v) is reached only on a genuine new slot (r == 1).
Propagate the tri-valued result by returning r from both helpers
instead of a hard-coded constant.
sd_dhcp6_client_add_vendor_option() is a public libsystemd sd_ API;
its success return changes from a constant 1 to {0, 1}.
sd_dhcp6_client_add_option() now also propagates the tri-valued
result (previously a hard-coded 0 on success), giving the two parallel
helpers identical return contracts.
The sole non-fuzz in-tree callers are both in dhcp6_configure()
(src/network/networkd-dhcp6.c), which treat any r >= 0 as success and
do not distinguish 0 from 1, so the contract widening is caller-safe.
Fixes: e7d5fe17db ("DHCP client: make SendOption work for DHCPv6 too.")
Assisted-by: kres (claude-opus-4-7)
Signed-off-by: Chris Mason <clm@meta.com>
Previously, DHCPNAK was sent only when the client is in INIT-REBOOT
state. But, on selecting or renewing, the request is directed to a
specific server, so we can safely reply with DHCPNAK.
Also, verify existing bound lease even when there is no static lease for
the client.
This also makes the conditions in dhcp_server_send_message() uses
the message that will be sent, rather than we received.
This does not change basic functionality, but changes/fixes several
minor behaviors, e.g.
- fix when the broadcast flag assignment,
- set server identifier in DHCPFORCERENEW.
This is mostly refactoring. This does not change basic behavior, but
changes/fixes some minor/corner cases, e.g.
- extend the minimum default lease time from 1 second to 30 seconds, as
1 second is too short and causes the network unstable (though 30
seconds is stll too short, but hopefully that does not make the
network unstable).
- error code on broken/malicious message received may be changed.
Otherwise, we may do something wrong by messages for another DHCP server.
Let's silently ignore messages with unmatching server identifier.
Also, logs something when we receive DHCPRELEASE but found lease does
not match the reported address.
This makes DHCPRequest stores
- the message type of the received message,
- acquired address,
- found static DHCP lease,
This also moves call of dhcp_request_get_lifetime_timestamp() from
dhcp_server_ack() to dhcp_server_set_lease(), and rename
DHCPRequest.server_id -> .server_address.
No functional change, just refactoring.
Then, this drops garbage in DHCP server lease in DBus and Varlink message.
This also drops fallback to use client ID as hardware address when chaddr
field is not set. In that case, we should broadcast reply.
This makes
- UDP socket fd is owned by IO event source,
- open RAW socket fd just before sending first packet,
- set TOS and socket priority,
- use AF_UNIX soxket pair in the unit test and fuzzer, so the unit test
can now run by unprivileged user.
Previously, sd-dhcp-server can be run as a DHCP relay agent.
But, DHCP server and DHCP relay agent behave completely differently,
hence there is almost no code that can be shared between the two modes.
Let's split out the DHCP relay agent feature from sd-dhcp-server.
The new DHCP relay agent supports:
- multiple upstream and downstream interfaces,
- gateway address (giaddr field in DHCP message header) is configurable,
- supports more DHCP relay agent information sub-options,
- each interface has their own socket fd, and each socket is bound to
the interface, so that we can enable/disable each interface
safely without affecting other interfaces, and we can filter out any
unexpected packets from unmanaged interfaces,
networkd integration and test cases will be added later.
Drop the -fundamental suffix from src/fundamental/ headers in favor of names
that match their src/basic/ or src/shared/ counterparts (e.g.
macro-fundamental.h -> macro.h, assert-fundamental.h -> assert-util.h,
cleanup-fundamental.h -> cleanup-util.h). Rename src/basic/{btrfs,label}.{c,h}
to use the -util suffix to match the existing shared/btrfs-util and
shared/label-util siblings. Rename src/shared/mkdir-label.{c,h} to mkdir.{c,h}
and src/shared/tmpfile-util-label.{c,h} to tmpfile-util.{c,h} to match the
corresponding src/basic names.
This saves us from having to come up with separate names for files that do
the same thing across tiers, and it makes it easier to move stuff between
src/fundamental, src/basic and src/shared: consumers just #include "foo.h"
and pick up whichever tier their -I path resolves to first, so call sites
don't need to be updated when an API moves between layers.
Where a higher-tier wrapper exists (e.g. src/basic/macro.h wrapping
src/fundamental/macro.h), the wrapper uses an explicit "../fundamental/foo.h"
or "../basic/foo.h" relative include for the lower-tier header. We can't use
GCC's #include_next directive for this — when the wrapper is reachable both
via same-dir-as-source lookup and via -I (e.g. -Isrc/shared) for the
directory it lives in, #include_next advances by exactly one slot in libcpp's
internal directory chain and lands on the same physical directory it was
already in, never reaching the lower-tier sibling (see make_cpp_dir() in
gcc/libcpp/files.cc:1986).
To make sure the right headers are always picked up, the include directories
are reordered so that e.g. src/shared always takes priority over src/basic and
similar for the other directories.
Co-developed-by: Claude Opus 4.7 <noreply@anthropic.com>
There exists a small window of time in icmp6_bind() between creating the
ICMPv6 socket and binding it to an ifindex, where the link-scoped socket
can process an ICMPv6 packet received on any interface. The applies to
both sd-radv and sd-ndisc codepaths.
This change adds an explicit check for ifindex on the receive path and
ignores packets received on other interfaces.
Co-developed-by: OpenAI Codex <noreply@openai.com>
Siteshwar Vashisht
Yu WatanabeandSiteshwar Vashisht
Anton Tiurin
Ronan Pigott
hanjinpeng
Luca Boccassi
Zbigniew Jędrzejewski-Szmek
Lennart Poettering
Chris Mason
Daan De Meyer
Patrick Rohr