libnvme, nvme: guarantee terminated discovery log entry strings - #3842
Open
martin-belanger wants to merge 3 commits into
Open
libnvme, nvme: guarantee terminated discovery log entry strings#3842martin-belanger wants to merge 3 commits into
martin-belanger wants to merge 3 commits into
Conversation
added 3 commits
August 14, 2026 14:41
A caller with a fixed-size buffer field, not a char *, cannot use shr_trim()'s leading-whitespace skip: it returns a pointer further into the buffer instead of moving the content, which only works for callers that reassign the result to a plain pointer variable. Right-trim alone, done in place, works for both cases. shr_trim() itself is unchanged: it still trims both ends and returns a pointer to the first non-whitespace character, now composed from the two new primitives instead of its own duplicate logic. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
Discovery Log Page Entry fields (trsvcid, subnqn, traddr) are
fixed-size char arrays with no guaranteed NUL terminator on the wire.
A non-compliant or malicious discovery controller that returns one
fully packed can trigger an out-of-bounds read wherever the field is
later passed to strlen()/strdup()/printf("%s"). This is part of
hardening the discovery log parsing path so invalid data from a
remote peer cannot crash the code.
Signed-off-by: Martin Belanger <martin.belanger@dell.com>
The discovery_log hook receives the log page exactly as fetched, raw and unsanitized. trsvcid/traddr are raw buffers that are space padded and don't have a NUL-terminating character. nvme_show_discovery_log()'s stdout and JSON backends read that raw trsvcid/traddr with strlen()-based string functions and no defense against a missing NUL. This rarely showed up as a visible crash because the bytes right after them (reserved fields, the TSAS union) are normally zero, so a read past the field usually finds a NUL nearby instead of running away. Each backend now defends itself instead of relying on sanitization elsewhere in the discovery log parsing path. subnqn is zero-padded and doesn't need this, but gets the same treatment anyway, as a belt-and-suspenders measure. The discovery_log hook's log parameter is now a pointer to const, so the compiler enforces that a hook must never modify the log page it was handed. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This is something I noticed while working on #3837.
Discovery Log Page Entry fields (
trsvcid,subnqn,traddr) are fixed-size char arrays copied off the wire with nothing guaranteeing a NUL terminator --trsvcid/traddrare normally space-padded per spec, not NUL-padded. Every consumer (strlen(),strdup(),printf("%s"),json_object_new_string()) assumes a real C string anyway. Usually harmless (the bytes right after a field are normally zero), but a DC that doesn't zero its reserved bytes can trigger a real out-of-bounds read, most concretely on the last entry'straddr. Not new -- traceable back to beforesanitize_discovery_log_entry()even existed.shared: split shr_trim() into shr_rtrim() and shr_ltrim()-- needed to sanitizetrsvcid,subnqn,traddr.libnvme: guarantee terminated discovery log entry strings-- force-NUL-terminate +shr_rtrim()each field, replacing the oldstrchomp().nvme: harden discovery log print paths against unterminated fields-- thediscovery_loghook has always handed over the raw, unsanitized log page (required fornvme discover --rawto stay truly raw), so the stdout/JSON backends were never defended against missing NULL.logis nowconstthroughout the hook chain to make sure the hook/callback cannot modify the log page.