Ssv 26770 codeql consolidated - #116
Draft
datacore-senthil wants to merge 12 commits into
Draft
Conversation
kmem_vasprintf() allocated size + 1 bytes and, on its error path, freed
the same pointer with size - one byte short of what was allocated - then
returned that freed pointer to the caller regardless:
ptr = kmem_alloc(size + 1, KM_SLEEP);
r = spl_vsnprintf(ptr, size + 1, fmt, ap);
if ((r < 0) || (r > size)) {
kmem_free(ptr, size);
r = -1; /* r is never read again */
}
...
return (ptr);
Three separate faults on one path. The wrong-size free is the serious
one: kmem_free() selects the cache from the size it is given, so a
buffer belonging to kmem_alloc_384 is returned to kmem_alloc_256's free
list. kmem_flags is 0 in every shipping driver configuration, so there
are no buftags, kmem_free() validates nothing, and the buffer is handed
out again later from the wrong cache - silent allocator corruption that
surfaces far from its cause. This is the same failure mode as the
KMERR_BADCACHE panic tracked under SSV-26896, and it has previously
been seen to crash nvlist and ABD teardown long after the bad free.
r is then set to -1 and never read, so the freed ptr is returned, and
the caller frees it a second time.
The companion kmem_asprintf() in the same file is already correct - it
has no free path at all - so only this one was missed.
Fix: because spl_vsnprintf() returns the length the result requires
rather than a truncation flag, one measuring call sizes the buffer
exactly and the write cannot truncate. That removes the retry, the
error branch and the free entirely, so none of the three faults has
anywhere left to live. The INT_MAX guard goes with it: spl_vsnprintf()
caps its own growth at SPL_VSNPRINTF_PROBE_MAX (1 MiB) and returns a
negative past that, so a measurement anywhere near INT_MAX was already
unreachable.
ap is reused for the write without a copy, which is what the existing
code did and is correct here: on x64 va_list is a plain pointer passed
by value, so a callee cannot advance the caller's copy - the same
assumption spl_vsnprintf() documents for its own internal copies.
A negative measurement degrades to an empty string rather than NULL.
kmem_asprintf() was given a NULL return in an earlier commit, but this
function and that one are declared in include/sys/zfs_context.h and
shared with the Linux and FreeBSD ports, where KM_SLEEP cannot fail and
callers do not check - kcf_spi.c:241, spl-kstat.c:576 and
spl-procfs-list.c:234 all use the result directly. Preserving the
never-NULL contract here is a two-line change against auditing every
caller on three platforms. If that trade is decided the other way,
kmem_asprintf()'s NULL return should stay and this should match it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
printBuffer() builds a thread-id prefix in a 1024-byte stack buffer and
appends the caller's formatted message after it. The append was given
the size of the whole buffer for a destination 17 bytes into it:
char buf[max_line_length]; /* 1024 */
_snprintf_s(buf, sizeof (buf), _TRUNCATE, "%p: ", ...);
int tmp = _vsnprintf_s(&buf[17], sizeof (buf), max_line_length,
fmt, args);
&buf[17] has 1007 bytes left, not 1024. A message long enough to fill
the buffer therefore writes 17 bytes past its end. Compiled
x64-Release, buf sits at rsp+0x40, the /GS cookie at rsp+0x440 and the
caller's saved rbx at rsp+0x450, so the overrun lands on the cookie and
the saved register - CWE-121, a genuine stack buffer overflow.
Not reachable today: it needs roughly 1007 characters of output in a
single call, and the messages on the paths with crash dumps are around
370 bytes. It is also fail-loud rather than silent, since /GS validates
the cookie before the epilogue restores rbx, so it would surface as
bugcheck 0xF7 STACK_BUFFER_OVERRUN. Neither makes it safe to keep.
This is not the mechanism behind the KMERR_BADCACHE panic under
SSV-26896 - that one restores a corrupted register with the cookie
intact, which this cannot do - so this commit is not a fix for it.
Worth recording how it survived: the CodeQL mustfix.qls pass changed
the line above it and the line below it (both _snprintf calls) and left
this one alone, and so did the independent remediation on the other
branch. cpp/drivers/extended-deprecated-apis matches banned function
names, and _vsnprintf_s is the recommended name, so it passes the check
no matter what its size argument says. Nothing in the WHCP driver suite
examines buffer arithmetic; cpp/overrunning-write, which targets this
shape, lives in codeql/cpp-queries and was never in scope.
Fix: bound the append by sizeof (buf) - prefix_len, the capacity that
actually remains. Take prefix_len from strlen(buf) rather than the
hardcoded 17, so the two cannot drift apart - "%p: " is 18 characters
on x64, and the literal 17 was already one short of it. Pass _TRUNCATE
as the count for consistency with the neighbouring calls, and test
tmp < 0: _vsnprintf_s returns -1 on truncation, never a value >= the
buffer size, so the old test was dead and its fallback unreachable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The strncpy sweep converted this to spl_strlcpy, which is not the same
operation:
spl_strlcpy(perf->zpoolHealthState, "", sizeof (perf->zpoolHealthState));
strncpy(dst, "", n) writes n bytes - it zero-fills the entire remainder
of the destination once the source is exhausted. strlcpy(dst, "", n)
writes exactly one byte, the terminator, and leaves the other
sizeof (zpoolHealthState) - 1 bytes holding whatever was there before.
The call is a "clear this buffer" idiom, not a string copy, so the
zero-fill was the whole point of it and the conversion silently dropped
it.
That matters here specifically because perf points into the IOCTL
output buffer for IOCTL_ZFS_GET_METRICS: the structure is copied back
to user mode. Leaving all but the first byte uninitialised discloses
whatever the pool allocator last left in that memory. The later
assignment at line 188 only overwrites the field on the is_zpool path,
so the dataset path returns the buffer with just the leading NUL
written.
Fix: use memset(), which is what strncpy was being used for, and which
states the intent plainly. Not converted back to a bounded string copy,
because there is no string to copy - the source is the empty literal.
The sibling strcpy on line 188 is a real string copy and correctly
became a bounded spl_strlcpy in the same sweep; it is left alone.
This is a class of defect, not a single site: any strncpy replaced by
strlcpy loses the zero-fill, and it only matters where the destination
crosses into user space or is otherwise read beyond the terminator. The
sweep touched roughly forty call sites and only this one and
dsl_prop.c's dodefault() have been checked for it so far.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second instance of the class described in the previous commit, in shared (non-Windows-specific) code this time. dodefault() fills a caller-supplied buffer with a property's default value. The original strncpy(buf, zfs_prop_default_string(prop), numints) zero-filled the remainder of buf whenever the default string was shorter than numints, which it almost always is - most defaults are short words like "off", "none" or "on" written into a buffer sized by the caller's numints. Replacing it with spl_strlcpy keeps the bound but drops the fill, so everything past the terminator is left holding whatever the allocation previously contained. buf here is not internal scratch: dodefault() is reached from dsl_prop_get_ds() and dsl_prop_get_dd(), and the result travels back out through the property nvlist to "zfs get". Uninitialised heap bytes past the terminator would be sent to userland with it. Fix: bzero() the buffer before the copy. bzero rather than memset to match the surrounding convention in this file and in the shared ZFS sources generally. The numeric branch below writes a full uint64_t and needs no equivalent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FsRtlAllocatePoolWithQuotaTag() was replaced with a plain pool allocation, which drops two properties the surrounding code depends on. It charges the calling process's pool quota. BufferUserBuffer() snapshots a user-supplied buffer of caller-controlled length into non-paged pool on the METHOD_NEITHER IOCTL path, so without the charge a user-mode caller can drive unbounded non-paged pool allocation with none of it accounted against the requesting process. It also raises an exception on failure rather than returning NULL, which is why the code that follows has no NULL check - there was never a NULL to check. After the swap the allocation can return NULL, and the function goes on to set IRP_DEALLOCATE_BUFFER on the Irp and hand the NULL back to its caller. The RtlCopyMemory is inside a try/except so the copy itself would be caught, but the exception code is discarded, the caller still receives NULL, and the completion path will try to free it. The documented replacement, ExAllocatePool2 with POOL_FLAG_USE_QUOTA, is not usable here: the WDK headers gate it behind NTDDI_VERSION >= NTDDI_WIN10_VB, above this project's WDK_WINVER target of 0x0601. Taking it would raise the driver's minimum supported Windows version tree-wide, which is not a decision this cleanup should be making. Fix: reproduce both effects with the lower-level primitives FsRtlAllocatePoolWithQuotaTag is itself built on - PsChargePoolQuota() before the allocation, ExAllocatePoolUninitialized() for the allocation itself (which is what clears the deprecated-API finding), and on failure PsReturnPoolQuota() followed by ExRaiseStatus(). Callers see exactly the behaviour they saw before. Uninitialized rather than Zero because the RtlCopyMemory immediately below writes all BufferLength bytes, so a zero-fill would be overwritten in full. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fletcher_4_param_get() formats the implementation list into a
PAGE_SIZE buffer, advancing a running offset:
cnt += spl_snprintf(buffer + cnt, PAGE_SIZE - cnt, fmt, ...);
Two problems, both only reachable once the output approaches PAGE_SIZE.
spl_snprintf() returns the length the result required, not the length
it wrote - that is the POSIX contract and what makes it usable for
measuring. On truncation the two differ, so cnt advances past what is
actually in the buffer and past PAGE_SIZE itself.
Once cnt exceeds PAGE_SIZE, buffer + cnt points outside buffer, and
PAGE_SIZE - cnt is a negative int that converts to an enormous size_t
when passed as the size argument - so the very next call is unbounded
and writes off the end of a PAGE_SIZE allocation. The bound defeats
itself precisely when it is needed.
Neither is reachable with today's implementation list, which is well
short of a page. Both become reachable if the list grows, and nothing
in the loop notices when it does.
Fix: keep the return value in a separate len, treat len < 0 or
len >= the remaining room as truncation, and stop there - returning
early for the first call and breaking out of the loop for the rest.
That maintains cnt < PAGE_SIZE as an invariant, so buffer + cnt stays
inside the buffer and PAGE_SIZE - cnt stays positive on every
iteration.
Truncating the listing is the correct outcome here: the caller is a
kernel parameter read, cnt is returned as the byte count, and a short
but well-formed list is preferable to a buffer overrun.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
__dprintf() writes the "file:line:func(): " prefix into buf, then
appends the caller's message immediately after it:
i = snprintf(buf, size, "%s%s:%d:%s(): ", prefix, newfile, line,
func);
roger = spl_vsnprintf(buf + i, size - i, fmt, adx);
i is used as an offset into buf, but snprintf() here is spl_snprintf(),
which returns the length the result *required* rather than the length
it wrote. Those differ exactly when the write truncated, and then i is
larger than the buffer: buf + i points past the end of the allocation,
and size - i is negative, converting to an enormous size_t as
spl_vsnprintf()'s size argument - so the append is unbounded and writes
off the end of the heap allocation.
Not reachable as the code stands, because size was computed as the
prefix length plus the body length plus one, so the prefix always fits
exactly. It becomes reachable the moment that arithmetic and this call
disagree - which is precisely what the earlier off-by-one fix in this
function was correcting.
The hazard is specific to the POSIX return contract spl_vsnprintf()
now provides. Under the previous truncation-returns-negative
convention, i would have gone negative instead, and buf + i would have
been a wild pointer below the allocation - the same defect with the
sign reversed.
Fix: derive i from strlen(buf). That is the real prefix length whether
or not the write truncated, it can never exceed size - 1 because the
buffer is null-terminated within its own bounds, and it therefore
always leaves size - i >= 1 for the append.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two size-accounting defects in the same structure. Both are latent today and both are in the class of the KMERR_BADCACHE panic tracked under SSV-26896 - a kmem_free() whose size does not match its kmem_alloc() - so they are worth closing while the code is open. __zfs_dbgmsg() allocates one block holding the header and the message, and copies into the message field: int size = sizeof (zfs_dbgmsg_t) + strlen(buf); /* 32 + len */ zfs_dbgmsg_t *zdm = kmem_zalloc(size, KM_SLEEP); strlcpy(zdm->zdm_msg, buf, size); zdm_msg does not start at the beginning of that block - it sits at offsetof(zfs_dbgmsg_t, zdm_msg), which is 28 - so the room available there is size - 28, or strlen(buf) + 4. The bound passed was 28 bytes larger than the destination. It does not overrun today only because strlcpy() stops at the source length, writing strlen(buf) + 1 bytes, three inside the real capacity. Nothing states or enforces that margin, and it does not survive a change to the structure layout or to how size is computed. zfs_dbgmsg_fini() then frees with a size recomputed from the stored message rather than the one recorded at allocation: int size = sizeof (zfs_dbgmsg_t) + strlen(zdm->zdm_msg); kmem_free(zdm, size); __zfs_dbgmsg() stores the allocation size in zdm_size for exactly this reason, and zfs_dbgmsg_purge() correctly frees with it - only this one site recomputes. Any truncation in the copy above, or any later edit of zdm_msg, makes the recomputed length smaller than what was allocated, and kmem_free() is handed a size that selects a different cache. The Linux and FreeBSD ports have no equivalent of this loop: both implement zfs_dbgmsg_fini() as a call to zfs_dbgmsg_purge(0), which uses zdm_size. The duplicated loop is a Windows-port divergence, and the divergence is what allowed the two to disagree. Fix: bound the copy by the room at zdm_msg, and free with zdm_size. Collapsing the loop into zfs_dbgmsg_purge(0) to match the other ports would remove the duplication entirely, but that restructures the function and changes its locking, so it is left for a separate change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kmem_asprintf() was given a NULL return for the case where its
spl_vsnprintf() measuring call fails - the new Tier 3 exhaustion path,
reachable when a format needs more than SPL_VSNPRINTF_PROBE_MAX or when
the caller is at DISPATCH_LEVEL. That is an honest signal, but it is a
change of contract, and the contract has callers.
kmem_asprintf() and kmem_vasprintf() are declared in
include/sys/zfs_context.h and implemented separately for Windows, Linux
and FreeBSD. On the other two ports they allocate with KM_SLEEP and
cannot fail, so no caller checks the result:
module/icp/spi/kcf_spi.c:241 ks_name = kmem_asprintf(...)
module/os/linux/spl/spl-kstat.c:576 parent = kmem_asprintf(...)
module/os/linux/spl/spl-procfs-list.c:234 modulestr = kmem_asprintf(...)
lib/libzfs/os/freebsd/libzfs_ioctl_compat.c:279,347,378
Each assigns and dereferences directly. A NULL return here converts a
formatting failure into a null dereference at the call site, which is
worse than the truncated string it replaced - and the call sites are in
code shared with the other ports, where the possibility does not exist
and a reviewer has no reason to look for it.
Fix: treat a failed measurement as a zero-length result, so the buffer
is still allocated and still a valid empty C string. Matches the
handling in kmem_vasprintf(), so the two functions in the same file now
agree, which they did not before.
Hardening every caller on three platforms is the alternative and is the
better long-term answer if a formatting failure ever needs to be
distinguishable. It is out of scope for a deprecated-API cleanup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ator spl_vsnprintf() tries the caller's own buffer, then a stack probe, then the heap. Only the third tier allocates, and the probe was 256 bytes. That is below the size of the log lines this driver actually emits. The metaslab_load message, the one this tree has crash dumps of, is 311 characters - 38 of prefix and 273 of body - so __dprintf()'s measuring call (buf == NULL, which skips Tier 1 by construction) fell through to Tier 3 on every single emission. The common case was the allocating case, which is the opposite of what the tier structure is for. That matters beyond the wasted alloc/free pair. __dprintf() is reachable from inside the kmem allocators themselves: kmem_error() calls dprintf() directly while reporting a corrupted buffer, and spl-kmem.c and spl-vmem.c call it from many other places. Measuring a log line by allocating means the allocator's own error path re-enters the allocator. sys/types.h states the constraint for this reason - the logger must stay to a single bounded allocation, no grow-and-retry and no helper that allocates more than once. Raising the probe to 1024 keeps every realistic log line on Tier 2, so the measuring call allocates nothing and the only allocation in __dprintf() is the one it makes for the message itself. Tier 3 remains for genuinely unbounded formats such as module/lua/lstrlib.c's channel programs, where an allocation is unavoidable and the IRQL guard already covers it. Cost is 768 additional bytes of stack in spl_vsnprintf()'s frame. The dbgmsg path measured from a crash dump uses roughly 2.9 KB from taskq_thread() down to __dprintf(), against a 12 KB kernel stack, and spl_vsnprintf() is a leaf below that - so the margin is ample. Also corrects two comments the change invalidates: Tier 3 described the probe by its literal size, and Tier 2 claimed every real caller writes "under a few hundred bytes", which was the assumption that produced 256 and is not true of __dprintf(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Must-Fix findings fixed across this branch came from a CodeQL scan that is not reproducible from anything currently in the tree - the scoping was done by hand and existed only in one working copy. Commit the script so the same scan can be re-run to verify the finding count drops, and re-run again on future changes. What it does differently from a naive scan: it traces a CMake+Ninja build of only the ZFSin target and what statically links into it (splkern, zlibkern, icpkern, luakern, zfskern, zfskern_os, zcommonkern, nvpairkern, unicodekern, zstdkern), rather than the whole tree. The user-mode tools and libraries share source files with the driver - module/zfs/*.c is also compiled into libzpool - but under different macros and headers, so a database containing both reports the same source line twice in two contexts and inflates the count. Scoping to the driver is also what the HLK Static Tools Logo Test actually requires. It then analyzes with the WHCP mustfix.qls suite and prints a summary grouped by rule and API. Worth stating plainly, because it bounds what this scan can tell us: CodeQL sees only what the build compiles. Files present in the tree but absent from the ZFSin target - spl-lookasidelist.c and module/icp/algs/blake3/blake3_impl.c today - are invisible to it, and both still contain deprecated APIs that will surface as new findings the moment either is added to the build. Functions excluded by the preprocessor are equally invisible; icp_aes_impl_get(), icp_gcm_impl_get(), zfs_vdev_raidz_impl_get() and spl-kstat.c's 32-bit compat block all contain raw sprintf or strcpy calls that are compiled out on Windows, confirmed by checking for their symbols in the built objects rather than by reading the #ifdefs. The suite is also narrower than "memory safety": mustfix.qls is a driver-certification suite whose deprecated-API query matches function names. It cannot see buffer arithmetic, which is how a genuine stack buffer overflow in printBuffer() survived two independent remediation passes - see the commit that fixes it. Running codeql/cpp-queries:codeql-suites/cpp-security-extended.qls over the same database would cover that class and is worth doing before this work is signed off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The strncpy and strcat sweeps introduced spl_strlcpy() and spl_strlcat() as static inlines in sys/types.h, on the stated grounds that strlcpy() "has no kernel-linkable implementation here, so provide one". That premise is wrong. Both are declared in sys/sunddi.h (lines 187-188) and defined for kernel mode in module/os/windows/spl/spl-ddi.c (lines 579 and 597), and they link - the parallel remediation on SSV-26896-fix converted these same call sites to plain strlcpy/strlcat and built clean. The tree therefore carried two implementations of each function, and the user-mode half of the pair (lib/libspl/include/os/windows/sys/types.h) was already nothing but a passthrough to the very function it was said not to have. Both implementations are equivalent - each returns strlen(src), always terminates when the destination size is non-zero, and never overruns - so this is a rename, not a behaviour change. Keeping strlcpy/strlcat rather than the spl_ names matters most for the shared sources. module/zfs, module/icp and module/lua are periodically merged from upstream openzfsonwindows/openzfs, and upstream, Linux and FreeBSD all spell these strlcpy/strlcat. Every spl_strlcpy in shared code is a permanent merge conflict for no benefit - the same reasoning the vsnprintf work used to argue for a header-only fix over touching ~140 call sites. 40 call sites across 16 files, plus removal of both inline definitions and the user-mode passthroughs. Verified with a full x64-Release build of the whole tree, driver and user-mode tools, 356/356 targets. Not addressed here, but adjacent and pre-existing: lib/libspl carries its own duplicate pair, with strlcpy and strlcat defined both in os/windows/posix.c (lines 715, 733) and in strlcpy.c/strlcat.c. That is what produces the LNK4006 "already defined in posix.c.obj" warnings when libspl is archived, and it is unaffected by this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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.
Motivation and Context
Description
How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by.