[PATCH] xeniface: Bound NumberPermissions before checking the input length

[Césaire Mounah <cesaire.mounah@xxxxxxxxxx>]

IoctlStoreSetPermissions() compares InLen with
FIELD_OFFSET(XENIFACE_STORE_SET_PERMISSIONS_IN, Permissions[N]), whose
64-bit result is truncated to ULONG. The count was only capped to 255
later, in __ConvertPermissions(), so the length check alone could be
satisfied by a huge count.

Check the cap first, with the same STATUS_INVALID_PARAMETER as before.

The check in __ConvertPermissions() is removed and fails are updated
accordingly.

Signed-off-by: Césaire Mounah <cesaire.mounah@xxxxxxxxxx>
---
 src/xeniface/ioctl_store.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/xeniface/ioctl_store.c b/src/xeniface/ioctl_store.c
index 97bef75..b38b07c 100644
--- a/src/xeniface/ioctl_store.c
+++ b/src/xeniface/ioctl_store.c
@@ -343,12 +343,9 @@ __ConvertPermissions(
     PXENBUS_STORE_PERMISSION        XenbusPermissions;
     ULONG                           Index;
 
-    if (NumberPermissions > 255)
-        goto fail1;
-
     XenbusPermissions = __AllocatePoolWithTag(NonPagedPool, NumberPermissions 
* sizeof(XENBUS_STORE_PERMISSION), XENIFACE_POOL_TAG);
     if (XenbusPermissions == NULL)
-        goto fail2;
+        goto fail1;
 
 #pragma warning(push)
 #pragma warning(disable:6385)
@@ -360,7 +357,7 @@ __ConvertPermissions(
         XENIFACE_STORE_PERMISSION_MASK  Mask = XenifacePermissions[Index].Mask;
 
         if (Mask & ~XENIFACE_STORE_ALLOWED_PERMISSIONS)
-            goto fail3;
+            goto fail2;
 
         XenbusPermissions[Index].Domain = XenifacePermissions[Index].Domain;
         XenbusPermissions[Index].Mask = 0;
@@ -375,12 +372,9 @@ __ConvertPermissions(
 
     return XenbusPermissions;
 
-fail3:
-    Error("Fail3\n");
-    __FreePoolWithTag(XenbusPermissions, XENIFACE_POOL_TAG);
-
 fail2:
     Error("Fail2\n");
+    __FreePoolWithTag(XenbusPermissions, XENIFACE_POOL_TAG);
 
 fail1:
     Error("Fail1\n");
@@ -417,27 +411,32 @@ IoctlStoreSetPermissions(
         goto fail1;
     }
 
-    if (InLen != (ULONG)FIELD_OFFSET(XENIFACE_STORE_SET_PERMISSIONS_IN, 
Permissions[In->NumberPermissions]))
+    status = STATUS_INVALID_PARAMETER;
+    if (In->NumberPermissions > 255)
         goto fail2;
 
+    status = STATUS_INVALID_BUFFER_SIZE;
+    if (InLen != (ULONG)FIELD_OFFSET(XENIFACE_STORE_SET_PERMISSIONS_IN, 
Permissions[In->NumberPermissions]))
+        goto fail3;
+
     status = STATUS_INVALID_PARAMETER;
     if (In->PathLength == 0 ||
         In->PathLength > XENSTORE_ABS_PATH_MAX) {
-        goto fail3;
+        goto fail4;
     }
 
     Permissions = __ConvertPermissions(In->NumberPermissions, In->Permissions);
     if (Permissions == NULL)
-        goto fail4;
+        goto fail5;
 
     status = __CaptureUserBuffer(In->Path, In->PathLength, &Path);
     if (!NT_SUCCESS(status))
-        goto fail5;
+        goto fail6;
 
     Path[In->PathLength - 1] = 0;
     status = STATUS_INVALID_PARAMETER;
     if (!__IsValidPath(Path, In->PathLength))
-        goto fail6;
+        goto fail7;
 
     Trace("> Path '%s', NumberPermissions %lu\n", Path, In->NumberPermissions);
 
@@ -457,22 +456,25 @@ IoctlStoreSetPermissions(
                           In->NumberPermissions);
 
     if (!NT_SUCCESS(status))
-        goto fail7;
+        goto fail8;
 
     __FreePermissions(Permissions);
     __FreeCapturedBuffer(Path);
     return status;
 
+fail8:
+    Error("Fail8\n");
+
 fail7:
     Error("Fail7\n");
+    __FreeCapturedBuffer(Path);
 
 fail6:
     Error("Fail6\n");
-    __FreeCapturedBuffer(Path);
+    __FreePermissions(Permissions);
 
 fail5:
     Error("Fail5\n");
-    __FreePermissions(Permissions);
 
 fail4:
     Error("Fail4\n");
-- 
2.55.0.windows.5



--
Césaire Mounah | Vates Windows Guest Tools Engineer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech