Fix use-after-free and double-free in kmem_vasprintf() (SSV-26896) - #115
Draft
datacore-senthil wants to merge 4 commits into
Draft
Fix use-after-free and double-free in kmem_vasprintf() (SSV-26896)#115datacore-senthil wants to merge 4 commits into
datacore-senthil wants to merge 4 commits into
Conversation
Fixes a kernel use-after-free plus double-free that corrupted the shared kmem/vmem freelists and produced bugchecks in unrelated subsystems (nvlist and ABD teardown) long after the fact. Introduced by the SSV-26770 CodeQL remediation; found by root-causing a cluster of ZFSin crash dumps, one of which was captured under Driver Verifier. Root cause kmem_vasprintf() used the "measure with a NULL destination, then allocate that much" idiom. The SSV-26770 work replaced the underlying _vsnprintf(NULL, 0, ...) - which returns the true, unbounded required length - with zfs_vsnprintf(NULL, 0, ...), which routes to zfs_vscprintf(). Kernel mode has no linkable way to measure a format without writing it (_vscprintf is declared by the WDK but not exported by the kernel-mode CRT import lib; _vsnprintf_s rejects count == 0), so zfs_vscprintf() measures by formatting into a bounded 1024-byte scratch buffer and therefore reports a capped 1023 for anything longer. Sizing an allocation from that capped length under-allocates, which made a previously unreachable error path in kmem_vasprintf() reachable for the first time: for any format whose output is >= 1024 characters, kmem_alloc(size + 1) is too small, the real write returns -1, and the error path ran kmem_free(ptr, size); /* allocated size + 1: mismatched size */ r = -1; /* r is discarded; ptr is NOT cleared */ ... return (ptr); /* returns the freed pointer */ so the caller received a dangling pointer into freed memory. Before the remediation this path was dead code: the measurement was exact, so the write always returned exactly size and the condition was never true. The principal caller is log_internal() in module/zfs/spa_history.c, which does msg = kmem_vasprintf(fmt, adx); fnvlist_add_string(nvl, ZPOOL_HIST_INT_STR, msg); /* UAF read */ kmem_strfree(msg); /* double free */ i.e. it reads freed memory into the pool-history nvlist and then frees the block a second time, with a size derived from whatever string happens to occupy it. That is reached from spa_history_log_internal(), which logs nearly every pool operation, so the resulting freelist corruption surfaced later as "bad free" panics in vmem_hash_delete() and as nvlists and ABDs containing another owner's data. Fix kmem_vasprintf() now grows a scratch buffer and retries the real write until the whole string fits, with no measurement pass, so the freed size always matches the allocated size and it never returns a freed pointer. It still returns a buffer allocated at exactly strlen() + 1: callers free these with kmem_strfree(), which computes the size as strlen(str) + 1, so anything else (for example returning the grown power-of-two buffer directly) would reintroduce the same mismatched-free bug at all ~25 kmem_asprintf() call sites. kmem_asprintf() now delegates to kmem_vasprintf(). Both keep their existing "never returns NULL" contract, accepting a truncated but valid string at a 64 KB ceiling. __dprintf() had the same dependency on an exact measurement, so it is rebuilt on the now-safe helpers instead of hand-computing offsets into a single buffer. That also removes a second off-by-one, missed by an earlier review, in the prefix write: snprintf(buf, size + 1, ...) told it one more byte than buf actually had. The allocation length is recorded before the trailing-newline strip edits the string in place, since strlen() + 1 afterwards is a byte short of what was allocated. The comment on zfs_vscprintf() in types.h asserted that a capped measurement was harmless because a later write would truncate identically. That reasoning is what produced this bug - it holds only when the measured length is not used to size the buffer. Replaced with an explicit warning that the value is capped and must never size an allocation. Audited for the same patterns: the only remaining buf==NULL/size==0 callers are under module/os/freebsd, which is not part of this driver. The vendored zlib's equivalent size==0 branch is unreachable (its callers always pass sizeof(buf)). No caller modifies a kmem_asprintf() result before kmem_strfree(). The ~23 sites that consume an snprintf() return value are unaffected, as both the old _snprintf and the new _vsnprintf_s(_TRUNCATE) return -1 on truncation, and none of them can reach the exact-fit boundary where the two differ. Not yet build-verified or re-scanned; pushed for review and testing.
The previous commit fixed a real use-after-free in kmem_vasprintf(), but also rewrote __dprintf() to build its message with kmem_vasprintf()/kmem_asprintf(). That second change was not required by the fix and should not have been bundled with it. __dprintf() is reachable from inside the kmem and vmem allocators themselves - spl-kmem.c calls dprintf() in 22 places and spl-vmem.c in 54, including from kmem_error() - so it must stay to a single bounded allocation. The rewrite turned one kmem_alloc/kmem_free pair into roughly six to eight allocator operations per debug message (two helper calls, each with a grow-and-retry loop and an exact-size copy), in the most reentrancy-sensitive path in the driver, in exchange for untruncated debug text. That is the wrong trade. __dprintf() is therefore reverted to its original single-allocation structure, keeping only the two genuine off-by-one corrections. The substantive delta against the long-shipping code is now three lines: - snprintf(buf, size + 1, ...) -> snprintf(buf, size, ...) - zfs_vsnprintf(buf + i, size - i + 1, ..) -> zfs_vsnprintf(buf + i, size - i, ..) - i = snprintf(...) -> i = (int)strlen(buf) buf holds exactly `size` bytes, so both writes now get the true remaining capacity rather than one byte more. The third change closes a latent hazard present in the original: on truncation _vsnprintf_s returns -1, and buf + (-1) is a wild pointer. A truncating write still null-terminates, so strlen() is always the real prefix length and always leaves size - i >= 1, with no branch and no failure mode. kmem_alloc(size)/kmem_free(buf, size) symmetry is restored. An alternative - making __dprintf allocation-free with a fixed stack buffer and a per-thread recursion guard - was considered and rejected: it is more new code in the same dangerous path, and it was motivated by a theory that does not hold up (below). __zfs_dbgmsg() still does kmem_zalloc, a pre-existing reentrancy that predates this work and is deliberately left alone. Theory withdrawn: an earlier analysis claimed this reentrancy caused the KMERR_BADCACHE crashes seen after the previous commit. That is not established. Reentrancy into the allocator produces deadlock, not a wrong-size free, and the original __dprintf allocated too without crashing. The rewrite is reverted because it was unjustified in a dangerous path, not because it was proven guilty. The cause of those crashes remains unidentified. kmem_vasprintf()/kmem_asprintf() are deliberately left exactly as committed previously - that fix is the one part of this work whose mechanism was traced end to end and is not in question. The zfs_vscprintf() comment in types.h previously stated a blanket prohibition on using its capped result to size an allocation. __dprintf legitimately does exactly that, so an absolute rule the code contradicts is worse than none. It now states the real rule: sizing an allocation from a capped measurement is safe if and only if every write into the buffer is bounded by the buffer's real size and the free passes the size that was allocated. __dprintf satisfies both; kmem_vasprintf violated both, which is why it corrupted the heap. Also adds contrib/windows/docs/ZFSin-kmem-corruption-investigation.md, a consolidated record of the whole investigation: every crash analysed, what was proven versus suspected, the hypotheses that were disproven (so they are not retried), the reference facts that were expensive to establish, and a WinDbg cookbook.
Issue
-----
printBuffer() writes a thread-id prefix into a 1024-byte stack buffer and
then appends the caller's formatted message after it:
char buf[max_line_length]; /* 1024 */
RtlStringCbPrintfA(buf, 18, "%p: ", PsGetCurrentThread());
int tmp = _vsnprintf_s(&buf[17], sizeof (buf), max_line_length,
fmt, args);
The destination of the second call is &buf[17], which has 1024 - 17 =
1007 bytes left. It was told sizeof (buf) = 1024, i.e. 17 bytes more than
exist. A message long enough to fill the buffer therefore writes past the
end of buf and into the frame above it. 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 squarely on the cookie and the saved
register. That is a genuine stack buffer overflow, CWE-121.
It has not been observed firing in the field: it needs roughly 1007
characters of output in a single call, and the messages on the paths we
have dumps for are around 370 bytes. It is also fail-loud rather than
silent, because /GS validates the cookie before the epilogue restores rbx
- so it would surface as bugcheck 0xF7 STACK_BUFFER_OVERRUN, not as
memory corruption. Neither of those makes it safe to keep.
Two further defects in the same six lines:
* The prefix was written with a hardcoded 18-byte destination. "%p" emits
16 hex digits on x64, so "%p: " needs 19 bytes including the
terminator, and RtlStringCbPrintfA silently truncated it - the trailing
space was being dropped on every line of trace output. The 18 and the
&buf[17] also had to be kept in agreement by hand.
* The truncation test could never be true. _vsnprintf_s returns -1 on
truncation, never a value >= the buffer size, so "tmp >=
max_line_length" was dead and the "buffer too small" fallback was
unreachable.
Fix
---
Give the prefix the whole buffer so it is no longer truncated, and read
the offset back out with strlen() instead of hardcoding it, so no
constant has to match another. Bound the message write by the capacity
that actually remains, sizeof (buf) - prefix_len. Pass _TRUNCATE
((size_t)-1) as the count, matching the convention used by
zfs_vsnprintf() elsewhere in this tree - _vsnprintf_s null-terminates on
truncation itself. Test tmp < 0, so the fallback is reachable.
Behaviour change: trace lines now carry the trailing space after the
thread id that the format string always intended.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(SSV-26896) Two size-accounting defects in the zfs_dbgmsg path. Both are latent today, and both are in the same class as the KMERR_BADCACHE panic still under investigation - a kmem_free() whose size does not match its kmem_alloc() - so they are worth closing on sight. Issue 1: strlcpy() bound is 28 bytes larger than the destination ---------------------------------------------------------------- __zfs_dbgmsg() allocates one block holding the header and the message: 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 is at offsetof(zfs_dbgmsg_t, zdm_msg), which is 28. So the space available at zdm_msg is size - 28 = strlen(buf) + 4 bytes, not size. The bound passed was 28 bytes too generous. It does not overrun today only because strlcpy() stops at the source length: it writes strlen(buf) + 1 bytes, which is 3 inside the real capacity. That margin is incidental - nothing states or enforces it, and it does not survive a change to the struct layout or to how the size is computed. Fixed by passing the capacity at zdm_msg, size - offsetof(zfs_dbgmsg_t, zdm_msg). Issue 2: zfs_dbgmsg_fini() frees with a recomputed size ------------------------------------------------------- __zfs_dbgmsg() records the allocation size in zdm->zdm_size for exactly this reason, and zfs_dbgmsg_purge() correctly frees with it. But zfs_dbgmsg_fini() recomputed it instead: int size = sizeof (zfs_dbgmsg_t) + strlen(zdm->zdm_msg); kmem_free(zdm, size); That re-derives the length from the stored message rather than the string it was allocated from. Any truncation, or any later edit of zdm_msg, makes strlen(zdm->zdm_msg) smaller than the strlen(buf) used at allocation, and kmem_free() is then handed a size smaller than the block. A wrong-size kmem_free() is not a benign accounting slip. It selects the cache by size, so a buffer from kmem_alloc_384 gets returned to kmem_alloc_256. In the shipping driver kmem_flags is 0, so there are no buftags and kmem_free() validates nothing - the buffer lands on the wrong cache's free list and is handed out again later, silently corrupting the allocator. It is only caught when the buffer happens to take the slab path rather than the magazine layer, which is why this class of bug surfaces intermittently and far from its cause. Fixed by freeing with zdm->zdm_size, matching zfs_dbgmsg_purge(). Neither issue is reachable on today's code paths. Both remove a way for the same failure we are already chasing to be introduced by an unrelated 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
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.
Fixes a kernel use-after-free plus double-free that corrupted the shared kmem/vmem freelists and produced bugchecks in unrelated subsystems (nvlist and ABD teardown) long after the fact. Introduced by the SSV-26770 CodeQL remediation; found by root-causing a cluster of ZFSin crash dumps, one of which was captured under Driver Verifier.
Root cause
kmem_vasprintf() used the "measure with a NULL destination, then allocate that much" idiom. The SSV-26770 work replaced the underlying _vsnprintf(NULL, 0, ...) - which returns the true, unbounded required length - with zfs_vsnprintf(NULL, 0, ...), which routes to zfs_vscprintf(). Kernel mode has no linkable way to measure a format without writing it (_vscprintf is declared by the WDK but not exported by the kernel-mode CRT import lib; _vsnprintf_s rejects count == 0), so zfs_vscprintf() measures by formatting into a bounded 1024-byte scratch buffer and therefore reports a capped 1023 for anything longer.
Sizing an allocation from that capped length under-allocates, which made a previously unreachable error path in kmem_vasprintf() reachable for the first time: for any format whose output is >= 1024 characters, kmem_alloc(size + 1) is too small, the real write returns -1, and the error path ran
so the caller received a dangling pointer into freed memory. Before the remediation this path was dead code: the measurement was exact, so the write always returned exactly size and the condition was never true.
The principal caller is log_internal() in module/zfs/spa_history.c, which does
i.e. it reads freed memory into the pool-history nvlist and then frees the block a second time, with a size derived from whatever string happens to occupy it. That is reached from spa_history_log_internal(), which logs nearly every pool operation, so the resulting freelist corruption surfaced later as "bad free" panics in vmem_hash_delete() and as nvlists and ABDs containing another owner's data.
Fix
kmem_vasprintf() now grows a scratch buffer and retries the real write until the whole string fits, with no measurement pass, so the freed size always matches the allocated size and it never returns a freed pointer. It still returns a buffer allocated at exactly strlen() + 1: callers free these with kmem_strfree(), which computes the size as strlen(str) + 1, so anything else (for example returning the grown power-of-two buffer directly) would reintroduce the same mismatched-free bug at all ~25 kmem_asprintf() call sites. kmem_asprintf() now delegates to kmem_vasprintf(). Both keep their existing "never returns NULL" contract, accepting a truncated but valid string at a 64 KB ceiling.
__dprintf() had the same dependency on an exact measurement, so it is rebuilt on the now-safe helpers instead of hand-computing offsets into a single buffer. That also removes a second off-by-one, missed by an earlier review, in the prefix write: snprintf(buf, size + 1, ...) told it one more byte than buf actually had. The allocation length is recorded before the trailing-newline strip edits the string in place, since strlen() + 1 afterwards is a byte short of what was allocated.
The comment on zfs_vscprintf() in types.h asserted that a capped measurement was harmless because a later write would truncate identically. That reasoning is what produced this bug - it holds only when the measured length is not used to size the buffer. Replaced with an explicit warning that the value is capped and must never size an allocation.
Audited for the same patterns: the only remaining buf==NULL/size==0 callers are under module/os/freebsd, which is not part of this driver. The vendored zlib's equivalent size==0 branch is unreachable (its callers always pass sizeof(buf)). No caller modifies a kmem_asprintf() result before kmem_strfree(). The ~23 sites that consume an snprintf() return value are unaffected, as both the old _snprintf and the new _vsnprintf_s(_TRUNCATE) return -1 on truncation, and none of them can reach the exact-fit boundary where the two differ.
Not yet build-verified or re-scanned; pushed for review and testing.
Motivation and Context
Description
How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by.