diff --git a/src/drivers/apps/queries/experimental/UnsafeCallInGlobalInit/UnsafeCallInGlobalInit.md b/src/drivers/apps/queries/experimental/UnsafeCallInGlobalInit/UnsafeCallInGlobalInit.md new file mode 100644 index 00000000..dbaad342 --- /dev/null +++ b/src/drivers/apps/queries/experimental/UnsafeCallInGlobalInit/UnsafeCallInGlobalInit.md @@ -0,0 +1,51 @@ +# UnsafeCallInGlobalInit +When using a DLL, it is frequently the case that any static construtors are called from DllMain. There are a number of constraints that apply to calling other functions from DllMain. In particular, it is possible to create memory leaks if the DLL is loaded and unloaded dynamically. SysAllocString is an example of a function that, in this case, could cause a memory leak. + + +## Recommendation +The ideal DllMain would be just an empty stub. However, given the complexity of many applications, this is generally too restrictive. A good rule of thumb for DllMain is to postpone as much initialization as possible. Lazy initialization increases robustness of the application because this initialization is not performed while the loader lock is held. Also, lazy initialization enables you to safely use much more of the Windows API. + + +## Example +DLLMain function + +```c + + BOOL WINAPI DllMain( + HINSTANCE hinstDLL, // handle to DLL module + DWORD fdwReason, // reason for calling function + LPVOID lpvReserved ) // reserved + { + // Perform actions based on the reason for calling. + switch( fdwReason ) + { + case DLL_PROCESS_ATTACH: + // Initialize once for each new process. + // Return FALSE to fail DLL load. + break; + + case DLL_THREAD_ATTACH: + // Do thread-specific initialization. + break; + + case DLL_THREAD_DETACH: + // Do thread-specific cleanup. + break; + + case DLL_PROCESS_DETACH: + + if (lpvReserved != nullptr) + { + break; // do not do cleanup if process termination scenario + } + + // Perform any necessary cleanup. + break; + } + return TRUE; // Successful DLL_PROCESS_ATTACH. + } + +``` + +## References +* [ C28637 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28637-calling-function-in-a-global-initializer-is-unsafe) diff --git a/src/drivers/apps/queries/experimental/UnsafeCallInGlobalInit/UnsafeCallInGlobalInit.qhelp b/src/drivers/apps/queries/experimental/UnsafeCallInGlobalInit/UnsafeCallInGlobalInit.qhelp index 4c92a0b1..e5f5312b 100644 --- a/src/drivers/apps/queries/experimental/UnsafeCallInGlobalInit/UnsafeCallInGlobalInit.qhelp +++ b/src/drivers/apps/queries/experimental/UnsafeCallInGlobalInit/UnsafeCallInGlobalInit.qhelp @@ -47,15 +47,9 @@ break; } return TRUE; // Successful DLL_PROCESS_ATTACH. - } }]]> - + - -

- -

-
  • diff --git a/src/drivers/general/DriverAlertSuppression.md b/src/drivers/general/DriverAlertSuppression.md new file mode 100644 index 00000000..872073e5 --- /dev/null +++ b/src/drivers/general/DriverAlertSuppression.md @@ -0,0 +1 @@ +# Driver alert suppression diff --git a/src/drivers/general/queries/AnnotationSyntax/AnnotationSyntax.md b/src/drivers/general/queries/AnnotationSyntax/AnnotationSyntax.md new file mode 100644 index 00000000..99df186e --- /dev/null +++ b/src/drivers/general/queries/AnnotationSyntax/AnnotationSyntax.md @@ -0,0 +1,44 @@ +# Annotation syntax error +A syntax error in the annotations was found for the property in the function. + + +## Recommendation +This warning indicates an error in the annotations, not in the code that is being analyzed. + + +## Example +_IRQL_saves_global_ not applied to entire function + +```c + + // FAIL + VOID test1( + _IRQL_saves_global_(OldIrql, *Irql) PKIRQL Irql) + { + // ... + ; + } + +``` +_Kernel_clear_do_init_ not used with either "yes" or "no" + +```c + + // FAIL + _Function_class_(DRIVER_ADD_DEVICE) + _IRQL_requires_(PASSIVE_LEVEL) + _IRQL_requires_same_ + _Kernel_clear_do_init_(IRP_MJ_CREATE) + NTSTATUS + test4( + _In_ PDRIVER_OBJECT DriverObject, + _In_ PDEVICE_OBJECT PhysicalDeviceObject) + + { + ; // do nothing + } + +``` + +## References +* [ C28266 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28266-function-property-syntax-error) diff --git a/src/drivers/general/queries/AnnotationSyntax/AnnotationSyntax.qhelp b/src/drivers/general/queries/AnnotationSyntax/AnnotationSyntax.qhelp index b4976dba..a69dbff4 100644 --- a/src/drivers/general/queries/AnnotationSyntax/AnnotationSyntax.qhelp +++ b/src/drivers/general/queries/AnnotationSyntax/AnnotationSyntax.qhelp @@ -21,7 +21,6 @@ { // ... ; - } }]]>

    @@ -40,14 +39,9 @@ { ; // do nothing - } }]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/CurrentFunctionTypeNotCorrect/CurrentFunctionTypeNotCorrect.md b/src/drivers/general/queries/CurrentFunctionTypeNotCorrect/CurrentFunctionTypeNotCorrect.md new file mode 100644 index 00000000..8d591f05 --- /dev/null +++ b/src/drivers/general/queries/CurrentFunctionTypeNotCorrect/CurrentFunctionTypeNotCorrect.md @@ -0,0 +1,10 @@ +# Current function type not correct (C28101) +This function appears to be an unannotated DriverEntry function + + +## Recommendation +DriverEntry functions should be declared using the DRIVER_INITIALIZE function typedef. + + +## References +* [ C28101 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28101-wrong-function-type) diff --git a/src/drivers/general/queries/DefaultPoolTag/DefaultPoolTag.md b/src/drivers/general/queries/DefaultPoolTag/DefaultPoolTag.md new file mode 100644 index 00000000..df4cb93f --- /dev/null +++ b/src/drivers/general/queries/DefaultPoolTag/DefaultPoolTag.md @@ -0,0 +1,30 @@ +# Use of default pool tag in memory allocation (C28147) +Memory should not be allocated with the default tags of ' mdW' or ' kdD'. + + +## Recommendation +The driver is specifying a default pool tag. Because the system tracks pool use by pool tag, only those drivers that use a unique pool tag can identify and distinguish their pool use. + + +## Example +In this example, the driver allocates memory with the default tag: + +```c + + PVOID InternalNonPagedAllocator(SIZE_T size) { + return ExAllocatePool3(POOL_FLAG_NON_PAGED, size, ' mdW'); + } + +``` +The driver should use a custom tag instead: + +```c + + PVOID InternalNonPagedAllocator(SIZE_T size) { + return ExAllocatePool3(POOL_FLAG_NON_PAGED, size, 'vdxE'); + } + +``` + +## References +* [ C28147 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28147-improper-use-of-default-pool-tag) diff --git a/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.md b/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.md new file mode 100644 index 00000000..7237b94b --- /dev/null +++ b/src/drivers/general/queries/DriverEntrySaveBuffer/DriverEntrySaveBuffer.md @@ -0,0 +1,85 @@ +# Driver Entry Save Buffer +The DriverEntry routine should save a copy of the argument, not the pointer, because the I/O Manager frees the buffer + + +## Recommendation +The driver's DriverEntry routine is saving a copy of the pointer to the buffer instead of saving a copy of the buffer. Because the buffer is freed when the DriverEntry routine returns, the pointer to the buffer will soon be invalid. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// +#include "ntstrsafe.h" + +#define SET_DISPATCH 1 +// Template. Not called in this test. +void top_level_call() {} + +PUNICODE_STRING g_RP1; + +NTSTATUS +DriverEntryBad( + PDRIVER_OBJECT DriverObject, + PUNICODE_STRING RegistryPath + ) +{ + g_RP1 = RegistryPath; + return 0; +} + + +UNICODE_STRING g_RP2; + +NTSTATUS +DriverEntryGood( + PDRIVER_OBJECT DriverObject, + PUNICODE_STRING RegistryPath + ) +{ + return RtlUnicodeStringCopy(&g_RP2,RegistryPath); +} + + +UNICODE_STRING g_RP3; + +NTSTATUS +DriverEntryGood2( + PDRIVER_OBJECT DriverObject, + PUNICODE_STRING RegistryPath + ) +{ + g_RP3 = *RegistryPath; + return 0; +} + +typedef struct _test_struct { + int a; + PUNICODE_STRING g_RP4; + char b; +} test_struct; + +test_struct g_test_struct; + +NTSTATUS +DriverEntryBad2( + PDRIVER_OBJECT DriverObject, + PUNICODE_STRING RegistryPath + ) +{ + test_struct* localPtr = &g_test_struct; + localPtr->g_RP4 = RegistryPath; + return 0; +} +``` + +## References +* [ C28131 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28131-driverentry-saving-pointer-to-buffer) + +## Semmle-specific notes +This rule reports a false positive when the registry path pointer is saved for use in functions such as HidRegisterMinidriver + diff --git a/src/drivers/general/queries/ExaminedValue/ExaminedValue.md b/src/drivers/general/queries/ExaminedValue/ExaminedValue.md new file mode 100644 index 00000000..eb405911 --- /dev/null +++ b/src/drivers/general/queries/ExaminedValue/ExaminedValue.md @@ -0,0 +1,42 @@ +# Return value not examined (C28193) +This warning indicates that the calling function is not checking the value of the specified variable, which was supplied by a function. + + +## Recommendation +Make sure to check the result of the function that is annotated with _Check_result_ or _Must_check_result. + + +## Example +In this example, the driver tries to acquire a mutex but does not check the return value. This can cause a concurrency bug. + +```c + + KeTryToAcquireGuardedMutex(&sharedMutex); + DoDriverWork(); + + +``` +The driver should check if the mutex was successfully acquired before using it: + +```c + + if(KeTryToAcquireGuardedMutex(&sharedMutex)) + { + DoDriverWork(); + } + else + { + // ... + } + + +``` + +## References +* [ Warning C28193 | Microsoft Learn ](https://docs.microsoft.com/en-us/cpp/code-quality/c28193) + +## Semmle-specific notes +To reduce noise, this rule only reports violations if more than 75% of the other calls to this function have their return values checked. + +Note that this will still report issues if the value is only checked via ASSERTs that are compiled away at release time. + diff --git a/src/drivers/general/queries/ExtendedDeprecatedApis/ExtendedDeprecatedApis.md b/src/drivers/general/queries/ExtendedDeprecatedApis/ExtendedDeprecatedApis.md new file mode 100644 index 00000000..856a9a71 --- /dev/null +++ b/src/drivers/general/queries/ExtendedDeprecatedApis/ExtendedDeprecatedApis.md @@ -0,0 +1,41 @@ +# Use of deprecated function or macro (C28719, C28726, C28735, C28750) +Deprecated, insecure APIs should not be used. + + +## Recommendation +Instead of using deprecated APIs, driver developers should refer to the output of the rule and replace any calls to deprecated APIs with the recommended replacements. These recommendations are based on the MSDN articles for Code Analysis rules C28719, C28726, C28735, and C28750, as linked in the references section below. If no recommended replacement is provided, developers should investigate alternate code or APIs for attaining the same functionality. + +Auto-generated WPP TMH headers are likely to flag this rule due to use of tracing APIs that are no longer meant for public use but are safely used in autogenerated code. These results can be ignored. + + +## Example +The driver attempts to call a deprecated string function: + +```c + + void example_func(PSTR src) + { + char dst[100]; + strcpy(dst, src); + } + + +``` +The driver should use a supported newer function: + +```c + + void example_func(PSTR src) + { + char dst[100]; + strcpy_s(dst, sizeof(dst), src); + } + + +``` + +## References +* [ C28719 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28719-banned-api-usage-use-updated-function-replacement) +* [ C28726 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28726-banned-api-usage-use-updated-function-replacement) +* [ C28735 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28735-banned-crimson-api-usage) +* [ C28750 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28750-banned-istrlen-usage) diff --git a/src/drivers/general/queries/FloatHardwareStateProtection/FloatHardwareStateProtection.md b/src/drivers/general/queries/FloatHardwareStateProtection/FloatHardwareStateProtection.md new file mode 100644 index 00000000..5bb2fa95 --- /dev/null +++ b/src/drivers/general/queries/FloatHardwareStateProtection/FloatHardwareStateProtection.md @@ -0,0 +1,38 @@ +# Float Hardware State Protection +Drivers must protect floating-point hardware state. + + +## Recommendation +This warning is only applicable in kernel mode. The driver is attempting to use a variable or constant of a float type when the code is not protected by KeSaveFloatingPointState and KeRestoreFloatingPointState, or EngSaveFloatingPointState and EngRestoreFloatingPointState. Display drivers should use EngSaveFloatingPointState and EngRestoreFloatingPointState. + + +## Example +Function that uses float without protecting floating-point hardware state + +```c + + void float_used_bad() + { + float f = 0.0f; + f = f + 1.0f; + } + +``` +Function that uses float with protected floating-point hardware state + +```c + + KFLOATING_SAVE saveData; + NTSTATUS status; + float f = 0.0f; + status = KeSaveFloatingPointState(&saveData); + for (int i = 0; i < 100; i++) + { + f = f + 1.0f; + } + KeRestoreFloatingPointState(&saveData); + +``` + +## References +* [ Warning C28110 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28110-floating-point-hardware-protect) diff --git a/src/drivers/general/queries/FloatHardwareStateProtection/FloatHardwareStateProtection.qhelp b/src/drivers/general/queries/FloatHardwareStateProtection/FloatHardwareStateProtection.qhelp index 3366e055..d6e87737 100644 --- a/src/drivers/general/queries/FloatHardwareStateProtection/FloatHardwareStateProtection.qhelp +++ b/src/drivers/general/queries/FloatHardwareStateProtection/FloatHardwareStateProtection.qhelp @@ -19,7 +19,6 @@ { float f = 0.0f; f = f + 1.0f; - } }]]>

    @@ -34,14 +33,9 @@ { f = f + 1.0f; } - KeRestoreFloatingPointState(&saveData); - }]]> + KeRestoreFloatingPointState(&saveData);]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/IRPStackEntryCopy/IRPStackEntryCopy.md b/src/drivers/general/queries/IRPStackEntryCopy/IRPStackEntryCopy.md new file mode 100644 index 00000000..5cb6bae7 --- /dev/null +++ b/src/drivers/general/queries/IRPStackEntryCopy/IRPStackEntryCopy.md @@ -0,0 +1,51 @@ +# Irp stack entry copy +Copying a whole IRP stack entry leaves certain fields initialized that should be cleared or updated + + +## Recommendation +The driver is copying an IRP improperly. Improperly copying an IRP can cause serious problems with a driver, including loss of data and system crashes. If an IRP must be copied and IoCopyCurrentIrpStackLocationToNext does not suffice, then certain members of the IRP should not be copied or should be zeroed after copying. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// + +#define SET_DISPATCH 1 +// Template. Not called in this test. +void top_level_call() {} + +void bad_irp_copy( + PIRP irp) +{ + PIO_STACK_LOCATION irpSp; + PIO_STACK_LOCATION nextIrpSp; + irpSp = IoGetCurrentIrpStackLocation(irp); + nextIrpSp = IoGetNextIrpStackLocation(irp); + RtlCopyMemory(nextIrpSp, irpSp, 0x24); + nextIrpSp->Control = 0; +} +void bad_irp_copy2( + PIRP irp) +{ + PIO_STACK_LOCATION irpSp; + PIO_STACK_LOCATION nextIrpSp; + irpSp = IoGetCurrentIrpStackLocation(irp); + nextIrpSp = IoGetNextIrpStackLocation(irp); + RtlCopyMemory(nextIrpSp+4, irpSp+4, FIELD_OFFSET(IO_STACK_LOCATION, DeviceObject)-4); + nextIrpSp->Control = 0; +} +void good_irp_copy( + PIRP irp) +{ + IoCopyCurrentIrpStackLocationToNext(irp); +} + +``` + +## References +* [ C28114 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28114-improper-irp-stack-copy) diff --git a/src/drivers/general/queries/ImportantFunctionCallOptimizedOut/ImportantFunctionCallOptimizedOut.md b/src/drivers/general/queries/ImportantFunctionCallOptimizedOut/ImportantFunctionCallOptimizedOut.md new file mode 100644 index 00000000..6ad2ed4d --- /dev/null +++ b/src/drivers/general/queries/ImportantFunctionCallOptimizedOut/ImportantFunctionCallOptimizedOut.md @@ -0,0 +1,41 @@ +# Important function call optimized out +Function call used to clear sensitive data will be optimized away + + +## Recommendation +This Function call may be optimized away during compile, resulting in sensitive data lingering in memory. Use SecureZeroMemory or RtlSecureZeroMemory instead. A heuristic looks for identifier names that contain items such as "key" or "pass" to trigger this warning. + + +## Example +Example of instance where function call may be optimized away + +```c + + void bad_func() + { + char Password[100]; + + /* + * The Buffer will be going out of scope + * anyway so the compiler optimises away + * the following + */ + ZeroMemory(Password, sizeof(Password)); + } + +``` +Using SecureZeroMemory or RtlSecureZeroMemory will prevent the compiler from optimizing away the function call. + +```c + + void good_func() + { + char Password[100]; + + RtlSecureZeroMemory(Password, sizeof(Password)); + } + +``` + +## References +* [ C28625 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28625-sensitive-data-may-be-retained) diff --git a/src/drivers/general/queries/ImportantFunctionCallOptimizedOut/ImportantFunctionCallOptimizedOut.qhelp b/src/drivers/general/queries/ImportantFunctionCallOptimizedOut/ImportantFunctionCallOptimizedOut.qhelp index 6de83cb5..538985e7 100644 --- a/src/drivers/general/queries/ImportantFunctionCallOptimizedOut/ImportantFunctionCallOptimizedOut.qhelp +++ b/src/drivers/general/queries/ImportantFunctionCallOptimizedOut/ImportantFunctionCallOptimizedOut.qhelp @@ -28,7 +28,6 @@ * the following */ ZeroMemory(Password, sizeof(Password)); - } }]]>

    @@ -40,14 +39,9 @@ char Password[100]; RtlSecureZeroMemory(Password, sizeof(Password)); - } }]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/ImproperNotOperatorOnZero/ImproperNotOperatorOnZero.md b/src/drivers/general/queries/ImproperNotOperatorOnZero/ImproperNotOperatorOnZero.md new file mode 100644 index 00000000..b51ee627 --- /dev/null +++ b/src/drivers/general/queries/ImproperNotOperatorOnZero/ImproperNotOperatorOnZero.md @@ -0,0 +1,22 @@ +# Improper Not Operator On Zero +The type for which !0 is being used does not treat it as failure case. Returning a status value such as !TRUE is not the same as returning a status value that indicates failure. + + +## Recommendation +Certain data types such as NTSTATUS and HRESULT have associated macros that classify values of these types into SUCCESS or FAILURE. These macros check the most significant bit of the returned value or values to determine this. Thus, 0 and 1 are both classified as SUCCESS values. The proper way to fix this warning is to return a proper error code instead of a generic value such as -1. + + +## Example +Returning !0 instead of a proper error code. + +```c + + NTSTATUS test_func() + { + return !0; + } + +``` + +## References +* [ C28650 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28650-generic-value-is-not-treated-as-failure) diff --git a/src/drivers/general/queries/ImproperNotOperatorOnZero/ImproperNotOperatorOnZero.qhelp b/src/drivers/general/queries/ImproperNotOperatorOnZero/ImproperNotOperatorOnZero.qhelp index aa9abeba..7c49ea2a 100644 --- a/src/drivers/general/queries/ImproperNotOperatorOnZero/ImproperNotOperatorOnZero.qhelp +++ b/src/drivers/general/queries/ImproperNotOperatorOnZero/ImproperNotOperatorOnZero.qhelp @@ -19,14 +19,9 @@ NTSTATUS test_func() { return !0; - } }]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/InvalidFunctionClassTypedef/InvalidFunctionClassTypedef.md b/src/drivers/general/queries/InvalidFunctionClassTypedef/InvalidFunctionClassTypedef.md new file mode 100644 index 00000000..c98c0972 --- /dev/null +++ b/src/drivers/general/queries/InvalidFunctionClassTypedef/InvalidFunctionClassTypedef.md @@ -0,0 +1,34 @@ +# Invalid Function Class Typedef +The function class on the function does not match the function class on the typedef used here + + +## Recommendation +This warning indicates that an annotation on a function class does not match the function type, as specified by the type declaration. This warning indicates an error in the annotations, not in the code that is being analyzed. + + +## Example +Example where typedef of one type is used to declare the function but typedef of a different type is used in the function definition inside __drv_functionClass + +```c + + typedef __drv_functionClass(TEST_ROUTINE) + VOID + TEST_ROUTINE( + VOID); + typedef TEST_ROUTINE *PTEST_ROUTINE; + + // declare function with above typedef + TEST_ROUTINE func2; + + // define function, but with different function class which causes the warning + __drv_functionClass(TEST_ROUTINE2) + VOID func2( + VOID) + { + ; // Don't need to do anything heres + } + +``` + +## References +* [ C28268 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28268-function-class-does-not-match-typedef) diff --git a/src/drivers/general/queries/InvalidFunctionClassTypedef/InvalidFunctionClassTypedef.qhelp b/src/drivers/general/queries/InvalidFunctionClassTypedef/InvalidFunctionClassTypedef.qhelp index 8dc627dc..57d193b3 100644 --- a/src/drivers/general/queries/InvalidFunctionClassTypedef/InvalidFunctionClassTypedef.qhelp +++ b/src/drivers/general/queries/InvalidFunctionClassTypedef/InvalidFunctionClassTypedef.qhelp @@ -30,15 +30,10 @@ VOID) { ; // Don't need to do anything heres - } - }]]> + }]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/InvalidFunctionPointerAnnotation/InvalidFunctionPointerAnnotation.md b/src/drivers/general/queries/InvalidFunctionPointerAnnotation/InvalidFunctionPointerAnnotation.md new file mode 100644 index 00000000..90c58cd0 --- /dev/null +++ b/src/drivers/general/queries/InvalidFunctionPointerAnnotation/InvalidFunctionPointerAnnotation.md @@ -0,0 +1,19 @@ +# Invalid Function Pointer Annotation +The function pointer annotation class does not match the function class + + +## Recommendation +A function pointer has a __drv_functionClass annotation that specifies that only functions of a particular functional class should be assigned to it. In an assignment or implied assignment in a function call, the source and target must be of the same function class, but the function classes do not match. + + +## Example +A call to IoQueueWorkItem is expecting a function pointer annotated with __drv_functionClass(IO_WORKITEM_ROUTINE) but in this example, badAnnotationFunc1 is annotated with __drv_functionClass(IO_TIMER_ROUTINE). + +```c + + IoQueueWorkItem(IoWorkItem, badAnnotationFunc1, DelayedWorkQueue, Context); + +``` + +## References +* [ Warning C28165 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28165-class-function-pointer-mismatch) diff --git a/src/drivers/general/queries/InvalidFunctionPointerAnnotation/InvalidFunctionPointerAnnotation.qhelp b/src/drivers/general/queries/InvalidFunctionPointerAnnotation/InvalidFunctionPointerAnnotation.qhelp index 66196a30..2c74c316 100644 --- a/src/drivers/general/queries/InvalidFunctionPointerAnnotation/InvalidFunctionPointerAnnotation.qhelp +++ b/src/drivers/general/queries/InvalidFunctionPointerAnnotation/InvalidFunctionPointerAnnotation.qhelp @@ -15,15 +15,10 @@ A call to IoQueueWorkItem is expecting a function pointer annotated with __drv_functionClass(IO_WORKITEM_ROUTINE) but in this example, badAnnotationFunc1 is annotated with __drv_functionClass(IO_TIMER_ROUTINE).

    + IoQueueWorkItem(IoWorkItem, badAnnotationFunc1, DelayedWorkQueue, Context);]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/IoInitializeTimerCall/IoInitializeTimerCall.md b/src/drivers/general/queries/IoInitializeTimerCall/IoInitializeTimerCall.md new file mode 100644 index 00000000..2288fbbe --- /dev/null +++ b/src/drivers/general/queries/IoInitializeTimerCall/IoInitializeTimerCall.md @@ -0,0 +1,10 @@ +# IoInitializeTimer is best called from AddDevice +IoInitializeTimer is best called from AddDevice + + +## Recommendation +IoInitializeTimer can only be called once per device object. Calling it from the AddDevice routine helps assure that it is not unexpectedly called more than once. + + +## References +* [ C28133 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28133-ioinitializetimer-is-best-called-from-add-device) diff --git a/src/drivers/general/queries/IrqlAnnotationIssue/IrqlAnnotationIssue.md b/src/drivers/general/queries/IrqlAnnotationIssue/IrqlAnnotationIssue.md new file mode 100644 index 00000000..14c915f4 --- /dev/null +++ b/src/drivers/general/queries/IrqlAnnotationIssue/IrqlAnnotationIssue.md @@ -0,0 +1,26 @@ +# Irql Annotation Issue +The value for an IRQL from annotation could not be evaluated in this context. + + +## Recommendation +This warning indicates that the Code Analysis tool cannot interpret the function annotation because the annotation is not coded correctly. As a result, the Code Analysis tool cannot determine the specified IRQL value. This warning can occur with any of the driver-specific annotations that mention an IRQL when the Code Analysis tool cannot evaluate the expression for the IRQL. + + +## Example +Incorrect IRQL annotation + +```c + + _IRQL_requires_(65) + +``` +Incorrect IRQL annotation + +```c + + _IRQL_always_function_max_(irql_variable) + +``` + +## References +* [ C28153 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28153-irql-annotation-eval-context) diff --git a/src/drivers/general/queries/IrqlAnnotationIssue/IrqlAnnotationIssue.qhelp b/src/drivers/general/queries/IrqlAnnotationIssue/IrqlAnnotationIssue.qhelp index 0216e52e..679f7d88 100644 --- a/src/drivers/general/queries/IrqlAnnotationIssue/IrqlAnnotationIssue.qhelp +++ b/src/drivers/general/queries/IrqlAnnotationIssue/IrqlAnnotationIssue.qhelp @@ -17,21 +17,15 @@ Incorrect IRQL annotation

    + _IRQL_requires_(65)]]>

    Incorrect IRQL annotation

    + _IRQL_always_function_max_(irql_variable)]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/IrqlCancelRoutine/IrqlCancelRoutine.md b/src/drivers/general/queries/IrqlCancelRoutine/IrqlCancelRoutine.md new file mode 100644 index 00000000..d5d1bef0 --- /dev/null +++ b/src/drivers/general/queries/IrqlCancelRoutine/IrqlCancelRoutine.md @@ -0,0 +1,26 @@ +# Irql Cancel Routine +Within a cancel routine, at the point of exit, the IRQL in Irp->CancelIrql should be the current IRQL. + + +## Recommendation +When the driver's Cancel routine exits, the value of the Irp->CancelIrql member is not the current IRQL. Typically, this error occurs when the driver does not call IoReleaseCancelSpinLock with the IRQL that was supplied by the most recent call to IoAcquireCancelSpinLock. + + +## Example +The following example shows an incorrect use of IoReleaseCncelSpinLock within a cancel routine + +```c + + IoReleaseCancelSpinLock(PASSIVE_LEVEL); + +``` +Correct use of IoReleaseCncelSpinLock within a cancel routine + +```c + + IoReleaseCancelSpinLock(Irp->CancelIrql); + +``` + +## References +* [ C28144 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28144-cancelirql-should-be-current-irql) diff --git a/src/drivers/general/queries/IrqlCancelRoutine/IrqlCancelRoutine.qhelp b/src/drivers/general/queries/IrqlCancelRoutine/IrqlCancelRoutine.qhelp index bbe722d3..ece0e4c6 100644 --- a/src/drivers/general/queries/IrqlCancelRoutine/IrqlCancelRoutine.qhelp +++ b/src/drivers/general/queries/IrqlCancelRoutine/IrqlCancelRoutine.qhelp @@ -17,21 +17,15 @@ The following example shows an incorrect use of IoReleaseCncelSpinLock within a cancel routine

    + IoReleaseCancelSpinLock(PASSIVE_LEVEL);]]>

    Correct use of IoReleaseCncelSpinLock within a cancel routine

    CancelIrql); - }]]> + IoReleaseCancelSpinLock(Irp->CancelIrql);]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/IrqlFloatStateMismatch/IrqlFloatStateMismatch.md b/src/drivers/general/queries/IrqlFloatStateMismatch/IrqlFloatStateMismatch.md new file mode 100644 index 00000000..eb7bd233 --- /dev/null +++ b/src/drivers/general/queries/IrqlFloatStateMismatch/IrqlFloatStateMismatch.md @@ -0,0 +1,54 @@ +# Irql Float State Mismatch +The IRQL where the floating-point state was saved does not match the current IRQL (for this restore operation). + + +## Recommendation +The IRQL at which the driver is executing when it restores a floating-point state is different than the IRQL at which it was executing when it saved the floating-point state. Because the IRQL at which the driver runs determines how the floating-point state is saved, the driver must be executing at the same IRQL when it calls the functions to save and to restore the floating-point state. + + +## Example +Example of incorrect code. Floating point state was saved at APC_LEVEL but restored at PASSIVE_LEVEL + +```c + + _IRQL_requires_(PASSIVE_LEVEL) + void driver_utility_bad(void) + { + KIRQL oldIRQL; + KeRaiseIrql(APC_LEVEL, &oldIRQL); + // running at APC level + KFLOATING_SAVE FloatBuf; + if (KeSaveFloatingPointState(&FloatBuf)) + { + KeLowerIrql(oldIRQL); // lower back to PASSIVE_LEVEL + // ... + KeRestoreFloatingPointState(&FloatBuf); + } + } + +``` +Correct example + +```c + + _IRQL_requires_(PASSIVE_LEVEL) + void driver_utility_good(void) + { + // running at APC level + KFLOATING_SAVE FloatBuf; + KIRQL oldIRQL; + KeRaiseIrql(APC_LEVEL, &oldIRQL); + + if (KeSaveFloatingPointState(&FloatBuf)) + { + KeLowerIrql(oldIRQL); + // ... + KeRaiseIrql(APC_LEVEL, &oldIRQL); + KeRestoreFloatingPointState(&FloatBuf); + } + } + +``` + +## References +* [ C28111 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28111-floating-point-irql-mismatch) diff --git a/src/drivers/general/queries/IrqlFloatStateMismatch/IrqlFloatStateMismatch.qhelp b/src/drivers/general/queries/IrqlFloatStateMismatch/IrqlFloatStateMismatch.qhelp index a41dac90..b3e1b967 100644 --- a/src/drivers/general/queries/IrqlFloatStateMismatch/IrqlFloatStateMismatch.qhelp +++ b/src/drivers/general/queries/IrqlFloatStateMismatch/IrqlFloatStateMismatch.qhelp @@ -29,8 +29,6 @@ // ... KeRestoreFloatingPointState(&FloatBuf); } - } - }]]>

    @@ -52,19 +50,13 @@ KeRaiseIrql(APC_LEVEL, &oldIRQL); KeRestoreFloatingPointState(&FloatBuf); } - } - }]]> + }]]> - -

    - TODO notes -

    -
  • - - Example link + + C28111
    • diff --git a/src/drivers/general/queries/IrqlFunctionNotAnnotated/IrqlFunctionNotAnnotated.md b/src/drivers/general/queries/IrqlFunctionNotAnnotated/IrqlFunctionNotAnnotated.md new file mode 100644 index 00000000..756190f1 --- /dev/null +++ b/src/drivers/general/queries/IrqlFunctionNotAnnotated/IrqlFunctionNotAnnotated.md @@ -0,0 +1,30 @@ +# Irql Function Not Annotated +The function changes the IRQL and does not restore the IRQL before it exits. It should be annotated to reflect the change or the IRQL should be restored. + + +## Recommendation +This warning indicates that the following conditions are true: 1. The function changes the IRQL at which the driver is running. 2. There is at least one path through a function that does not, by function exit, restore the IRQL to the original IRQL that the driver was running at function entry. + + +## Example +Function which potentially raises the IRQL level but is not annotated to reflect the change. + +```c + + void fail1(PKIRQL oldIrql) + { + + if (oldIrql == PASSIVE_LEVEL) + { + KeLowerIrql(*oldIrql); + } + else + { + KeRaiseIrql(DISPATCH_LEVEL, oldIrql); // Function exits at DISPATCH_LEVEL + } + } + +``` + +## References +* [ C28167 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28167-function-changes-irql-without-restore) diff --git a/src/drivers/general/queries/IrqlFunctionNotAnnotated/IrqlFunctionNotAnnotated.qhelp b/src/drivers/general/queries/IrqlFunctionNotAnnotated/IrqlFunctionNotAnnotated.qhelp index 4bca85d4..acf583b7 100644 --- a/src/drivers/general/queries/IrqlFunctionNotAnnotated/IrqlFunctionNotAnnotated.qhelp +++ b/src/drivers/general/queries/IrqlFunctionNotAnnotated/IrqlFunctionNotAnnotated.qhelp @@ -28,14 +28,9 @@ { KeRaiseIrql(DISPATCH_LEVEL, oldIrql); // Function exits at DISPATCH_LEVEL } - } }]]> - + - -

    -

    -
  • diff --git a/src/drivers/general/queries/IrqlIllegalValue/IrqlIllegalValue.md b/src/drivers/general/queries/IrqlIllegalValue/IrqlIllegalValue.md new file mode 100644 index 00000000..55c3cb2d --- /dev/null +++ b/src/drivers/general/queries/IrqlIllegalValue/IrqlIllegalValue.md @@ -0,0 +1,37 @@ +# Irql Illegal Value +The value is not a legal value for an IRQL + + +## Recommendation +The IRQL should be within the range of valid values for an IRQL (0-31). + + +## Example +Example of a function with an OK IRQL requirement. + +```c + + + _IRQL_requires_(PASSIVE_LEVEL) + VOID DoNothing_RequiresPassive(void) + { + __noop; + } + +``` +Exmaple of a function with an IRQL requirement that is too high. + +```c + + + // This function has an IRQL requirement that is too high. + _IRQL_requires_(42) + VOID DoNothing_bad(void) + { + __noop; + } + +``` + +## References +* [ Warning C28151 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28151-illegal-irql-value) diff --git a/src/drivers/general/queries/IrqlIllegalValue/IrqlIllegalValue.qhelp b/src/drivers/general/queries/IrqlIllegalValue/IrqlIllegalValue.qhelp index 62e75eef..c13ee5c1 100644 --- a/src/drivers/general/queries/IrqlIllegalValue/IrqlIllegalValue.qhelp +++ b/src/drivers/general/queries/IrqlIllegalValue/IrqlIllegalValue.qhelp @@ -20,8 +20,6 @@ VOID DoNothing_RequiresPassive(void) { __noop; - } - }]]>

    @@ -34,15 +32,9 @@ VOID DoNothing_bad(void) { __noop; - } }]]> - -

    - -

    -
  • diff --git a/src/drivers/general/queries/IrqlInconsistentWithRequired/IrqlInconsistentWithRequired.md b/src/drivers/general/queries/IrqlInconsistentWithRequired/IrqlInconsistentWithRequired.md new file mode 100644 index 00000000..8e2760ea --- /dev/null +++ b/src/drivers/general/queries/IrqlInconsistentWithRequired/IrqlInconsistentWithRequired.md @@ -0,0 +1,30 @@ +# Irql Inconsistent With Required +The actual IRQL is inconsistent with the required IRQL + + +## Recommendation +An _IRQL_requires_same_ annotation specifies that the driver should be executing at a particular IRQL when the function completes, but there is at least one path in which the driver is executing at a different IRQL when the function completes. + + +## Example +Function annotated with _IRQL_requires_same_ but can possibly exit at a different IRQL level. + +```c + + _IRQL_requires_same_ void fail1(PKIRQL oldIrql) + { + + if (oldIrql == PASSIVE_LEVEL) + { + KeLowerIrql(*oldIrql); + } + else + { + KeRaiseIrql(DISPATCH_LEVEL, oldIrql); // Function exits at DISPATCH_LEVEL + } + } + +``` + +## References +* [ C28166 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28166-function-does-not-restore-irql-value) diff --git a/src/drivers/general/queries/IrqlInconsistentWithRequired/IrqlInconsistentWithRequired.qhelp b/src/drivers/general/queries/IrqlInconsistentWithRequired/IrqlInconsistentWithRequired.qhelp index 7742155a..1651c005 100644 --- a/src/drivers/general/queries/IrqlInconsistentWithRequired/IrqlInconsistentWithRequired.qhelp +++ b/src/drivers/general/queries/IrqlInconsistentWithRequired/IrqlInconsistentWithRequired.qhelp @@ -26,14 +26,9 @@ { KeRaiseIrql(DISPATCH_LEVEL, oldIrql); // Function exits at DISPATCH_LEVEL } - } }]]> - + - -

    -

    -
  • diff --git a/src/drivers/general/queries/IrqlLoweredImproperly/IrqlLoweredImproperly.md b/src/drivers/general/queries/IrqlLoweredImproperly/IrqlLoweredImproperly.md new file mode 100644 index 00000000..ef1fb599 --- /dev/null +++ b/src/drivers/general/queries/IrqlLoweredImproperly/IrqlLoweredImproperly.md @@ -0,0 +1,28 @@ +# IRQL Lowered Improperly +The argument causes the IRQ Level to be set below the current IRQL, and this function cannot be used for that purpose + + +## Recommendation +A function call that lowers the IRQL at which a caller is executing is being used inappropriately. Typically, the function call lowers the IRQL as part of a more general routine or is intended to raise the caller's IRQL. + + +## Example +The following code example elicits this warning. + +```c + + KeRaiseIrql(DISPATCH_LEVEL, &OldIrql); + KeRaiseIrql(PASSIVE_LEVEL, &OldIrql); + +``` +The following code example avoids this warning. + +```c + + KeRaiseIrql(DISPATCH_LEVEL, &OldIrql); + KeLowerIrql(OldIrql); + +``` + +## References +* [ C28141 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28141-argument-lowers-irq-level) diff --git a/src/drivers/general/queries/IrqlLoweredImproperly/IrqlLoweredImproperly.qhelp b/src/drivers/general/queries/IrqlLoweredImproperly/IrqlLoweredImproperly.qhelp index ea06606d..47989cb1 100644 --- a/src/drivers/general/queries/IrqlLoweredImproperly/IrqlLoweredImproperly.qhelp +++ b/src/drivers/general/queries/IrqlLoweredImproperly/IrqlLoweredImproperly.qhelp @@ -16,22 +16,16 @@

    + KeRaiseIrql(PASSIVE_LEVEL, &OldIrql);]]>

    The following code example avoids this warning.

    + KeLowerIrql(OldIrql);]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/IrqlNotSaved/IrqlNotSaved.md b/src/drivers/general/queries/IrqlNotSaved/IrqlNotSaved.md new file mode 100644 index 00000000..f6f40856 --- /dev/null +++ b/src/drivers/general/queries/IrqlNotSaved/IrqlNotSaved.md @@ -0,0 +1,37 @@ +# IRQL not saved (C28158) +A parameter annotated _IRQL_saves_ must have the IRQL value saved to it. + + +## Recommendation +Make sure that any parameter annotated "_IRQL_saves_" has a code path where the current system IRQL is saved to it. + + +## Example +In this example, the driver does not save the IRQL to a parameter annotated to have the IRQL saved to it: + +```c + + VOID IrqlNotSaved_fail(_IRQL_saves_ KIRQL outIrql, PKSPIN_LOCK myLock) { + KIRQL localIrql; + KeAcquireSpinLock(myLock, &localIrql); + } + +``` +The driver should make sure to save the IRQL in the annotated parameter. + +```c + + VOID IrqlNotSaved_pass(_IRQL_saves_ KIRQL outIrql, PKSPIN_LOCK myLock) { + KeAcquireSpinLock(myLock, &outIrqlPass); + } + +``` + +## Semmle-specific notes +This version of the query only checks for functions that have no path where the IRQL is saved at all. In a future update, we will use the must-flow library to check for functions where there are _any_ paths where the IRQL is not saved. + +False positives may occur if UNREFERENCED_PARAMETER() is used on an annotated parameter. + + +## References +* [ C28158 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28158-no-irql-was-saved) diff --git a/src/drivers/general/queries/IrqlNotUsed/IrqlNotUsed.md b/src/drivers/general/queries/IrqlNotUsed/IrqlNotUsed.md new file mode 100644 index 00000000..1c822784 --- /dev/null +++ b/src/drivers/general/queries/IrqlNotUsed/IrqlNotUsed.md @@ -0,0 +1,34 @@ +# IRQL not restored (C28157) +A parameter annotated _IRQL_restores_ must be read and used to restore the IRQL value. + + +## Recommendation +Make sure that any parameter annotated "_IRQL_restores_" has a code path where the IRQL is restored by calling an OS function. + + +## Example +In this example, the driver has a parameter annotated to restore the IRQL, but it never uses this parameter: + +```c + + VOID ReleaseMyLock(_IRQL_restores_ KIRQL inIrql, PKSPIN_LOCK myLock) { + KeReleaseSpinLock(myLock, PASSIVE_LEVEL); + } + +``` +The driver should make sure to restore the IRQL from this parameter, or adjust its annotations: + +```c + + VOID ReleaseMyLock(_IRQL_restores_ KIRQL inIrql, PKSPIN_LOCK myLock) { + KeReleaseSpinLock(myLock, inIrql); + } + +``` + +## Semmle-specific notes +This version of the query only checks for functions that have no path where the IRQL is restored at all. In a future update, we will use the must-flow library to check for functions where there are _any_ paths where the IRQL is not restored. + + +## References +* [ C28157 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28157-function-irql-never-restored) diff --git a/src/drivers/general/queries/IrqlNotUsed/IrqlNotUsed.qhelp b/src/drivers/general/queries/IrqlNotUsed/IrqlNotUsed.qhelp index 5753c86a..3746b9f2 100644 --- a/src/drivers/general/queries/IrqlNotUsed/IrqlNotUsed.qhelp +++ b/src/drivers/general/queries/IrqlNotUsed/IrqlNotUsed.qhelp @@ -17,7 +17,7 @@ + }]]>

    The driver should make sure to restore the IRQL from this parameter, or adjust its annotations: diff --git a/src/drivers/general/queries/IrqlSetTooHigh/IrqlSetTooHigh.md b/src/drivers/general/queries/IrqlSetTooHigh/IrqlSetTooHigh.md new file mode 100644 index 00000000..b83ce92f --- /dev/null +++ b/src/drivers/general/queries/IrqlSetTooHigh/IrqlSetTooHigh.md @@ -0,0 +1,30 @@ +# IRQL set too high (C28150) +The function has raised the IRQL to a level above what is allowed. + + +## Recommendation +A function has been annotated as having a max IRQL, but the execution of that function raises the IRQL above that maximum. If you have applied custom IRQL annotations to your own functions, confirm that they are accurate. + + +## Example +In this example, the driver tries to raise the IRQL to HIGH_LEVEL while in a dispatch routine. This should be avoided. + +```c + + // Within a dispatch routine + KeRaiseIrql(HIGH_LEVEL, &oldIRQL); + + +``` + +## References +* [ C28150 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28150-function-causes-irq-level-to-be-set-above-max) +* [ IRQL annotations for drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/irql-annotations-for-drivers) + +## Semmle-specific notes +This query uses interprocedural data-flow analysis and can take a large amount of CPU time and memory to run. + +This query may provide false positives in cases where functions are not annotated with their expected IRQL ranges or behaviors. + +For information on how to annotate your functions with information about how they adjust the IRQL, see "IRQL annotations for drivers" in the references section. + diff --git a/src/drivers/general/queries/IrqlSetTooLow/IrqlSetTooLow.md b/src/drivers/general/queries/IrqlSetTooLow/IrqlSetTooLow.md new file mode 100644 index 00000000..ecdee849 --- /dev/null +++ b/src/drivers/general/queries/IrqlSetTooLow/IrqlSetTooLow.md @@ -0,0 +1,30 @@ +# IRQL set too low (C28124) +The function has lowered the IRQL to a level below what is allowed. + + +## Recommendation +A function has been annotated as having a minimum IRQL, but the execution of that function lowers the IRQL below that minimum. If you have applied custom IRQL annotations to your own functions, confirm that they are accurate. If your function is a dispatch routine or callback, review the expected IRQL levels for that role. + + +## Example +In this example, the driver tries to lower the IRQL to PASSIVE_LEVEL within a KEDEFERRED_ROUTINE callback, which must run at DISPATCH_LEVEL or higher. This should be avoided. + +```c + + // Within a KDEFERRED_ROUTINE callback + KeLowerIrql(PASSIVE_LEVEL, &oldIRQL); + + +``` + +## References +* [ C28124 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28124-call-below-minimum-irq-level) +* [ IRQL annotations for drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/irql-annotations-for-drivers) + +## Semmle-specific notes +This query uses interprocedural data-flow analysis and can take a large amount of CPU time and memory to run. + +This query may provide false positives in cases where functions are not annotated with their expected IRQL ranges or behaviors. + +For information on how to annotate your functions with information about how they adjust the IRQL, see "IRQL annotations for drivers" in the references section. + diff --git a/src/drivers/general/queries/IrqlTooHigh/IrqlTooHigh.md b/src/drivers/general/queries/IrqlTooHigh/IrqlTooHigh.md new file mode 100644 index 00000000..2bb42bd8 --- /dev/null +++ b/src/drivers/general/queries/IrqlTooHigh/IrqlTooHigh.md @@ -0,0 +1,55 @@ +# IRQL too high (C28121) +The function is not permitted to be called at the current IRQ level. The current level is too high. + + +## Recommendation +The driver is executing at an IRQL that is too high for the function that it is calling. Consult the WDK documentation for the function and verify the IRQL at which the function can be called. If you have applied custom IRQL annotations to your own functions, confirm that they are accurate. + + +## Example +In this example, the driver is at too high of an IRQL to acquire a spinlock: + +```c + + NTSTATUS + IrqlTooHigh(PKSPIN_LOCK myLock, PKIRQL oldIrql){ + NTSTATUS status; + KIRQL lockIrql; + KeRaiseIrql(HIGH_LEVEL, oldIrql); + KeAcquireSpinLock(myLock, &lockIrql); + KeReleaseSpinLock(myLock, &lockIrql); + KeLowerIrql(*oldIrql); + return status; + } + + +``` +The driver should be careful not to raise the IRQL too high: + +```c + + NTSTATUS + IrqlTooHigh(PKSPIN_LOCK myLock, PKIRQL oldIrql){ + NTSTATUS status; + KIRQL lockIrql; + KeRaiseIrql(APC_LEVEL, oldIrql); + KeAcquireSpinLock(myLock, &lockIrql); + KeReleaseSpinLock(myLock, &lockIrql); + KeLowerIrql(*oldIrql); + return status; + } + + +``` + +## References +* [ C28121 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28121-irq-execution-too-high) +* [ IRQL annotations for drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/irql-annotations-for-drivers) + +## Semmle-specific notes +This query uses interprocedural data-flow analysis and can take a large amount of CPU time and memory to run. + +This query may provide false positives in cases where functions are not annotated with their expected IRQL ranges or behaviors. + +For information on how to annotate your functions with information about how they adjust the IRQL, see "IRQL annotations for drivers" in the references section. + diff --git a/src/drivers/general/queries/IrqlTooLow/IrqlTooLow.md b/src/drivers/general/queries/IrqlTooLow/IrqlTooLow.md new file mode 100644 index 00000000..6b8dbad6 --- /dev/null +++ b/src/drivers/general/queries/IrqlTooLow/IrqlTooLow.md @@ -0,0 +1,45 @@ +# IRQL too low (C28120) +The function is not permitted to be called at the current IRQ level. The current level is too low. + + +## Recommendation +The driver is executing at an IRQL that is too low for the function that it is calling. Consult the WDK documentation for the function and verify the IRQL at which the function can be called. If you have applied custom IRQL annotations to your own functions, confirm that they are accurate. + + +## Example +In this example, the driver is calling a DDI that must be called at DISPATCH_LEVEL or higher: + +```c + + // Within a standard thread running at APC_LEVEL: + if (KeShouldYieldProcessor()) + { + KeLowerIrql(PASSIVE_LEVEL); + } + + +``` +The driver should be careful to only call from a DISPATCH_LEVEL context: + +```c + + // Within a work loop running at DISPATCH_LEVEL + if (KeShouldYieldProcessor()) + { + KeLowerIrql(PASSIVE_LEVEL); + } + + +``` + +## References +* [ C28120 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28120-irql-execution-too-low) +* [ IRQL annotations for drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/irql-annotations-for-drivers) + +## Semmle-specific notes +This query uses interprocedural data-flow analysis and can take a large amount of CPU time and memory to run. + +This query may provide false positives in cases where functions are not annotated with their expected IRQL ranges or behaviors. + +For information on how to annotate your functions with information about how they adjust the IRQL, see "IRQL annotations for drivers" in the references section. + diff --git a/src/drivers/general/queries/KeSetEventPageable/KeSetEventPageable.md b/src/drivers/general/queries/KeSetEventPageable/KeSetEventPageable.md new file mode 100644 index 00000000..96b05ca7 --- /dev/null +++ b/src/drivers/general/queries/KeSetEventPageable/KeSetEventPageable.md @@ -0,0 +1,74 @@ +# KeSetEvent called in pageable segment with wait +KeSetEvent must not be called in a paged segment when the Wait argument is set to TRUE. This can cause a system crash the segment is paged out. + + +## Recommendation +Adjust the KeSetEvent call to pass FALSE to the wait parameter. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// + +#define SET_DISPATCH 1 + +// Template. Not called in this test. +void top_level_call() {} + +#include + +void KeSetEventIrql_Fail1(PRKEVENT Event); + +_IRQL_always_function_min_(APC_LEVEL) +void KeSetEventIrql_Fail2(PRKEVENT Event); + +_IRQL_always_function_min_(PASSIVE_LEVEL) +void KeSetEventIrql_Pass1(PRKEVENT Event); + +_IRQL_always_function_min_(PASSIVE_LEVEL) +void KeSetEventIrql_Pass2(PRKEVENT Event); + +#pragma alloc_text(PAGE, KeSetEventIrql_Fail1) +#pragma alloc_text(PAGE, KeSetEventIrql_Fail2) +#pragma alloc_text(PAGE, KeSetEventIrql_Pass2) + +void KeSetEventIrql_Fail1(PRKEVENT Event) +{ + // This is a paged function. We assume a lower limit of PASSIVE_LEVEL and an upper limit of APC_LEVEL on the IRQL. + + KeSetEvent(Event, HIGH_PRIORITY, TRUE); // ERROR: Calling with wait set to TRUE in a pageable context +} + +void KeSetEventIrql_Fail2(PRKEVENT Event) +{ + // This is a paged function. Even though it runs at APC_LEVEL, not PASSIVE_LEVEL, that's still an error. + + KeSetEvent(Event, HIGH_PRIORITY, TRUE); // ERROR: Calling with wait set to TRUE in a pageable context +} + +void KeSetEventIrql_Pass1(PRKEVENT Event) +{ + // This function will potentially run at passive level but it's not pageable, so there's no issue. + + KeSetEvent(Event, HIGH_PRIORITY, TRUE); +} + +void KeSetEventIrql_Pass2(PRKEVENT Event) +{ + // This function will runs at passive level and is pageable, but correctly uses FALSE in its call to KeSetEvent. + + KeSetEvent(Event, HIGH_PRIORITY, FALSE); +} + +// TODO multi-threaded tests +// function has max IRQL requirement, creates two threads where one is above that requirement and one is below + +``` + +## References +* [ KeSetEvent (MSDN) ](https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-kesetevent) diff --git a/src/drivers/general/queries/MultipleFunctionClassAnnotations/MultipleFunctionClassAnnotations.md b/src/drivers/general/queries/MultipleFunctionClassAnnotations/MultipleFunctionClassAnnotations.md new file mode 100644 index 00000000..f0d4d0c5 --- /dev/null +++ b/src/drivers/general/queries/MultipleFunctionClassAnnotations/MultipleFunctionClassAnnotations.md @@ -0,0 +1,40 @@ +# Multiple Function Class Annotations +Function is annotated with more than one function class. All but one will be ignored. + + +## Recommendation +This warning can be generated when there is a chain of typedefs. Only use one function class annotation. + + +## Example +Example function with multiple __drv_functionClass annotations + +```c + + __drv_functionClass(FAKE_DRIVER_ADD_DEVICE) + __drv_functionClass(FAKE_DRIVER_ADD_DEVICE2) + __drv_maxFunctionIRQL(PASSIVE_LEVEL) + __drv_requiresIRQL(PASSIVE_LEVEL) + __drv_sameIRQL + __drv_when(return >= 0, __drv_clearDoInit(yes)) typedef NTSTATUS + FAKE_DRIVER_ADD_DEVICE( + __in struct _DRIVER_OBJECT *DriverObject, + __in struct _DEVICE_OBJECT *PhysicalDeviceObject); + + typedef FAKE_DRIVER_ADD_DEVICE *PDRIVER_ADD_DEVICE; + + FAKE_DRIVER_ADD_DEVICE FakeDriverAddDevice; + + _Use_decl_annotations_ + NTSTATUS + FakeDriverAddDevice( + __in struct _DRIVER_OBJECT *DriverObject, + __in struct _DEVICE_OBJECT *PhysicalDeviceObject) + { + return STATUS_SUCCESS; + } + +``` + +## References +* [ C28177 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28177-function-annotated-with-more-than-one-class) diff --git a/src/drivers/general/queries/MultipleFunctionClassAnnotations/MultipleFunctionClassAnnotations.qhelp b/src/drivers/general/queries/MultipleFunctionClassAnnotations/MultipleFunctionClassAnnotations.qhelp index 9ea87c95..c4bce3a8 100644 --- a/src/drivers/general/queries/MultipleFunctionClassAnnotations/MultipleFunctionClassAnnotations.qhelp +++ b/src/drivers/general/queries/MultipleFunctionClassAnnotations/MultipleFunctionClassAnnotations.qhelp @@ -36,16 +36,10 @@ __in struct _DEVICE_OBJECT *PhysicalDeviceObject) { return STATUS_SUCCESS; - } }]]> - -

    - -

    -
  • diff --git a/src/drivers/general/queries/MultithreadedAVCondition/MultithreadedAVCondition.md b/src/drivers/general/queries/MultithreadedAVCondition/MultithreadedAVCondition.md new file mode 100644 index 00000000..d04ef9bd --- /dev/null +++ b/src/drivers/general/queries/MultithreadedAVCondition/MultithreadedAVCondition.md @@ -0,0 +1,47 @@ +# Multithreaded Access Violation Condition +This warning indicates that a thread has potential to access deleted objects if preempted. Unlike the Code Analysis version of this query, this query does not currently verify the use of any synchronization mechanisms, so it may produce false positives. + + +## Recommendation +There should be no access to a reference-counted object after the reference count is at zero + + +## Example +In this example, m_cRef is a member of this. A thread T1 executes the if condition, decrements m_cRef to 1, and is then preempted. Another thread T2 executes the if condition, decrements m_cRef to 0, executes the if body (where this is deleted), and returns NULL. + +```c + + ULONG Release_bad() + { + if (0 == InterlockedDecrement(&m_cRef)) + { + delete this; + return NULL; + } + /* this.m_cRef isn't thread safe */ + return m_cRef; + } + + +``` +The following code does not reference any heap memory after the object is deleted. + +```c + + ULONG CObject::Release() + { + ASSERT(0 != m_cRef); + ULONG cRef = InterlockedDecrement(&m_cRef); + if (0 == cRef) + { + delete this; + return NULL; + } + return cRef; + } + + +``` + +## References +* [ Warning C28616 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28616-multithreaded-av-condition) diff --git a/src/drivers/general/queries/NtstatusExplicitCast/NtstatusExplicitCast.md b/src/drivers/general/queries/NtstatusExplicitCast/NtstatusExplicitCast.md new file mode 100644 index 00000000..6c3ab0e6 --- /dev/null +++ b/src/drivers/general/queries/NtstatusExplicitCast/NtstatusExplicitCast.md @@ -0,0 +1,36 @@ +# NTSTATUS Explicit Cast +Cast between semantically different integer types. This warning indicates that an NTSTATUS value is being explicitly cast to a Boolean type. This is likely to give undesirable results. For example, the typical success value for NTSTATUS, STATUS_SUCCESS, is false when tested as a Boolean. + + +## Recommendation +In most cases, the NT_SUCCESS macro should be used to test the value of an NTSTATUS. This macro returns true if the returned status value is neither a warning nor an error code. If a function returns a Boolean to indicate its failure/success, it should explicitly return the appropriate Boolean type rather than depend on casting of NTSTATUS to a Boolean type. Also, occasionally a program may attempt to reuse a Boolean local variable to store NTSTATUS values. This practice is often error-prone; it is much safer (and likely more efficient) to use a separate NTSTATUS variable. + + +## Example +If statement with explicit cast from NTSTATUS to Boolean + +```c + + NTSTATUS status; + ... + if (!((BOOLEAN)status)){ + ; + } + +``` +Use of NT_SUCCESS macro + +```c + + NTSTATUS status; + ... + if (!NT_SUCCESS(status)) + { + ; + // handle error + } + +``` + +## References +* [ Warning C28714 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28714-ntstatus-cast-between-semantically-different-integer-types) diff --git a/src/drivers/general/queries/NtstatusExplicitCast/NtstatusExplicitCast.qhelp b/src/drivers/general/queries/NtstatusExplicitCast/NtstatusExplicitCast.qhelp index 6e162f62..7b37469f 100644 --- a/src/drivers/general/queries/NtstatusExplicitCast/NtstatusExplicitCast.qhelp +++ b/src/drivers/general/queries/NtstatusExplicitCast/NtstatusExplicitCast.qhelp @@ -20,8 +20,6 @@ ... if (!((BOOLEAN)status)){ ; - } - }]]>

    @@ -34,14 +32,9 @@ { ; // handle error - } - }]]> + }]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.md b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.md new file mode 100644 index 00000000..53f0ace5 --- /dev/null +++ b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.md @@ -0,0 +1,40 @@ +# Ntstatus Explicit Cast 2 +Cast between semantically different integer types. This warning indicates that a Boolean is being cast to NTSTATUS. This is likely to give undesirable results. For example, the typical failure value for functions that return a Boolean (FALSE) is a success status when tested as an NTSTATUS. + + +## Recommendation +Typically, a function that returns Boolean returns either 1 (for TRUE) or 0 (for FALSE). Both these values are treated as success codes by the NT_SUCCESS macro. Thus, the failure case will never be detected. + + +## Example +Bad cast from Boolean to NTSTATUS + +```c + + if (NT_SUCCESS(SomeFunction())) + { + return 0; + } + else + { + return -1; + } + +``` +Correct use of Boolean + +```c + + if (SomeFunction() == TRUE) + { + return 0; + } + else + { + return -1; + } + +``` + +## References +* [ Warning C28715 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28715-boolean-cast-between-semantically-different-integer-types) diff --git a/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.qhelp b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.qhelp index fe033d83..3c8a7dbf 100644 --- a/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.qhelp +++ b/src/drivers/general/queries/NtstatusExplicitCast2/NtstatusExplicitCast2.qhelp @@ -23,7 +23,6 @@ else { return -1; - } }]]>

    @@ -37,14 +36,9 @@ else { return -1; - } }]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/NtstatusExplicitCast3/NtstatusExplicitCast3.md b/src/drivers/general/queries/NtstatusExplicitCast3/NtstatusExplicitCast3.md new file mode 100644 index 00000000..f0665d14 --- /dev/null +++ b/src/drivers/general/queries/NtstatusExplicitCast3/NtstatusExplicitCast3.md @@ -0,0 +1,38 @@ +# Ntstatus Explicit Cast 3 +Compiler-inserted cast between semantically different integral types (Boolean to NTSTATUS). This warning indicates that a Boolean is being used as an NTSTATUS without being explicitly cast. + + +## Recommendation +This warning indicates that a Boolean is being used as an NTSTATUS without being explicitly cast. This is likely to give undesirable results. For instance, the typical failure value for functions that return a Boolean (false) indicates a success status when tested as an NTSTATUS. + + +## Example +Example of SomeMemAllocFunction that has a Boolean return value but is being returned in a function with NTSTATUS return type. + +```c + + extern bool SomeMemAllocFunction(void **); + + NTSTATUS test_bad() + { + void *MyPtr; + return SomeMemAllocFunction(&MyPtr); + } + +``` +This example avoids the warning + +```c + + extern bool SomeMemAllocFunction(void **); + + if (SomeMemAllocFunction(&MyPtr) == true) { + return STATUS_SUCCESS; + } else { + return STATUS_NO_MEMORY; + } + +``` + +## References +* [ Warning C28716 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28716-compiler-inserted-cast-between-semantically-different-integral) diff --git a/src/drivers/general/queries/NtstatusExplicitCast3/NtstatusExplicitCast3.qhelp b/src/drivers/general/queries/NtstatusExplicitCast3/NtstatusExplicitCast3.qhelp index 400fdba8..2aea1ce2 100644 --- a/src/drivers/general/queries/NtstatusExplicitCast3/NtstatusExplicitCast3.qhelp +++ b/src/drivers/general/queries/NtstatusExplicitCast3/NtstatusExplicitCast3.qhelp @@ -21,7 +21,6 @@ { void *MyPtr; return SomeMemAllocFunction(&MyPtr); - } }]]>

    @@ -34,14 +33,9 @@ return STATUS_SUCCESS; } else { return STATUS_NO_MEMORY; - } }]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/NullCharacterPointerAssignment/NullCharacterPointerAssignment.md b/src/drivers/general/queries/NullCharacterPointerAssignment/NullCharacterPointerAssignment.md new file mode 100644 index 00000000..f82c9e8d --- /dev/null +++ b/src/drivers/general/queries/NullCharacterPointerAssignment/NullCharacterPointerAssignment.md @@ -0,0 +1,32 @@ +# Null Character Pointer Assignment +Possible assignment of '\\\\0' directly to a pointer + + +## Recommendation +This warning indicates a probable typographical error: a null character is being assigned to a pointer; it is probably the case that the character is intended as a string terminator and should be assigned to the memory where the pointer is pointing. + + +## Example +Example of incorrect assignment of '\\0' to a pointer + +```c + + char a[8]; + char *p = a; + char x = 0; + char y = '0'; + p = '\0'; // should be *p = '\0'; + +``` +Example of correct assignment of '\\0' to where the pointer is pointing + +```c + + char a[8]; + char *p = a; + *p = '\0'; // correct! + +``` + +## References +* [ Warning C28730 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28730-possible-null-character-assignment) diff --git a/src/drivers/general/queries/NullCharacterPointerAssignment/NullCharacterPointerAssignment.qhelp b/src/drivers/general/queries/NullCharacterPointerAssignment/NullCharacterPointerAssignment.qhelp index 0922c480..617ba059 100644 --- a/src/drivers/general/queries/NullCharacterPointerAssignment/NullCharacterPointerAssignment.qhelp +++ b/src/drivers/general/queries/NullCharacterPointerAssignment/NullCharacterPointerAssignment.qhelp @@ -19,8 +19,7 @@ char *p = a; char x = 0; char y = '0'; - p = '\0'; // should be *p = '\0'; - }]]> + p = '\0'; // should be *p = '\0';]]>

    Example of correct assignment of '\0' to where the pointer is pointing @@ -28,14 +27,9 @@ + *p = '\0'; // correct!]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/OperandAssignment/OperandAssignment.md b/src/drivers/general/queries/OperandAssignment/OperandAssignment.md new file mode 100644 index 00000000..36165ba4 --- /dev/null +++ b/src/drivers/general/queries/OperandAssignment/OperandAssignment.md @@ -0,0 +1,36 @@ +# Operand Assignment +An assignment has been made to an operand, which should only be modified using bit sets and clears + + +## Recommendation +The driver is using an assignment to modify an operand. Assigning a value might unintentionally change the values of bits other than those that it needs to change, resulting in unexpected consequences. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// + +#define SET_DISPATCH 1 +// Template. Not called in this test. +void top_level_call() {} +PDEVICE_OBJECT fdo = NULL; + +void bad_operand_assignment() +{ + fdo->Flags = DO_BUFFERED_IO; +} +void good_operand_assignment() +{ + fdo->Flags |= DO_BUFFERED_IO; + +} + +``` + +## References +* [ C28129 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28129-assignment-made-to-operand) diff --git a/src/drivers/general/queries/PointerVariableSize/PointerVariableSize.md b/src/drivers/general/queries/PointerVariableSize/PointerVariableSize.md new file mode 100644 index 00000000..01cbd441 --- /dev/null +++ b/src/drivers/general/queries/PointerVariableSize/PointerVariableSize.md @@ -0,0 +1,32 @@ +# Pointer Variable Size +The driver is taking the size of a pointer variable, not the size of the value that is pointed to + + +## Recommendation +If the driver needs the size of the pointed-to value, change the code so that it references the value. If the driver actually needs the size of the pointer, take the size of the pointer type (for example, LPSTR, char\* or even void\*) to clarify that this is the intent. + + +## Example +The following code example elicits this warning. + +```c + + void bad(){ + char* b = 0; + memset(b, 0, sizeof(b)); + } + +``` +The following code example avoids this warning. + +```c + + void good(){ + char* b = 0; + memset(b, 0, sizeof(*b)); + } + +``` + +## References +* [ Warning C28132 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28132-driver-taking-the-size-of-pointer) diff --git a/src/drivers/general/queries/PointerVariableSize/PointerVariableSize.qhelp b/src/drivers/general/queries/PointerVariableSize/PointerVariableSize.qhelp index 5950616e..de19b029 100644 --- a/src/drivers/general/queries/PointerVariableSize/PointerVariableSize.qhelp +++ b/src/drivers/general/queries/PointerVariableSize/PointerVariableSize.qhelp @@ -18,8 +18,7 @@ void bad(){ char* b = 0; memset(b, 0, sizeof(b)); - } - }]]> + }]]>

    The following code example avoids this warning. @@ -28,8 +27,7 @@ void good(){ char* b = 0; memset(b, 0, sizeof(*b)); - } - }]]> + }]]> diff --git a/src/drivers/general/queries/PoolTagIntegral/PoolTagIntegral.md b/src/drivers/general/queries/PoolTagIntegral/PoolTagIntegral.md new file mode 100644 index 00000000..2d12a35b --- /dev/null +++ b/src/drivers/general/queries/PoolTagIntegral/PoolTagIntegral.md @@ -0,0 +1,24 @@ +# Use of string in pool tag instead of integral (C28134) +The type of a pool tag should be integral, not a string or string pointer. + + +## Recommendation +Instead of using strings or string pointers, pool tags should be four-character integrals. For example, '_gaT'. + + +## Example +In this example, the driver uses a string rather than a four-character integral as a pool tag: + +```c + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, "_gaT"); + +``` +The driver should use a four-character integral as a tag. + +```c +ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, '_gaT'); + +``` + +## References +* [ C28134 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28134-pool-tag-type-should-be-integral) diff --git a/src/drivers/general/queries/RoleTypeCorrectlyUsed/RoleTypeCorrectlyUsed.md b/src/drivers/general/queries/RoleTypeCorrectlyUsed/RoleTypeCorrectlyUsed.md new file mode 100644 index 00000000..b67addbc --- /dev/null +++ b/src/drivers/general/queries/RoleTypeCorrectlyUsed/RoleTypeCorrectlyUsed.md @@ -0,0 +1,84 @@ +# Incorrect Role Type Use +Driver callback functions should be declared with the appropriate function role type. + + +## Recommendation +Make sure the role type of the function being used matches the expected role type. + + +## Example +In this example, the driver does not correctly annotate FdoDevPrepareHardware before setting it as the EvtDevicePrepareHardware callback function. + +```c + + NTSTATUS FdoDevPrepareHardware; + + VOID DriverSetDeviceCallbackEvents( + _In_ PWDFDEVICE_INIT _DeviceInit + ) + // Initialize device callback events + { + WDF_POWER_POLICY_EVENT_CALLBACKS PowerPolicyCallbacks; + WDF_PNPPOWER_EVENT_CALLBACKS PnpPowerCallbacks; + + PAGED_CODE(); + + DoTrace(LEVEL_INFO, TFLAG_PNP,("+DriverSetDeviceCallbackEvents")); + + // + // Set event callbacks + // 1. Pnp & Power events + // 2. Power Policy events + // + + WDF_PNPPOWER_EVENT_CALLBACKS_INIT(&PnpPowerCallbacks); + + // + // Register PnP callback + // + PnpPowerCallbacks.EvtDevicePrepareHardware = FdoDevPrepareHardware; + + } + +``` +The driver annotate the FdoDevPrepareHardware function with the appropriate role type. + +```c + + EVT_WDF_DEVICE_PREPARE_HARDWARE FdoDevPrepareHardware; + + VOID DriverSetDeviceCallbackEvents( + _In_ PWDFDEVICE_INIT _DeviceInit + ) + // Initialize device callback events + { + WDF_POWER_POLICY_EVENT_CALLBACKS PowerPolicyCallbacks; + WDF_PNPPOWER_EVENT_CALLBACKS PnpPowerCallbacks; + + PAGED_CODE(); + + DoTrace(LEVEL_INFO, TFLAG_PNP,("+DriverSetDeviceCallbackEvents")); + + // + // Set event callbacks + // 1. Pnp & Power events + // 2. Power Policy events + // + + WDF_PNPPOWER_EVENT_CALLBACKS_INIT(&PnpPowerCallbacks); + + // + // Register PnP callback + // + PnpPowerCallbacks.EvtDevicePrepareHardware = FdoDevPrepareHardware; + + } + +``` + +## Semmle-specific notes +C++ functions not currently supported. See https://github.com/github/codeql/issues/14869 + + +## References +* [ C28158 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/declaring-functions-using-function-role-types-for-wdm-drivers) diff --git a/src/drivers/general/queries/RoleTypeCorrectlyUsed/RoleTypeCorrectlyUsed.qhelp b/src/drivers/general/queries/RoleTypeCorrectlyUsed/RoleTypeCorrectlyUsed.qhelp index 267cb5c2..8ec55292 100644 --- a/src/drivers/general/queries/RoleTypeCorrectlyUsed/RoleTypeCorrectlyUsed.qhelp +++ b/src/drivers/general/queries/RoleTypeCorrectlyUsed/RoleTypeCorrectlyUsed.qhelp @@ -42,8 +42,7 @@ // PnpPowerCallbacks.EvtDevicePrepareHardware = FdoDevPrepareHardware; - } - }]]> + }]]>

    The driver annotate the FdoDevPrepareHardware function with the appropriate role type. @@ -76,8 +75,7 @@ // PnpPowerCallbacks.EvtDevicePrepareHardware = FdoDevPrepareHardware; - } - }]]> + }]]> diff --git a/src/drivers/general/queries/RoutineFunctionTypeNotExpected/RoutineFunctionTypeNotExpected.md b/src/drivers/general/queries/RoutineFunctionTypeNotExpected/RoutineFunctionTypeNotExpected.md new file mode 100644 index 00000000..dd7317c4 --- /dev/null +++ b/src/drivers/general/queries/RoutineFunctionTypeNotExpected/RoutineFunctionTypeNotExpected.md @@ -0,0 +1,50 @@ +# Unexpected function return type for routine (C28127) +The driver is passing or assigning a function (pointer) of an unexpected type (that is, function signature) + + +## Recommendation +Verify function pointer is correct + + +## Example +In this example, the driver provides a pointer to a function that returns long where a function that returns int is expected: + +```c + + typedef long + functionCallLong(void); + typedef functionCallLong *funcCallLong; + + int useFP(funcCallLong); + + int badFunction(void); + + void doError() { + useFP(&badFunction); + } + +``` +Ensure your function definitions match what is expected, and use roletypes and typedefs to help reduce issues. + +```c + + typedef long + functionCallLong(void); + typedef functionCallLong *funcCallLong; + + int useFP(funcCallLong); + + functionCallLong goodFunction; + + void doCorrect() { + useFP(&goodFunction); + } + +``` + +## References +* [ C28127 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28127-function-routine-mismatch) + +## Semmle-specific notes +In some cases, this rule may report false positives where it claims two identical parameter types do not match. This issue is under investigation. + diff --git a/src/drivers/general/queries/StaticInitializer/StaticInitializer.md b/src/drivers/general/queries/StaticInitializer/StaticInitializer.md new file mode 100644 index 00000000..bdb4be88 --- /dev/null +++ b/src/drivers/general/queries/StaticInitializer/StaticInitializer.md @@ -0,0 +1,50 @@ +# Static Initializer +Static initializers of global or static const variables can often be fully evaluated at compile time, thus generated in RDATA. However if any initializer is a pointer-to-member-function where it is a non-static function, the entire initialier may be placed in copy-on-write pages, which has a performance cost. + + +## Recommendation +For binaries which require fast loading and minimizing copy on write pages, consider making sure all function pointer in the static initializer are not pointer-to-member-function. If a pointer-to-member-function is required, write a simple static member function that wraps a call to the actual member function. + + +## Example +Code which triggers this query: + +```c + + class MyClass + { + ... + bool memberFunc(); + ... + }; + const StructType MyStruct[] = { + ... + &MyClass::memberFunc, + ... + }; + + +``` +Good code: + +```c + + class MyClass + { + ... + bool memberFunc(); + static bool memberFuncWrap(MyClass *thisPtr) + { return thisPtr->memberFunc(); } + ... + }; + const StructType MyStruct[] = { + ... + &MyClass::memberFuncWrap, + ... + }; + + +``` + +## References +* [ C28651 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28651-member-function-pointers-cause-write-pages-copy) diff --git a/src/drivers/general/queries/StaticInitializer/StaticInitializer.qhelp b/src/drivers/general/queries/StaticInitializer/StaticInitializer.qhelp index 08035c9a..8f963470 100644 --- a/src/drivers/general/queries/StaticInitializer/StaticInitializer.qhelp +++ b/src/drivers/general/queries/StaticInitializer/StaticInitializer.qhelp @@ -56,11 +56,6 @@ ]]> - -

    - -

    -
  • diff --git a/src/drivers/general/queries/StrSafe/StrSafe.md b/src/drivers/general/queries/StrSafe/StrSafe.md new file mode 100644 index 00000000..678e0c75 --- /dev/null +++ b/src/drivers/general/queries/StrSafe/StrSafe.md @@ -0,0 +1,31 @@ +# Unsafe string header (C28146) +Kernel Mode drivers should use ntstrsafe.h, not strsafe.h. + + +## Recommendation +The header ntstrsafe.h contains versions of the functions found in strsafe.h that are suitable for use in kernel mode code. The header ntstrsafe.h contains versions of the functions found in strsafe.h that are suitable for use in kernel mode code. + + +## Example +In this example, the driver incorrectly imports strsafe.h: + +```c + + #include + #include + + +``` +The driver should import ntstrsafe.h: + +```c + + #include + #define NTSTRSAFE_LIB + #include + + +``` + +## References +* [ C28146 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28146-kernel-mode-drivers-should-use-ntstrsafe) diff --git a/src/drivers/general/queries/StrictTypeMatch/StrictTypeMatch.md b/src/drivers/general/queries/StrictTypeMatch/StrictTypeMatch.md new file mode 100644 index 00000000..277c04bb --- /dev/null +++ b/src/drivers/general/queries/StrictTypeMatch/StrictTypeMatch.md @@ -0,0 +1,36 @@ +# Strict Type Match +The argument should exactly match the type + + +## Recommendation +An enumerated value in a function call does not match the type specified for the parameter in the function declaration. This error can occur when parameters are mis-coded, missing, or out of order. Because C permits enumerated values to be used interchangeably, and to be used interchangeably with integer constants, it is not unusual to pass the wrong enumerated value to a function without recognizing the error. + + +## Example +The following code example elicits this warning. + +```c + + KeWaitForSingleObject( + &EventDone, + Executive, + Executive, + FALSE, + NULL); + +``` +The following code example avoids this warning. + +```c + + KeWaitForSingleObject( + &EventDone, + Executive, + KernelMode, + FALSE, + NULL); + +``` + +## References +* [ C28139 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28139-argument-operand-should-exactly-match) diff --git a/src/drivers/general/queries/StrictTypeMatch/StrictTypeMatch.qhelp b/src/drivers/general/queries/StrictTypeMatch/StrictTypeMatch.qhelp index 5e889488..1def6a78 100644 --- a/src/drivers/general/queries/StrictTypeMatch/StrictTypeMatch.qhelp +++ b/src/drivers/general/queries/StrictTypeMatch/StrictTypeMatch.qhelp @@ -20,8 +20,7 @@ Executive, Executive, FALSE, - NULL); - }]]> + NULL);]]>

    The following code example avoids this warning. @@ -32,14 +31,9 @@ Executive, KernelMode, FALSE, - NULL); - }]]> + NULL);]]> - -

    -

    -
  • diff --git a/src/drivers/general/queries/WdkDeprecatedApis/wdk-deprecated-api.md b/src/drivers/general/queries/WdkDeprecatedApis/wdk-deprecated-api.md new file mode 100644 index 00000000..466259e5 --- /dev/null +++ b/src/drivers/general/queries/WdkDeprecatedApis/wdk-deprecated-api.md @@ -0,0 +1,38 @@ +# Use of deprecated WDK API +For the Windows 10 Version 2004 release, Microsoft has introduced new pool APIs that zero by default: [ExAllocatePool2](https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-exallocatepool2) and [ExAllocatePool3](https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-exallocatepool3). + +ExAllocatePool2 takes less parameters making it easier to use. It covers the most common scenarios. + +Less common scenarios (such as priority allocations) that require more flexible parameters go through ExAllocatePool3. Both APIs are designed to be extensible for the future so we do not need to continue adding new APIs. + + +## Recommendation +It is recommended to use the new APIs for any driver code with a minimum supported target of versions of Windows 10, version 2004. + +If you are building a driver that targets versions of Windows prior to Windows 10, version 2004, use ExAllocatePoolZero, ExAllocatePoolUninitialized, ExAllocatePoolQuotaZero, or ExAllocatePoolQuotaUninitialized. + +NOTE: Do not use `POOL_FLAG_UNINITIALIZED`. + + +## Example +In this example, the driver attempts to call the now-deprecated ExAllocatePool: + +```c + + PVOID myMemory = ExAllocatePool(NonPagedPool, sizeof(int)); + + +``` +The driver should instead call ExAllocatePool2 or ExAllocatePool3. + +```c + + PVOID myMemory = ExAllocatePool2(NonPagedPool, sizeof(int), 'gaTM'); + + +``` + +## References +* [ExAllocatePool2 function (wdm.h)](https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-exallocatepool2) +* [ExAllocatePool3 function (wdm.h)](https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-exallocatepool3) +* [Solving Uninitialized Kernel Pool Memory on Windows](https://msrc-blog.microsoft.com/2020/07/02/solving-uninitialized-kernel-pool-memory-on-windows/) diff --git a/src/drivers/general/queries/experimental/DefaultPoolTagExtended/DefaultPoolTagExtended.md b/src/drivers/general/queries/experimental/DefaultPoolTagExtended/DefaultPoolTagExtended.md new file mode 100644 index 00000000..169e1864 --- /dev/null +++ b/src/drivers/general/queries/experimental/DefaultPoolTagExtended/DefaultPoolTagExtended.md @@ -0,0 +1,65 @@ +# Use of default pool tag in memory allocation (C28147) +Memory should not be allocated with the default tags of ' mdW' or ' kdD'. + + +## Recommendation +The driver is specifying a default pool tag. Because the system tracks pool use by pool tag, only those drivers that use a unique pool tag can identify and distinguish their pool use. + + +## Semmle-specific notes +This version of the query looks for bad tags passed through variables instead of just literals. Due to limitations in CodeQL data-flow analysis, the analysis will report a false negative if there is a global variable that is initialized with a default tag and there exists both a path where the variable is assigned a non-default tag, and a path where the variable is not assigned a non-default tag. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// + +#define SET_DISPATCH 1 +const extern ULONG myTag; +char * myString = "Hello!"; +const ULONG myTag2 = '2gat'; +ULONG * myTag3; + +const ULONG defaultTag1 = ' mdW'; +const ULONG defaultTag2 = ' kdD'; +ULONG defaultTag3 = ' kdD'; +ULONG changedTagGood = ' kdD'; +ULONG changedTagBad = ' daB'; + +// Template. Not called in this test. +void top_level_call() {} + +VOID PoolTagIntegral() { + + const ULONG defaultTagLocal = ' kdD'; + + myTag3 = ExAllocatePool2(POOL_FLAG_NON_PAGED, sizeof(ULONG), 'gaT_'); + *myTag3 = '3gat'; + changedTagGood = 'dooG'; + changedTagBad = ' mdW'; + changedTagBad = 'fjio'; // Change to a good value... + changedTagBad = ' kdD'; // And then back to bad, to test dataflow + + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, myTag); // GOOD + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, myTag2); // GOOD + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, *myTag3); // GOOD + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, 'gaT_'); // GOOD + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, changedTagGood); // GOOD + + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, defaultTag1); // ERROR + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, defaultTag2); // ERROR + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, defaultTag3); // ERROR + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, ' mdW'); // ERROR + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, ' kdD'); // ERROR + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, changedTagBad); // ERROR + ExAllocatePool2(POOL_FLAG_NON_PAGED, 30, defaultTagLocal); // ERROR +} +``` + +## References +* [ C28147 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28147-improper-use-of-default-pool-tag) diff --git a/src/drivers/general/queries/experimental/DriverIsolationRtlViolation/DriverIsolationRtlViolation.md b/src/drivers/general/queries/experimental/DriverIsolationRtlViolation/DriverIsolationRtlViolation.md new file mode 100644 index 00000000..e476c8f4 --- /dev/null +++ b/src/drivers/general/queries/experimental/DriverIsolationRtlViolation/DriverIsolationRtlViolation.md @@ -0,0 +1,36 @@ +# Driver Isolation Rtl Violation +There is a driver isolation violation if there is an Rtl\* registry function call with with a RelativeTo parameter != RTL_REGISTRY_DEVICEMAP or a RelativeTo parameter == RTL_REGISTRY_DEVICEMAP and writes to registry (reads are OK) + + +## Recommendation +If using an Rtl\* registry function, use RelativeTo parameter == RTL_REGISTRY_DEVICEMAP and only read from the registry + + +## Example +Example of a driver isolation violation. A call to RtlWriteRegistryValue with RelativeTo parameter != RTL_REGISTRY_DEVICEMAP. + +```c + + RtlWriteRegistryValue(RTL_REGISTRY_HANDLE, + (PCWSTR)DriverKey, + ValueName, + REG_SZ, + ValueValue, + sizeof ValueValue); + +``` +TODO example 2 + +```c + + RtlQueryRegistryValues( + RTL_REGISTRY_SERVICES, + L"Serenum", + QueryTable, + NULL, + NULL) + +``` + +## References +* [ Driver package isolation ](https://learn.microsoft.com/en-us/windows-hardware/drivers/develop/driver-isolation) diff --git a/src/drivers/general/queries/experimental/DriverIsolationRtlViolation/DriverIsolationRtlViolation.qhelp b/src/drivers/general/queries/experimental/DriverIsolationRtlViolation/DriverIsolationRtlViolation.qhelp index 695f35dd..0c0e5811 100644 --- a/src/drivers/general/queries/experimental/DriverIsolationRtlViolation/DriverIsolationRtlViolation.qhelp +++ b/src/drivers/general/queries/experimental/DriverIsolationRtlViolation/DriverIsolationRtlViolation.qhelp @@ -20,8 +20,7 @@ ValueName, REG_SZ, ValueValue, - sizeof ValueValue); - }]]> + sizeof ValueValue);]]>

    TODO example 2 @@ -32,15 +31,9 @@ L"Serenum", QueryTable, NULL, - NULL) - }]]> + NULL)]]> - -

    - -

    -
  • diff --git a/src/drivers/general/queries/experimental/DriverIsolationZwViolation1/DriverIsolationZwViolation1.md b/src/drivers/general/queries/experimental/DriverIsolationZwViolation1/DriverIsolationZwViolation1.md new file mode 100644 index 00000000..a2e210d0 --- /dev/null +++ b/src/drivers/general/queries/experimental/DriverIsolationZwViolation1/DriverIsolationZwViolation1.md @@ -0,0 +1,40 @@ +# Driver Isolation Zw Violation 1 +a Driver isolation violation occurs if there is a Zw\* registry function call with OBJECT_ATTRIBUTES parameter passed to it with RootDirectory!=NULL and the handle specified in RootDirectory comes from an unapproved ddi. + + +## Recommendation +A Zw\* registry function call with OBJECT_ATTRIBUTES parameter passed to it with RootDirectory!=NULL should obtain the handle from an approved ddi https://learn.microsoft.com/en-us/windows-hardware/drivers/develop/driver-isolation + + +## Example +Example of Driver Isolation violation: ZwOpenKey call with an ObjectAttributes parameter with a RootDirectory value from an invalid source + +```c + + + status = ZwOpenKey(&serviceKey, + KEY_READ, + &objectAttributes); + RtlInitUnicodeString(¶mStr, L"Parameters"); + + InitializeObjectAttributes(&objectAttributes, + ¶mStr, + OBJ_CASE_INSENSITIVE | OBJ_KERNEL_HANDLE, + serviceKey, + NULL); + + status = ZwOpenKey(¶metersKey, + KEY_READ, + &objectAttributes); + +``` +Example of no violation + +```c + + TODO + +``` + +## References +* [ Driver package isolation ](https://learn.microsoft.com/en-us/windows-hardware/drivers/develop/driver-isolation) diff --git a/src/drivers/general/queries/experimental/DriverIsolationZwViolation1/DriverIsolationZwViolation1.qhelp b/src/drivers/general/queries/experimental/DriverIsolationZwViolation1/DriverIsolationZwViolation1.qhelp index 2ccd0981..2955a1b0 100644 --- a/src/drivers/general/queries/experimental/DriverIsolationZwViolation1/DriverIsolationZwViolation1.qhelp +++ b/src/drivers/general/queries/experimental/DriverIsolationZwViolation1/DriverIsolationZwViolation1.qhelp @@ -29,23 +29,15 @@ status = ZwOpenKey(¶metersKey, KEY_READ, - &objectAttributes); - }]]> + &objectAttributes);]]>

    Example of no violation

    + TODO]]> - -

    - -

    -
  • diff --git a/src/drivers/general/queries/experimental/DriverIsolationZwViolation2/DriverIsolationZwViolation2.md b/src/drivers/general/queries/experimental/DriverIsolationZwViolation2/DriverIsolationZwViolation2.md new file mode 100644 index 00000000..8359fc72 --- /dev/null +++ b/src/drivers/general/queries/experimental/DriverIsolationZwViolation2/DriverIsolationZwViolation2.md @@ -0,0 +1,40 @@ +# Driver Isolation Zw Violation 2 +A driver isolation violation occurs if there is a Zw\* registry function call with OBJECT_ATTRIBUTES parameter passed to it with RootDirectory=NULL and invalid OBJECT_ATTRIBUTES->ObjectName, or RootDirectory=NULL and valid OBJECT_ATTRIBUTES->ObjectName but with write access. + + +## Recommendation +For Zw\* registry function calls where the OBJECT_ATTRIBUTES parameter passed to it has RootDirectory=NULL, the OBJECT_ATTRIBUTES->ObjectName value should start with L"\\\\Registry\\\\Machine\\\\Hardware and only read. + + +## Example +Example of a driver isolation violation where the OBJECT_ATTRIBUTES->ObjectName value is invalid + +```c + + RtlInitUnicodeString(&RegistryKeyName, L"\\Registry\\Machine\\Software\\Microsoft\\wtt\\MachineConfig"); + InitializeObjectAttributes(&ObjectAttributes, + &RegistryKeyName, + OBJ_CASE_INSENSITIVE | OBJ_KERNEL_HANDLE, + NULL, // handle + NULL); + + ntstatus = ZwOpenKey(&handleRegKey, KEY_READ, &ObjectAttributes); + +``` +Example of a driver isolation violation where the OBJECT_ATTRIBUTES->ObjectName value is valid but with write access + +```c + + RtlInitUnicodeString(&RegistryKeyName, L"\\Registry\\Machine\\Hardware\\Microsoft\\wtt\\MachineConfig"); + InitializeObjectAttributes(&ObjectAttributes, + &RegistryKeyName, + OBJ_CASE_INSENSITIVE | OBJ_KERNEL_HANDLE, + NULL, // handle + NULL); + + ntstatus = ZwOpenKey(&handleRegKey, KEY_ALL_ACCESS, &ObjectAttributes); + +``` + +## References +* [ Driver package isolation ](https://learn.microsoft.com/en-us/windows-hardware/drivers/develop/driver-isolation) diff --git a/src/drivers/general/queries/experimental/DriverIsolationZwViolation2/DriverIsolationZwViolation2.qhelp b/src/drivers/general/queries/experimental/DriverIsolationZwViolation2/DriverIsolationZwViolation2.qhelp index b8fd8d10..97546f88 100644 --- a/src/drivers/general/queries/experimental/DriverIsolationZwViolation2/DriverIsolationZwViolation2.qhelp +++ b/src/drivers/general/queries/experimental/DriverIsolationZwViolation2/DriverIsolationZwViolation2.qhelp @@ -22,8 +22,7 @@ NULL, // handle NULL); - ntstatus = ZwOpenKey(&handleRegKey, KEY_READ, &ObjectAttributes); - }]]> + ntstatus = ZwOpenKey(&handleRegKey, KEY_READ, &ObjectAttributes);]]>

    Example of a driver isolation violation where the OBJECT_ATTRIBUTES->ObjectName value is valid but with write access @@ -36,15 +35,9 @@ NULL, // handle NULL); - ntstatus = ZwOpenKey(&handleRegKey, KEY_ALL_ACCESS, &ObjectAttributes); - }]]> + ntstatus = ZwOpenKey(&handleRegKey, KEY_ALL_ACCESS, &ObjectAttributes);]]> - -

    - -

    -
  • diff --git a/src/drivers/general/queries/experimental/KeSetEventIrql/KeSetEventIrql.md b/src/drivers/general/queries/experimental/KeSetEventIrql/KeSetEventIrql.md new file mode 100644 index 00000000..4f49789c --- /dev/null +++ b/src/drivers/general/queries/experimental/KeSetEventIrql/KeSetEventIrql.md @@ -0,0 +1,76 @@ +# KeSetEvent called at wrong IRQL +KeSetEvent must be called at DISPATCH_LEVEL or below. If the Wait argument is set to TRUE, it must be called at APC_LEVEL or below. Failure to follow these guidelines can lead to system crashes. + + +## Recommendation +Ensure that the IRQL at this statement is low enough. If you are calling with Wait set to TRUE, consider setting it to FALSE instead. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// + +#define SET_DISPATCH 1 + +// Template. Not called in this test. +void top_level_call() {} + +_IRQL_always_function_max_(HIGH_LEVEL) +_IRQL_always_function_min_(5) +void KeSetEventIrql_Fail1(PRKEVENT Event); + +_IRQL_always_function_min_(DISPATCH_LEVEL) +void KeSetEventIrql_Fail2(PRKEVENT Event); + +_IRQL_always_function_min_(PASSIVE_LEVEL) +void KeSetEventIrql_Pass1(PRKEVENT Event); + +_IRQL_always_function_min_(DISPATCH_LEVEL) +void KeSetEventIrql_Pass2(PRKEVENT Event); + +#pragma alloc_text(PAGE, KeSetEventIrql_Fail2) +#pragma alloc_text(PAGE, KeSetEventIrql_Pass2) + +#include + +void KeSetEventIrql_Fail1(PRKEVENT Event) +{ + // This function is running at a high IRQL. It should not call KeSetEvent. + + KeSetEvent(Event, HIGH_PRIORITY, FALSE); // Calling at too high of an IRQL +} + +void KeSetEventIrql_Fail2(PRKEVENT Event) +{ + // This function runs at DISPATCH_LEVEL, which is too high to call KeSetEvent with the Wait argument. + + KeSetEvent(Event, HIGH_PRIORITY, TRUE); // Calling at too high of an IRQL +} + +void KeSetEventIrql_Pass1(PRKEVENT Event) +{ + // This function runs at passive, so there's no problem. + + KeSetEvent(Event, HIGH_PRIORITY, TRUE); +} + +void KeSetEventIrql_Pass2(PRKEVENT Event) +{ + // This function runs at DISPATCH_LEVEL but is calling with Wait set to FALSE, so there is no issue. + + KeSetEvent(Event, HIGH_PRIORITY, FALSE); +} + +// TODO multi-threaded tests +// function has max IRQL requirement, creates two threads where one is above that requirement and one is below + + +``` + +## References +* [ KeSetEvent (MSDN) ](https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-kesetevent) diff --git a/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.md b/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.md new file mode 100644 index 00000000..4cf94436 --- /dev/null +++ b/src/drivers/general/queries/experimental/UnicodeStringFreed/UnicodeStringFreed.md @@ -0,0 +1,57 @@ +# Unicode String Not Freed +A UNICODE_STRING pointer that is allocated with RtlCreateUnicodeString is allocated from paged pool and must be freed by calling RtlFreeUnicodeString + + +## Recommendation +Ensure that a UNICODE_STRING allocated with RtlCreateUnicodeString is freed using RtlFreeUnicodeString + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// + +#include "ntifs.h" +#define SET_DISPATCH 1 +// Template. Not called in this test. +void top_level_call() {} + +PUNICODE_STRING unicodeStringGlobal; +void free_unicode_string(PUNICODE_STRING unicodeStr) +{ + RtlFreeUnicodeString(unicodeStr); +} + +void free_global_unicode_string() +{ + RtlFreeUnicodeString(unicodeStringGlobal); + +} + +void create_unicode_string(void) +{ + PUNICODE_STRING unicodeStringLocal = NULL; + PUNICODE_STRING unicodeStringNotFreed = NULL; + PUNICODE_STRING unicodeStringArg = NULL; + + PCWSTR sourceString = L"Hello World"; + RtlCreateUnicodeString(unicodeStringLocal, sourceString); + RtlCreateUnicodeString(unicodeStringNotFreed, sourceString); + RtlCreateUnicodeString(unicodeStringArg, sourceString); + RtlCreateUnicodeString(unicodeStringGlobal, sourceString); + + + RtlFreeUnicodeString(unicodeStringLocal); + + free_unicode_string(unicodeStringArg); + +} + +``` + +## References +* [ RtlCreateUnicodeString function (MSDN) ](https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-rtlcreateunicodestring) diff --git a/src/drivers/kmdf/queries/FloatSafeExit/FloatSafeExit.md b/src/drivers/kmdf/queries/FloatSafeExit/FloatSafeExit.md new file mode 100644 index 00000000..f8116040 --- /dev/null +++ b/src/drivers/kmdf/queries/FloatSafeExit/FloatSafeExit.md @@ -0,0 +1,55 @@ +# Float Safe Exit +Exiting while holding the right to use floating-point hardware + + +## Recommendation +The _Kernel_float_restored_ annotation was used to release the right to use floating point, but a path through the function was detected where no function known to perform that operation was successfully called. This warning might indicate that a conditional (_When_) annotation is needed, or it might indicate a coding error. + + +## Example +Example of function with multiple issues + +```c + + + void func(){ + + float f = 0.0f; + int some_condition = 1; + KFLOATING_SAVE saveData; + NTSTATUS status; + + if (some_condition) + { + status = KeSaveFloatingPointState(&saveData); + if (!NT_SUCCESS(status)) + { + return; + } + for (int i = 0; i < 100; i++) + { + f = f + 1; + } + // doesn't restore the floating point state + } + else + { + status = KeSaveFloatingPointState(&saveData); + if (!NT_SUCCESS(status)) + { + return; + } + for (int i = 0; i < 100; i++) + { + f = f + 1.0f; + } + + // doesn't check the return value of KeRestoreFloatingPointState + status = KeRestoreFloatingPointState(&saveData); + } + } + +``` + +## References +* [ Warning C28162 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28162-exiting-while-holding-right-to-use-floating-hardware) diff --git a/src/drivers/kmdf/queries/FloatSafeExit/FloatSafeExit.qhelp b/src/drivers/kmdf/queries/FloatSafeExit/FloatSafeExit.qhelp index ed561fca..af359882 100644 --- a/src/drivers/kmdf/queries/FloatSafeExit/FloatSafeExit.qhelp +++ b/src/drivers/kmdf/queries/FloatSafeExit/FloatSafeExit.qhelp @@ -51,14 +51,9 @@ // doesn't check the return value of KeRestoreFloatingPointState status = KeRestoreFloatingPointState(&saveData); } - } }]]> - + - -

    -

    -
  • diff --git a/src/drivers/kmdf/queries/FloatUnsafeExit/FloatUnsafeExit.md b/src/drivers/kmdf/queries/FloatUnsafeExit/FloatUnsafeExit.md new file mode 100644 index 00000000..74a898db --- /dev/null +++ b/src/drivers/kmdf/queries/FloatUnsafeExit/FloatUnsafeExit.md @@ -0,0 +1,68 @@ +# Float Unsafe Exit +Exiting without acquiring the right to use floating hardware + + +## Recommendation +The _Kernel_float_saved_ annotation was used to acquire the right to use floating point, but a path through the function was detected where no function known to perform that operation was successfully called. This warning might indicate that a conditional (_When_) annotation is needed, or it might indicate a coding error. + + +## Example +Function has _Kernel_float_saved_ annotation but has a path where the floating point state is not saved. Additionally, KeSaveFloatingPointState return value is not checked so the call might fail + +```c + + _Kernel_float_saved_ + void float_used_bad3() + { + float f = 0.0f; + // Status not checked here + int some_condition = 1; + KFLOATING_SAVE saveData; + NTSTATUS status; + + if (some_condition) + { + // This code path doesn't save the floating point state + for (int i = 0; i < 100; i++) + { + f = f + 1; + } + } + else + { + status = KeSaveFloatingPointState(&saveData); + // Status not checked here + for (int i = 0; i < 100; i++) + { + f = f + 1.0f; + } + } + } + +``` +Good example + +```c + + Kernel_float_saved_ + void float_used_good1() + { + KFLOATING_SAVE saveData; + NTSTATUS status; + float f = 0.0f; + status = KeSaveFloatingPointState(&saveData); + if (status != STATUS_SUCCESS) + { + return; + } + for (int i = 0; i < 100; i++) + { + f = f + 1.0f; + } + KeRestoreFloatingPointState(&saveData); + } + +``` + +## References +* [ Warning C28161 ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28161-exiting-without-right-to-use-floating-hardware) diff --git a/src/drivers/kmdf/queries/FloatUnsafeExit/FloatUnsafeExit.qhelp b/src/drivers/kmdf/queries/FloatUnsafeExit/FloatUnsafeExit.qhelp index 354fe0bc..54be31c3 100644 --- a/src/drivers/kmdf/queries/FloatUnsafeExit/FloatUnsafeExit.qhelp +++ b/src/drivers/kmdf/queries/FloatUnsafeExit/FloatUnsafeExit.qhelp @@ -41,7 +41,6 @@ f = f + 1.0f; } } - } }]]>

    @@ -64,14 +63,9 @@ f = f + 1.0f; } KeRestoreFloatingPointState(&saveData); - } }]]> - -

    -

    -
  • diff --git a/src/drivers/kmdf/queries/experimental/DeviceInitApi/DeviceInitApi.md b/src/drivers/kmdf/queries/experimental/DeviceInitApi/DeviceInitApi.md new file mode 100644 index 00000000..72d4b687 --- /dev/null +++ b/src/drivers/kmdf/queries/experimental/DeviceInitApi/DeviceInitApi.md @@ -0,0 +1 @@ +# Calling WDF object initialization API after WdfDeviceCreate diff --git a/src/drivers/kmdf/queries/wfp/ConnectRedirectHandleCreation/ConnectRedirectMultipleCallsHandleCreation.md b/src/drivers/kmdf/queries/wfp/ConnectRedirectHandleCreation/ConnectRedirectMultipleCallsHandleCreation.md new file mode 100644 index 00000000..3af5db71 --- /dev/null +++ b/src/drivers/kmdf/queries/wfp/ConnectRedirectHandleCreation/ConnectRedirectMultipleCallsHandleCreation.md @@ -0,0 +1 @@ +# Connect Redirect Callouts diff --git a/src/drivers/kmdf/queries/wfp/ConnectRedirectPendClassify/ConnectRedirectPendClassify.md b/src/drivers/kmdf/queries/wfp/ConnectRedirectPendClassify/ConnectRedirectPendClassify.md new file mode 100644 index 00000000..258583ab --- /dev/null +++ b/src/drivers/kmdf/queries/wfp/ConnectRedirectPendClassify/ConnectRedirectPendClassify.md @@ -0,0 +1 @@ +# Connect Redirect Callout Pend Classify diff --git a/src/drivers/kmdf/queries/wfp/FlowLayerCalloutReturnsBlock/FlowLayerCalloutReturnsBlock.md b/src/drivers/kmdf/queries/wfp/FlowLayerCalloutReturnsBlock/FlowLayerCalloutReturnsBlock.md new file mode 100644 index 00000000..7d1f4891 --- /dev/null +++ b/src/drivers/kmdf/queries/wfp/FlowLayerCalloutReturnsBlock/FlowLayerCalloutReturnsBlock.md @@ -0,0 +1 @@ +# Flow Layer Callouts diff --git a/src/drivers/kmdf/queries/wfp/InlineConnectRedirect/InlineConnectRedirectCalloutShouldNotSetReauthorize.md b/src/drivers/kmdf/queries/wfp/InlineConnectRedirect/InlineConnectRedirectCalloutShouldNotSetReauthorize.md new file mode 100644 index 00000000..b6bb41c7 --- /dev/null +++ b/src/drivers/kmdf/queries/wfp/InlineConnectRedirect/InlineConnectRedirectCalloutShouldNotSetReauthorize.md @@ -0,0 +1 @@ +# Connect Redirect Inline Callout cannot set reauth diff --git a/src/drivers/kmdf/queries/wfp/OobStreamInjection/OobStreamInjectionReturnsBlock.md b/src/drivers/kmdf/queries/wfp/OobStreamInjection/OobStreamInjectionReturnsBlock.md new file mode 100644 index 00000000..158a9955 --- /dev/null +++ b/src/drivers/kmdf/queries/wfp/OobStreamInjection/OobStreamInjectionReturnsBlock.md @@ -0,0 +1 @@ +# Stream Injection Action Type explicitly set to FWP_ACTION_BLOCK diff --git a/src/drivers/kmdf/queries/wfp/StreamCalloutsSetActionType/StreamCalloutsSetActionType.md b/src/drivers/kmdf/queries/wfp/StreamCalloutsSetActionType/StreamCalloutsSetActionType.md new file mode 100644 index 00000000..57c3dcb9 --- /dev/null +++ b/src/drivers/kmdf/queries/wfp/StreamCalloutsSetActionType/StreamCalloutsSetActionType.md @@ -0,0 +1 @@ +# For non-inspection (injection) Stream callouts must set the actionType regardless diff --git a/src/drivers/kmdf/queries/wfp/StreamInspectionCallViolation/StreamInspectionFunctionCallViolation.md b/src/drivers/kmdf/queries/wfp/StreamInspectionCallViolation/StreamInspectionFunctionCallViolation.md new file mode 100644 index 00000000..e7e4eb1d --- /dev/null +++ b/src/drivers/kmdf/queries/wfp/StreamInspectionCallViolation/StreamInspectionFunctionCallViolation.md @@ -0,0 +1 @@ +# Stream Inspection OOB (Out of Band) functional contract diff --git a/src/drivers/kmdf/queries/wfp/TransportLayerCannotInjectCloneDuringClassify/TransportLayerCannotInjectCloneDuringClassify.md b/src/drivers/kmdf/queries/wfp/TransportLayerCannotInjectCloneDuringClassify/TransportLayerCannotInjectCloneDuringClassify.md new file mode 100644 index 00000000..cbf4dbf8 --- /dev/null +++ b/src/drivers/kmdf/queries/wfp/TransportLayerCannotInjectCloneDuringClassify/TransportLayerCannotInjectCloneDuringClassify.md @@ -0,0 +1 @@ +# Transport Layer Cannot Inject Clone During Classify diff --git a/src/drivers/test/TestTemplates/QueryTemplate/QueryTemplate.qhelp b/src/drivers/test/TestTemplates/QueryTemplate/QueryTemplate.qhelp index f44c2856..5e53ca8a 100644 --- a/src/drivers/test/TestTemplates/QueryTemplate/QueryTemplate.qhelp +++ b/src/drivers/test/TestTemplates/QueryTemplate/QueryTemplate.qhelp @@ -16,14 +16,14 @@

    + ]]>

    TODO example 2

    + ]]> diff --git a/src/drivers/wdm/queries/IllegalFieldAccess/IllegalFieldAccess.md b/src/drivers/wdm/queries/IllegalFieldAccess/IllegalFieldAccess.md new file mode 100644 index 00000000..f76e102f --- /dev/null +++ b/src/drivers/wdm/queries/IllegalFieldAccess/IllegalFieldAccess.md @@ -0,0 +1,30 @@ +# Incorrect access to protected field (C28128) +Assignments to private fields of DeviceObjects, DPCs, and IRPs should not be made directly. + +The MSDN documentation for the Code Analysis version of this check is not consistent with the behavior of the check; it states that the check looks for incorrect assignments to an IRP's CancelRoutine field, while the check actually looks for incorrect assignments to DPCs or DPC fields. This verison of the check implements both these behaviors. + + +## Recommendation +Instead of making direct assignments, refer to MSDN and the output of running this query for the correct API to use. + + +## Example +In this example, the driver directly edits an IRP's CancelRoutine, which is not supported. + +```c + + irp->CancelRoutine = myCancelRoutine; + + +``` +The driver should instead call IoSetCancelRoutine: + +```c + + oldCancel = IoSetCancelRoutine(irp, myCancelRoutine); + + +``` + +## References +* [ C28128 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28128-structure-member-directly-accessed) diff --git a/src/drivers/wdm/queries/IllegalFieldAccess2/IllegalFieldAccess2.md b/src/drivers/wdm/queries/IllegalFieldAccess2/IllegalFieldAccess2.md new file mode 100644 index 00000000..c0088b4a --- /dev/null +++ b/src/drivers/wdm/queries/IllegalFieldAccess2/IllegalFieldAccess2.md @@ -0,0 +1,37 @@ +# Illegal access to a protected field (C28175) +Drivers should not read inaccessible fields of driver structs. + + +## Recommendation +Driver developers should avoid reading these fields in their drivers. + + +## Example +In this example, the driver directly reads a DeviceObject's "Size" field. + +```c + + NTSTATUS CompletionRoutine( + PDEVICE_OBJECT DeviceObject, + PIRP Irp, + PVOID Context + ) + { + if (DeviceObject->Size > 0x10) + { + // Do some logic + } + return; + } + + +``` +The driver should not access this field. + + +## References +* [ C28175 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28175struct-member-should-not-be-accessed-by-driver) + +## Semmle-specific notes +It is legal for filesystem drivers to access some fields that should not be accessed by other drivers; this case is not accounted for in the rule at this time. + diff --git a/src/drivers/wdm/queries/IllegalFieldWrite/IllegalFieldWrite.md b/src/drivers/wdm/queries/IllegalFieldWrite/IllegalFieldWrite.md new file mode 100644 index 00000000..415c1bda --- /dev/null +++ b/src/drivers/wdm/queries/IllegalFieldWrite/IllegalFieldWrite.md @@ -0,0 +1,31 @@ +# Illegal write to a protected field (C28176) +The driver is writing to a read-only field of a driver struct. + +Note that this check only reports violations when the field is explicitly read-only, and not completely inaccessible. For finding reads of inaccessible fields, use IllegalFieldAccess and IllegalFieldAccess2. + + +## Recommendation +Driver writers should not make changes to these fields except in specific contexts. + + +## Example +In this example, the driver directly edits a DriverObject's flags in a DriverUnload callback, which is not supported. + +```c + + VOID + DriverUnload ( + PDRIVER_OBJECT DriverObject + ) + { + DriverObject->Flags &= 0x100000; + return; + } + + +``` +The driver should only adjust the Flags field in DriverEntry. + + +## References +* [ C28176 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28176-struct-member-should-not-be-modified-by-driver) diff --git a/src/drivers/wdm/queries/InconsistentDispatchAnnotations.md b/src/drivers/wdm/queries/InconsistentDispatchAnnotations.md new file mode 100644 index 00000000..c7b55079 --- /dev/null +++ b/src/drivers/wdm/queries/InconsistentDispatchAnnotations.md @@ -0,0 +1 @@ +# Inconsistent annotation of WDM dispatch routine. diff --git a/src/drivers/wdm/queries/InitNotCleared/InitNotCleared.md b/src/drivers/wdm/queries/InitNotCleared/InitNotCleared.md new file mode 100644 index 00000000..2bf882a9 --- /dev/null +++ b/src/drivers/wdm/queries/InitNotCleared/InitNotCleared.md @@ -0,0 +1,136 @@ +# Failure to clear DO_DEVICE_INITIALIZING (C28152) +A WDM AddDevice routine must clear the DO_DEVICE_INITIALIZING flag of the filter device object or functional device object it creates. + + +## Recommendation +Clear the DO_DEVICE_INITIALIZING flag of the FDO with code like "FunctionalDeviceObject->Flags &= ~DO_DEVICE_INITIALIZING;" + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// + +#define SET_DISPATCH 1 + +DRIVER_ADD_DEVICE AddDevice_Pass1; +DRIVER_ADD_DEVICE AddDevice_Pass2; +DRIVER_ADD_DEVICE AddDevice_Fail1; +DRIVER_ADD_DEVICE AddDevice_Fail2; + +// Template. Not called in this test. +void top_level_call() {} + +// PASS: Directly creates the FDO and clears flags + +NTSTATUS +AddDevice_Pass1 ( + PDRIVER_OBJECT DriverObject, + PDEVICE_OBJECT PhysicalDeviceObject + ) +{ + PDEVICE_OBJECT device; + PDEVICE_OBJECT TopOfStack; + NTSTATUS status; + + UNREFERENCED_PARAMETER(PhysicalDeviceObject); + + PAGED_CODE(); + + status = IoCreateDevice(DriverObject, + sizeof(DRIVER_DEVICE_EXTENSION), + NULL, + FILE_DEVICE_DISK, + 0, + FALSE, + &device + ); + if(status==STATUS_SUCCESS) + { + device->Flags &= ~DO_DEVICE_INITIALIZING; + } + + return status; +} + +// PASS: Indirectly creates the FDO and clears flags +NTSTATUS +AddDevice_Pass2 ( + PDRIVER_OBJECT DriverObject, + PDEVICE_OBJECT PhysicalDeviceObject + ) +{ + NTSTATUS status; + + UNREFERENCED_PARAMETER(PhysicalDeviceObject); + + PAGED_CODE(); + + status = AddDevice_Pass1(DriverObject, PhysicalDeviceObject); + + return status; +} + +// FAIL: Creates the FDO but doesn't clear flags +NTSTATUS +AddDevice_Fail1 ( + PDRIVER_OBJECT DriverObject, + PDEVICE_OBJECT PhysicalDeviceObject + ) +{ + PDEVICE_OBJECT device; + PDEVICE_OBJECT TopOfStack; + NTSTATUS status; + + UNREFERENCED_PARAMETER(PhysicalDeviceObject); + + PAGED_CODE(); + + status = IoCreateDevice(DriverObject, + sizeof(DRIVER_DEVICE_EXTENSION), + NULL, + FILE_DEVICE_DISK, + 0, + FALSE, + &device + ); + return status; +} + +// FAIL: Creates the FDO but doesn't clear the correct flags +NTSTATUS +AddDevice_Fail2 ( + PDRIVER_OBJECT DriverObject, + PDEVICE_OBJECT PhysicalDeviceObject + ) +{ + PDEVICE_OBJECT device; + PDEVICE_OBJECT TopOfStack; + NTSTATUS status; + + UNREFERENCED_PARAMETER(PhysicalDeviceObject); + + PAGED_CODE(); + + status = IoCreateDevice(DriverObject, + sizeof(DRIVER_DEVICE_EXTENSION), + NULL, + FILE_DEVICE_DISK, + 0, + FALSE, + &device + ); + if(status==STATUS_SUCCESS) + { + device->Flags &= ~DO_POWER_PAGABLE; + } + return status; +} +``` + +## References +* [ C28152 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28152-do-device-initializing-flag-not-cleared) diff --git a/src/drivers/wdm/queries/KeWaitLocal/KeWaitLocal.md b/src/drivers/wdm/queries/KeWaitLocal/KeWaitLocal.md new file mode 100644 index 00000000..d596d968 --- /dev/null +++ b/src/drivers/wdm/queries/KeWaitLocal/KeWaitLocal.md @@ -0,0 +1,37 @@ +# Use of local variable and UserMode in call to KeWaitSingleObject +If the first argument to KeWaitForSingleObject is a local variable, the Mode parameter must be KernelMode The driver is waiting in user mode. As such, the kernel stack can be swapped out during the wait. If the driver attempts to pass parameters on the stack, a system crash can result. + + +## Recommendation +If the first argument to KeWaitForSingleObject is a local variable, the Mode parameter must be KernelMode. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +//Macros to enable or disable a code section that may or maynot conflict with this test. +#define SET_DISPATCH 1 + +void good_use(){ + //Raises Warning + KEVENT kevent1; + KeWaitForSingleObject(&kevent1, UserRequest, UserMode, FALSE, NULL); +} + +void bad_use(){ + //Avoids warning as it's AccessMode is KernelMode for a local first argument, kevent2. + KEVENT kevent2; + KeWaitForSingleObject(&kevent2, UserRequest, KernelMode, FALSE, NULL); +} + +void top_level_call() { + good_use(); + bad_use(); +} +``` + +## References +* [ C28135 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28135-first-argument-to-kewaitforsingleobject) diff --git a/src/drivers/wdm/queries/MultiplePagedCode/MultiplePagedCode.md b/src/drivers/wdm/queries/MultiplePagedCode/MultiplePagedCode.md new file mode 100644 index 00000000..2b561625 --- /dev/null +++ b/src/drivers/wdm/queries/MultiplePagedCode/MultiplePagedCode.md @@ -0,0 +1,74 @@ +# Multiple instances of PAGED_CODE or PAGED_CODE_LOCKED +The function has more than one instance of PAGED_CODE or PAGED_CODE_LOCKED. This warning indicates that there is more than one instance of the PAGED_CODE or PAGED_CODE_LOCKED macro in a function. This error is reported at the second or subsequent instances of the PAGED_CODE or PAGED_CODE_LOCKED macro. + + +## Recommendation +Remove all but one PAGED_CODE OR PAGED_CODE_LOCKED macro. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + + +//Macros to enable or disable a code section that may or maynot conflict with this test. +#define SET_DISPATCH 1 +#define SET_PAGE_CODE 1 + + +_Dispatch_type_(IRP_MJ_CLEANUP) +DRIVER_DISPATCH DispatchCleanup; + +_Dispatch_type_(IRP_MJ_SHUTDOWN) +DRIVER_DISPATCH DispatchShutdown; + +#ifndef __cplusplus +#pragma alloc_text (PAGE, DispatchCleanup) +#pragma alloc_text (PAGE, DispatchShutdown) +#endif + + +//Template +void top_level_call(){ +} + +//Passes +NTSTATUS +DispatchCleanup ( + PDEVICE_OBJECT DriverObject, + PIRP Irp + ) +{ + UNREFERENCED_PARAMETER(DriverObject); + UNREFERENCED_PARAMETER(Irp); + PAGED_CODE(); + + return STATUS_SUCCESS; +} + +//Fails +NTSTATUS +DispatchShutdown ( + PDEVICE_OBJECT DriverObject, + PIRP Irp + ) +{ + UNREFERENCED_PARAMETER(DriverObject); + UNREFERENCED_PARAMETER(Irp); + PAGED_CODE(); + PAGED_CODE(); + + return STATUS_SUCCESS; +} + + + + + + +``` + +## References +* [ C28171 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28171-function-has-more-than-one-page-macro-instance) diff --git a/src/drivers/wdm/queries/NoPagedCode/NoPagedCode.md b/src/drivers/wdm/queries/NoPagedCode/NoPagedCode.md new file mode 100644 index 00000000..abd7f263 --- /dev/null +++ b/src/drivers/wdm/queries/NoPagedCode/NoPagedCode.md @@ -0,0 +1,89 @@ +# No PAGED_CODE invocation +The function has been declared to be in a paged segment, but neither PAGED_CODE nor PAGED_CODE_LOCKED was found. + + +## Recommendation +The functions in pageable code must contain a PAGED_CODE or PAGED_CODE_LOCKED macro at the beginning of the function. The PAGED_CODE macro ensures that the calling thread is running at an IRQL that is low enough to permit paging. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + + +//Macros to enable or disable a code section that may or maynot conflict with this test. +#define SET_DISPATCH 1 +#define SET_PAGE_CODE 1 + + +_Dispatch_type_(IRP_MJ_CLEANUP) +DRIVER_DISPATCH DispatchCleanup; + +_Dispatch_type_(IRP_MJ_SHUTDOWN) +DRIVER_DISPATCH DispatchShutdown; + +#ifndef __cplusplus +#pragma alloc_text (PAGE, DispatchCleanup) +#pragma alloc_text (PAGE, DispatchShutdown) +#endif + + +//Template +void top_level_call(){ +} + +//Passes +NTSTATUS +DispatchCleanup ( + PDEVICE_OBJECT DriverObject, + PIRP Irp + ) +{ + UNREFERENCED_PARAMETER(DriverObject); + UNREFERENCED_PARAMETER(Irp); + PAGED_CODE(); + + return STATUS_SUCCESS; +} + +//Fails +NTSTATUS +DispatchShutdown ( + PDEVICE_OBJECT DriverObject, + PIRP Irp + ) +{ + UNREFERENCED_PARAMETER(DriverObject); + UNREFERENCED_PARAMETER(Irp); + + return STATUS_SUCCESS; +} + + +#pragma code_seg("PAGE") +//Fails +NTSTATUS func1(){ + if(TRUE){ + + } + return STATUS_SUCCESS; +} +#pragma code_seg() + +//Passes +NTSTATUS func2(){ + return STATUS_SUCCESS; +} + +#define PAGED_CODE_SEG __declspec(code_seg("PAGE")) +//Fails +PAGED_CODE_SEG +NTSTATUS func3(){ + return STATUS_SUCCESS; +} +``` + +## References +* [ C28170 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28170-pageable-code-macro-not-found) diff --git a/src/drivers/wdm/queries/NoPagingSegment/NoPagingSegment.md b/src/drivers/wdm/queries/NoPagingSegment/NoPagingSegment.md new file mode 100644 index 00000000..cd31eb87 --- /dev/null +++ b/src/drivers/wdm/queries/NoPagingSegment/NoPagingSegment.md @@ -0,0 +1,91 @@ +# No paging segment for PAGED_CODE macro invocation +The function has PAGED_CODE or PAGED_CODE_LOCKED but is not declared to be in a paged segment. A function that contains a PAGED_CODE or PAGED_CODE_LOCKED macro has not been placed in paged memory by using \#pragma alloc_text or \#pragma code_seg. + + +## Recommendation +Put a function/routine that calls PAGED_CODE in a paged section using \#pragma alloc_text or \#pragma code_seg. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + + +//Macros to enable or disable a code section that may or maynot conflict with this test. +#define SET_DISPATCH 1 +#define SET_PAGE_CODE 1 + + +_Dispatch_type_(IRP_MJ_CLEANUP) +DRIVER_DISPATCH DispatchCleanup; + +_Dispatch_type_(IRP_MJ_SHUTDOWN) +DRIVER_DISPATCH DispatchShutdown; + +#ifndef __cplusplus +#pragma alloc_text (PAGE, DispatchCleanup) +#endif + + +//Template +void top_level_call(){ +} + +//Passes +NTSTATUS +DispatchCleanup ( + PDEVICE_OBJECT DriverObject, + PIRP Irp + ) +{ + UNREFERENCED_PARAMETER(DriverObject); + UNREFERENCED_PARAMETER(Irp); + PAGED_CODE(); + + return STATUS_SUCCESS; +} + +//Fails +NTSTATUS +DispatchShutdown ( + PDEVICE_OBJECT DriverObject, + PIRP Irp + ) +{ + UNREFERENCED_PARAMETER(DriverObject); + UNREFERENCED_PARAMETER(Irp); + PAGED_CODE(); + + return STATUS_SUCCESS; +} + + +#pragma code_seg("PAGE") +//Passes +NTSTATUS func1(){ + PAGED_CODE(); + if(TRUE){ + } + return STATUS_SUCCESS; +} +#pragma code_seg() + +//Fails +NTSTATUS func2(){ + PAGED_CODE(); + return STATUS_SUCCESS; +} + +#define PAGED_CODE_SEG __declspec(code_seg("PAGE")) +//Passes +PAGED_CODE_SEG +NTSTATUS func3(){ + PAGED_CODE(); + return STATUS_SUCCESS; +} +``` + +## References +* [ C28172 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28172-function-macros-not-in-paged-segment) diff --git a/src/drivers/wdm/queries/ObReferenceMode/ObReferenceMode.md b/src/drivers/wdm/queries/ObReferenceMode/ObReferenceMode.md new file mode 100644 index 00000000..86978a1e --- /dev/null +++ b/src/drivers/wdm/queries/ObReferenceMode/ObReferenceMode.md @@ -0,0 +1,45 @@ +# The AccessMode parameter to ObReferenceObject* should be IRP->RequestorMode (C28126) +In a dispatch routine call to ObReferenceObjectByHandle or ObReferenceObjectByPointer, the driver is passing UserMode or KernelMode for the AccessMode parameter, instead of using Irp->RequestorMode. + +This check applies only to the top driver in the stack. It can be ignored or suppressed otherwise. + + +## Recommendation +The top-level driver in the driver stack should use Irp->RequestorMode, rather than specifying UserMode or KernelMode. This allows the senders of kernel-mode IRP to supply kernel-mode handles safely. + + +## Example + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// driver_snippet.c +// + +#define SET_DISPATCH 1 +#define SET_CUSTOM_CREATE + +_Dispatch_type_(IRP_MJ_PNP) +DRIVER_DISPATCH DispatchPnp; +_Dispatch_type_(IRP_MJ_READ) +DRIVER_DISPATCH DispatchRead; + +// Template. Not called in this test. +void top_level_call() {} + +_Dispatch_type_(IRP_MJ_CREATE) +NTSTATUS +DispatchCreate ( + PDEVICE_OBJECT DeviceObject, + PIRP Irp + ) +{ + ObReferenceObjectByPointer(NULL, 0, 0, KernelMode); // ERROR + ObReferenceObjectByPointer(NULL, 0, 0, Irp->RequestorMode); // GOOD + return STATUS_SUCCESS; +} +``` + +## References +* [ C28126 warning - Windows Drivers ](https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/28126-accessmode-param-incorrect) diff --git a/src/drivers/wdm/queries/OpaqueMdlUse/OpaqueMdlUse.md b/src/drivers/wdm/queries/OpaqueMdlUse/OpaqueMdlUse.md new file mode 100644 index 00000000..7f635f4d --- /dev/null +++ b/src/drivers/wdm/queries/OpaqueMdlUse/OpaqueMdlUse.md @@ -0,0 +1,54 @@ +# Direct access of opaque MDL field +Direct access of opaque MDL fields should be avoided, as opaque struct layouts may change without warning. + + +## Recommendation +Instead of accessing MDL fields directly, driver writers should make use of the [MmGetMdlVirtualAddress](https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmgetmdlvirtualaddress), [MmGetMdlByteCount](https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmgetmdlbytecount), [MmGetMdlByteOffset](https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmgetmdlbyteoffset), and [MmGetSystemAddressForMdlSafe](https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-mmgetsystemaddressformdlsafe) macros. + + +## Example +In this example, the driver directly accesses the ByteCount field of an MDL. + +```c + + VOID DummyMdlFunction(PMDL Mdl) + { + PMDL currentMdl, nextMdl; + + for (currentMdl = Mdl; currentMdl != NULL; currentMdl = nextMdl) + { + nextMdl = currentMdl->Next; + if (currentMdl->MdlFlags & MDL_PAGES_LOCKED && currentMdl->ByteCount > 0) + { + MmUnlockPages(currentMdl); + } + IoFreeMdl(currentMdl); + } + } + + +``` +The driver should instead use the MmGetMdlByteCount function. + +```c + + VOID DummyMdlFunction(PMDL Mdl) + { + PMDL currentMdl, nextMdl; + + for (currentMdl = Mdl; currentMdl != NULL; currentMdl = nextMdl) + { + nextMdl = currentMdl->Next; + if (currentMdl->MdlFlags & MDL_PAGES_LOCKED && MmGetMdlByteCount(currentMdl) > 0) + { + MmUnlockPages(currentMdl); + } + IoFreeMdl(currentMdl); + } + } + + +``` + +## References +* [ Using MDLs (MSDN) ](https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/using-mdls) diff --git a/src/drivers/wdm/queries/OpaqueMdlWrite/OpaqueMdlWrite.md b/src/drivers/wdm/queries/OpaqueMdlWrite/OpaqueMdlWrite.md new file mode 100644 index 00000000..4958e177 --- /dev/null +++ b/src/drivers/wdm/queries/OpaqueMdlWrite/OpaqueMdlWrite.md @@ -0,0 +1,34 @@ +# Write to opaque MDL field (C28145) +Drivers should not write to opaque MDL fields. + + +## Recommendation +If the driver is using an MDL in an NDIS driver, you can call the [NdisAdjustMdlLength](https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/ndis/nf-ndis-ndisadjustmdllength) macro to modify the length of the data that is associated with an MDL. Otherwise, you should avoid modifying MDL fields after creation. + + +## Example +In this example, the driver directly edits the ByteCount field of the MDL. This should not be directly edited. + +```c + + VOID DummyMdlFunction(PMDL Mdl) + { + PMDL currentMdl, nextMdl; + + for (currentMdl = Mdl; currentMdl != NULL; currentMdl = nextMdl) + { + nextMdl = currentMdl->Next; + if (currentMdl->MdlFlags & MDL_PAGES_LOCKED && currentMdl->ByteCount > 0) + { + MmUnlockPages(currentMdl); + } + IoFreeMdl(currentMdl); + } + } + + +``` + +## References +* [ Using MDLs (MSDN) ](https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/using-mdls) +* [ C28145 (Code Analysis for Drivers) ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28145-opaque-mdl-structure-should-not-be-modified) diff --git a/src/drivers/wdm/queries/PendingStatusError/PendingStatusError.md b/src/drivers/wdm/queries/PendingStatusError/PendingStatusError.md new file mode 100644 index 00000000..3310a84c --- /dev/null +++ b/src/drivers/wdm/queries/PendingStatusError/PendingStatusError.md @@ -0,0 +1,32 @@ +# Did not return STATUS_PENDING after IoMarkIrpPending call +A dispatch routine that calls IoMarkIrpPending must also return STATUS_PENDING + + +## Recommendation +A dispatch routine that calls IoMarkIrpPending includes at least one path in which the driver returns a value other than STATUS_PENDING. The IoMarkIrpPending routine marks the specified IRP, indicating that a driver's dispatch routine subsequently returned STATUS_PENDING because further processing is required by other driver routines. + + +## Example +In this example, the driver marks an IRP pending but returns STATUS_SUCCESS. + +```c + + IoMarkIrpPending(Irp); + ... + return STATUS_SUCCESS; + + +``` +The driver should instead ensure that it returns STATUS_PENDING. + +```c + + IoMarkIrpPending(Irp); + ... + return STATUS_PENDING; + + +``` + +## References +* [ C28143 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28143-iomarkirppending-must-return-statuspending) diff --git a/src/drivers/wdm/queries/WrongDispatchTableAssignment/WrongDispatchTableAssignment.md b/src/drivers/wdm/queries/WrongDispatchTableAssignment/WrongDispatchTableAssignment.md new file mode 100644 index 00000000..0a8576b7 --- /dev/null +++ b/src/drivers/wdm/queries/WrongDispatchTableAssignment/WrongDispatchTableAssignment.md @@ -0,0 +1,33 @@ +# Incorrect dispatch table assignment +The dispatch table assignment satisfies any of these 3 scenarios: 1) The dispatch table assignment has a function whose type is not DRIVER_DISPATCH, or 2) The dispatch table assignment has a DRIVER_DISPATCH function at its right-hand side but the function doesn't have a driver dispatch type annotation, or 3) The dispatch function satisfies both of the above conditions but its dispatch type doesn't match the expected type for the dispatch table entry. + + +## Recommendation +This defect can be corrected either using a DRIVER_DISPATCH type function or by adding a _Dispatch_type_ annotation to the function or correcting the dispatch table entry being used. + + +## Example +In this example, the driver has a DRIVER_DISPATCH routine that is not annotated with the type(s) of IRP it handles. + +```c + + DRIVER_DISPATCH SampleCreate; + ... + pDo->MajorFunction[IRP_MJ_CREATE] = SampleCreate; + + +``` +The driver should instead annotate its dispatch routines appropriately. + +```c + + _Dispatch_type_(IRP_MJ_CREATE) DRIVER_DISPATCH SampleCreate; + ... + pDo->MajorFunction[IRP_MJ_CREATE] = SampleCreate;... + + +``` + +## References +* [ C28168 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28168-dispatch-function-dispatch-annotation) +* [ C28169 warning - Windows Drivers ](https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/28169-dispatch-function-does-not-have-proper-annotation) diff --git a/src/microsoft/Likely Bugs/Boundary Violations/PaddingByteInformationDisclosure.md b/src/microsoft/Likely Bugs/Boundary Violations/PaddingByteInformationDisclosure.md new file mode 100644 index 00000000..a7ba501e --- /dev/null +++ b/src/microsoft/Likely Bugs/Boundary Violations/PaddingByteInformationDisclosure.md @@ -0,0 +1,46 @@ +# Possible information leakage from uninitialized padding bytes. +A newly allocated struct or class that is initialized member-by-member may leak information if it includes padding bytes. + + +## Recommendation +Make sure that all padding bytes in the struct or class are initialized. + +If possible, use `memset` to initialize the whole structure/class. + + +## Example +The following example shows a scenario where padding between the first and second elements are not initialized. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +typedef enum { Unknown = 0, Known = 1, Other = 2 } MyStructType; +struct MyStruct { MyStructType type; UINT64 id; }; + +MyStruct testReturn() +{ + // BAD: Padding between the first and second elements not initialized. + MyStruct myBadStackStruct = { Unknown }; + return myBadStackStruct; +} +``` +To correct it, we will initialize all bytes using `memset`. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +typedef enum { Unknown = 0, Known = 1, Other = 2 } MyStructType; +struct MyStruct { MyStructType type; UINT64 id; }; + +MyStruct testReturn() +{ + // GOOD: All padding bytes initialized + MyStruct* myGoodHeapStruct = (struct MyStruct*)malloc(sizeof(struct MyStruct)); + memset(myGoodHeapStruct, 0, sizeof(struct MyStruct)); + return *myGoodHeapStruct; +} +``` diff --git a/src/microsoft/Likely Bugs/Conversion/BadOverflowGuard.md b/src/microsoft/Likely Bugs/Conversion/BadOverflowGuard.md new file mode 100644 index 00000000..23b81aed --- /dev/null +++ b/src/microsoft/Likely Bugs/Conversion/BadOverflowGuard.md @@ -0,0 +1,44 @@ +# Bad overflow check +Checking for overflow of an addition by comparing against one of the arguments of the addition fails if the size of all the argument types are smaller than 4 bytes. This is because the result of the addition is promoted to a 4 byte int. + + +## Recommendation +Check the overflow by comparing the addition against a value that is at least 4 bytes. + + +## Example +In this example, the result of the comparison will result in an integer overflow. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +unsigned short CheckForInt16OverflowBadCode(unsigned short v, unsigned short b) +{ + if (v + b < v) // BUG: "v + b" will be promoted to 32 bits + { + // ... do something + } + + return v + b; +} + +``` +To fix the bug, check the overflow by comparing the addition against a value that is at least 4 bytes. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +unsigned short CheckForInt16OverflowCorrectCode(unsigned short v, unsigned short b) +{ + if (v + b > 0x00FFFF) + { + // ... do something + } + + return v + b; +} +``` diff --git a/src/microsoft/Likely Bugs/Conversion/InfiniteLoop.md b/src/microsoft/Likely Bugs/Conversion/InfiniteLoop.md new file mode 100644 index 00000000..c52e4e8c --- /dev/null +++ b/src/microsoft/Likely Bugs/Conversion/InfiniteLoop.md @@ -0,0 +1,40 @@ +# Comparison of narrow type with wide type in loop condition +Comparisons between types of different widths in a loop condition can cause the loop to fail to terminate. + + +## Recommendation +Use appropriate types in the loop condition. + + +## Example +In this example, the result of the comparison may result in an infinite loop if the value for argument `a` is larger than `SHRT_MAX`. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +void InfiniteLoop(int a) +{ + for (short i = 0; i < a; i++) // BUG: infinite loop + { + // ... + } +} + +``` +To fix the bug, we are changing the type for the variable `i` to match the width of `a`. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +void NotInfiniteLoop(int a) +{ + for (int i = 0; i < a; i++) + { + // ... + } +} +``` diff --git a/src/microsoft/Likely Bugs/Memory Management/UseAfterFree/ProbableUseAfterFree.md b/src/microsoft/Likely Bugs/Memory Management/UseAfterFree/ProbableUseAfterFree.md new file mode 100644 index 00000000..4dd8b81b --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UseAfterFree/ProbableUseAfterFree.md @@ -0,0 +1,66 @@ +# Potential use after free (low precision, for drivers) +This version of the query has lower precision than `cpp/use-after-free-high-precision`. It detects some additional scenarios, but also has a higher rate of false positives. + +An allocated memory block is used after it has been freed (also known as dangling pointer). + +Behavior in such cases is undefined and in practice can have many unintended consequences including memory corruption, use incorrect values, or arbitrary code execution. + + +## Recommendation +If possible set the pointers to NULL immediately after they are freed. + + +## Example +In the following example, `pSomePointer` is freed only if `Status` value was not zero, and before deferrencing `pSomePointer` to call `Method`, we check again `Status`; unfortunately `Status` was changed between the two references to `pSomePointer`, which allows for the possiblity that the call to `pSomePointer->Method()` is being performed over a previously freed pointer. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +NTSTATUS Status = x(); + +if (Status != 0) +{ + // Release pSomePointer if the call to x() failed + + ExFreePool(pSomePointer); +} + +Status = y(); + +if (Status == 0) +{ + // Because Status may no longer be the same value than it was before the pointer was released, + // this code may be using pSomePointer after it was freed, potentially executing arbitrary code. + + Status = pSomePointer->Method(); +} +``` +In the corrected example, `pSomePointer` is set to `NULL` immediately after being freed, and the condition to check if it is safe to call `pSomePointer->Method()` checks for this additional condition to prevent the possible bug. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +NTSTATUS Status = x(); + +if (Status != 0) +{ + // Release pSomePointer if the call to x() failed + + ExFreePool(pSomePointer); + + // Setting pSomePointer to NULL after being freed + pSomePointer = NULL; +} + +Status = y(); + +// If pSomePointer was freed above, its value must have been set to NULL +if (Status == 0 && pSomePointer != NULL) +{ + Status = pSomePointer->Method(); +} +``` diff --git a/src/microsoft/Likely Bugs/Memory Management/UseAfterFree/UseAfterFree.md b/src/microsoft/Likely Bugs/Memory Management/UseAfterFree/UseAfterFree.md new file mode 100644 index 00000000..9435d166 --- /dev/null +++ b/src/microsoft/Likely Bugs/Memory Management/UseAfterFree/UseAfterFree.md @@ -0,0 +1,66 @@ +# Potential use after free (high precision, for drivers) +This version of the query has high precision, which helps in bug automation, but there are some limitations and therefore it will not be able to detect some cases. + +An allocated memory block is used after it has been freed (also known as dangling pointer). + +Behavior in such cases is undefined and in practice can have many unintended consequences including memory corruption, use incorrect values, or arbitrary code execution. + + +## Recommendation +If possible set the pointers to NULL immediately after they are freed. + + +## Example +In the following example, `pSomePointer` is freed only if `Status` value was not zero, and before deferrencing `pSomePointer` to call `Method`, we check again `Status`; unfortunately `Status` was changed between the two references to `pSomePointer`, which allows for the possiblity that the call to `pSomePointer->Method()` is being performed over a previously freed pointer. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +NTSTATUS Status = x(); + +if (Status != 0) +{ + // Release pSomePointer if the call to x() failed + + ExFreePool(pSomePointer); +} + +Status = y(); + +if (Status == 0) +{ + // Because Status may no longer be the same value than it was before the pointer was released, + // this code may be using pSomePointer after it was freed, potentially executing arbitrary code. + + Status = pSomePointer->Method(); +} +``` +In the corrected example, `pSomePointer` is set to `NULL` immediately after being freed, and the condition to check if it is safe to call `pSomePointer->Method()` checks for this additional condition to prevent the possible bug. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +NTSTATUS Status = x(); + +if (Status != 0) +{ + // Release pSomePointer if the call to x() failed + + ExFreePool(pSomePointer); + + // Setting pSomePointer to NULL after being freed + pSomePointer = NULL; +} + +Status = y(); + +// If pSomePointer was freed above, its value must have been set to NULL +if (Status == 0 && pSomePointer != NULL) +{ + Status = pSomePointer->Method(); +} +``` diff --git a/src/microsoft/Likely Bugs/UninitializedPtrField.md b/src/microsoft/Likely Bugs/UninitializedPtrField.md new file mode 100644 index 00000000..cf55d277 --- /dev/null +++ b/src/microsoft/Likely Bugs/UninitializedPtrField.md @@ -0,0 +1,122 @@ +# Dereference of potentially uninitialized pointer field +A pointer field which was not initialized during or since class construction will cause a null pointer dereference. + + +## Recommendation +Make sure to initialize all pointer fields before usage. + + +## Example +The following example shows a scenario where the field `ptr_` is not initialzied and later dereferenced. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +template +class ComPtr +{ +public: + T* ptr_; + + ComPtr() throw() : ptr_(nullptr) + { + } + + ComPtr(T* ptr) throw() : ptr_(ptr) + { + } + + T* operator->() const throw() + { + return ptr_; + } + + void set(T* ptr) { + ptr_ = ptr; + } + + T** addr() { + return &ptr_; + } +}; + +class Test +{ +public: + int it_; + int it() { + return it_; + } +}; + +void test() { + Test t; + int val; + + ComPtr ptr; + // BAD: pointer is not initialized here + val = ptr->it(); +} + +``` +To correct the problem, we set the field before usage. + + +```c +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +template +class ComPtr +{ +public: + T* ptr_; + + ComPtr() throw() : ptr_(nullptr) + { + } + + ComPtr(T* ptr) throw() : ptr_(ptr) + { + } + + T* operator->() const throw() + { + return ptr_; + } + + void set(T* ptr) { + ptr_ = ptr; + } + + T** addr() { + return &ptr_; + } +}; + +class Test +{ +public: + int it_; + int it() { + return it_; + } +}; + +void test() { + Test t; + int val; + + ComPtr ptr2(&t); + // GOOD: pointer was initialized + val = ptr2->it(); + + ComPtr ptr3; + ptr3.set(&t); + // GOOD: pointer was set in between + val = ptr3->it(); +} + +``` diff --git a/src/microsoft/Security/CWE/CWE-704/WcharCharConversionLimited.md b/src/microsoft/Security/CWE/CWE-704/WcharCharConversionLimited.md new file mode 100644 index 00000000..a31600b0 --- /dev/null +++ b/src/microsoft/Security/CWE/CWE-704/WcharCharConversionLimited.md @@ -0,0 +1,32 @@ +# Cast from char* to wchar_t* (ignore PUCHAR casts) +This rule indicates a potentially incorrect cast from an byte string (`char *`) to a wide-character string (`wchar_t *`). + +This cast might yield strings that are not correctly terminated; including potential buffer overruns when using such strings with some dangerous APIs. + +This version of the query is a subset of the GitHub query with ID `>cpp/incorrect-string-type-conversion` that limits the detection of `PUCHAR` casting to avoid certain commonly used patterns. + + +## Recommendation +Do not explicitly cast byte strings to wide-character strings. + +For string literals, prepend the literal string with the letter "L" to indicate that the string is a wide-character string (`wchar_t *`). + +For converting a byte literal to a wide-character string literal, you would need to use the appropriate conversion function for the platform you are using. Please see the references section for options according to your platform. + + +## Example +In the following example, an byte string literal (`"a"`) is cast to a wide-character string. + + +```cpp +wchar_t* pSrc; + +pSrc = (wchar_t*)"a"; // casting a byte-string literal "a" to a wide-character string +``` +To fix this issue, prepend the literal with the letter "L" (`L"a"`) to define it as a wide-character string. + + +## References +* General resources: [std::mbstowcs](https://en.cppreference.com/w/cpp/string/multibyte/mbstowcs) +* Microsoft specific resources: [Security Considerations: International Features](https://docs.microsoft.com/en-us/windows/desktop/Intl/security-considerations--international-features) +* Common Weakness Enumeration: [CWE-704](https://cwe.mitre.org/data/definitions/704.html). diff --git a/src/microsoft/Security/Crytpography/HardcodedIVCNG.md b/src/microsoft/Security/Crytpography/HardcodedIVCNG.md new file mode 100644 index 00000000..69a68e71 --- /dev/null +++ b/src/microsoft/Security/Crytpography/HardcodedIVCNG.md @@ -0,0 +1,15 @@ +# Weak cryptography +An initialization vector (IV) is an input to a cryptographic primitive being used to provide the initial state. The IV is typically required to be random or pseudorandom (randomized scheme), but sometimes an IV only needs to be unpredictable or unique (stateful scheme). + +Randomization is crucial for some encryption schemes to achieve semantic security, a property whereby repeated usage of the scheme under the same key does not allow an attacker to infer relationships between (potentially similar) segments of the encrypted message. + + +## Recommendation +All symmetric block ciphers must also be used with an appropriate initialization vector (IV) according to the mode of operation being used. + +If using a randomized scheme such as CBC, it is recommended to use cryptographically secure pseudorandom number generator such as `BCryptGenRandom`. + + +## References +* [BCryptEncrypt function (bcrypt.h)](https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptencrypt) [BCryptGenRandom function (bcrypt.h)](https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom) [Initialization vector (Wikipedia)](https://en.wikipedia.org/wiki/Initialization_vector) +* Common Weakness Enumeration: [CWE-327](https://cwe.mitre.org/data/definitions/327.html).