[Bug 54741] New: integer overflow in get_dib_stride / NtGdiCreateBitmap

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=54741

Bug ID: 54741
           Summary: integer overflow in get_dib_stride / NtGdiCreateBitmap
           Product: Wine
           Version: 8.4
          Hardware: x86-64
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: gdi32
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: ---

Here's the source code of NtGdiCreateBitmap:

97 HBITMAP WINAPI NtGdiCreateBitmap( INT width, INT height, UINT planes,       
 98                                   UINT bpp, const void *bits )              
 99 {

Having INT for width and height is already bad, but can't be helped because you
are implementing a crappy API.

105     if (width > 0x7ffffff || height > 0x7ffffff)                            
106     {                                                                       
107         RtlSetLastWin32Error( ERROR_INVALID_PARAMETER );                    
108         return 0;                                                           
109     }                                                                       
110                                                                             
111     if (!width || !height)                                                  
112         return 0;                                                           
113                                                                             
114     if (height < 0)                                                         
115         height = -height;                                                   
116     if (width < 0)                                                          
117         width = -width;

After this the value of width and height is 1..0x7ffffff (7 digits, not 8).

bpp is validated to be at most 32.

140     dib_stride = get_dib_stride( width, bpp );                              
141     size = dib_stride * height;                                             
142     /* Check for overflow (dib_stride itself must be ok because of the
constraint on bm.bmWidth above). */                          
143     if (dib_stride != size / height)

Here's the code of get_dib_stride:

282 static inline int get_dib_stride( int width, int bpp )                      
283 {                                                                           
284     return ((width * bpp + 31) >> 3) & ~3;                                  
285 }

width can be 0x7ffffff, bpp can be 32.
width*bpp is then 0xffffffe0 which looks at first glance like no overflow
happened, but the type is still int, i.e. signed 32-bit, i.e. max value is
0x7fffffff (8 digits), max bpp to trigger that would be 16 not 32.

We are right-shifting afterwards, so we ought to be fine? Nope, because we are
operating on signed ints, so it's an arithmetic right shift, not a logical one.
The & does not remove the sign bit. So what happens here is that get_dib_stride
can return a negative value.

OK but NtGdiCreateBitmap checks for overflow and will notice something is
wrong.
Not really. If width=height=0x7ffffff and bpp=32 then get_dib_stride=-4.
Then size is -4*0x7ffffff promoted to size_t, or 0xe0000004.
The overflow check does this:

143     if (dib_stride != size / height)

size/0x7ffffff is 0xfffffffc.
dib_stride is -4, promoted to size_t for the comparison is also 0xfffffffc.

Also: On the way we had undefined behavior. The compiler would be allowed to do
all kinds of nasty things.