RE: [PATCH 3/4 v2] SDV: ZwRegistryOpen rule violations

[Owen Smith <owen.smith@xxxxxxxxxx>]

-----Original Message-----
From: Paul Durrant <paul@xxxxxxx> 
Sent: 16 February 2022 10:45
To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
Cc: Owen Smith <owen.smith@xxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>
Subject: [PATCH 3/4 v2] SDV: ZwRegistryOpen rule violations

[CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
unless you have verified the sender and know the content is safe.

From: Owen Smith <owen.smith@xxxxxxxxxx>

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>

Remove DriverGetParametersKey() from xenfilt/driver.h and don't add the 
implementation of DriverOpenParametersKey() in xenfilt/driver.c.

Signed-off-by: Paul Durrant <paul@xxxxxxx>



I completely missed that the ParametersKey was never used, I was just fixing 
SDV issues. Removing this unused code is good.

Acked-By: Owen Smith <owen.smith@xxxxxxxxxx>

---
 src/xenfilt/driver.c | 101 +++++++++++++++++++------------------------
 src/xenfilt/driver.h |   5 ---
 2 files changed, 45 insertions(+), 61 deletions(-)

diff --git a/src/xenfilt/driver.c b/src/xenfilt/driver.c index 
f131282fd795..8a8396e528b8 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,31 @@ 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;
 
-HANDLE
-DriverGetParametersKey(
-    VOID
-    )
-{
-    return __DriverGetParametersKey();
+    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
@@ -244,7 +246,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 +258,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 +275,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 +420,6 @@ DriverUnload(
     IN  PDRIVER_OBJECT  DriverObject
     )
 {
-    HANDLE              ParametersKey;
-
     ASSERT3P(DriverObject, ==, __DriverGetDriverObject());
 
     Trace("====>\n");
@@ -428,10 +437,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 +749,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 +761,6 @@ DriverGetEmulatedType(
     do {
         ULONG           Length;
         PANSI_STRING    Ansi;
-        NTSTATUS        status;
 
         Length = (ULONG)strlen(&Id[Index]);
         if (Length == 0)
@@ -779,7 +786,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 +900,6 @@ DriverEntry(
     IN  PUNICODE_STRING         RegistryPath
     )
 {
-    HANDLE                      ServiceKey;
-    HANDLE                      ParametersKey;
     PXENFILT_EMULATED_CONTEXT   EmulatedContext;
     ULONG                       Index;
     NTSTATUS                    status;
@@ -926,19 +938,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 +950,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 
+966,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");
 
diff --git a/src/xenfilt/driver.h b/src/xenfilt/driver.h index 
286580bed8a0..6a5665d58e45 100644
--- a/src/xenfilt/driver.h
+++ b/src/xenfilt/driver.h
@@ -37,11 +37,6 @@ DriverGetDriverObject(
     VOID
     );
 
-extern HANDLE
-DriverGetParametersKey(
-    VOID
-    );
-
 extern VOID
 DriverAcquireMutex(
     VOID
--
2.17.1