Re: [PATCH 3/4] SDV: ZwRegistryOpen rule violations

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

On 07/02/2022 13:15, Owen Smith wrote:
> Dont hold the ParametersKey open, SDV treats this as a mismatched
> ZwRegistryOpen and ZwClose pair.
> Open the registry key when required, and close it once its no longer
> required.
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
Shame to have to jump through such hoops but...

Reviewed-by: Paul Durrant <paul@xxxxxxx>

> ---
>   src/xenfilt/driver.c | 103 +++++++++++++++++++++----------------------
>   1 file changed, 50 insertions(+), 53 deletions(-)
>
> diff --git a/src/xenfilt/driver.c b/src/xenfilt/driver.c
> index f131282..ce9e0cd 100644
> --- a/src/xenfilt/driver.c
> +++ b/src/xenfilt/driver.c
> @@ -47,7 +47,6 @@
>   
>   typedef struct _XENFILT_DRIVER {
>       PDRIVER_OBJECT              DriverObject;
> -    HANDLE                      ParametersKey;
>   
>       MUTEX                       Mutex;
>       LIST_ENTRY                  List;
> @@ -113,28 +112,39 @@ DriverGetDriverObject(
>       return __DriverGetDriverObject();
>   }
>   
> -static FORCEINLINE VOID
> -__DriverSetParametersKey(
> -    IN  HANDLE  Key
> +static FORCEINLINE NTSTATUS
> +__DriverOpenParametersKey(
> +    OUT PHANDLE     ParametersKey
>       )
>   {
> -    Driver.ParametersKey = Key;
> -}
> +    HANDLE          ServiceKey;
> +    NTSTATUS        status;
>   
> -static FORCEINLINE HANDLE
> -__DriverGetParametersKey(
> -    VOID
> -    )
> -{
> -    return Driver.ParametersKey;
> +    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;
>   }
>   
> -HANDLE
> -DriverGetParametersKey(
> -    VOID
> +NTSTATUS
> +DriverOpenParametersKey(
> +    OUT PHANDLE     ParametersKey
>       )
>   {
> -    return __DriverGetParametersKey();
> +    return __DriverOpenParametersKey(ParametersKey);
>   }
>   
>   static FORCEINLINE VOID
> @@ -244,7 +254,9 @@ __DriverGetActive(
>   
>       ASSERT3U(KeGetCurrentIrql(), ==, PASSIVE_LEVEL);
>   
> -    ParametersKey = __DriverGetParametersKey();
> +    status = __DriverOpenParametersKey(&ParametersKey);
> +    if (!NT_SUCCESS(status))
> +        goto fail1;
>   
>       status = RtlStringCbPrintfA(Name, MAXNAMELEN, "Active%s", Key);
>       ASSERT(NT_SUCCESS(status));
> @@ -254,14 +266,14 @@ __DriverGetActive(
>                                     NULL,
>                                     &Ansi);
>       if (!NT_SUCCESS(status))
> -        goto fail1;
> +        goto fail2;
>   
>       Length = Ansi[0].Length + sizeof (CHAR);
>       *Value = __AllocatePoolWithTag(NonPagedPool, Length, 'TLIF');
>   
>       status = STATUS_NO_MEMORY;
>       if (*Value == NULL)
> -        goto fail2;
> +        goto fail3;
>   
>       status = RtlStringCbPrintfA(*Value,
>                                   Length,
> @@ -271,13 +283,20 @@ __DriverGetActive(
>   
>       RegistryFreeSzValue(Ansi);
>   
> +    RegistryCloseKey(ParametersKey);
> +
>       Trace("<====\n");
>   
>       return STATUS_SUCCESS;
>   
> +fail3:
> +    Error("fail3\n");
> +
>   fail2:
>       Error("fail2\n");
>   
> +    RegistryCloseKey(ParametersKey);
> +
>   fail1:
>       if (status != STATUS_OBJECT_NAME_NOT_FOUND)
>           Error("fail1 (%08x)\n", status);
> @@ -409,8 +428,6 @@ DriverUnload(
>       IN  PDRIVER_OBJECT  DriverObject
>       )
>   {
> -    HANDLE              ParametersKey;
> -
>       ASSERT3P(DriverObject, ==, __DriverGetDriverObject());
>   
>       Trace("====>\n");
> @@ -428,10 +445,6 @@ DriverUnload(
>       EmulatedTeardown(Driver.EmulatedContext);
>       Driver.EmulatedContext = NULL;
>   
> -    ParametersKey = __DriverGetParametersKey();
> -    __DriverSetParametersKey(NULL);
> -    RegistryCloseKey(ParametersKey);
> -
>       RegistryTeardown();
>   
>       Info("XENFILT %d.%d.%d (%d) (%02d.%02d.%04d)\n",
> @@ -744,8 +757,11 @@ DriverGetEmulatedType(
>       HANDLE                          ParametersKey;
>       XENFILT_EMULATED_OBJECT_TYPE    Type;
>       ULONG                           Index;
> +    NTSTATUS                        status;
>   
> -    ParametersKey = __DriverGetParametersKey();
> +    status = __DriverOpenParametersKey(&ParametersKey);
> +    if (!NT_SUCCESS(status))
> +        goto fail1;
>   
>       Type = XENFILT_EMULATED_OBJECT_TYPE_UNKNOWN;
>       Index = 0;
> @@ -753,7 +769,6 @@ DriverGetEmulatedType(
>       do {
>           ULONG           Length;
>           PANSI_STRING    Ansi;
> -        NTSTATUS        status;
>   
>           Length = (ULONG)strlen(&Id[Index]);
>           if (Length == 0)
> @@ -779,7 +794,14 @@ DriverGetEmulatedType(
>           Index += Length + 1;
>       } while (Type == XENFILT_EMULATED_OBJECT_TYPE_UNKNOWN);
>   
> +    RegistryCloseKey(ParametersKey);
> +
>       return Type;
> +
> +fail1:
> +    Error("fail1 %08x\n", status);
> +
> +    return XENFILT_EMULATED_OBJECT_TYPE_UNKNOWN;
>   }
>   
>   DRIVER_ADD_DEVICE   DriverAddDevice;
> @@ -886,8 +908,6 @@ DriverEntry(
>       IN  PUNICODE_STRING         RegistryPath
>       )
>   {
> -    HANDLE                      ServiceKey;
> -    HANDLE                      ParametersKey;
>       PXENFILT_EMULATED_CONTEXT   EmulatedContext;
>       ULONG                       Index;
>       NTSTATUS                    status;
> @@ -926,19 +946,9 @@ DriverEntry(
>       if (!NT_SUCCESS(status))
>           goto fail1;
>   
> -    status = RegistryOpenServiceKey(KEY_READ, &ServiceKey);
> -    if (!NT_SUCCESS(status))
> -        goto fail2;
> -
> -    status = RegistryOpenSubKey(ServiceKey, "Parameters", KEY_READ, 
> &ParametersKey);
> -    if (!NT_SUCCESS(status))
> -        goto fail3;
> -
> -    __DriverSetParametersKey(ParametersKey);
> -
>       status = EmulatedInitialize(&EmulatedContext);
>       if (!NT_SUCCESS(status))
> -        goto fail4;
> +        goto fail2;
>   
>       __DriverSetEmulatedContext(EmulatedContext);
>   
> @@ -948,8 +958,6 @@ DriverEntry(
>                                     sizeof (Driver.EmulatedInterface));
>       ASSERT(NT_SUCCESS(status));
>   
> -    RegistryCloseKey(ServiceKey);
> -
>       DriverObject->DriverExtension->AddDevice = DriverAddDevice;
>   
>       for (Index = 0; Index <= IRP_MJ_MAXIMUM_FUNCTION; Index++) {
> @@ -966,17 +974,6 @@ done:
>       Trace("<====\n");
>       return STATUS_SUCCESS;
>   
> -fail4:
> -    Error("fail4\n");
> -
> -    __DriverSetParametersKey(NULL);
> -    RegistryCloseKey(ParametersKey);
> -
> -fail3:
> -    Error("fail3\n");
> -
> -    RegistryCloseKey(ServiceKey);
> -
>   fail2:
>       Error("fail2\n");
>