Repository navigation
ocp: Fix OCP telemetry timestamp field decode - #1
Closed
jeff-lien-sndk wants to merge 1 commit into
Closed
jeff-lien-sndk wants to merge 1 commit into
jeff-lien-sndk wants to merge 1 commit into
Conversation
Signed-off-by: Jeff Lien <jeff.lien@sandisk.com>
jeff-lien-sndk
requested review from
brandon-paupore-sndk
and
a lite review from Copilot
October 12, 2026 20:57
There was a problem hiding this comment.
🟡 Changes recommended
Add regression coverage for non-zero timestamp attribute/reserved bytes; also fix the comment typo.
2 open findings
What changed in this PR
Fixes OCP telemetry timestamp decoding by using only the field’s 48-bit timestamp value.
Changes:
- Uses
struct nvme_timestampandint48_to_long(). - Adds integer utility support and field-layout comments.
| File | Description |
|---|---|
src/field-parser.c |
Corrects OCP telemetry timestamp decoding. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| struct nvme_timestamp *ts = | ||
| (struct nvme_timestamp *)&buf[offset]; | ||
|
|
||
| shr_format_ts(int48_to_long(ts->timestamp), ts_buf); |
| /* | ||
| * TELDF-4 is defined by OCP to hold the NVMe Timestamp | ||
| * (Feature Identifier 0Eh) get features structure, not a | ||
| * flat 64-bit value; reintrepret it as such rather than |
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
The printers of the Namespace Identification Descriptor list walk it
without checking that a descriptor fits into the 4 KiB page. A
descriptor at its end makes them read past the buffer:
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 16
#1 stdout_id_ns_descs nvme-print-stdout-id.c:2181
They also copy the size of the identifier of its type whatever NIDL
says, and step over a CSI descriptor by adding its length to the one of
the previous descriptor, so the descriptor behind it is missed.
Take a descriptor only if its header and NIDL bytes fit into the page,
step over it by its NIDL as the spec says, and skip a descriptor of a
known type with another length as one of an unknown type. The output
for a list as the spec defines it is the same, except for a descriptor
behind a CSI descriptor, which is now printed.
Fixes: 84937e8 ("nvme-cli: add ns-descs subcommand")
Fixes: 7f8d194 ("Added new namespace list command on list-ns, 4056 modification added")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
wdc_do_id_ctrl() copies the 24 bytes of the serial number from the
vendor specific area of Identify Controller into a 24 byte buffer and
prints it as a string. A serial number which uses all 24 bytes is not
terminated, and strlen() reads past the buffer:
ERROR: AddressSanitizer: stack-buffer-overflow
#1 wdc_do_id_ctrl wdc-nvme.c
Make the buffer a byte longer.
Fixes: 5902e8a ("nvme-cli: Western Digital/HGST plug-in.")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
intel_id_ctrl() copies the health, bl, mic_bl and mic_fw fields of the
vendor specific area with %s. A field without a NUL character makes
snprintf() read on, past the end of the Identify Controller data:
SUMMARY: AddressSanitizer: heap-buffer-overflow
#1 snprintf
#2 intel_id_ctrl intel-nvme.c:110
Bound each string by the size of its field.
Fixes: 74d5070 ("Fixing Empty Fields of Intel VU id-ctrl")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
try_read_ana() moves past the NSIDs of an ANA group by the NNSIDS of the device times 4, and only then checks the end of the buffer. With a 32 bit size_t the product and the pointer wrap around: to_read lands inside the buffer again, nothing is read, and the next group descriptor is read through a wild pointer: Program received signal SIGSEGV, Segmentation fault. #0 libnvme_get_ana_log_atomic () from libnvme3.so.1 #1 get_ana_log () Check that the NSIDs fit the buffer before moving to_read, and fail with ENOSPC like for any other log larger than the buffer. The fuzz suite found it on i386 in CI. The ioctl tests do not run in the cross builds, so add the input to the seeds of the device target too. Fixes: 25aad27 ("ioctl: add support for atomic, piecewise ANA log fetch") Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
The firmware activation history log has room for 20 entries, but the
printers walk as many as the valid entries field of the device says,
and read past the log:
ocp-print-json.c:161:54: runtime error: index 21 out of bounds for
type 'const struct fw_activation_history_entry[20]'
#0 json_fw_activation_history ocp-print-json.c:161
#1 ocp_fw_activation_history_log ocp-fw-activation-history.c:78
Stop at the end of the entries.
Fixes: 7bcc2b2 ("plugins/ocp: Add firmware activation history log to OCP plugin")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 22, 2026
The management address of a descriptor is 508 bytes the device fills
in, not a C string. Both printers of the management address list log
use it as one, and read past the log for an address without a NUL
character:
SUMMARY: AddressSanitizer: heap-buffer-overflow
#1 stdout_kv_addv nvme-print-stdout.c:443
linux-nvme#3 stdout_mgmt_addr_list_log nvme-print-stdout-log.c:2930
SUMMARY: AddressSanitizer: heap-buffer-overflow
json_mgmt_addr_list_log nvme-print-json.c:6057
Bound the address by the size of its field.
Fixes: 7c57400 ("nvme: add mgmt-addr-list-log command")
Fixes: 872288d ("nvme-print-json: add json_mgmt_addr_list_log()")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 23, 2026
vt_parse_detail_identify() prints the serial number and the model
number of the Identify Controller data with %s. They are space padded
fields, not C strings: a device which fills them makes printf() read
past the data on the stack:
SUMMARY: AddressSanitizer: stack-buffer-overflow
#1 printf
#2 vt_parse_detail_identify virtium-nvme.c:720
Bound them by their size, like the firmware revision below.
Fixes: fb7f63b ("Add Virtium plugin extension")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 26, 2026
nvme_get_ext_smart_cloud_log() hands its buffer to the caller also
when the Get Log Page fails or the GUID doesn't match, but vs-device-waf
and vs-drive-info free it only on success:
ERROR: LeakSanitizer: detected memory leaks
#1 nvme_get_ext_smart_cloud_log wdc-nvme.c:6563
Free the buffer on failure, and hand it over only on success, which is
when all callers use it. Add the input which found it as a regression
input of the device fuzz target.
Fixes: d7a516e ("wdc: add vs-device-waf command")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
brandon-paupore-sndk
pushed a commit
that referenced
this pull request
Oct 26, 2026
nvme_get_hw_rev_log() hands its buffer to the caller also when the Get
Log Page fails or the GUID doesn't match, but vs-drive-info frees it
only on success:
ERROR: LeakSanitizer: detected memory leaks
#1 nvme_get_hw_rev_log wdc-nvme.c:6622
Free the buffer on failure, and hand it over only on success, as for
the extended SMART cloud log. Add the input which found it as a
regression input of the device fuzz target.
Fixes: 66ae0df ("wdc: Plugin changes to support new SN810-2 drive")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
jeff-lien-sndk
pushed a commit
that referenced
this pull request
Oct 27, 2026
An NBFT string is the bytes its heap object counts and the NUL after
them. __get_heap_obj() checks that NUL by scanning one byte past the
object, which may be past the table when the object ends it:
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 38, 0 bytes after 1805-byte region
#0 strnlen
#1 __get_heap_obj nbft.c:144
Scan within the table only, and refuse a string without its NUL in it as
not zero terminated. The fuzz target of nbft show found it.
Fixes: 28ad0e9 ("nbft: add NBFT v1.0 table support")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
jeff-lien-sndk
pushed a commit
that referenced
this pull request
Oct 27, 2026
A Discovery Descriptor may leave out the NQN of its Discovery
Controller, meaning the well-known Discovery NQN, and libnvme then
leaves it NULL since "libnvme: use a Discovery Descriptor's own NQN
when specified". The JSON output of nbft show checks for that, but the
normal output takes its length and prints it:
SIGSEGV in strlen
#1 print_nbft_discovery_info nbft-plugin.c:534
Print an empty NQN, as the JSON output leaves it out. The fuzz target of
nbft show found it.
Fixes: b7388c9 ("libnvme: use a Discovery Descriptor's own NQN when specified")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
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
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 PR will fix the problem described in OSS-182. The decoded timestamp value is incorrect because bytes 6 and 7 of the timestamp field used for timestamp attributes are not masked off.