Reduce false positives compared to SDV for driver tests#217
Merged
NateD-MSFT merged 9 commits intomicrosoft:developmentfrom Apr 23, 2026
Merged
Reduce false positives compared to SDV for driver tests#217NateD-MSFT merged 9 commits intomicrosoft:developmentfrom
NateD-MSFT merged 9 commits intomicrosoft:developmentfrom
Conversation
The double-fetch query flags pairs of dereferences through memory origins where originCanWrite() is true. MdlOrigin had this set unconditionally, so every pair of accesses through an MDL-mapped pointer was flagged, even pure writes to an output buffer: void *Buf = MmGetSystemAddressForMdlSafe(Mdl, NormalPagePriority); ((MY_IOCTL *)Buf)->Flags = 0; // flagged ((MY_IOCTL *)Buf)->Count = 0; // flagged While usermode does retain write access to MDL-locked pages through its original VA, flagging all of these is not useful in practice, since virtually all direct I/O drivers access the buffer more than once. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
The query only allowed MajorFunction access directly inside a
DriverEntry function. Drivers that split initialization across helper
functions were falsely flagged:
VOID InitDispatch(DRIVER_OBJECT *DriverObject) {
DriverObject->MajorFunction[IRP_MJ_CREATE] = MyCreate;
}
NTSTATUS DriverEntry(DRIVER_OBJECT *DriverObject, ...) {
InitDispatch(DriverObject);
}
Follow call chains transitively from DriverEntry so that helpers are
recognized.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Author
|
CC @NateD-MSFT |
Collaborator
|
@zx2c4 Acknowledging. Will review when I have free cycles. |
Author
|
Thanks! I think you have to at least click some button to let the CI work? Not sure. Anyway, will sit tight. |
Author
|
Those test failures seem more to do with the testing infra? |
Collaborator
Yes, I should have proactively mentioned that - we have various problems with our pipeline at the moment (tracked in #197). The core build section of the pipeline looks to have passed, however. |
Author
|
Ah okay. I'll leave this be, then. Hopefully you can get to it before the next HLK release! |
Collaborator
|
So far this LGTM but I'm running it on our internal test corpuses to check the diff there before approving. The contribution is much appreciated. |
NateD-MSFT
reviewed
Apr 20, 2026
Collaborator
NateD-MSFT
left a comment
NateD-MSFT
left a comment
There was a problem hiding this comment.
Largely looks good to me. See my question about where to put the When logic for analyzing function calls in IRQL queries.
NateD-MSFT
requested changes
Apr 20, 2026
KeSetEvent is annotated _When_(Wait==1, _IRQL_requires_max_(APC_LEVEL)). When called with Wait=FALSE, the restriction does not apply and DISPATCH_LEVEL is fine. The query unconditionally applied the annotation regardless of argument values: KeAcquireInStackQueuedSpinLock(&Lock, &Handle); // raises to DISPATCH KeSetEvent(&Event, IO_NO_INCREMENT, FALSE); // flagged Evaluate _When_ conditions against compile-time argument values and skip annotations whose conditions are demonstrably false. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
getIrqlLevel() returns -1 when it cannot parse the annotation, such as _IRQL_saves_ or _When_ conditionals with complex expressions. The query flagged all of these as invalid annotations: _IRQL_saves_ VOID KeAcquireSpinLock(PKSPIN_LOCK Lock, PKIRQL OldIrql); // flagged Filter out the -1 sentinel since these are valid annotations beyond the analyzer's current parsing capability. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
DRIVER_DISPATCH_PAGED is a valid paged variant of DRIVER_DISPATCH for dispatch routines that only run at PASSIVE_LEVEL. The query flagged the mismatch between function class and typedef: _Dispatch_type_(IRP_MJ_DEVICE_CONTROL) static DRIVER_DISPATCH_PAGED MyDispatch; // flagged Recognize _PAGED suffixed typedefs as compatible with their unsuffixed base function class. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Drivers sometimes construct synthetic MDLs on the stack for zero-copy
operations. Direct field access is the only way to initialize these:
MDL Mdl = { 0 };
Mdl.MappedSystemVa = Buffer;
Mdl.ByteCount = Len;
Mdl.MdlFlags = MDL_MAPPED_TO_SYSTEM_VA;
Exclude accesses on locally-declared MDL struct variables. Accesses
through MDL pointers are not excluded, since those typically reference
system-provided MDLs.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
When a possibly-null return value is passed to a function whose parameter is annotated _In_opt_, the function explicitly handles null. The query was flagging these as unguarded dereferences: OBJ *Obj = LookupObject(...); PutObject(Obj); // flagged, but PutObject takes _In_opt_ OBJ * Exclude calls where the argument's corresponding parameter carries an _opt_ SAL annotation. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
_Analysis_assume_(Expr) tells the analyzer to treat Expr as true. MSVC compiles __assume() into an empty statement with no AST node, but the EmptyStmt remains in the control flow graph at the macro invocation site. The query was not recognizing this as a null guard: NBL *Nbl = DequeueNbl(&Queue); _Analysis_assume_(Nbl); NET_BUFFER_LIST_STATUS(Nbl) = NDIS_STATUS_FAILURE; // flagged Match the EmptyStmt at the _Analysis_assume_ location as a barrier by correlating it with the macro invocation that names the guarded variable. Also match AssumeExpr directly for compilers that emit it. Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
zx2c4
force-pushed
the
jd/false-positives
branch
from
c7db942 to
0576474
Compare
Author
|
Alright, fixed up your two remarks. Hopefully good to go now. |
zx2c4-bot
pushed a commit
to WireGuard/wireguard-nt
that referenced
this pull request
Apr 21, 2026
I fixed the CodeQL rules. Link: microsoft/Windows-Driver-Developer-Supplemental-Tools#217 Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Author
|
@NateD-MSFT I think you have to press the buttons for the CI to run again. But anyway, hopefully this looks good to you now. |
Collaborator
NateD-MSFT
approved these changes
Apr 22, 2026
Collaborator
|
The only failure is on our end for a misconfiguration (publish shouldn't be running on PR, just post-PR commits.) Merging. |
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.
Moving from SDV to CodeQL introduced a lot of false positives that I've had to suppress. This small series mostly gets things back on par with SDV for my project WireGuardNT. There's extensive description in each commit message, so be sure to review this commit-by-commit.