Fix and consolidate CMSG_* implementations #4903
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change makes sure that the header of `next` is within max, returning null if not. This is similar to how `glibc` does it. No checks were previously being done to assert that `next as usize + size_of::<cmsghdr>() < max`. Wrapping offset calculations could then lead to buffer over-reads in the following `(*next).cmsg_len`. [glibc ref](https://github.com/bminor/glibc/blob/b71d59074b98ad4abd23c136ec9ad4c26e29ee6d/sysdeps/unix/sysv/linux/cmsg_nxthdr.c#L49-L51)
Likely written to make assertions in the unsound CMSG_NXTHDR implementations introduced in rust-lang#1235. CMSG_NXTHDR(mhdr, current_cmsghdr) should not be concerned with the value next_cmsghdr.cmsg_len, which the previous implementation did.
Setting `(*pcmsghdr).cmsg_len = cmsg_len as _;` when cmsg_len ranges from 0 to 64 is invalid as it must always be `>= size_of::<cmsghdr>()`, rounded up to the nearest alignment boundary. Some implementations (notably glbic) do check that `cmsg_len >= size_of::<cmsghdr>()` in `CMSG_NXTHDR`, returning null if so. But this is more so an extra precaution that is not mentioned in the POSIX 1003.1-2024. It can therefore not be relied on for tests executed on multiple platforms. The change also removes the ignoring of some testvalues when targeting AIX.
gibbz00
force-pushed
the
consolidate_cmsg
branch
6 times, most recently
from
6e31ebc to
9b838e1
Compare
gibbz00
force-pushed
the
consolidate_cmsg
branch
11 times, most recently
from
d7e8e08 to
08f764e
Compare
The original intent was to simplify the reasoning for how they differ across platforms. It should as a consequence also make it easier to extend target support. Changes: - CMSG_* functions which can be const are all marked const. - Removes unsafe from `CMSG_ALIGN`, `CMSG_SPACE`, `CMSG_LEN`. Fixes: Properly reflect that musl and some descendants do not allow zero-sized payloads being in `CMSG_NXTHDR`. Custom CMSG_FIRSTHDR implementation for VxWorks had missed to check that `mhdr.msg_controllen >= size_of::<cmsghdr>()`, as per POSIX 1003.1-2024. Documents: - Usage and safety requirements. - Adds missing references to upstream headers. [musl_nxthdr]: https://www.openwall.com/lists/musl/2025/12/29/1
gibbz00
force-pushed
the
consolidate_cmsg
branch
from
08f764e to
ded01b4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apologies for this PR being a bit large 🥲 Just let me know if you want me to split it up.
The original intent was to simplify the reasoning for how they differ across platforms. It should as a consequence also make it easier to extend target support.
Changes:
CMSG_ALIGN,CMSG_SPACE,CMSG_LEN.Fixes:
Properly reflect that musl and some descendants do not allow zero-sized payloads being in
CMSG_NXTHDR, (musl patch).Custom CMSG_FIRSTHDR implementation for VxWorks had missed to check that
mhdr.msg_controllen >= size_of::<cmsghdr>(), as per POSIX 1003.1-2024.Unsoundness in linux and l4re. Makes sure that the header of next is within max, returning null if not. (Similar to how glibc does it.) It is the fix that should have been made Fix cmsg(3) bugs for musl and OSX #1235, but instead it added pretty severe soundness issues. (Mentioning it for context, not to point fingers.) No checks were previously being done to assert that next as usize + size_of::() < max. Wrapping offset calculations could then lead to buffer over-reads in the following (*next).cmsg_len.
CMSG_NXTHDR Testing:
Documents:
Breaking API change Suggestions
Perhaps CMSG_FIRSTHDR can take mhdr by reference? First thing being done is otherwise an raw pointer deref under practically no safety guarantees. Doing so would also remove the need to mark it unsafe.
Same reasoning can be applied to mhdr in CMSG_NXTHDR, difference being that the function must remain unsafe for dereferencing cmsg.
Footnotes
Except
CMSG_DATAon solarish. It doesalign(cmsg_addr + size_of(cmsg))rather thancmsg_addr + align(size_of(cmsg>)). The others can get away with not casting the cmsg pointer to usize by instead casting cmsg to a byte pointer and doing the addition usingbyte_ptr.offset(). ↩