Integration: Merge development into main (v1.10)#223
Closed
Copilot wants to merge 5 commits into
Closed
Conversation
…ecommended.) (#216) * Add additional STL rules (both experimental and recommended.) * Bring over documentation for new rules. * Fix double word typo in comment Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> --------- Signed-off-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Fix upload-result-diff and azure-login pipeline failures - Skip Azure Login, Download, Upload, and comparison steps on pull_request events where OIDC secrets are unavailable - Separate the "Upload result diff" step from the "Fail if diff detected" step so uploads succeed independently and failures have clear error messages - Apply fixes to both test-query-health and test-codeql-latest-vs-current jobs Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/9c85d93e-42c1-4451-81ad-2a71ece7bc2e Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Fix empty-string arg passed to test script on PR runs Building the python command with an empty $compareFlag variable caused PowerShell to pass an empty string as a positional arg, producing: "unrecognized arguments: ". Switched to building an argument array and splatting it so the flag is only included when present. Applied to both test-query-health and test-codeql-latest-vs-current. Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/a5ce6b58-e7aa-4b94-b731-d0242b47b40e Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Surface msbuild errors from codeql database create and fail loudly on test failures Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/1a592ad2-4448-490f-ae16-a9c34d72bf79 Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Remove stray .pyc accidentally committed in previous commit Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/1a592ad2-4448-490f-ae16-a9c34d72bf79 Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Optimize pipeline for faster PR runs Removed unnecessary build steps for CA ported queries (covered by the build-all step) and adjusted dependencies in workflow to speed up PR runs (test steps will build as part of work.) Signed-off-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Add Directory.Build.props to wire WDK/SDK NuGet packages into driver test projects Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/7795692b-bc9c-4c2e-a3d3-18f00aed9bcb Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Fix stampinf, ApiValidator, and Dvl.exe failures in CI tests - Remove <Inf> items from KMDFTestTemplate.vcxproj and CppKMDFTestTemplate.vcxproj so the StampInf MSBuild task is not triggered (stampinf.exe is not available in a NuGet-only WDK environment). These templates exist solely for CodeQL analysis, not for building a production driver, so INF stamping is not needed. - Add <ApiValidatorEnabled>false</ApiValidatorEnabled> to Directory.Build.props. ApiValidator.exe is not included in the WDK NuGet packages and is not available in the CI environment. Disabling it allows the ApplicationForDriversTestTemplate (WindowsApplicationForDrivers10.0 toolset) to compile successfully for CodeQL. - Update dvl_tests.ps1 to locate Dvl.exe dynamically from the NuGet packages directory instead of assuming the traditional system WDK install path C:\Program Files (x86)\Windows Kits\10\Tools\dvl\Dvl.exe. Falls back to the system path for developer machines with a full WDK install, and gracefully skips the dvl command-type tests when Dvl.exe is not found in either location. - Fix typo -test_emtpy -> -test_empty in the second Test-DVL "dvl" call inside Test-Driver (the typo was previously unreachable because the test exited earlier due to the Dvl.exe failure). Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/8484509b-2924-427f-bd9a-63e365e4b404 Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * plan: fix ApiValidator and parallelize test runner Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/71e6036e-2c98-4bb7-85ab-2270cbce9c68 Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Fix ApiValidator failures, harden Dvl.exe check, parallelize test runner - Add src/drivers/test/Directory.Build.targets that overrides the WDK's ApiValidator MSBuild target with an empty target. The WDK NuGet packages reference ApiValidator.exe in WindowsDriver.common.targets but do not ship the binary itself, so the post-build step always failed with MSB3721 for builds using the Universal driver target platform. Replacing the target with a no-op cleanly suppresses the step; API validation is irrelevant for the CodeQL static analysis these tests perform. This is the cause of the 6 still-failing tests (UnsafeCallInGlobalInit, MultithreadedAVCondition, StaticInitializer, DeviceInitApi, FloatSafeExit, FloatUnsafeExit). - Remove the previous <ApiValidatorEnabled>false</ApiValidatorEnabled> workaround from Directory.Build.props -- the WDK targets do not honor that property name, so it had no effect. - dvl_tests.ps1: when Dvl.exe cannot be located in either the NuGet packages directory or the system WDK install path, exit with a clear failure instead of silently skipping the dvl command-type tests. Skipping let regressions go undetected. - build_create_analyze_test.py: parallelize the test runner. Each ql_test uses isolated working/<name>, TestDB/<name>, and AnalysisFiles/<name>.sarif paths, so multiple tests are safe to execute concurrently. Use multiprocessing.pool.ThreadPool (already imported) with a worker count controlled by a new -j/--jobs flag, defaulting to os.cpu_count(). Pass --jobs 1 to fall back to the legacy sequential behaviour. Print output is already protected by print_mutex; added results_mutex around the shared health_df / detailed_health_df DataFrame writes. Refactored the body of the per-test loop into _run_single_test for use by the pool's imap_unordered. Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/71e6036e-2c98-4bb7-85ab-2270cbce9c68 Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Parallelize test-query-health and test-codeql-latest-vs-current jobs Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/694e9d3d-f942-4564-8e96-8af21abeb87a Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Fix LNK1318 mspdbsrv race; wire --jobs into workflow Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/694e9d3d-f942-4564-8e96-8af21abeb87a Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Pipeline tweaks: progress log, parallel query compile, scoped pack-version gate, broader publish needs Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/96f4dbad-dab0-45e8-9a14-94efd2c96d27 Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> --------- Signed-off-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com>
* UserModeMemoryReadMultipleTimes: Remove MdlOrigin.originCanWrite
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>
* IllegalFieldAccess2: Follow call chains from DriverEntry
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>
* Irql: Evaluate _When_ conditions at call sites
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>
* IrqlAnnotationIssue: Skip the -1 parse-failure sentinel
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>
* InvalidFunctionClassTypedef: Allow _PAGED variants
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>
* OpaqueMdlUse, OpaqueMdlWrite: Exclude local MDL variables
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>
* UnguardedNullReturnDereference: Exclude calls to _In_opt_ parameters
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>
* UnguardedNullReturnDereference: Honor _Analysis_assume_
_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>
---------
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com>
* [AI] Initial stab at optimizing suppressions and IRQL. * [AI] Revise IRQL queries and libraries to reduce false positives. * Compress repetitive code and fix misplaced comments in IRQL queries Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/8ab9c70a-a38a-46ec-9f14-f90475a0d87c Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Optimize MultiplePagedCode.ql to avoid timeouts on large codebases The original query joined MacroInvocation x MacroInvocation and only filtered by macro name and PagedFunctionDeclaration after the join. On large codebases this could hit the analysis timeout because MacroInvocation.getEnclosingFunction() (cpp-all 7.0.0 Macro.qll:178) is built on getAnAffectedElement(), which materialises a join of inmacroexpansion and macrolocationbind whose size scales poorly with code volume regardless of how the receiver set is constrained. Changes: * src/drivers/libraries/Page.qll: add class PagedCodeMacro extends MacroInvocation. Pre-filters by macro name (PAGED_CODE / PAGED_CODE_LOCKED) and exposes getEnclosingPagedFunction() that uses getStmt().getEnclosingFunction() (built on the cheaper getAnExpandedElement()) and excludes FunctionTemplateInstantiation. * src/drivers/wdm/queries/MultiplePagedCode/MultiplePagedCode.ql: rewrite body as an asymmetric existential over PagedCodeMacro + Function so only the outer macro is in the from clause. Excluding FunctionTemplateInstantiation also fixes a latent FP: each template instantiation produces its own Function and its own MacroInvocation records with line attribution that can drift by +-1 across instantiations, which produced false 'multiple PAGED_CODE' reports in templated functions. Counting only source-level (uninstantiated) template MIs preserves real positives in templates while eliminating the spurious match. Verification: * Windows driver samples corpus: pre-existing single TP preserved on the same (file, line); wall time roughly halved. * Large representative codebase that previously timed out at the 2 h limit: now completes in ~2.5 minutes with no findings; spot-checked source files with multiple textual PAGED_CODE() calls confirm each invocation lives in a distinct function (constructor/destructor pairs, per-overload operators, etc.), so the empty result is correct. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bump @query-version on queries changed since development branch Each of these queries has functional changes (new predicates, additional where-clause filters, refactored body) relative to the development branch but the @query-version metadata had not been incremented. Bumping the versions so downstream consumers can detect the behavioural change. * IrqlFloatStateMismatch.ql: v1 -> v2 * IrqlNotSaved.ql: v1 -> v2 * IrqlSetTooHigh.ql: v1 -> v2 * IrqlTooHigh.ql: v3 -> v4 * IrqlTooLow.ql: v4 -> v5 * MultiplePagedCode.ql: v1 -> v2 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Refine IrqlFloatStateMismatch suppression to be CFG-independent The v2 `irqlChangesBetween` predicate used `ControlFlowNode.getASuccessor+()` from a `DataFlow::Node.asIndirectExpr()` anchor. Indirect-expression nodes are synthetic and are not connected to the cpp control-flow graph, so `getASuccessor+()` from them returns the empty set in some extracted databases. As a result the predicate held for nothing on those databases and all true positives were silently suppressed. Replace the CFG reachability check with a same-function source-position check between the `KeSaveFloatingPointState` and `KeRestoreFloatingPointState` calls. The new predicate preserves the original intent of suppressing the `no-actual-irql-transition` false positive category while remaining well-defined regardless of how the control-flow graph is materialised. Bumps @query-version v2 -> v3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Catch IrqlFloatStateMismatch through unannotated helper wrappers Driver code commonly wraps the IRQL primitives (KeRaiseIrql, KeLowerIrql, KfRaiseIrql, KfLowerIrql) in unannotated helper functions. When a floating-point save/restore pair has its IRQL transition performed inside such a helper, the previous filter -- which only inspected calls in the enclosing function -- discarded the finding as a may-analysis artifact and the true positive was lost. Extend `isIrqlChangingCall` with a transitive helper-detection pass: `isIrqlChangingFunction(f)` holds either when `f` is annotated as an IRQL-changing function or when its own body contains an IRQL-changing call (recursively). This is bounded by the call graph and behaves idempotently on cycles. Add two test snippets to driver_snippet.c that exercise the new logic: `driver_utility_helper_bad` (must fire -- IRQL changes via an unannotated helper) and `driver_utility_nested_block_good` (must not fire -- IRQL change inside a nested compound statement). Update the checked-in baseline SARIF to expect the additional helper finding. Bumps @query-version v3 -> v4. Out-of-tree corpus measurements showed no change in finding counts and only a small evaluation-time regression on the smallest databases (roughly +10 percent), with no measurable change on larger databases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add adversarial tests + document known false negatives in qhelp Empirically validated three review-flagged regressions on the optimization branch by adding adversarial test cases to the local test drivers and running the queries with and without the branch's changes: * IrqlTooHigh / IrqlTooLow isInConstantFalseBranch. The predicate intended to silence NDIS dead-branch macros is too lax: compound assignments (|=, &=, +=) extract as AssignOperation, and ++/-- as CrementOperation, so the predicate's ot exists AssignExpr guard considers them as no mutation; pass-by-reference helpers and globals reassigned in another function also bypass the check. Four adversarial cases per direction were added; the on-branch query reports 0 of them, while the pre-branch query reports all 4. * IrqlFloatStateMismatch cross-function save / restore. The new irqlChangesBetween filter requires the save and restore calls to share an enclosing function. A wrapper that thinly forwards to KeSaveFloatingPointState / KeRestoreFloatingPointState with the IRQL change happening in the caller is now silently missed; the pre-branch query flagged it correctly. * MultiplePagedCode template-instantiation exclusion. A C++ function template containing two PAGED_CODE() invocations produced one finding from the pre-branch query and zero from the on-branch query (the latter excludes every FunctionTemplateInstantiation when locating the enclosing function and, on this database, never sees a non-instantiation parent). Adversarial template test driver lives in the out-of-tree workspace. For each finding, the corresponding qhelp file gains a "Known false negatives" / "Lower-on-exit pattern" section describing the limitation in user-facing terms; the matching .md files were regenerated via codeql generate query-help. The IrqlSetTooHigh "lower-on-exit" exemption (heuristic risk) and the IrqlFloatStateMismatch indirect-call / loop-order limitations are also documented. The on-branch driver_snippet baselines remain +0/-0 because each adversarial case is, by construction, a missed positive. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix three false negatives surfaced by the prior review * IrqlTooHigh / IrqlTooLow isInConstantFalseBranch. Replaced the AssignExpr-only mutation guard with the broader Assignment superclass (covers =, |=, &=, +=, etc.) and added a CrementOperation check for ++ / --. Restricted the variable-access case to non-static LocalVariable instances and bailed out when the variable's address is taken anywhere in the database, so globals reassigned in another function and locals mutated through a pointer are no longer treated as "constantly false". The compile-time-constant-zero condition is preserved unchanged. IrqlTooHigh: v4 -> v5. IrqlTooLow: v5 -> v6. The four adversarial cases in each driver_snippet now produce findings; baseline TPs are preserved unchanged. * IrqlFloatStateMismatch cross-function wrappers. Replaced the same-enclosing-function constraint in irqlChangesBetween with an anchor-line predicate that lets the candidate function be either the enclosing function of the call directly, or the function that calls a one-level helper containing it. The predicate then looks for an IRQL-changing call in between the two anchor lines. This handles thin save/restore wrappers around the Ke*FloatingPointState primitives where the IRQL change happens in the common caller, and also the asymmetric case where one side is a wrapper and the other is direct. v4 -> v5. * MultiplePagedCode template-instantiation handling. Replaced the blanket ot result instanceof FunctionTemplateInstantiation exclusion with a projection back to the underlying TemplateFunction via getTemplate(). Page-segment heuristics are still checked on the concrete instantiation, but match keys are deduplicated to the source-level template, so a duplicate PAGED_CODE inside a templated function body is reported once at the source location regardless of how many instantiations the extractor produced. v2 -> v3. Validation (in-tree tests): * IrqlTooHigh: 4 baseline TPs preserved; 4 adversarial cases now flagged (compound assign, increment, by-reference helper, file-scope global). diff +0/-0 against the updated baseline. * IrqlTooLow: same shape; 2 + 4 = 6 findings. * IrqlFloatStateMismatch: 4 baseline TPs preserved; 1 adversarial cross-function case now flagged. 5 findings. * MultiplePagedCode: existing baseline TP preserved; diff +0/-0. Validation (out-of-tree C++ KMDF template database): * MultiplePagedCode now reports the templated duplicate at the source location (one finding), matching the pre-branch baseline instead of silently dropping it. qhelp updated for each query: the prior "Known false negatives" sections were rewritten to describe the now-handled scope and to keep the documented limitations that still apply (indirect calls, deeply-nested helper IRQL changes, multi-level wrapper chains, loop-restore-before-save). MD files regenerated via codeql generate query-help. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix MultiplePagedCode cross-file conflation FP The previous fix projected FunctionTemplateInstantiation back to its underlying TemplateFunction to deduplicate findings inside a templated function body. However, the cpp extractor sometimes consolidates two ODR-equivalent template definitions in different headers into a single TemplateFunction entity. When two driver-private headers each defined e.g. emplate<class T> Aggregator_SendProperty(...) containing one PAGED_CODE(), my projection collapsed both definitions to one match key and the query reported a spurious "duplicate" across the two unrelated functions. This change reverts the projection: getEnclosingPagedFunction() now returns the concrete Function produced by getStmt().getEnclosingFunction() (which can be a FunctionTemplateInstantiation for templated bodies). The query joins on enclosing-function identity AND requires the two PagedCodeMacro invocations and the enclosing function to all share the same file, so the cross-file conflation can no longer match. Same-file deduplication across template instantiations within a single TU still produces a single finding because the multiple (mi2, f) tuples project to one mi2 at the SARIF level. Validation: * In-tree MultiplePagedCode test: diff +0/-0 (the existing DispatchShutdown baseline TP is preserved). * Out-of-tree C++ KMDF template DB: still reports 1 finding for the templated duplicate PAGED_CODE, matching the pre-branch baseline. * Large representative codebase that previously surfaced the cross-file FP: now reports 0 findings (down from the spurious 1). v3 -> v4. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Pretty-print SARIF baselines compressed by #220 Agent-Logs-Url: https://github.com/microsoft/Windows-Driver-Developer-Supplemental-Tools/sessions/e23e0bbb-d5d4-4b2c-808b-de74c56ed9dc Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> * Tidy up: indentation, ASCII-only comments, dedupe IFSM where, refresh SARIF baselines Style and consistency cleanups uncovered during the final code review, plus baseline regeneration so all checked-in SARIF baselines have consistent (2-space) pretty-printing and reflect the current CodeQL build's output: * Irql.qll - whenConditionIsFalseAtCallSite: realign the new "!= NULL" and "(arg & 0x1) <op> 0" or-branches to match the indentation of the pre-existing "Param != 0" / "Param == N" branches (one extra leading space each before this change). - isInConstantFalseBranch: indent the predicate body to match the surrounding 1-space-leading column convention used throughout the file. * Page.qll: replace "MacroInvocation x MacroInvocation" en-quad with ASCII "x" so the file is pure ASCII (matching the rest of the codebase). * IrqlSetTooHigh.ql: replace em-dash with "--" in the same-line rationale comment for the lower-on-exit exemption. * IrqlNotSaved.ql: - Replace em-dash in the "Only track flow to assignment targets or parameters" comment with a comma. - Rewrite the "exists(sink.asParameter())" idiom as "sink.asParameter() instanceof Parameter" for readability; behaviour is unchanged. * IrqlFloatStateMismatch.ql: - Drop the now-redundant getName().matches("KeSave...") / getName().matches("KeRestore...") constraints from the where clause: the FloatStateFlow source/sink predicates already enforce them, so saveCall and restoreCall are uniquely bound by their arg(0) equalities. Adds a short explanatory comment. - Refresh the comment on the irqlChangesBetween conjunct so it no longer says "in the same function": the predicate now also handles one-level wrapper / common-caller patterns. * Adversarial driver_snippet.c comments (IrqlTooHigh, IrqlTooLow, IrqlFloatStateMismatch): the original test headers described the pre-fix predicate behaviour ("under the current predicate they are not [flagged]" / "known false negative of the current intraprocedural filter"). Those statements no longer reflect reality: each adversarial case is now flagged by the corresponding query. Rewrote the headers to describe what the test now exercises (regression guards against the former FN classes). * SARIF baselines: regenerated all six against the current CodeQL build using the in-tree test runner, then pretty-printed with 2-space indent (matching the convention used by the majority of checked-in baselines under src/drivers/). MPC's baseline previously used a Jackson-style "key" : value formatting from the older CodeQL 2.11.5; the regenerated file uses the modern "key": value style consistent with the rest of the repo. Validation: in-tree test runner reports +0/-0 for IrqlTooHigh, IrqlTooLow, IrqlFloatStateMismatch, MultiplePagedCode, IrqlNotSaved, and IrqlSetTooHigh. No query semantics changed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * IFSM: add AST-loop OR-branch to irqlChangesBetween for intra-function loop and re-entrant cases The `irqlChangesBetween` predicate previously decided whether an IRQL-changing call sits between a save and a restore using only source-line position via the `anchorLineForCall` mechanism. This correctly handles cross-function and acyclic in-function cases but trivially fails on loops where the restore is textually above the save (the bracketing line range is empty), even though at runtime the loop back-edge means each iteration's restore is preceded by every IRQL-changing call in the loop body. This change adds a second disjunct to `irqlChangesBetween` that uses AST-loop containment (`Loop.getStmt().getAChild*()`): when the save, restore, and an IRQL-changing call all share a loop body in their common enclosing function, the predicate fires. Combining the two branches with OR is strictly additive and cannot regress any existing true positive. CFG-based reachability was rejected for two reasons documented during investigation: BasicBlock CFG forward-reach is truncated in the extracted DBs we test against, and ControlFlowNode forward reach breaks at `if (call(...))` boundaries (the canonical IFSM pattern). AST-loop containment sidesteps both issues by relying on a densely populated AST relation that reflects the syntactic loop body. Documents the residual limitation: the upstream IRQL analysis library does not consistently bind `getPotentialExitIrqlAtCfn` at the argument expression of `KeSaveFloatingPointState` when the call is inside a loop body, so the `irqlSource != irqlSink` filter still rejects the loop case before `irqlChangesBetween` is consulted. Recovering this true positive requires improvements to the IRQL analysis library, not just to this query. `driver_utility_loop_bad` is retained in `driver_snippet.c` as a documented known false negative so any future improvement to the IRQL library will be detected via SARIF diff. Refreshes `IrqlFloatStateMismatch.qhelp`, regenerates the rendered `IrqlFloatStateMismatch.md`, refreshes the in-tree `IrqlFloatStateMismatch.sarif` (rule-help text), and bumps `@query-version` from v5 to v6. Local test diff: +0/-0 (loop FN does not fire due to upstream binding limitation; no existing TP is regressed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Irql.qll: add loop-body AST fallback to getPotentialExitIrqlAtCfn The recursive cascade in `getPotentialExitIrqlAtCfn` returns no value for argument-expression CFNs of function calls reached via a loop back-edge in some extracted databases. When the cascade is empty, downstream queries that consume the result either drop the relevant finding (because their `irqlSource = getPotentialExitIrqlAtCfn(...)` binding fails) or fire spuriously (because they treat empty as out-of-range). This is the upstream complement to the IFSM `irqlChangesBetween` AST-loop OR-branch added in 39dd725. Fix: rename the existing predicate body to a private `getPotentialExitIrqlAtCfnRaw` helper and introduce a public wrapper that returns Raw's bindings unchanged, plus -- only when Raw yields no value at all -- a single-step AST-level fallback that returns the cascade's IRQL at the closest source-line preceding sibling Stmt. The fallback is restricted to CFNs whose enclosing Stmt sits inside a loop body, which is the empirical failure mode and avoids over-approximating on linear branching code. Layering preserves existing semantics on every input where Raw binds: the wrapper agrees with Raw setwise there, so no query that previously bound correctly can regress. The only behavioural change is to fill in silence with one conservative value derived from textually-preceding code in the loop body. Internal callers inside Irql.qll that participated in the recursive stratum (getIrqlLevel for the three save-globals classes and functionExitIrql) are switched to call Raw directly so the wrapper's negation does not cycle back through them. Validation: Local IFSM TestDB: the documented "known false negative" `driver_utility_loop_bad` at driver_snippet.c:156 (a for-loop where the restore is textually above the save) now fires as a true positive, recovered exactly as the source comment block at lines 127-145 predicted would require improvements to the IRQL analysis library. SARIF baseline updated to include the new finding. Out-of-tree corpora (clean cache, --ram tuned per DB): for the five queries that call this predicate and have non-zero baseline: * IrqlFunctionNotAnnotated, IrqlInconsistentWithRequired, IrqlTooHigh, IrqlTooLow: identical findings before and after on both small corpora. * KeSetEventIrql: one fewer finding on the larger of the two small corpora (a true false-positive suppression in a worker-thread routine where a release-spinlock immediately precedes a SetEvent inside a `for(;;)` loop body; the cascade was empty pre-fix and the KeSetEvent query treated empty as out-of-range, warning spuriously). The fallback walks back to the release sibling stmt, binds PASSIVE, and the warning is correctly silenced. Sanity-check queries that do not call the predicate (IrqlNotUsed, IrqlCancelRoutine) are unchanged, confirming the rename did not perturb unrelated paths. Performance: no measurable wall-time regression observed on either small corpus. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * IrqlFloatStateMismatch: pragma[inline_late] on irqlChangesBetween The `irqlChangesBetween/2` predicate is the hottest single predicate in the IFSM query at HEAD (~109 s of CPU and 3.43 M result tuples in the 18-query suite measurement on the WDS sample database, accounting for roughly a quarter of the IFSM query's total cost). Without a planner hint, the predicate is materialized as a standalone relation over every `(FunctionCall, FunctionCall)` pair in the codebase that satisfies its constraints, and only then intersected with the ~25-row dataflow result set produced by `FloatStateFlow::flow`. With `pragma[inline_late]` plus the matching `bindingset[saveCall, restoreCall]`, the body is specialized at the single call site after the dataflow result has bound both arguments, so the predicate body is evaluated only on the small set of dataflow-derived pairs. Validation on the WDS sample database (single-query run, cold cache): - SARIF result count for cpp/drivers/irql-float-state-mismatch: 0 (matches the HEAD baseline of 0; correctness preserved) - `irqlChangesBetween` no longer appears as a discrete predicate in the evaluator log (it has been fully inlined into its call site) - New top single-query predicate: 28.9 s, vs 109 s for the standalone `irqlChangesBetween` in the baseline suite measurement This is a planner-hint change only. The predicate body is byte-for-byte unchanged, so the set of `(saveCall, restoreCall)` pairs the predicate admits (and therefore the set of `select` rows the query produces) is unchanged on every database. The `bindingset` is honest: the only caller (line 215) binds both arguments via the `FloatStateFlow::flow` result and the `asIndirectExpr` constraints in the same `where` clause. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * [AI] More IRQL correctness tweaks * [AI] Reduce verbosity of AI comments + other cleanup. * Refactor a Supression predicate into a class * Address a few more review comments --------- Signed-off-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
NateD-MSFT
May 14, 2026 00:26
View session
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.
Summary
Standard integration PR bringing the
developmentbranch changes intomain.Merge type: Standard merge commit (no squash, no rebase)
Source branch:
development— will not be deleted after mergeChanges included