Xen project Mailing List

On Sat, Jun 29, 2024 at 4:40 PM Durrant, Paul <xadimgnik@xxxxxxxxx> wrote:

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;
>

Re: [RFC PATCH 1/5] Add RegistryOpenParametersKey