Re: [RFC PATCH 1/5] Add RegistryOpenParametersKey

["Durrant, Paul" <xadimgnik@xxxxxxxxx>]

On 07/06/2024 08:16, Owen Smith wrote:
> Use IoOpenDriverRegistryKey to avoid opening an absolute registry path.
> Driver Verifier can detect registry isolation violations when running WHQL
> tests on Server 2025. The rule states that a driver may not open an absolute
> registry key path. Use the specific API to open the 'Parameters' key with
> KEY_READ when querying settings.
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
> ---
>   src/common/registry.c | 22 +++++++++++++++++++++-
>   src/common/registry.h |  9 ++++++++-
>   src/xen/driver.c      |  2 +-
>   src/xenbus/driver.c   | 19 ++-----------------
>   src/xenfilt/driver.c  | 33 +++------------------------------
>   5 files changed, 35 insertions(+), 50 deletions(-)
>
> diff --git a/src/common/registry.c b/src/common/registry.c
> index 0d29ddc..73a55d2 100644
> --- a/src/common/registry.c
> +++ b/src/common/registry.c
> @@ -38,6 +38,7 @@
>   
>   #define REGISTRY_TAG 'GERX'
>   
> +static PDRIVER_OBJECT   DriverObject;
>   static UNICODE_STRING   RegistryPath;
>   
>   static FORCEINLINE PVOID
> @@ -58,7 +59,8 @@ __RegistryFree(
>   
>   NTSTATUS
>   RegistryInitialize(
> -    IN PUNICODE_STRING  Path
> +    IN  PDRIVER_OBJECT  DrvObj,
> +    IN  PUNICODE_STRING Path
Acked-by: Paul Durrant <paul@xxxxxxx>

... but I don't like the 'DrvObj' thing.... I'll call it 'DriverObject' 
and change the global to 'RegistryDriverObject'.
>       )
>   {
>       NTSTATUS            status;
> @@ -69,6 +71,9 @@ RegistryInitialize(
>       if (!NT_SUCCESS(status))
>           goto fail1;
>   
> +    ASSERT3P(DriverObject, ==, NULL);
> +    DriverObject = DrvObj;
> +
>       return STATUS_SUCCESS;
>   
>   fail1:
> @@ -82,11 +87,26 @@ RegistryTeardown(
>       VOID
>       )
>   {
> +    DriverObject = NULL;
> +
>       RtlFreeUnicodeString(&RegistryPath);
>       RegistryPath.Buffer = NULL;
>       RegistryPath.MaximumLength = RegistryPath.Length = 0;
>   }
>   
> +NTSTATUS
> +RegistryOpenParametersKey(
> +    IN  ACCESS_MASK     DesiredAccess,
> +    OUT PHANDLE         Key
> +    )
> +{
> +    return IoOpenDriverRegistryKey(DriverObject,
> +                                   DriverRegKeyParameters,
> +                                   DesiredAccess,
> +                                   0,
> +                                   Key);
> +}
> +
>   NTSTATUS
>   RegistryOpenKey(
>       IN  HANDLE          Parent,
> diff --git a/src/common/registry.h b/src/common/registry.h
> index cbe9015..efa96ea 100644
> --- a/src/common/registry.h
> +++ b/src/common/registry.h
> @@ -37,7 +37,8 @@
>   
>   extern NTSTATUS
>   RegistryInitialize(
> -    IN PUNICODE_STRING  Path
> +    IN  PDRIVER_OBJECT  DrvObj,
> +    IN  PUNICODE_STRING Path
>       );
>   
>   extern VOID
> @@ -45,6 +46,12 @@ RegistryTeardown(
>       VOID
>       );
>   
> +extern NTSTATUS
> +RegistryOpenParametersKey(
> +    IN  ACCESS_MASK     DesiredAccess,
> +    OUT PHANDLE         Key
> +    );
> +
>   extern NTSTATUS
>   RegistryOpenKey(
>       IN  HANDLE          Parent,
> diff --git a/src/xen/driver.c b/src/xen/driver.c
> index 8fe6c5c..e04a772 100644
> --- a/src/xen/driver.c
> +++ b/src/xen/driver.c
> @@ -515,7 +515,7 @@ DllInitialize(
>       if (!NT_SUCCESS(status))
>           goto fail1;
>   
> -    status = RegistryInitialize(RegistryPath);
> +    status = RegistryInitialize(NULL, RegistryPath);
>       if (!NT_SUCCESS(status))
>           goto fail2;
>   
> diff --git a/src/xenbus/driver.c b/src/xenbus/driver.c
> index 522acef..d6efe89 100644
> --- a/src/xenbus/driver.c
> +++ b/src/xenbus/driver.c
> @@ -811,7 +811,6 @@ DriverEntry(
>       IN  PUNICODE_STRING RegistryPath
>       )
>   {
> -    HANDLE              ServiceKey;
>       HANDLE              ParametersKey;
>       ULONG               Index;
>       LOG_LEVEL           LogLevel;
> @@ -839,21 +838,14 @@ DriverEntry(
>            MONTH,
>            YEAR);
>   
> -    status = RegistryInitialize(RegistryPath);
> +    status = RegistryInitialize(DriverObject, RegistryPath);
>       if (!NT_SUCCESS(status))
>           goto fail1;
>   
> -    status = RegistryOpenServiceKey(KEY_READ, &ServiceKey);
> +    status = RegistryOpenParametersKey(KEY_READ, &ParametersKey);
>       if (!NT_SUCCESS(status))
>           goto fail2;
>   
> -    status = RegistryOpenSubKey(ServiceKey,
> -                                "Parameters",
> -                                KEY_READ,
> -                                &ParametersKey);
> -    if (!NT_SUCCESS(status))
> -        goto fail3;
> -
>       __DriverSetParametersKey(ParametersKey);
>   
>       status = LogReadLogLevel(ParametersKey,
> @@ -864,8 +856,6 @@ DriverEntry(
>   
>       __DriverSetConsoleLogLevel(LogLevel);
>   
> -    RegistryCloseKey(ServiceKey);
> -
>       status = XenTouch(__MODULE__,
>                         MAJOR_VERSION,
>                         MINOR_VERSION,
> @@ -900,11 +890,6 @@ done:
>   
>       return STATUS_SUCCESS;
>   
> -fail3:
> -    Error("fail3\n");
> -
> -    RegistryCloseKey(ServiceKey);
> -
>   fail2:
>       Error("fail2\n");
>   
> diff --git a/src/xenfilt/driver.c b/src/xenfilt/driver.c
> index 724d418..aee663a 100644
> --- a/src/xenfilt/driver.c
> +++ b/src/xenfilt/driver.c
> @@ -113,33 +113,6 @@ DriverGetDriverObject(
>       return __DriverGetDriverObject();
>   }
>   
> -static FORCEINLINE NTSTATUS
> -__DriverOpenParametersKey(
> -    OUT PHANDLE     ParametersKey
> -    )
> -{
> -    HANDLE          ServiceKey;
> -    NTSTATUS        status;
> -
> -    status = RegistryOpenServiceKey(KEY_READ, &ServiceKey);
> -    if (!NT_SUCCESS(status))
> -        goto fail1;
> -
> -    status = RegistryOpenSubKey(ServiceKey, "Parameters", KEY_READ, 
> ParametersKey);
> -    if (!NT_SUCCESS(status))
> -        goto fail2;
> -
> -    RegistryCloseKey(ServiceKey);
> -
> -    return STATUS_SUCCESS;
> -
> -fail2:
> -    RegistryCloseKey(ServiceKey);
> -
> -fail1:
> -    return status;
> -}
> -
>   static FORCEINLINE VOID
>   __DriverSetEmulatedContext(
>       IN  PXENFILT_EMULATED_CONTEXT   Context
> @@ -247,7 +220,7 @@ __DriverGetActive(
>   
>       ASSERT3U(KeGetCurrentIrql(), ==, PASSIVE_LEVEL);
>   
> -    status = __DriverOpenParametersKey(&ParametersKey);
> +    status = RegistryOpenParametersKey(KEY_READ, &ParametersKey);
>       if (!NT_SUCCESS(status))
>           goto fail1;
>   
> @@ -752,7 +725,7 @@ DriverGetEmulatedType(
>       ULONG                           Index;
>       NTSTATUS                        status;
>   
> -    status = __DriverOpenParametersKey(&ParametersKey);
> +    status = RegistryOpenParametersKey(KEY_READ, &ParametersKey);
>       if (!NT_SUCCESS(status))
>           goto fail1;
>   
> @@ -947,7 +920,7 @@ DriverEntry(
>       if (!NT_SUCCESS(status))
>           goto done;
>   
> -    status = RegistryInitialize(RegistryPath);
> +    status = RegistryInitialize(DriverObject, RegistryPath);
>       if (!NT_SUCCESS(status))
>           goto fail1;
>