Re: [PATCH 2/2] Fix CodeAnalysis issues

[Owen Smith <owen.smith@xxxxxxxxx>]

I think there are bigger issues with the V1 -> V2 conversion code here... I may look at refactoring this IOCTL handler to improve the buffer accesses

Owen

On Mon, Sep 8, 2025 at 9:58 AM Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx> wrote:

On 08/09/2025 09:46, Owen Smith wrote:
> * RegistryInitialize is incorrectly detected as DRIVER_INITIALIZE
> * Add ASSERT to avoid detection of potential NULL pointer access
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
> ---
>   src/xeniface/ioctl_gnttab.c | 1 +
>   src/xeniface/registry.c     | 1 +
>   2 files changed, 2 insertions(+)
>
> diff --git a/src/xeniface/ioctl_gnttab.c b/src/xeniface/ioctl_gnttab.c
> index 026f29b..ef77295 100644
> --- a/src/xeniface/ioctl_gnttab.c
> +++ b/src/xeniface/ioctl_gnttab.c
> @@ -387,6 +387,7 @@ IoctlGnttabPermitForeignAccess(
>       In = NULL;
>   
>       if (ControlCode == IOCTL_XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS) {
> +        ASSERT3P(In1, !=, NULL);

I think the following would capture the intention better while also
fixing the warning:

diff --git a/src/xeniface/ioctl_gnttab.c b/src/xeniface/ioctl_gnttab.c
index 026f29b..a5a7d88 100644
--- a/src/xeniface/ioctl_gnttab.c
+++ b/src/xeniface/ioctl_gnttab.c
@@ -326,6 +326,9 @@ IoctlGnttabPermitForeignAccess(
      ULONG Page;
      ULONG ControlCode =
IoGetCurrentIrpStackLocation(Irp)->Parameters.DeviceIoControl.IoControlCode;

+    ASSERT(ControlCode == IOCTL_XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS ||
+                      ControlCode ==
IOCTL_XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_V2);
+
      Status = STATUS_INVALID_BUFFER_SIZE;
      if ((InLen != sizeof(XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_IN) &&
ControlCode == IOCTL_XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS)
          || (InLen !=
sizeof(XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_IN_V2) && ControlCode ==
IOCTL_XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_V2))
diff --git a/src/xeniface/ioctls.c b/src/xeniface/ioctls.c
index 076750a..dadf0f9 100644
--- a/src/xeniface/ioctls.c
+++ b/src/xeniface/ioctls.c
@@ -42,9 +42,12 @@

  NTSTATUS
  __CaptureUserBuffer(
-    __in  PVOID Buffer,
-    __in  ULONG Length,
-    __out PVOID *CapturedBuffer
+    _In_reads_bytes_(Length) PVOID  Buffer,
+    _In_ ULONG                      Length,
+    _Outptr_result_buffer_maybenull_(Length)
+    _When_(Length == 0, _At_(*CapturedBuffer, _Post_null_))
+    _When_(Length != 0, _At_(*CapturedBuffer, _Post_notnull_))
+    PVOID                           *CapturedBuffer
      )
  {
      NTSTATUS Status;
diff --git a/src/xeniface/ioctls.h b/src/xeniface/ioctls.h
index b4c8de2..26ef02b 100644
--- a/src/xeniface/ioctls.h
+++ b/src/xeniface/ioctls.h
@@ -91,9 +91,12 @@ typedef struct _XENIFACE_GNTTAB_CONTEXT {

  NTSTATUS
  __CaptureUserBuffer(
-    __in  PVOID Buffer,
-    __in  ULONG Length,
-    __out PVOID *CapturedBuffer
+    _In_reads_bytes_(Length) PVOID  Buffer,
+    _In_ ULONG                      Length,
+    _Outptr_result_buffer_maybenull_(Length)
+    _When_(Length == 0, _At_(*CapturedBuffer, _Post_null_))
+    _When_(Length != 0, _At_(*CapturedBuffer, _Post_notnull_))
+    PVOID                           *CapturedBuffer
      );

  VOID


>           Context->UseRequestId = TRUE;
>           Context->RequestId = In1->RequestId;
>           __FreeCapturedBuffer(In1);
> diff --git a/src/xeniface/registry.c b/src/xeniface/registry.c
> index 1ab85c9..423c4cc 100644
> --- a/src/xeniface/registry.c
> +++ b/src/xeniface/registry.c
> @@ -62,6 +62,7 @@ __RegistryFree(
>   }
>   
>   NTSTATUS
> +#pragma prefast(suppress:28101) // unannotated DriverEntry function
>   RegistryInitialize(
>       IN  PDRIVER_OBJECT  DriverObject,
>       IN  PUNICODE_STRING Path



--
Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech