[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] More IOCTL parameter checks

  • To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
  • From: "Durrant, Paul" <xadimgnik@xxxxxxxxx>
  • Date: Thu, 28 Apr 2022 11:44:21 +0100
  • Delivery-date: Thu, 28 Apr 2022 10:44:29 +0000
  • List-id: Developer list for the Windows PV Drivers subproject <win-pv-devel.lists.xenproject.org>
On 25/04/2022 13:14, Owen Smith wrote:

Also expands ASSERT(NT_SUCCESS(function)) calls in cleanup paths

Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
---
  src/xeniface/ioctl_evtchn.c     | 15 ++++++++++-----
  src/xeniface/ioctl_gnttab.c     | 26 ++++++++++++++++----------
  src/xeniface/ioctl_sharedinfo.c |  5 +++++
  src/xeniface/ioctl_store.c      | 17 ++++++++++-------
  src/xeniface/ioctl_suspend.c    | 11 +++++++++--
  src/xeniface/ioctls.c           |  4 ++--
  6 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/src/xeniface/ioctl_evtchn.c b/src/xeniface/ioctl_evtchn.c
index f528485..90e9468 100644
--- a/src/xeniface/ioctl_evtchn.c
+++ b/src/xeniface/ioctl_evtchn.c
@@ -166,7 +166,8 @@ IoctlEvtchnBindUnbound(
status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_EVTCHN_BIND_UNBOUND_IN) ||
-        OutLen != sizeof(XENIFACE_EVTCHN_BIND_UNBOUND_OUT)) {
+        OutLen != sizeof(XENIFACE_EVTCHN_BIND_UNBOUND_OUT) ||
+        Buffer == NULL) {
          goto fail1;
      }
@@ -259,7 +260,8 @@ IoctlEvtchnBindInterdomain( status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_EVTCHN_BIND_INTERDOMAIN_IN) ||
-        OutLen != sizeof(XENIFACE_EVTCHN_BIND_INTERDOMAIN_OUT)) {
+        OutLen != sizeof(XENIFACE_EVTCHN_BIND_INTERDOMAIN_OUT) ||
+        Buffer == NULL) {
          goto fail1;
      }
@@ -353,7 +355,8 @@ IoctlEvtchnClose( status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_EVTCHN_CLOSE_IN) ||
-        OutLen != 0) {
+        OutLen != 0 ||
+        Buffer == NULL) {
          goto fail1;
      }
@@ -430,7 +433,8 @@ IoctlEvtchnNotify( status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_EVTCHN_NOTIFY_IN) ||
-        OutLen != 0) {
+        OutLen != 0 ||
+        Buffer == NULL) {
          goto fail1;
      }
@@ -462,7 +466,8 @@ IoctlEvtchnUnmask( status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_EVTCHN_UNMASK_IN) ||
-        OutLen != 0) {
+        OutLen != 0 ||
+        Buffer == NULL) {
          goto fail1;
      }
diff --git a/src/xeniface/ioctl_gnttab.c b/src/xeniface/ioctl_gnttab.c 
index c3cf129..5777ac8 100644
--- a/src/xeniface/ioctl_gnttab.c
+++ b/src/xeniface/ioctl_gnttab.c
@@ -174,7 +174,8 @@ IoctlGnttabPermitForeignAccess(
      }
status = STATUS_INVALID_BUFFER_SIZE; 
-    if (OutLen != (ULONG)FIELD_OFFSET(XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_OUT, 
References[In->NumberPages]))
+    if (OutLen != (ULONG)FIELD_OFFSET(XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_OUT, 
References[In->NumberPages]) ||
+        Out == NULL)
          goto fail5;
status = STATUS_NO_MEMORY; 
@@ -310,11 +311,12 @@ fail11:
      Error("Fail11: Page = %lu\n", Page);
while (Page > 0) { 
-        ASSERT(NT_SUCCESS(XENBUS_GNTTAB(RevokeForeignAccess,
+        NTSTATUS status2 = XENBUS_GNTTAB(RevokeForeignAccess,
                                          &Fdo->GnttabInterface,
                                          Fdo->GnttabCache,
                                          FALSE,
-                                        Context->Grants[Page - 1])));
+                                        Context->Grants[Page - 1]);
+        ASSERT(NT_SUCCESS(status2));
--Page; 
      }
@@ -425,7 +427,7 @@ IoctlGnttabRevokeForeignAccess(
      PXENIFACE_CONTEXT_ID ContextId;
status = STATUS_INVALID_BUFFER_SIZE; 
-    if (InLen != sizeof(XENIFACE_GNTTAB_REVOKE_FOREIGN_ACCESS_IN))
+    if (InLen != sizeof(XENIFACE_GNTTAB_REVOKE_FOREIGN_ACCESS_IN) || Buffer == 
NULL)
          goto fail1;
Id.Type = XENIFACE_CONTEXT_GRANT; 
@@ -476,7 +478,8 @@ IoctlGnttabMapForeignPages(
status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen < sizeof(XENIFACE_GNTTAB_MAP_FOREIGN_PAGES_IN) ||
-        OutLen != sizeof(XENIFACE_GNTTAB_MAP_FOREIGN_PAGES_OUT)) {
+        OutLen != sizeof(XENIFACE_GNTTAB_MAP_FOREIGN_PAGES_OUT) ||
+        Buffer == NULL) {
          goto fail1;
      }
@@ -618,10 +621,12 @@ fail10: fail9: 
      Error("Fail9\n");
-    ASSERT(NT_SUCCESS(XENBUS_GNTTAB(UnmapForeignPages,
-                                    &Fdo->GnttabInterface,
-                                    Context->Address
-                                    )));
+    {
+        NTSTATUS status2 = XENBUS_GNTTAB(UnmapForeignPages,
+                                         &Fdo->GnttabInterface,
+                                         Context->Address);
+        ASSERT(NT_SUCCESS(status2));


Why?


+    }
fail8: 
      Error("Fail8\n");
@@ -714,7 +719,8 @@ IoctlGnttabUnmapForeignPages(
status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_GNTTAB_UNMAP_FOREIGN_PAGES_IN) ||
-        OutLen != 0) {
+        OutLen != 0 ||
+        Buffer == NULL) {
          goto fail1;
      }
diff --git a/src/xeniface/ioctl_sharedinfo.c b/src/xeniface/ioctl_sharedinfo.c 
index 7870732..eab35a5 100644
--- a/src/xeniface/ioctl_sharedinfo.c
+++ b/src/xeniface/ioctl_sharedinfo.c
@@ -56,6 +56,9 @@ IoctlSharedInfoGetTime(
      if (OutLen != sizeof(XENIFACE_SHAREDINFO_GET_TIME_OUT))
          goto fail2;
+ if (Buffer == NULL) 
+        goto fail3;
+
      Out = (PXENIFACE_SHAREDINFO_GET_TIME_OUT)Buffer;
      XENBUS_SHARED_INFO(GetTime, &Fdo->SharedInfoInterface, &Time,
                         &Local);
@@ -66,6 +69,8 @@ IoctlSharedInfoGetTime(
return STATUS_SUCCESS; +fail3: 
+    Error("fail3\n");
  fail2:
      Error("Fail2\n");
  fail1:
diff --git a/src/xeniface/ioctl_store.c b/src/xeniface/ioctl_store.c
index 97062df..3964662 100644
--- a/src/xeniface/ioctl_store.c
+++ b/src/xeniface/ioctl_store.c
@@ -105,7 +105,7 @@ IoctlStoreRead(
      BOOLEAN     SquashError = FALSE;
status = STATUS_INVALID_BUFFER_SIZE; 
-    if (InLen == 0)
+    if (InLen == 0 || Buffer == NULL)
          goto fail1;
status = STATUS_INVALID_PARAMETER; 
@@ -173,7 +173,7 @@ IoctlStoreWrite(
      ULONG       Length;
status = STATUS_INVALID_BUFFER_SIZE; 
-    if (InLen == 0 || OutLen != 0)
+    if (InLen == 0 || OutLen != 0 || Buffer == NULL)
          goto fail1;
status = STATUS_INVALID_PARAMETER; 
@@ -221,7 +221,7 @@ IoctlStoreDirectory(
      BOOLEAN     SquashError = FALSE;
status = STATUS_INVALID_BUFFER_SIZE; 
-    if (InLen == 0)
+    if (InLen == 0 || Buffer == NULL)
          goto fail1;
status = STATUS_INVALID_PARAMETER; 
@@ -289,7 +289,7 @@ IoctlStoreRemove(
      NTSTATUS    status;
status = STATUS_INVALID_BUFFER_SIZE; 
-    if (InLen == 0 || OutLen != 0)
+    if (InLen == 0 || OutLen != 0 || Buffer == NULL)
          goto fail1;
status = STATUS_INVALID_PARAMETER; 
@@ -392,7 +392,8 @@ IoctlStoreSetPermissions(
status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen < sizeof(XENIFACE_STORE_SET_PERMISSIONS_IN) ||
-        OutLen != 0) {
+        OutLen != 0 ||
+        Buffer == NULL) {
          goto fail1;
      }
@@ -508,7 +509,8 @@ IoctlStoreAddWatch( status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_STORE_ADD_WATCH_IN) ||
-        OutLen != sizeof(XENIFACE_STORE_ADD_WATCH_OUT)) {
+        OutLen != sizeof(XENIFACE_STORE_ADD_WATCH_OUT) ||
+        Buffer == NULL) {
          goto fail1;
      }
@@ -648,7 +650,8 @@ IoctlStoreRemoveWatch( status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_STORE_REMOVE_WATCH_IN) ||
-        OutLen != 0) {
+        OutLen != 0 ||
+        Buffer == NULL) {
          goto fail1;
      }
diff --git a/src/xeniface/ioctl_suspend.c b/src/xeniface/ioctl_suspend.c 
index 6289a94..e7e8437 100644
--- a/src/xeniface/ioctl_suspend.c
+++ b/src/xeniface/ioctl_suspend.c
@@ -55,6 +55,9 @@ IoctlSuspendGetCount(
      if (OutLen != sizeof(ULONG))
          goto fail2;
+ if (Buffer == NULL) 
+        goto fail3;
+
      Value = (PULONG)Buffer;
      *Value = XENBUS_SUSPEND(GetCount, &Fdo->SuspendInterface);
      *Info = (ULONG_PTR)sizeof(ULONG);
@@ -62,6 +65,8 @@ IoctlSuspendGetCount(
return status; +fail3: 
+    Error("Fail3\n");
  fail2:
      Error("Fail2\n");
  fail1:
@@ -87,7 +92,8 @@ IoctlSuspendRegister(
status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_SUSPEND_REGISTER_IN) ||
-        OutLen != sizeof(XENIFACE_SUSPEND_REGISTER_OUT)) {
+        OutLen != sizeof(XENIFACE_SUSPEND_REGISTER_OUT) ||
+        Buffer == NULL) {
          goto fail1;
      }
@@ -163,7 +169,8 @@ IoctlSuspendDeregister( status = STATUS_INVALID_BUFFER_SIZE; 
      if (InLen != sizeof(XENIFACE_SUSPEND_REGISTER_OUT) ||
-        OutLen != 0) {
+        OutLen != 0 ||
+        Buffer == NULL) {
          goto fail1;
      }
diff --git a/src/xeniface/ioctls.c b/src/xeniface/ioctls.c 
index a624bd1..20a7669 100644
--- a/src/xeniface/ioctls.c
+++ b/src/xeniface/ioctls.c
@@ -48,9 +48,9 @@ __CaptureUserBuffer(
      NTSTATUS Status;
      PVOID TempBuffer = NULL;
- if (Length == 0) { 
+    if (Length == 0 || Buffer == NULL) {
          *CapturedBuffer = NULL;
-        return STATUS_SUCCESS;
+        return STATUS_INVALID_PARAMETER;


That's a semantic change. Do we want that?

  Paul


      }
Status = STATUS_NO_MEMORY;

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

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