Changeset 81300 in vbox for trunk/src/VBox/Devices/VirtIO

Timestamp:

Oct 17, 2019 5:51:54 AM (5 years ago)

Author:

vboxsync

Message:

Storage/DevVirtioSCSI.cpp: Improved bounds checking for MMIO functions, and tried to address Guru meditation reported by aichner in comment #9440 Comment #103 and the issues in Comment #110. See Comment #112

Location:

trunk/src/VBox/Devices/VirtIO

Files:

• Virtio_1_0.cpp (modified) (19 diffs)
• Virtio_1_0.h (modified) (1 diff)

trunk/src/VBox/Devices/VirtIO/Virtio_1_0.cpp

@@ -188 +188 @@
     } while (desc.fFlags & VIRTQ_DESC_F_NEXT);
 
-
     PRTSGBUF pSgPhysIn = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF));
     AssertReturn(pSgPhysIn, VERR_NO_MEMORY);
@@ -194 +193 @@
     RTSgBufInit(pSgPhysIn, (PCRTSGSEG)paSegsIn, cSegsIn);
 
-    void *pVirtOut = RTMemAlloc(cbOut);
-    AssertReturn(pVirtOut, VERR_NO_MEMORY);
-
-    /* If there's any guest → device data in phys. memory pulled
-     * from queue, copy it into virtual memory to return to caller */
-
-    if (cSegsOut)
-    {
-        uint8_t *outSgVirt = (uint8_t *)pVirtOut;
-        RTSGBUF outSgPhys;
-        RTSgBufInit(&outSgPhys, (PCRTSGSEG)paSegsOut, cSegsOut);
-        for (size_t cb = cbOut; cb;)
-        {
-            size_t cbSeg = cb;
-            RTGCPHYS GCPhys = (RTGCPHYS)RTSgBufGetNextSegment(&outSgPhys, &cbSeg);
-            PDMDevHlpPhysRead(((PVIRTIOSTATE)hVirtio)->CTX_SUFF(pDevIns), GCPhys, outSgVirt, cbSeg);
-            outSgVirt = ((uint8_t *)outSgVirt) + cbSeg;
-            cb -= cbSeg;
-        }
-        RTMemFree(paSegsOut);
-    }
+    PRTSGBUF pSgPhysOut = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF));
+    AssertReturn(pSgPhysOut, VERR_NO_MEMORY);
+
+    RTSgBufInit(pSgPhysOut, (PCRTSGSEG)paSegsOut, cSegsOut);
 
     PVIRTIO_DESC_CHAIN_T pDescChain = (PVIRTIO_DESC_CHAIN_T)RTMemAllocZ(sizeof(VIRTIO_DESC_CHAIN_T));
@@ -220 +202 @@
 
     pDescChain->uHeadIdx   = uHeadIdx;
-    pDescChain->cbVirtSrc  = cbOut;
-    pDescChain->pVirtSrc   = pVirtOut;
-    pDescChain->cbPhysDst  = cbIn;
-    pDescChain->pSgPhysDst = pSgPhysIn;
+    pDescChain->cbPhysSend  = cbOut;
+    pDescChain->pSgPhysSend = pSgPhysOut;
+    pDescChain->cbPhysReturn  = cbIn;
+    pDescChain->pSgPhysReturn = pSgPhysIn;
     *ppDescChain = pDescChain;
 
@@ -240 +222 @@
     PVIRTIOSTATE pVirtio = (PVIRTIOSTATE)hVirtio;
     PVIRTQSTATE  pVirtq = &pVirtio->virtqState[qIdx];
-    PRTSGBUF pSgPhysReturn = pDescChain->pSgPhysDst;
+    PRTSGBUF pSgPhysReturn = pDescChain->pSgPhysReturn;
+
 
     AssertMsgReturn(DRIVER_OK(pVirtio) /*&& pVirtio->uQueueEnable[qIdx]*/,
@@ -251 +234 @@
 
     /*
-     * Copy virtual memory s/g buffer containing data to return to the guest
-     * to phys. memory described by (IN direction ) s/g buffer of the descriptor chain
-     * (pulled from avail ring of queue), essentially giving result back to the guest driver.
-     */
+     * Copy s/g buf (virtual memory) to guest phys mem (IN direction). This virtual memory
+     * block will be small (fixed portion of response header + sense buffer area or
+     * control commands or error return values)... The bulk of req data xfers to phys mem
+     * is handled by client */
+
     size_t cbCopy = 0;
     size_t cbRemain = RTSgBufCalcTotalLength(pSgVirtReturn);
+    RTSgBufReset(pSgPhysReturn); /* Reset ptr because req data may have already been written */
     while (cbRemain)
     {
@@ -272 +257 @@
 
     if (fFence)
-        RT_UNTRUSTED_NONVOLATILE_COPY_FENCE();
+        RT_UNTRUSTED_NONVOLATILE_COPY_FENCE(); /* needed? */
 
     /** If this write-ahead crosses threshold where the driver wants to get an event flag it */
@@ -287 +272 @@
             (uint32_t)(cbCopy & 0xffffffff));
 
-    Log2Func((".... Copied %ul bytes to %ul byte buffer, residual=%ul\n",
-         cbCopy, pDescChain->cbPhysDst, pDescChain->cbPhysDst - cbCopy));
+    Log2Func((".... Copied %lu bytes to %lu byte buffer, residual=%lu\n",
+         cbCopy, pDescChain->cbPhysReturn, pDescChain->cbPhysReturn - cbCopy));
 
     Log6Func(("Write ahead used_idx=%d, %s used_idx=%d\n",
@@ -561 +546 @@
     {
         if (fWrite) /* Guest WRITE pCommonCfg>uDeviceFeatures */
+        {
             LogFunc(("Guest attempted to write readonly virtio_pci_common_cfg.device_feature\n"));
+            return VINF_SUCCESS;
+        }
         else /* Guest READ pCommonCfg->uDeviceFeatures */
         {
@@ -581 +569 @@
                     LogFunc(("Guest read uDeviceFeatures with out of range selector (%d), returning 0\n",
                           pVirtio->uDeviceFeaturesSelect));
-                    return VERR_ACCESS_DENIED;
+                    return VINF_IOM_MMIO_UNUSED_00;
             }
         }
@@ -604 +592 @@
                     LogFunc(("Guest wrote uDriverFeatures with out of range selector (%d), returning 0\n",
                          pVirtio->uDriverFeaturesSelect));
-                    return VERR_ACCESS_DENIED;
+                    return VINF_SUCCESS;
             }
         }
@@ -626 +614 @@
                     LogFunc(("Guest read uDriverFeatures with out of range selector (%d), returning 0\n",
                          pVirtio->uDriverFeaturesSelect));
-                    return VERR_ACCESS_DENIED;
+                    return VINF_IOM_MMIO_UNUSED_00;
             }
         }
@@ -635 +623 @@
         {
             Log2Func(("Guest attempted to write readonly virtio_pci_common_cfg.num_queues\n"));
-            return VERR_ACCESS_DENIED;
+            return VINF_SUCCESS;
         }
         else
@@ -712 +700 @@
         Log2Func(("Bad guest %s access to virtio_pci_common_cfg: uOffset=%d, cb=%d\n",
             fWrite ? "write" : "read ", uOffset, cb));
-        rc = VERR_ACCESS_DENIED;
+        return fWrite ? VINF_SUCCESS : VINF_IOM_MMIO_UNUSED_00;
     }
     return rc;
@@ -770 +758 @@
     {
         uint32_t uOffset = GCPhysAddr - pVirtio->pGcPhysCommonCfg;
-        virtioCommonCfgAccessed(pVirtio, false /* fWrite */, uOffset, cb, (void const *)pv);
+        rc = virtioCommonCfgAccessed(pVirtio, false /* fWrite */, uOffset, cb, (void const *)pv);
     }
     else
@@ -784 +772 @@
                 "                  pVirtio=%#p GCPhysAddr=%RGp cb=%u\n",
                 pVirtio, GCPhysAddr, cb));
+        return VINF_IOM_MMIO_UNUSED_00;
     }
     return rc;
@@ -802 +791 @@
     RT_NOREF(pvUser);
     PVIRTIOSTATE pVirtio = *PDMINS_2_DATA(pDevIns, PVIRTIOSTATE *);
-    int rc = VINF_SUCCESS;
 
     MATCH_VIRTIO_CAP_STRUCT(pVirtio->pGcPhysDeviceCap, pVirtio->pDeviceCap,     fDevSpecific);
@@ -815 +803 @@
          * Pass this MMIO write access back to the client to handle
          */
-        rc = pVirtio->virtioCallbacks.pfnVirtioDevCapWrite(pDevIns, uOffset, pv, cb);
+        (void)pVirtio->virtioCallbacks.pfnVirtioDevCapWrite(pDevIns, uOffset, pv, cb);
     }
     else
@@ -821 +809 @@
     {
         uint32_t uOffset = GCPhysAddr - pVirtio->pGcPhysCommonCfg;
-        virtioCommonCfgAccessed(pVirtio, true /* fWrite */, uOffset, cb, pv);
+        (void)virtioCommonCfgAccessed(pVirtio, true /* fWrite */, uOffset, cb, pv);
     }
     else
@@ -847 +835 @@
                 pVirtio, GCPhysAddr, pv, cb, pv, cb));
     }
-    return rc;
+    return VINF_SUCCESS;
 }
 

trunk/src/VBox/Devices/VirtIO/Virtio_1_0.h

@@ -65 +65 @@
 typedef struct VIRTIO_DESC_CHAIN
 {
-    uint32_t  uHeadIdx;                                    /**< Head idx of associated desc chain        */
-    uint32_t  cbVirtSrc;                                   /**< Size of virt source buffer               */
-    void     *pVirtSrc;                                    /**< Virt mem buf holding out data from guest */
-    uint32_t  cbPhysDst;                                   /**< Total size of dst buffer                 */
-    PRTSGBUF  pSgPhysDst;                                  /**< Phys S/G buf to store result for guest   */
+    uint32_t  uHeadIdx;                                          /**< Head idx of associated desc chain        */
+    uint32_t  cbPhysSend;                                        /**< Total size of src buffer                 */
+    PRTSGBUF  pSgPhysSend;                                       /**< Phys S/G/ buf for data from guest        */
+    uint32_t  cbPhysReturn;                                      /**< Total size of dst buffer                 */
+    PRTSGBUF  pSgPhysReturn;                                     /**< Phys S/G buf to store result for guest   */
 } VIRTIO_DESC_CHAIN_T, *PVIRTIO_DESC_CHAIN_T, **PPVIRTIO_DESC_CHAIN_T;