[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/3] Fix issues raised by CodeQL


  • To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
  • From: Paul Durrant <xadimgnik@xxxxxxxxx>
  • Date: Fri, 20 Aug 2021 09:15:15 +0100
  • Delivery-date: Fri, 20 Aug 2021 08:15:21 +0000
  • List-id: Developer list for the Windows PV Drivers subproject <win-pv-devel.lists.xenproject.org>

On 10/08/2021 16:40, Owen Smith wrote:
- ExAllocatePoolWithTag is deprecated in Windows 10 2004 and replaced with
     ExAllocatePool2. Use ExAllocatePoolUninitialized to maintain support for
     earlier versions of Windows.
- strtol can fail, check for LONG_MIN or LONG_MAX as indicators of an error

I think these probably ought to be separate patches. As for the strtol() change... There also uses of strtoul() (and _strtoui64()) elsewhere. If we're going to catch overflow here then ought we not have similar checking there?

  Paul


Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
---
  src/common/util.h      |  4 ++++
  src/xenfilt/emulated.c | 34 ++++++++++++++++++++++++++--------
  2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/src/common/util.h b/src/common/util.h
index eddad4a..36a36dd 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -151,8 +151,12 @@ __AllocatePoolWithTag(
      __analysis_assume(PoolType == NonPagedPool ||
                        PoolType == PagedPool);
+#if (_MSC_VER >= 1928) // VS 16.9 (EWDK 20344 or later)
+    Buffer = ExAllocatePoolUninitialized(PoolType, NumberOfBytes, Tag);
+#else
  #pragma warning(suppress:28160) // annotation error
      Buffer = ExAllocatePoolWithTag(PoolType, NumberOfBytes, Tag);
+#endif
      if (Buffer == NULL)
          return NULL;
diff --git a/src/xenfilt/emulated.c b/src/xenfilt/emulated.c
index b7ae510..c0b666e 100644
--- a/src/xenfilt/emulated.c
+++ b/src/xenfilt/emulated.c
@@ -159,9 +159,9 @@ EmulatedSetObjectDiskData(
      )
  {
      PCHAR                               End;
-    ULONG                               Controller;
-    ULONG                               Target;
-    ULONG                               Lun;
+    LONG                                Controller;
+    LONG                                Target;
+    LONG                                Lun;
      NTSTATUS                            status;
UNREFERENCED_PARAMETER(DeviceID);
@@ -171,36 +171,54 @@ EmulatedSetObjectDiskData(
      if (Type != XENFILT_EMULATED_OBJECT_TYPE_IDE)
          goto fail1;
+ status = STATUS_INVALID_PARAMETER;
      Controller = strtol(InstanceID, &End, 10);
+    if (Controller == LONG_MIN || Controller == LONG_MAX)
+        goto fail2;
status = STATUS_INVALID_PARAMETER;
      if (*End != '.' || Controller > 1)
-        goto fail2;
+        goto fail3;
End++; + status = STATUS_INVALID_PARAMETER;
      Target = strtol(End, &End, 10);
+    if (Target == LONG_MIN || Target == LONG_MAX)
+        goto fail4;
status = STATUS_INVALID_PARAMETER;
      if (*End != '.' || Target > 1)
-        goto fail3;
+        goto fail5;
End++; + status = STATUS_INVALID_PARAMETER;
      Lun = strtol(End, &End, 10);
+    if (Lun == LONG_MIN || Lun == LONG_MAX)
+        goto fail6;
status = STATUS_INVALID_PARAMETER;
      if (*End != '\0')
-        goto fail4;
+        goto fail7;
status = STATUS_NOT_SUPPORTED;
      if (Lun != 0)
-        goto fail5;
+        goto fail8;
- EmulatedObject->Data.Disk.Index = Controller << 1 | Target;
+    EmulatedObject->Data.Disk.Index = (ULONG)Controller << 1 | (ULONG)Target;
return STATUS_SUCCESS; +fail8:
+    Error("fail8\n");
+
+fail7:
+    Error("fail7\n");
+
+fail6:
+    Error("fail6\n");
+
  fail5:
      Error("fail5\n");




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.