Skip to content

Commit

Permalink
Add IrqlSetTooHigh, IrqlSetTooLow queries + update IRQL library (#88)
Browse files Browse the repository at this point in the history 
* Initial work at IRQL-checking

* Significant extra IRQL work.

* In-progress work

* More puttering around with IRQL

* Update to CodeQL 2.14.4

Update cpp-all to 0.9.2, cpp-queries to 0.7.4

* Commit more IRQL code.  Needs cleanup.

* Some cleanup and minor fixes to entry IRQL evaluation.

* Replace old Irql high/low checks with new version and update library.

Still needs cleanup.

* Irql.qll cleanup

* Get rid of old prototype version of IrqlTooLow

* Update README.md

* Clean up file names

* Clean up queries.

* Update test script for IRQL queries.

* Update build-codeql.yaml

Signed-off-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com>

* Update ported_driver_ca_checks.qls

* Add IrqlSetTooHigh/IrqlSetTooLow queries.

* Bugfix for IrqlTooHigh/IrqlTooLow

The changes to Irql.qll needed for IrqlSetTooHigh, etc. means we are more likely to see IRQL evaluations that return -1.  Update queries to exclude those numbers.

* Fix test issues for several IRQL checks.

* WIP unit tests for IrqlSetTooHigh and IrqlSetTooLow queries

* WIP unit tests for IrqlSetTooHigh and IrqlSetTooLow queries

* WIP more tests and comments

* bug fixes

* WIP updates to tests

* WIP update tests

* remove bad tests. Fix run script to run all tests again. run script now cleans first automatically.

* update tests for IrqlSetTooHigh

* WIP IrqlSetTooLow tests

* Fix typo in Irql.qll

* Fix typo in Irql.qll

* irqlSetTooHigh tests remove calls to KeGetCurrentIRQL as they are not needed

* update IrqlSetTooLow tests

* update tests. line 90 should be a failling test but isnt

* fix IrqlLowerWithFunctionCall1 to call IrqlMinDispatchLowerIrql_fail1

* Revert"fix IrqlLowerWithFunctionCall1 to call IrqlMinDispatchLowerIrql_fail1"

This reverts commit fd9084b.

* fix IrqlLowerWithFunctionCall1 to call IrqlMinDispatchLowerIrql_fail1

* Add some interprocedural IRQL analysis + comments

* Add some interprocedural IRQL analysis + comments

* Fix typos

* Restore non-IRQL test results

* Fix bug in driver_snippet.c that stopped compilation.

* Fix bug in IrqlSetTooHigh

Also refactor IrqlSetTooLow to match

* Fix up test results for IRQL queries

* Fix typos

* Fix bug in IrqlSetTooHigh

Also refactor IrqlSetTooLow to match

* Fix line endings in diffs

* Regressions due to IRQL changes (+1 benign change)

* Update src/drivers/general/queries/experimental/IrqlSetTooHigh/driver_snippet.c

Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com>
Signed-off-by: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com>

* Update src/drivers/general/queries/experimental/IrqlSetTooHigh/driver_snippet.c

Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com>
Signed-off-by: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com>

* Update src/drivers/general/queries/experimental/IrqlSetTooHigh/driver_snippet.c

Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com>
Signed-off-by: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com>

* Update src/drivers/general/queries/experimental/IrqlSetTooHigh/driver_snippet.c

Co-authored-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com>
Signed-off-by: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com>

* Fix IrqlNotSaved with the new library.

* Update InitNotUsed with new DataFlow and a fix.

Removes a false positive in our unit tests.

* Update ported_driver_ca_checks.qls

---------

Signed-off-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com>
Signed-off-by: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com>
Co-authored-by: jacob-ronstadt <jacobronstadt@pm.me>
Co-authored-by: Jacob Ronstadt <147542405+jacob-ronstadt@users.noreply.github.com>
  • Loading branch information
@NateD-MSFT @jacob-ronstadt
3 people authored Oct 27, 2023
1 parent f0d7685 commit 984e2ad
Show file tree
Hide file tree
Showing 21 changed files with 1,771 additions and 231 deletions.
104 changes: 42 additions & 62 deletions src/drivers/general/queries/IrqlNotSaved/IrqlNotSaved.ql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,8 @@

import cpp
import drivers.libraries.Irql
import semmle.code.cpp.dataflow.DataFlow
import semmle.code.cpp.dataflow.DataFlow2

/**
* A function that has at least one parameter annotated with "\_IRQL\_save\_".
*/
class IrqlSaveFunction extends Function {
Parameter p;
int irqlIndex;

IrqlSaveFunction() {
p = this.getParameter(irqlIndex) and
p instanceof IrqlSaveParameter
}

int getIrqlIndex() { result = irqlIndex }
}
import semmle.code.cpp.dataflow.new.DataFlow
import semmle.code.cpp.dataflow.new.DataFlow2

/**
* A data-flow configuration describing flow from an
Expand All @@ -55,7 +40,12 @@ class IrqlFlowConfiguration extends DataFlow::Configuration {
override predicate isSink(DataFlow::Node sink) {
exists(FunctionCall fc, FundamentalIrqlSaveFunction fisf |
fc.getTarget() = fisf and
sink.asExpr() = fc.getArgument(fisf.getIrqlIndex())
(
sink.asExpr() =
fc.getArgument(fisf.(IrqlSavesGlobalAnnotatedFunction).getIrqlParameterSlot())
or
sink.asExpr() = fc.getArgument(fisf.(IrqlSavesToParameterFunction).getIrqlParameterSlot())
)
)
}
}
Expand All @@ -65,17 +55,25 @@ class IrqlFlowConfiguration extends DataFlow::Configuration {
* by the Windows OS itself. This is in general in a Windows Kits header. For
* extra clarity and internal use, we also list the exact header files.
*/
class FundamentalIrqlSaveFunction extends IrqlSaveFunction {
class FundamentalIrqlSaveFunction extends IrqlSavesFunction {
FundamentalIrqlSaveFunction() {
this.getFile().getAbsolutePath().matches("%Windows Kits%.h") or
this.getFile()
.getBaseName()
.matches(["wdm.h", "wdfsync.h", "ntifs.h", "ndis.h", "video.h", "wdfinterrupt.h"])
(
this.getFile().getAbsolutePath().matches("%Windows Kits%.h")
or
this.getFile()
.getBaseName()
.matches(["wdm.h", "wdfsync.h", "ntifs.h", "ndis.h", "video.h", "wdfinterrupt.h"])
) and
(
this instanceof IrqlSavesToParameterFunction or
this instanceof IrqlSavesViaReturnFunction or
this instanceof IrqlSavesGlobalAnnotatedFunction
)
}
}

/**
* A simple data flow from any IrqlSaveParameter to another variable.
* A simple data flow from any IrqlSaveParameter.
*/
class IrqlSaveParameterFlowConfiguration extends DataFlow2::Configuration {
IrqlSaveParameterFlowConfiguration() { this = "IrqlSaveParameterFlowConfiguration" }
Expand All @@ -84,7 +82,7 @@ class IrqlSaveParameterFlowConfiguration extends DataFlow2::Configuration {
source.asParameter() instanceof IrqlSaveParameter
}

override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof VariableAccess }
override predicate isSink(DataFlow::Node sink) { sink instanceof DataFlow::Node }
}

/**
Expand All @@ -97,29 +95,15 @@ class IrqlAssignmentFlowConfiguration extends DataFlow::Configuration {

override predicate isSource(DataFlow::Node source) {
source.asExpr() instanceof FunctionCall and
source
.asExpr()
.(FunctionCall)
.getTarget()
.getName()
.matches([
"KeRaiseIrqlToDpcLevel", "KfRaiseIrql", "KfAcquireSpinLock",
"KeAcquireSpinLockAtDpcLevel", "KeAcquireSpinLock", "KeAcquireSpinLockRaiseToDpc"
])
source.asExpr().(FunctionCall).getTarget() instanceof FundamentalIrqlSaveFunction and
source.asExpr().(FunctionCall).getTarget() instanceof IrqlSavesViaReturnFunction
}

override predicate isSink(DataFlow::Node sink) {
// Either we're sinking to a direct reference of a parameter, or...
sink.asExpr().(VariableAccess).getTarget() instanceof IrqlSaveParameter
or
// We a dereferenced pointer to the variable.
sink.asPartialDefinition()
.(PointerDereferenceExpr)
.getOperand()
.(AddressOfExpr)
.getOperand()
.(VariableAccess)
.getTarget() instanceof IrqlSaveVariableFlowedTo
exists(Assignment a |
a.getLValue().getAChild*().(VariableAccess).getTarget() instanceof IrqlSaveVariableFlowedTo and
a.getRValue() = sink.asExpr()
)
}
}

Expand All @@ -132,11 +116,14 @@ class IrqlSaveVariableFlowedTo extends Variable {

IrqlSaveVariableFlowedTo() {
exists(
IrqlSaveParameterFlowConfiguration difca, DataFlow::Node parameter, DataFlow::Node access
IrqlSaveParameterFlowConfiguration ispfc, DataFlow::Node parameter, DataFlow::Node assignment
|
access.asExpr().(VariableAccess).getTarget() = this and
(
this.getAnAssignedValue() = assignment.asExpr() or
this = assignment.asParameter()
) and
parameter.asParameter() = isp and
difca.hasFlow(parameter, access)
ispfc.hasFlow(parameter, assignment)
)
or
this = isp
Expand All @@ -150,26 +137,19 @@ where
// Exclude OS functions
not isp.getFunction() instanceof FundamentalIrqlSaveFunction and
/*
* Case one: does the IrqlSaveParameter (or an alias of it) have the IRQL assigned to it
* directly by calling, for example, KeRaiseIrql?
* Case one: does the IrqlSaveParameter (or an alias of it) have the IRQL assigned to it
* directly by calling, for example, KeRaiseIrql?
*/

not exists(
DataFlow::Node node, IrqlSaveVariableFlowedTo isvft, IrqlAssignmentFlowConfiguration difc
DataFlow::Node node, IrqlSaveVariableFlowedTo isvft, IrqlAssignmentFlowConfiguration iafc
|
isvft.getSaveParameter() = isp and
(
node.asExpr().(VariableAccess).getTarget() = isvft
or
node.asPartialDefinition()
.(PointerDereferenceExpr)
.getOperand()
.(AddressOfExpr)
.getOperand()
.(VariableAccess)
.getTarget() = isvft
exists(Assignment a |
a.getLValue().getAChild*().(VariableAccess).getTarget() = isvft and
a.getRValue() = node.asExpr()
) and
difc.hasFlow(_, node)
iafc.hasFlow(_, node)
) and
// Case two: is the IrqlSaveParameter passed into an OS function that will save a value to it?
not exists(DataFlow::Node node, IrqlFlowConfiguration ifc |
Expand Down
171 changes: 106 additions & 65 deletions src/drivers/general/queries/IrqlNotSaved/IrqlNotSaved.sarif
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,23 @@
"driver" : {
"name" : "CodeQL",
"organization" : "GitHub",
"semanticVersion" : "2.11.5",
"semanticVersion" : "2.14.4",
"notifications" : [ {
"id" : "cpp/baseline/expected-extracted-files",
"name" : "cpp/baseline/expected-extracted-files",
"shortDescription" : {
"text" : "Expected extracted files"
},
"fullDescription" : {
"text" : "Files appearing in the source archive that are expected to be extracted."
},
"defaultConfiguration" : {
"enabled" : true
},
"properties" : {
"tags" : [ "expected-extracted-files", "telemetry" ]
}
} ],
"rules" : [ {
"id" : "cpp/drivers/irql-not-saved",
"name" : "cpp/drivers/irql-not-saved",
Expand Down Expand Up @@ -42,7 +58,7 @@
},
"extensions" : [ {
"name" : "microsoft/windows-drivers",
"semanticVersion" : "0.1.0+933e876f096a70922173e4d5ad604d99d4481af4",
"semanticVersion" : "0.2.0+be14ce4fcaa9006e7636d8605fc53ea7c422a561",
"locations" : [ {
"uri" : "file:///C:/codeql-home/Windows-Driver-Developer-Supplemental-Tools/src/",
"description" : {
Expand All @@ -54,28 +70,99 @@
"text" : "The QL pack definition file."
}
} ]
} ]
},
"invocations" : [ {
"toolExecutionNotifications" : [ {
"locations" : [ {
"physicalLocation" : {
"artifactLocation" : {
"uri" : "driver/fail_driver1.c",
"uriBaseId" : "%SRCROOT%",
"index" : 1
}
}
} ],
"message" : {
"text" : ""
},
"level" : "none",
"descriptor" : {
"id" : "cpp/baseline/expected-extracted-files",
"index" : 0
},
"properties" : {
"formattedMessage" : {
"text" : ""
}
}
}, {
"name" : "legacy-upgrades",
"semanticVersion" : "0.0.0",
"locations" : [ {
"uri" : "file:///C:/codeql-home/codeql/legacy-upgrades/",
"description" : {
"text" : "The QL pack root directory."
"physicalLocation" : {
"artifactLocation" : {
"uri" : "driver/driver_snippet.c",
"uriBaseId" : "%SRCROOT%",
"index" : 0
}
}
}, {
"uri" : "file:///C:/codeql-home/codeql/legacy-upgrades/qlpack.yml",
"description" : {
"text" : "The QL pack definition file."
} ],
"message" : {
"text" : ""
},
"level" : "none",
"descriptor" : {
"id" : "cpp/baseline/expected-extracted-files",
"index" : 0
},
"properties" : {
"formattedMessage" : {
"text" : ""
}
} ]
} ]
},
}
}, {
"locations" : [ {
"physicalLocation" : {
"artifactLocation" : {
"uri" : "driver/fail_driver1.h",
"uriBaseId" : "%SRCROOT%",
"index" : 2
}
}
} ],
"message" : {
"text" : ""
},
"level" : "none",
"descriptor" : {
"id" : "cpp/baseline/expected-extracted-files",
"index" : 0
},
"properties" : {
"formattedMessage" : {
"text" : ""
}
}
} ],
"executionSuccessful" : true
} ],
"artifacts" : [ {
"location" : {
"uri" : "driver/driver_snippet.c",
"uriBaseId" : "%SRCROOT%",
"index" : 0
}
}, {
"location" : {
"uri" : "driver/fail_driver1.c",
"uriBaseId" : "%SRCROOT%",
"index" : 1
}
}, {
"location" : {
"uri" : "driver/fail_driver1.h",
"uriBaseId" : "%SRCROOT%",
"index" : 2
}
} ],
"results" : [ {
"ruleId" : "cpp/drivers/irql-not-saved",
Expand All @@ -85,7 +172,7 @@
"index" : 0
},
"message" : {
"text" : "The parameter [outIrql](1) is annotated \"_IRQL_saves_\" but never has the IRQL saved to it."
"text" : "The parameter [outIrqlFail](1) is annotated \"_IRQL_saves_\" but never has the IRQL saved to it."
},
"locations" : [ {
"physicalLocation" : {
Expand All @@ -97,12 +184,12 @@
"region" : {
"startLine" : 51,
"startColumn" : 44,
"endColumn" : 51
"endColumn" : 55
}
}
} ],
"partialFingerprints" : {
"primaryLocationLineHash" : "6276e1ac4864af21:1",
"primaryLocationLineHash" : "e054bbd9d7acc717:1",
"primaryLocationStartColumnFingerprint" : "43"
},
"relatedLocations" : [ {
Expand All @@ -116,57 +203,11 @@
"region" : {
"startLine" : 51,
"startColumn" : 44,
"endColumn" : 51
}
},
"message" : {
"text" : "outIrql"
}
} ]
}, {
"ruleId" : "cpp/drivers/irql-not-saved",
"ruleIndex" : 0,
"rule" : {
"id" : "cpp/drivers/irql-not-saved",
"index" : 0
},
"message" : {
"text" : "The parameter [myLock](1) is annotated \"_IRQL_saves_\" but never has the IRQL saved to it."
},
"locations" : [ {
"physicalLocation" : {
"artifactLocation" : {
"uri" : "driver/driver_snippet.c",
"uriBaseId" : "%SRCROOT%",
"index" : 0
},
"region" : {
"startLine" : 43,
"startColumn" : 48,
"endColumn" : 54
}
}
} ],
"partialFingerprints" : {
"primaryLocationLineHash" : "342705500599b584:1",
"primaryLocationStartColumnFingerprint" : "47"
},
"relatedLocations" : [ {
"id" : 1,
"physicalLocation" : {
"artifactLocation" : {
"uri" : "driver/driver_snippet.c",
"uriBaseId" : "%SRCROOT%",
"index" : 0
},
"region" : {
"startLine" : 43,
"startColumn" : 48,
"endColumn" : 54
"endColumn" : 55
}
},
"message" : {
"text" : "myLock"
"text" : "outIrqlFail"
}
} ]
} ],
Expand Down
Loading

0 comments on commit 984e2ad

Please sign in to comment.