Repository navigation
plugins: bound the string buffers - #4219
Merged
Merged
Conversation
The plugins build strings with strcpy(), strcat() and sprintf() into fixed buffers, which overflow if the input is longer than expected. Add shr_strscpy() and shr_strscat(), which copy or append a string and cut it off at the size of the buffer, always terminated, and report the cut with -E2BIG, and shr_snprintf_append(), which appends formatted text the same way and keeps track of the length, like the buf_add() of argconfig. Signed-off-by: Daniel Wagner <dwagner@suse.de>
buf_add() moved to shared as shr_snprintf_append(). The help output is unchanged. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Several plugins print a GUID or another byte array as hex with a loop of sprintf(p, "%02x") into a buffer whose size the loop never checks. Add shr_bin_to_hex(), which writes the bytes as hex digits in lower or upper case and fails with -E2BIG if the buffer is too small. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use snprintf(), shr_strscpy() and shr_bin_to_hex() instead of sprintf() and strncpy(), which don't check the size of the buffer or leave it unterminated. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use snprintf() and shr_strscpy() instead of sprintf() and strcpy(), and pass getcwd() the size of the buffer instead of a number. Signed-off-by: Daniel Wagner <dwagner@suse.de>
setup_file() wrote the file name with sprintf() into a buffer whose size it didn't know. Pass the size and use snprintf(). Signed-off-by: Daniel Wagner <dwagner@suse.de>
The EUI-64 and NGUID were written byte by byte with sprintf() into a buffer whose size the loop didn't check. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The helpers wrote the NGUID, the sizes and the device name with sprintf() and strncpy() into buffers whose size they didn't know, strncpy() possibly unterminated. Give the buffers a defined length and use shr_bin_to_hex(), snprintf() and shr_strscpy(). Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use snprintf(), shr_strscpy() and shr_bin_to_hex() instead of sprintf(), strcpy() and strcat(): the GUIDs and the debug log size are written with shr_bin_to_hex(), the FIFO strings are copied with memcpy() and the reserved fields are appended with shr_snprintf_append(), each into a buffer whose size is checked. The output is unchanged. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use snprintf() instead of sprintf(), and print the names of the 64 bit attributes directly instead of copying them into a small buffer first. The output is unchanged. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use snprintf() instead of sprintf() for the log page and drive info strings; micron_readFixedBytesFromBuffer() takes the size of the datastr of hyperscale_BSSD_nand_stats, which all its callers pass. Copy the fixed size identify fields and the file path prefix with memcpy() of their size instead of strncpy() with a number. The output is unchanged. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use snprintf(), shr_strscpy() and shr_strscat() instead of sprintf(), strcpy(), strncpy() and strcat(), and pass wdc_get_commit_action_bin() the size of the buffer. The 128-bit GUIDs are read with shr_get_unaligned_le64() instead of an unaligned load. stringify_log_page_guid() printed each byte of the C0 log page GUID with "%x", which dropped the leading zero of the bytes below 0x10, so the printed GUID was too short and ambiguous. It prints two digits per byte with shr_bin_to_hex() now. The other output is unchanged. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use snprintf(), shr_strscpy() and shr_strscat() instead of sprintf(), strcpy(), strncpy() and strcat(), and pass sndk_get_commit_action_bin() the size of the buffer. The 128-bit GUID of the extended SMART log is read with shr_get_unaligned_le64() instead of an unaligned load. sndk_stringify_log_page_guid() printed each byte of the C0 log page GUID with "%x", which dropped the leading zero of the bytes below 0x10. It prints two digits per byte with shr_bin_to_hex() now. The other output is unchanged. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use snprintf() instead of sprintf() for the file path and the request message. Signed-off-by: Daniel Wagner <dwagner@suse.de>
Use snprintf() with the path formats as literals, which the compiler checks, instead of sprintf() with formats in variables. Signed-off-by: Daniel Wagner <dwagner@suse.de>
The latency histogram units are only ever string literals: point to them instead of copying them with strcpy() into three byte buffers. Copy the firmware version with shr_strscpy(). Signed-off-by: Daniel Wagner <dwagner@suse.de>
The status command copied the controller name with strcpy() into an 8 byte buffer, which a name like nvme10n1 already overflows. It read the PCIe AER files with read() into a 512 byte buffer without terminating it, so sscanf() could read past the end, and closed the AER file a second time when the fatal error count couldn't be parsed. Make the name buffer large enough and copy it with shr_strscpy(), read the AER files with shr_read_all(), which terminates the buffer, drop the second close(), and copy the other strings with shr_strscpy() instead of strcpy() and strncpy(). Signed-off-by: Daniel Wagner <dwagner@suse.de>
The smart log and header lines were built with strcpy() and strcat() into the caller's buffer without its size, through a temporary buffer, and the file name with strcat() into a static buffer. The hex dump helper didn't know the size of its output either. Pass the sizes and append with shr_snprintf_append(), write the hex dumps with shr_bin_to_hex(), build the file name with one snprintf() and copy the header strings with shr_strscpy(). The serial number and model number are fixed size fields without terminator: print them with their size instead of %s, which read past a field which is full. Signed-off-by: Daniel Wagner <dwagner@suse.de>
workload-tracker appended the names of the trigger fields with strcat() to field_options, which was never initialized, so the list in the error message started with whatever was on the stack and could overflow the buffer. Start the list with shr_strscpy() and append with shr_strscat(), bounded by the size of the buffer, in join_fields() and join_options(). Signed-off-by: Daniel Wagner <dwagner@suse.de>
remove read the number of the entry with sscanf("%zu"), which takes
"3abc" as 3 and a negative number as a huge one, so a typo could delete
an entry.
Parse the answer with shr_parse_u64() up to the number of entries,
whitespace around it trimmed: anything else cancels.
Signed-off-by: Daniel Wagner <dwagner@suse.de>
The Controller and Namespace Metadata features are printed element by element. The value was copied with strncpy() into an uninitialized buffer, which left it unterminated, so the output included whatever was on the stack. The next element was found with the length field without le16_to_cpu(), and nothing stopped the walk at the end of the 4096 byte structure, so a large length made it read and print past the buffer. Stop at the end of the structure, cut the length of a value off there and print it with its length, in the normal and the JSON output. Signed-off-by: Daniel Wagner <dwagner@suse.de>
dwsuse
force-pushed
the
plugin-string-audit
branch
from
October 17, 2026 14:51
98ad51c to
020e510
Compare
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
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.
An audit of
sprintf(),strcpy(),strcat(),strncpy()andsscanf()in the plugins. All of the first four are gone fromplugins/; the remainingsscanf()calls only match device names (nvme%dn%d) or skip with%*s, so they write no strings.New helpers in
shared/first:shr_strscpy(),shr_strscat(): copy/append, always terminated,-E2BIGon truncationshr_snprintf_append(): append formatted text and track the length (argconfig'sbuf_add(), which now uses it)shr_bin_to_hex(): bytes as hex,SHR_HEX_UPPER/SHR_HEX_REVERSE(little-endian GUIDs)Then one commit per plugin. Most of it is
snprintf(buf, sizeof(buf), ...)and the helpers instead of unbounded calls. Real bugs found on the way:strcpy()ed into an 8-byte buffer (nvme10n1overflows it); the AER sysfs files wereread()into a buffer without a terminator beforesscanf(), and the fatal path closed the fd twice.strcat()ed field names onto an uninitialized buffer.%s, reading past a full field.%x, dropping the leading zero of bytes below 0x10. It is printed with two digits per byte now (a visible output change, only where it was wrong).src/nvme-print-*): the element value wasstrncpy()ed into an uninitialized buffer and printed unterminated, the next element was found withoutle16_to_cpu(), and a large element length walked past the 4096-byte structure. The walk is bounded now, in the normal and JSON output; new testhost-metadata-cli.removeread the menu choice withsscanf("%zu"), so3abcdeleted entry 3. It is parsed withshr_parse_u64()now.Output is otherwise unchanged: I replayed 6450 generated device answers through all 129 commands of the fuzz-device harness with the old and new binary and the output was identical, apart from one
get-feature --verbosecase whose output differed between two runs of the old binary: that is the metadata walk fixed in the last commit, and it is deterministic now. The help of all commands is byte-identical.Every commit builds and passes the tests on its own; checkpatch is clean; nojson and Windows cross builds pass.