Re: [ros-dev] [ros-diffs] [jgardou] 48465: [WIN32K] - Rewrite NtGdiStretchDIBitsInternal : clearer, faster, stronger (+1 wine test)

[Timo Kreuzer]

Hi,

Please take care about proper protection of the user mode buffer. The
current solution: probe and forget is not safe.

Possibilities are:
1) SEH protected copying of the buffer, pass the copy of the buffer to
lower level functions -> Easy to do, large overhead for large bitmaps.
2) SEH protected call to a lower level function, passing the user mode
buffer. -> Not possible if the lower level function is either allocating
any resources (unless also protected by SEH + finally) or can pass
execution to 3rd party provided code, like drivers.
3) Be sure to have SEH at the lowest level (DIB) -> Not possible as the
function might end up in a driver.
4) Use Mm to protect the buffer. Either with MmSecureVirtualMemory or
double mapping using MmProbeAndLockPages + MmGetSystemAddressForMdlSafe.

I think 4 is the way to go. While the overhead of remapping should be
relatively small compared to a full copy, we are still wasting large
ammounts of system address space.
MmSecureVirtualMemory might at first sound like a good solution, but
beware, it has some pitfalls. While it protects a memory range from
being freed, it doesn't protect it from being paged out. That wouldn't
be a problem, unless the memory is not backed by the page file, but let
say a network resource, which becomes unavailable after a page was paged
out. In this case we would get an in page error when trying to access
the page, leading to a kernel crash. So unless we can be sure that the
memory is backed by the page file, we need to additionally lock the
pages into memory to be safe. Final thing to note is that
MmSecureVirtualMemory is not implemented yet, but I hope with current
work on the VAD code, we'll soon get a present (hint).

Regards,
Timo

jgardou(a)svn.reactos.org wrote:
>   Author: jgardou
>  Date: Thu Aug  5 21:12:57 2010
>  New Revision: 48465
>
>  URL: http://svn.reactos.org/svn/reactos?rev=48465&view=rev
>  Log:
>  [WIN32K]
>    - Rewrite NtGdiStretchDIBitsInternal : clearer, faster, stronger (+1 wine test)
>
>  Modified:
>      branches/reactos-yarotows/subsystems/win32/win32k/objects/dibobj.c
>
>  Modified: branches/reactos-yarotows/subsystems/win32/win32k/objects/dibobj.c
>  URL:
> http://svn.reactos.org/svn/reactos/branches/reactos-yarotows/subsystems/win…
>  ==============================================================================
>  --- branches/reactos-yarotows/subsystems/win32/win32k/objects/dibobj.c [iso-8859-1]
> (original)
>  +++ branches/reactos-yarotows/subsystems/win32/win32k/objects/dibobj.c [iso-8859-1] Thu
> Aug  5 21:12:57 2010
>  @@ -290,7 +290,7 @@
>   
>   	if(!(psurfSrc && psurfDst))
>   	{
>  -		DPRINT1("Error, could not lock surfaces.\n");
>  +		DPRINT1("Error, could not lock surfaces\n");
>   		goto cleanup;
>   	}
>   
>  @@ -663,15 +663,15 @@
>   		if(pbmci)
>   		{
>   			pbmci->bmciHeader.bcWidth = psurf->SurfObj.sizlBitmap.cx;
>  -			pbmci->bmciHeader.bcHeight = (psurf->SurfObj.fjBitmap & BMF_TOPDOWN) ? 
>  -				-psurf->SurfObj.sizlBitmap.cy : 
>  +			pbmci->bmciHeader.bcHeight = (psurf->SurfObj.fjBitmap & BMF_TOPDOWN) ?
>  +				-psurf->SurfObj.sizlBitmap.cy :
>   			    psurf->SurfObj.sizlBitmap.cy;
>   			pbmci->bmciHeader.bcPlanes = 1;
>   			pbmci->bmciHeader.bcBitCount = BitsPerFormat(psurf->SurfObj.iBitmapFormat);
>   		}
>   		Info->bmiHeader.biWidth = psurf->SurfObj.sizlBitmap.cx;
>  -		Info->bmiHeader.biHeight = (psurf->SurfObj.fjBitmap & BMF_TOPDOWN) ? 
>  -			-psurf->SurfObj.sizlBitmap.cy : 
>  +		Info->bmiHeader.biHeight = (psurf->SurfObj.fjBitmap & BMF_TOPDOWN) ?
>  +			-psurf->SurfObj.sizlBitmap.cy :
>   		    psurf->SurfObj.sizlBitmap.cy;;
>   		Info->bmiHeader.biPlanes = 1;
>   		Info->bmiHeader.biBitCount = BitsPerFormat(psurf->SurfObj.iBitmapFormat);
>  @@ -711,15 +711,15 @@
>   	case 8:
>   		Info->bmiHeader.biClrUsed = 0;
>   
>  -		/* If the bitmap if a DIB section and has the same format than what 
>  +		/* If the bitmap if a DIB section and has the same format than what
>   		 * we're asked, go ahead! */
>  -		if((psurf->hSecure) && 
>  +		if((psurf->hSecure) &&
>   			(BitsPerFormat(psurf->SurfObj.iBitmapFormat) == bpp))
>   		{
>   			if(Usage == DIB_RGB_COLORS)
>   			{
>   				unsigned int colors = min(psurf->ppal->NumColors, 1 << bpp);
>  -				
>  +
>   				if(pbmci)
>   				{
>   					for(i=0; i < colors; i++)
>  @@ -773,7 +773,7 @@
>                           rgbTriples[i].rgbtGreen = pDcPal->IndexedColors[i].peGreen;
>                           rgbTriples[i].rgbtBlue  = pDcPal->IndexedColors[i].peBlue;
>                       }
>  -                
>  +
>                       rgbQuads[i].rgbRed      = pDcPal->IndexedColors[i].peRed;
>                       rgbQuads[i].rgbGreen    = pDcPal->IndexedColors[i].peGreen;
>                       rgbQuads[i].rgbBlue     = pDcPal->IndexedColors[i].peBlue;
>  @@ -822,7 +822,7 @@
>   							{
>                                   for(g = 0; g <= 5; g++)
>   								{
>  -                                    for(b = 0; b <= 5; b++) 
>  +                                    for(b = 0; b <= 5; b++)
>   									{
>                                           colorTriple->rgbtRed =   (r * 0xff) / 5;
>                                           colorTriple->rgbtGreen = (g * 0xff) / 5;
>  @@ -935,7 +935,7 @@
>   		/* Restore them */
>   		Info->bmiHeader.biWidth = width;
>   		Info->bmiHeader.biHeight = height;
>  -		
>  +
>   		if(!hBmpDest)
>   		{
>   			DPRINT1("Unable to create a DIB Section!\n");
>  @@ -972,7 +972,7 @@
>   			_SEH2_TRY
>   			{
>   				RtlCopyMemory(Bits, pDIBits, DIB_GetDIBImageBytes (width, height, bpp));
>  -			} 
>  +			}
>   			_SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
>   			{
>   				Status = _SEH2_GetExceptionCode();
>  @@ -1021,95 +1021,104 @@
>       UINT cjMaxBits,
>       HANDLE hcmXform)
>   {
>  -    HBITMAP hBitmap, hOldBitmap = NULL;
>  -    HDC hdcMem = NULL;
>  +    PDC pdc;
>  +    INT ret = 0;
>  +    LONG height;
>  +    LONG width;
>  +    WORD planes, bpp;
>  +    DWORD compr, size;
>  +    HBITMAP hBitmap;
>  +    BOOL fastpath = FALSE;
>       NTSTATUS Status = STATUS_SUCCESS;
>  -	PVOID pvDIBits;
>  -	INT Ret = 0;
>  +
>   
>       if (!Bits || !BitsInfo)
>  -    {
>  -        SetLastWin32Error(ERROR_INVALID_PARAMETER);
>           return 0;
>  -    }
>  -
>  -    /* Create a DIB Section, data will be probed there */
>  -	hBitmap = NtGdiCreateDIBSection(hDC,
>  -								  NULL,
>  -								  0,
>  -								  BitsInfo,
>  -								  Usage,
>  -								  0,
>  -								  0,
>  -								  0,
>  -								  &pvDIBits);
>  -
>  -	if(!hBitmap)
>  -	{
>  -		DPRINT1("Failed to create a DIB.\n");
>  -		return 0;
>  -	}
>  -
>  -	/* Set bits */
>  -	_SEH2_TRY
>  -	{
>  -		ProbeForRead(Bits, cjMaxBits, 1);
>  -		RtlCopyMemory(pvDIBits, Bits, DIB_GetDIBImageBytes(BitsInfo->bmiHeader.biWidth,
>  -														   BitsInfo->bmiHeader.biHeight,
>  -														   BitsInfo->bmiHeader.biBitCount));
>  -	}
>  -	_SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
>  -	{
>  -		Status = _SEH2_GetExceptionCode();
>  -	}
>  -	_SEH2_END
>  -
>  -	if(!NT_SUCCESS(Status))
>  -	{
>  -		DPRINT1("Error : Could not read DIB bits\n");
>  -		SetLastNtError(Status);
>  -		goto cleanup;
>  -	}
>  -
>  -	hdcMem = NtGdiCreateCompatibleDC(0);
>  -	if(!hdcMem)
>  -	{
>  -		DPRINT1("Failed to create a memory DC!");
>  -		goto cleanup;
>  -	}
>  -
>  -	hOldBitmap = NtGdiSelectBitmap(hdcMem, hBitmap);
>  -	if(!hOldBitmap)
>  -	{
>  -		DPRINT1("Could not select the DIB into the memory DC\n");
>  -		goto cleanup;
>  -	}
>  -
>  -	/* Do we want to stretch ? */
>  -	if((SrcWidth == DestWidth) && (SrcHeight == DestHeight))
>  -	{
>  -		Ret = NtGdiBitBlt(hDC, XDest, YDest, XDest + DestWidth, YDest + DestHeight, 
>  -						  hdcMem, XSrc, YSrc, ROP, 0, 0);
>  -	}
>  -	else
>  -	{
>  -		Ret = NtGdiStretchBlt(hDC, XDest, YDest, XDest + DestWidth, YDest + DestHeight,
>  -							hdcMem, XSrc, YSrc, XSrc + SrcWidth, YSrc + SrcHeight,
>  -							ROP, 0);
>  -	}
>  -
>  -	if(Ret)
>  -		Ret = SrcHeight ;
>  -
>  +
>  +    if (!(pdc = DC_LockDc(hDC))) return 0;
>  +
>  +
>  +    _SEH2_TRY
>  +    {
>  +        ProbeForRead(BitsInfo, cjMaxInfo, 1);
>  +        ProbeForRead(Bits, cjMaxBits, 1);
>  +        if (DIB_GetBitmapInfo( &BitsInfo->bmiHeader, &width, &height,
> &planes, &bpp, &compr, &size ) == -1)
>  +        {
>  +            DPRINT1("Invalid bitmap\n");
>  +            Status = STATUS_INVALID_PARAMETER;
>  +        }
>  +    }
>  +    _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
>  +    {
>  +        Status = _SEH2_GetExceptionCode();
>  +    }
>  +    _SEH2_END
>  +
>  +    if(!NT_SUCCESS(Status))
>  +    {
>  +        DPRINT1("Error, failed to read the DIB bits\n");
>  +        goto cleanup;
>  +    }
>  +
>  +    if (width < 0)
>  +    {
>  +        DPRINT1("Bitmap has a negative width\n");
>  +        return 0;
>  +    }
>  +
>  +    hBitmap = NtGdiGetDCObject(hDC, OBJ_BITMAP);
>  +
>  +    if (XDest == 0 && YDest == 0 && XSrc == 0 && XSrc == 0
> &&
>  +        DestWidth == SrcWidth && DestHeight == SrcHeight &&
>  +        compr == BI_RGB &&
>  +        ROP == SRCCOPY)
>  +    {
>  +        BITMAP bmp;
>  +        if (IntGdiGetObject(hBitmap, sizeof(bmp), &bmp) == sizeof(bmp))
>  +        {
>  +            if (bmp.bmBitsPixel == bpp &&
>  +                bmp.bmWidth == SrcWidth &&
>  +                bmp.bmHeight == SrcHeight &&
>  +                bmp.bmPlanes == planes)
>  +                fastpath = TRUE;
>  +        }
>  +    }
>  +
>  +    if (fastpath)
>  +    {
>  +        /* fast path */
>  +        DPRINT1("using fast path\n");
>  +        ret = IntSetDIBits( pdc, hBitmap, 0, height, Bits, BitsInfo, Usage);
>  +    }
>  +    else
>  +    {
>  +        /* slow path - need to use StretchBlt */
>  +        HBITMAP hOldBitmap;
>  +        HPALETTE hpal = NULL;
>  +        HDC hdcMem;
>  +        PVOID pvBits;
>  +
>  +        hdcMem = NtGdiCreateCompatibleDC( hDC );
>  +        hBitmap = DIB_CreateDIBSection(pdc, BitsInfo, Usage, &pvBits, NULL, 0, 0);
>  +        RtlCopyMemory(pvBits, Bits, cjMaxBits);
>  +        hOldBitmap = NtGdiSelectBitmap( hdcMem, hBitmap );
>  +
>  +        /* Origin for DIBitmap may be bottom left (positive biHeight) or top
>  +           left (negative biHeight) */
>  +        ret = NtGdiStretchBlt( hDC, XDest, YDest, DestWidth, DestHeight,
>  +                             hdcMem, XSrc, abs(height) - SrcHeight - YSrc,
>  +                             SrcWidth, SrcHeight, ROP, 0 );
>  +        if(hpal)
>  +            GdiSelectPalette(hdcMem, hpal, FALSE);
>  +        if(ret)
>  +            ret = SrcHeight;
>  +        NtGdiSelectBitmap( hdcMem, hOldBitmap );
>  +        NtGdiDeleteObjectApp( hdcMem );
>  +        GreDeleteObject( hBitmap );
>  +    }
>   cleanup:
>  -	if(hdcMem)
>  -	{
>  -		if(hOldBitmap) NtGdiSelectBitmap(hdcMem, hOldBitmap);
>  -		NtGdiDeleteObjectApp(hdcMem);
>  -	}
>  -	GreDeleteObject(hBitmap);
>  -
>  -    return Ret;
>  +    DC_UnlockDc(pdc);
>  +    return ret;
>   }
>   
>   
>  @@ -1462,7 +1471,7 @@
>   			hpal = (HPALETTE) 0xFFFFFFFF;
>   		}
>       }
>  -    else 
>  +    else
>   	{
>           hpal = BuildDIBPalette(bmi);
>   	}
>  @@ -1563,7 +1572,7 @@
>    * Get the info from a bitmap header.
>    * Return 0 for COREHEADER, 1 for INFOHEADER, -1 for error.
>    */
>  -int 
>  +int
>   FASTCALL
>   DIB_GetBitmapInfo( const BITMAPINFOHEADER *header, LONG *width,
>                          LONG *height, WORD *planes, WORD *bpp, DWORD *compr, DWORD *size
> )
>  @@ -1575,7 +1584,7 @@
>           *height = core->bcHeight;
>           *planes = core->bcPlanes;
>           *bpp    = core->bcBitCount;
>  -        *compr  = 0;
>  +        *compr  = BI_RGB;
>           *size   = 0;
>           return 0;
>       }
>  @@ -1692,10 +1701,10 @@
>   			ppalEntries[i].peBlue = 0;
>   			ppalEntries[i].peFlags = 0;
>           }
>  -        
>  +
>           lpIndex++;
>       }
>  -    
>  +
>   	hpal = PALETTE_AllocPalette(PAL_INDEXED, nNumColors, (ULONG*)ppalEntries, 0, 0, 0);
>   
>   	ExFreePoolWithTag(ppalEntries, TAG_COLORMAP);
>
>
>
>

[Jérôme Gardou]

Le vendredi 06 août 2010 19:03:57, Timo Kreuzer a écrit :
>   Hi,
>  
>  Please take care about proper protection of the user mode buffer. The
>  current solution: probe and forget is not safe.
>  
>  Possibilities are:
>  1) SEH protected copying of the buffer, pass the copy of the buffer to
>  lower level functions -> Easy to do, large overhead for large bitmaps.
>  2) SEH protected call to a lower level function, passing the user mode
>  buffer. -> Not possible if the lower level function is either allocating
>  any resources (unless also protected by SEH + finally) or can pass
>  execution to 3rd party provided code, like drivers.
>  3) Be sure to have SEH at the lowest level (DIB) -> Not possible as the
>  function might end up in a driver.
>  4) Use Mm to protect the buffer. Either with MmSecureVirtualMemory or
>  double mapping using MmProbeAndLockPages + MmGetSystemAddressForMdlSafe.
>  
>  I think 4 is the way to go. While the overhead of remapping should be
>  relatively small compared to a full copy, we are still wasting large
>  ammounts of system address space.
>  MmSecureVirtualMemory might at first sound like a good solution, but
>  beware, it has some pitfalls. While it protects a memory range from
>  being freed, it doesn't protect it from being paged out. That wouldn't
>  be a problem, unless the memory is not backed by the page file, but let
>  say a network resource, which becomes unavailable after a page was paged
>  out. In this case we would get an in page error when trying to access
>  the page, leading to a kernel crash. So unless we can be sure that the
>  memory is backed by the page file, we need to additionally lock the
>  pages into memory to be safe. Final thing to note is that
>  MmSecureVirtualMemory is not implemented yet, but I hope with current
>  work on the VAD code, we'll soon get a present (hint).
>  
>  Regards,
>  Timo
>   
I'm OK for 4. Looking for MmSecureVirtualMemory to be implemented.
Please note that for now, SURFACE::hSecure is hacked to be (HANDLE)1 so we can 
detect if the bitmap is a DIB. As now all **DIB** functions create a DIB, it 
will be securised in the process. Others bits can be PSEH-accessed during the 
DIB creation, as it the only place it's used.

Regards.
Jérôme.