Changeset 19398 in vbox

Timestamp:

May 5, 2009 9:05:11 PM (16 years ago)

Author:

vboxsync

Message:

GVMM: Address the GVMMR0GetVMByEMT issue and added access restrictions on gvmmR0ByVM (only same process).

File:

• trunk/src/VBox/VMM/VMMR0/GVMMR0.cpp (modified) (19 diffs)

trunk/src/VBox/VMM/VMMR0/GVMMR0.cpp

@@ -45 +45 @@
 #include <VBox/vm.h>
 #include <VBox/vmm.h>
+#include <VBox/param.h>
 #include <VBox/err.h>
 #include <iprt/alloc.h>
@@ -51 +52 @@
 #include <VBox/log.h>
 #include <iprt/thread.h>
+#include <iprt/process.h>
 #include <iprt/param.h>
 #include <iprt/string.h>
@@ -79 +81 @@
     /** The session this VM is associated with. */
     PSUPDRVSESSION      pSession;
-    /** The ring-0 handle of the EMT thread (VCPU 0).
-     * This is used for assertions and similar cases where we need to find the VM handle. */
-    RTNATIVETHREAD      hEMTCpu0;
+    /** The ring-0 handle of the EMT0 thread.
+     * This is used for ownership checks as well as looking up a VM handle by thread
+     * at times like assertions. */
+    RTNATIVETHREAD      hEMT0;
+    /** The process ID of the handle owner.
+     * This is used for access checks. */
+    RTPROCESS           ProcId;
 } GVMHANDLE;
 /** Pointer to a global VM handle. */
@@ -521 +527 @@
         return VERR_INVALID_PARAMETER;
 
-    RTNATIVETHREAD hEMT = RTThreadNativeSelf();
-    AssertReturn(hEMT != NIL_RTNATIVETHREAD, VERR_INTERNAL_ERROR);
+    RTNATIVETHREAD hEMT0 = RTThreadNativeSelf();
+    AssertReturn(hEMT0 != NIL_RTNATIVETHREAD, VERR_INTERNAL_ERROR);
+    RTNATIVETHREAD ProcId = RTProcSelf();
+    AssertReturn(ProcId != NIL_RTPROCESS, VERR_INTERNAL_ERROR);
 
     /*
@@ -561 +569 @@
                 pHandle->pGVM     = NULL;
                 pHandle->pSession = pSession;
-                pHandle->hEMTCpu0 = NIL_RTNATIVETHREAD;
+                pHandle->hEMT0    = NIL_RTNATIVETHREAD;
+                pHandle->ProcId   = NIL_RTPROCESS;
 
                 gvmmR0UsedUnlock(pGVMM);
@@ -641 +650 @@
                                         pHandle->pVM        = pVM;
                                         pHandle->pGVM       = pGVM;
-                                        pHandle->hEMTCpu0   = hEMT;
+                                        pHandle->hEMT0      = hEMT0;
+                                        pHandle->ProcId     = ProcId;
                                         pGVM->pVM           = pVM;
-                                        pGVM->aCpus[0].hEMT = hEMT;
+                                        pGVM->aCpus[0].hEMT = hEMT0;
 
                                         gvmmR0UsedUnlock(pGVMM);
@@ -839 +849 @@
     AssertReturn(pHandle->pVM == pVM, VERR_NOT_OWNER);
 
+    RTPROCESS      ProcId = RTProcSelf();
     RTNATIVETHREAD hSelf = RTThreadNativeSelf();
-    AssertReturn(pHandle->hEMTCpu0 == hSelf || pHandle->hEMTCpu0 == NIL_RTNATIVETHREAD, VERR_NOT_OWNER);
+    AssertReturn(   (   pHandle->hEMT0  == hSelf
+                     && pHandle->ProcId == ProcId)
+                 || pHandle->hEMT0 == NIL_RTNATIVETHREAD, VERR_NOT_OWNER);
 
     /*
@@ -852 +865 @@
     /* be careful here because we might theoretically be racing someone else cleaning up. */
     if (    pHandle->pVM == pVM
-        &&  (   pHandle->hEMTCpu0 == hSelf
-             || pHandle->hEMTCpu0 == NIL_RTNATIVETHREAD)
+        &&  (   (   pHandle->hEMT0  == hSelf
+                 && pHandle->ProcId == ProcId)
+             || pHandle->hEMT0 == NIL_RTNATIVETHREAD)
         &&  VALID_PTR(pHandle->pvObj)
         &&  VALID_PTR(pHandle->pSession)
@@ -867 +881 @@
     else
     {
-        SUPR0Printf("GVMMR0DestroyVM: pHandle=%p:{.pVM=%p, hEMTCpu0=%p, .pvObj=%p} pVM=%p hSelf=%p\n",
-                    pHandle, pHandle->pVM, pHandle->hEMTCpu0, pHandle->pvObj, pVM, hSelf);
+        SUPR0Printf("GVMMR0DestroyVM: pHandle=%p:{.pVM=%p, .hEMT0=%p, .ProcId=%u, .pvObj=%p} pVM=%p hSelf=%p\n",
+                    pHandle, pHandle->pVM, pHandle->hEMT0, pHandle->ProcId, pHandle->pvObj, pVM, hSelf);
         gvmmR0CreateDestroyUnlock(pGVMM);
         rc = VERR_INTERNAL_ERROR;
@@ -1049 +1063 @@
     ASMAtomicXchgPtr((void * volatile *)&pHandle->pvObj, NULL);
     ASMAtomicXchgPtr((void * volatile *)&pHandle->pSession, NULL);
-    ASMAtomicXchgSize(&pHandle->hEMTCpu0, NIL_RTNATIVETHREAD);
+    ASMAtomicXchgSize(&pHandle->hEMT0, NIL_RTNATIVETHREAD);
+    ASMAtomicXchgSize(&pHandle->ProcId, NIL_RTPROCESS);
 
     gvmmR0UsedUnlock(pGVMM);
@@ -1120 +1135 @@
 /**
  * Lookup a GVM structure by the shared VM structure.
+ *
+ * The calling thread must be in the same process as the VM. All current lookups
+ * are by threads inside the same process, so this will not be an issue.
  *
  * @returns VBox status code.
@@ -1133 +1151 @@
 static int gvmmR0ByVM(PVM pVM, PGVM *ppGVM, PGVMM *ppGVMM, bool fTakeUsedLock)
 {
+    RTPROCESS ProcId = RTProcSelf();
     PGVMM pGVMM;
     GVMM_GET_VALID_INSTANCE(pGVMM, VERR_INTERNAL_ERROR);
@@ -1163 +1182 @@
         pGVM = pHandle->pGVM;
         if (RT_UNLIKELY(    pHandle->pVM != pVM
+                        ||  pHandle->ProcId != ProcId
                         ||  !VALID_PTR(pHandle->pvObj)
                         ||  !VALID_PTR(pGVM)
@@ -1174 +1194 @@
     {
         if (RT_UNLIKELY(pHandle->pVM != pVM))
+            return VERR_INVALID_HANDLE;
+        if (RT_UNLIKELY(pHandle->ProcId != ProcId))
             return VERR_INVALID_HANDLE;
         if (RT_UNLIKELY(!VALID_PTR(pHandle->pvObj)))
@@ -1202 +1224 @@
 GVMMR0DECL(PGVM) GVMMR0ByVM(PVM pVM)
 {
+    PGVM pGVM;
     PGVMM pGVMM;
-    PGVM pGVM;
     int rc = gvmmR0ByVM(pVM, &pGVM, &pGVMM, false /* fTakeUsedLock */);
     if (RT_SUCCESS(rc))
@@ -1213 +1235 @@
 
 /**
- * Lookup a GVM structure by the shared VM structure
- * and ensuring that the caller is the EMT thread.
+ * Lookup a GVM structure by the shared VM structure and ensuring that the
+ * caller is an EMT thread.
  *
  * @returns VBox status code.
@@ -1245 +1267 @@
     PGVMHANDLE pHandle = &pGVMM->aHandles[hGVM];
     AssertReturn(pHandle->pVM == pVM, VERR_NOT_OWNER);
+    RTPROCESS ProcId = RTProcSelf();
+    AssertReturn(pHandle->ProcId == ProcId, VERR_NOT_OWNER);
     AssertPtrReturn(pHandle->pvObj, VERR_INTERNAL_ERROR);
 
@@ -1315 +1339 @@
     if (hEMT == NIL_RTNATIVETHREAD)
         hEMT = RTThreadNativeSelf();
+    RTPROCESS ProcId = RTProcSelf();
 
     /*
@@ -1322 +1347 @@
     {
         if (    pGVMM->aHandles[i].iSelf == i
+            &&  pGVMM->aHandles[i].ProcId == ProcId
             &&  VALID_PTR(pGVMM->aHandles[i].pvObj)
             &&  VALID_PTR(pGVMM->aHandles[i].pVM)
             &&  VALID_PTR(pGVMM->aHandles[i].pGVM))
         {
-            if (pGVMM->aHandles[i].hEMTCpu0 == hEMT)
+            if (pGVMM->aHandles[i].hEMT0 == hEMT)
                 return pGVMM->aHandles[i].pVM;
 
-            /** @todo this isn't safe as GVM may be deallocated while we're running.
-             * Will change this to use RTPROCESS on the handle level as storing all the
-             * thread handles there doesn't scale very well. */
+            /* This is fearly safe with the current process per VM approach. */
             PGVM pGVM = pGVMM->aHandles[i].pGVM;
-            for (VMCPUID idCpu = 0; idCpu < pGVM->cCpus; idCpu++)
+            VMCPUID const cCpus = pGVM->cCpus;
+            if (    cCpus < 1
+                ||  cCpus > VMM_MAX_CPUS)
+                continue;
+            for (VMCPUID idCpu = 1; idCpu < cCpus; idCpu++)
                 if (pGVM->aCpus[idCpu].hEMT == hEMT)
                     return pGVMM->aHandles[i].pVM;