Changeset 55844 in vbox

Timestamp:

May 13, 2015 12:46:52 PM (10 years ago)

Author:

vboxsync

Message:

DevVGA: screen resize cleanup, logging

Location:

trunk/src/VBox/Devices/Graphics

Files:

• DevVGA.h (modified) (1 diff)
• DevVGA_VBVA.cpp (modified) (5 diffs)
• DevVGA_VDMA.cpp (modified) (2 diffs)

trunk/src/VBox/Devices/Graphics/DevVGA.h

@@ -661 +661 @@
 void VBVARaiseIrqNoWait(PVGASTATE pVGAState, uint32_t fFlags);
 
-int VBVAInfoView(PVGASTATE pVGAState, VBVAINFOVIEW *pView);
-int VBVAInfoScreen(PVGASTATE pVGAState, VBVAINFOSCREEN *pScreen);
+int VBVAInfoView(PVGASTATE pVGAState, const VBVAINFOVIEW *pView);
+int VBVAInfoScreen(PVGASTATE pVGAState, const VBVAINFOSCREEN *pScreen);
 int VBVAGetInfoViewAndScreen(PVGASTATE pVGAState, uint32_t u32ViewIndex, VBVAINFOVIEW *pView, VBVAINFOSCREEN *pScreen);
 

trunk/src/VBox/Devices/Graphics/DevVGA_VBVA.cpp

@@ -1991 +1991 @@
 }
 
-int VBVAInfoView(PVGASTATE pVGAState, VBVAINFOVIEW *pView)
-{
-    LogFlowFunc(("VBVA_INFO_VIEW: index %d, offset 0x%x, size 0x%x, max screen size 0x%x\n",
-                     pView->u32ViewIndex, pView->u32ViewOffset, pView->u32ViewSize, pView->u32MaxScreenSize));
+int VBVAInfoView(PVGASTATE pVGAState, const VBVAINFOVIEW *pView)
+{
+    LogFlowFunc(("VBVA_INFO_VIEW: u32ViewIndex %d, u32ViewOffset 0x%x, u32ViewSize 0x%x, u32MaxScreenSize 0x%x\n",
+                 pView->u32ViewIndex, pView->u32ViewOffset, pView->u32ViewSize, pView->u32MaxScreenSize));
 
     PHGSMIINSTANCE pIns = pVGAState->pHGSMI;
-    VBVACONTEXT *pCtx = (VBVACONTEXT *)HGSMIContext (pIns);
-
-    /* @todo verify view data. */
-    if (pView->u32ViewIndex >= pCtx->cViews)
-    {
-        Log(("View index too large %d!!!\n",
-             pView->u32ViewIndex));
-        return VERR_INVALID_PARAMETER;
-    }
-
-    pCtx->aViews[pView->u32ViewIndex].view = *pView;
-
-    return VINF_SUCCESS;
-}
-
-int VBVAInfoScreen(PVGASTATE pVGAState, VBVAINFOSCREEN *pScreen)
-{
+    VBVACONTEXT *pCtx = (VBVACONTEXT *)HGSMIContext(pIns);
+
+    if (   pView->u32ViewIndex < pCtx->cViews
+        && pView->u32ViewOffset <= pVGAState->vram_size
+        && pView->u32ViewSize <= pVGAState->vram_size
+        && pView->u32ViewOffset <= pVGAState->vram_size - pView->u32ViewSize
+        && pView->u32MaxScreenSize <= pView->u32ViewSize)
+    {
+        pCtx->aViews[pView->u32ViewIndex].view = *pView;
+        return VINF_SUCCESS;
+    }
+
+    LogRelFlow(("VBVA_INFO_VIEW: invalid data: index %d(%d), offset 0x%x, size 0x%x, max 0x%x, vram size 0x%x\n",
+                pView->u32ViewIndex, pCtx->cViews, pView->u32ViewOffset, pView->u32ViewSize,
+                pView->u32MaxScreenSize, pVGAState->vram_size));
+    return VERR_INVALID_PARAMETER;
+}
+
+int VBVAInfoScreen(PVGASTATE pVGAState, const VBVAINFOSCREEN *pScreen)
+{
+    LogRel(("VBVA_INFO_SCREEN: [%d] @%d,%d %dx%d, line 0x%x, BPP %d, flags 0x%x\n",
+            pScreen->u32ViewIndex, pScreen->i32OriginX, pScreen->i32OriginY,
+            pScreen->u32Width, pScreen->u32Height,
+            pScreen->u32LineSize, pScreen->u16BitsPerPixel, pScreen->u16Flags));
+
     PHGSMIINSTANCE pIns = pVGAState->pHGSMI;
-    VBVACONTEXT *pCtx = (VBVACONTEXT *)HGSMIContext (pIns);
-    VBVAINFOVIEW *pView = &pCtx->aViews[pScreen->u32ViewIndex].view;
-    /* Calculate the offset of the  end of the screen so we can make
-     * sure it is inside the view.  I assume that screen rollover is not
-     * implemented. */
-    int64_t offEnd =   (int64_t)pScreen->u32Height * pScreen->u32LineSize
-                     + pScreen->u32Width + pScreen->u32StartOffset;
-    LogRel(("VBVA_INFO_SCREEN: [%d] @%d,%d %dx%d, line 0x%x, BPP %d, flags 0x%x\n",
-                    pScreen->u32ViewIndex, pScreen->i32OriginX, pScreen->i32OriginY,
-                    pScreen->u32Width, pScreen->u32Height,
-                    pScreen->u32LineSize,  pScreen->u16BitsPerPixel, pScreen->u16Flags));
-
-    if (   pScreen->u32ViewIndex < RT_ELEMENTS (pCtx->aViews)
+    VBVACONTEXT *pCtx = (VBVACONTEXT *)HGSMIContext(pIns);
+
+    /* Allow pScreen->u16BitsPerPixel == 0 because legacy guest code used it for screen blanking. */
+    if (   pScreen->u32ViewIndex < pCtx->cViews
         && pScreen->u16BitsPerPixel <= 32
         && pScreen->u32Width <= UINT16_MAX
         && pScreen->u32Height <= UINT16_MAX
-        && pScreen->u32LineSize <= UINT16_MAX * 4
-        && offEnd < pView->u32MaxScreenSize)
-    {
-        vbvaResize (pVGAState, &pCtx->aViews[pScreen->u32ViewIndex], pScreen);
-        return VINF_SUCCESS;
-    }
-
-    LogRelFlow(("VBVA_INFO_SCREEN [%lu]: bad data: %lux%lu, line 0x%lx, BPP %u, start offset %lu, max screen size %lu\n",
-                    (unsigned long)pScreen->u32ViewIndex,
-                    (unsigned long)pScreen->u32Width,
-                    (unsigned long)pScreen->u32Height,
-                    (unsigned long)pScreen->u32LineSize,
-                    (unsigned long)pScreen->u16BitsPerPixel,
-                    (unsigned long)pScreen->u32StartOffset,
-                    (unsigned long)pView->u32MaxScreenSize));
+        && pScreen->u32LineSize <= UINT16_MAX * 4)
+    {
+        const VBVAINFOVIEW *pView = &pCtx->aViews[pScreen->u32ViewIndex].view;
+        const uint32_t u32BytesPerPixel = (pScreen->u16BitsPerPixel + 7) / 8;
+        if (pScreen->u32Width <= pScreen->u32LineSize / (u32BytesPerPixel? u32BytesPerPixel: 1))
+        {
+            const uint64_t u64ScreenSize = (uint64_t)pScreen->u32LineSize * pScreen->u32Height;
+            if (   pScreen->u32StartOffset <= pView->u32ViewSize
+                && u64ScreenSize <= pView->u32MaxScreenSize
+                && pScreen->u32StartOffset <= pView->u32ViewSize - (uint32_t)u64ScreenSize)
+            {
+                vbvaResize(pVGAState, &pCtx->aViews[pScreen->u32ViewIndex], pScreen);
+                return VINF_SUCCESS;
+            }
+
+            LogRelFlow(("VBVA_INFO_SCREEN: invalid data: size 0x%RX64, max 0x%RX32\n",
+                        u64ScreenSize, pView->u32MaxScreenSize));
+        }
+    }
+    else
+    {
+        LogRelFlow(("VBVA_INFO_SCREEN: invalid data: index %RU32(%RU32)\n",
+                     pScreen->u32ViewIndex, pCtx->cViews));
+    }
+
     return VERR_INVALID_PARAMETER;
 }
@@ -2241 +2250 @@
 #endif
 
-            if (cbBuffer < sizeof (VBVAINFOVIEW))
+            /* Expect at least one VBVAINFOVIEW structure. */
+            if (cbBuffer < sizeof(VBVAINFOVIEW))
             {
                 rc = VERR_INVALID_PARAMETER;
@@ -2248 +2258 @@
 
             /* Guest submits an array of VBVAINFOVIEW structures. */
-            VBVAINFOVIEW *pView = (VBVAINFOVIEW *)pvBuffer;
-
+            const VBVAINFOVIEW *pView = (VBVAINFOVIEW *)pvBuffer;
             for (;
-                 cbBuffer >= sizeof (VBVAINFOVIEW);
-                 pView++, cbBuffer -= sizeof (VBVAINFOVIEW))
-            {
-                VBVAINFOVIEW View = *pView;
-                rc = VBVAInfoView(pVGAState, &View);
+                 cbBuffer >= sizeof(VBVAINFOVIEW);
+                 ++pView, cbBuffer -= sizeof(VBVAINFOVIEW))
+            {
+                VBVAINFOVIEW view = *pView;
+                rc = VBVAInfoView(pVGAState, &view);
                 if (RT_FAILURE(rc))
                     break;
@@ -2293 +2302 @@
         case VBVA_INFO_SCREEN:
         {
-            if (cbBuffer < sizeof (VBVAINFOSCREEN))
-            {
-                rc = VERR_INVALID_PARAMETER;
-                break;
-            }
-
 #ifdef VBOX_WITH_CRHGSMI
             if (vboxCmdVBVAIsEnabled(pVGAState))
@@ -2308 +2311 @@
 #endif
 
-            VBVAINFOSCREEN *pScreen = (VBVAINFOSCREEN *)pvBuffer;
-            VBVAINFOSCREEN Screen = *pScreen;
+            if (cbBuffer < sizeof(VBVAINFOSCREEN))
+            {
+                rc = VERR_INVALID_PARAMETER;
+                break;
+            }
+
+            VBVAINFOSCREEN Screen = *(VBVAINFOSCREEN *)pvBuffer;
             rc = VBVAInfoScreen(pVGAState, &Screen);
         } break;

trunk/src/VBox/Devices/Graphics/DevVGA_VDMA.cpp

@@ -1437 +1437 @@
 }
 
+static int vboxVDMASetupScreenInfo(PVGASTATE pVGAState, VBVAINFOSCREEN *pScreen)
+{
+    const uint32_t u32ViewIndex = pScreen->u32ViewIndex;
+    const bool fDisabled = RT_BOOL(pScreen->u16Flags & VBVA_SCREEN_F_DISABLED);
+
+    if (fDisabled)
+    {
+        if (   u32ViewIndex < pVGAState->cMonitors
+            || u32ViewIndex == UINT32_C(0xFFFFFFFF))
+        {
+            RT_ZERO(*pScreen);
+            pScreen->u32ViewIndex = u32ViewIndex;
+            pScreen->u16Flags = VBVA_SCREEN_F_ACTIVE | VBVA_SCREEN_F_DISABLED;
+            return VINF_SUCCESS;
+        }
+    }
+    else
+    {
+        if (   u32ViewIndex < pVGAState->cMonitors
+            && pScreen->u16BitsPerPixel <= 32
+            && pScreen->u32Width <= UINT16_MAX
+            && pScreen->u32Height <= UINT16_MAX
+            && pScreen->u32LineSize <= UINT16_MAX * 4)
+        {
+            const uint32_t u32BytesPerPixel = (pScreen->u16BitsPerPixel + 7) / 8;
+            if (pScreen->u32Width <= pScreen->u32LineSize / (u32BytesPerPixel? u32BytesPerPixel: 1))
+            {
+                const uint64_t u64ScreenSize = (uint64_t)pScreen->u32LineSize * pScreen->u32Height;
+                if (   pScreen->u32StartOffset <= pVGAState->vram_size
+                    && u64ScreenSize <= pVGAState->vram_size
+                    && pScreen->u32StartOffset <= pVGAState->vram_size - (uint32_t)u64ScreenSize)
+                {
+                    return VINF_SUCCESS;
+                }
+            }
+        }
+    }
+
+    return VERR_INVALID_PARAMETER;
+}
+
 static int vboxVDMACrGuestCtlResizeEntryProcess(struct VBOXVDMAHOST *pVdma, VBOXCMDVBVA_RESIZE_ENTRY *pEntry)
 {
     PVGASTATE pVGAState = pVdma->pVGAState;
     VBVAINFOSCREEN Screen = pEntry->Screen;
+
+    /* Verify and cleanup local copy of the input data. */
+    int rc = vboxVDMASetupScreenInfo(pVGAState, &Screen);
+    if (RT_FAILURE(rc))
+    {
+        WARN(("invalid screen data\n"));
+        return rc;
+    }
+
+    VBOXCMDVBVA_SCREENMAP_DECL(uint32_t, aTargetMap);
+    memcpy(aTargetMap, pEntry->aTargetMap, sizeof(aTargetMap));
+    ASMBitClearRange(aTargetMap, pVGAState->cMonitors, VBOX_VIDEO_MAX_SCREENS);
+
+    rc = pVdma->CrSrvInfo.pfnResize(pVdma->CrSrvInfo.hSvr, &Screen, aTargetMap);
+    if (RT_FAILURE(rc))
+    {
+        WARN(("pfnResize failed %d\n", rc));
+        return rc;
+    }
+
+    /* A fake view which contains the current screen for the 2D VBVAInfoView. */
     VBVAINFOVIEW View;
-    VBOXCMDVBVA_SCREENMAP_DECL(uint32_t, aTargetMap);
-    uint32_t u32ViewIndex = Screen.u32ViewIndex;
-    uint16_t u16Flags = Screen.u16Flags;
-    bool fDisable = false;
-
-    memcpy(aTargetMap, pEntry->aTargetMap, sizeof (aTargetMap));
-
-    ASMBitClearRange(aTargetMap, pVGAState->cMonitors, VBOX_VIDEO_MAX_SCREENS);
-
-    if (u16Flags & VBVA_SCREEN_F_DISABLED)
-    {
-        fDisable = true;
-        memset(&Screen, 0, sizeof (Screen));
-        Screen.u32ViewIndex = u32ViewIndex;
-        Screen.u16Flags = VBVA_SCREEN_F_ACTIVE | VBVA_SCREEN_F_DISABLED;
-    }
-
-    if (u32ViewIndex > pVGAState->cMonitors)
-    {
-        if (u32ViewIndex != 0xffffffff)
-        {
-            WARN(("invalid view index\n"));
-            return VERR_INVALID_PARAMETER;
-        }
-        else if (!fDisable)
-        {
-            WARN(("0xffffffff view index only valid for disable requests\n"));
-            return VERR_INVALID_PARAMETER;
-        }
-    }
-
     View.u32ViewOffset = 0;
     View.u32ViewSize = Screen.u32LineSize * Screen.u32Height + Screen.u32StartOffset;
-    View.u32MaxScreenSize = View.u32ViewSize + Screen.u32Width + 1; /* <- make VBVAInfoScreen logic (offEnd < pView->u32MaxScreenSize) happy */
-
-    int rc = VINF_SUCCESS;
-
-    rc = pVdma->CrSrvInfo.pfnResize(pVdma->CrSrvInfo.hSvr, &Screen, aTargetMap);
-    if (RT_FAILURE(rc))
-    {
-        WARN(("pfnResize failed %d\n", rc));
-        return rc;
-    }
+    View.u32MaxScreenSize = Screen.u32LineSize * Screen.u32Height;
+
+    const bool fDisable = RT_BOOL(Screen.u16Flags & VBVA_SCREEN_F_DISABLED);
 
     for (int i = ASMBitFirstSet(aTargetMap, pVGAState->cMonitors);
@@ -1520 +1544 @@
         }
     }
-
-    if (RT_FAILURE(rc))
-        return rc;
-
-    Screen.u32ViewIndex = u32ViewIndex;
 
     return rc;