[PATCH v5 0/1] MR2262: winspool: Check dmSize in IsValidDevmodeW

[Tingzhong Luo (＠tzluo)]

When dmSize is zero or greter than size of input buffer, `IsValidDevmodeW()`
 failed in Windows 10.

But current implementation in wine, it will return `true` because
there is no check to `dm.dmSize`.

--
  v5: winspool: Check dmSize in IsValidDevmodeW

https://gitlab.winehq.org/wine/wine/-/merge_requests/2262

[PATCH v5 1/1] winspool: Check dmSize in IsValidDevmodeW

[Tingzhong Luo]

From: Tingzhong Luo

---
 dlls/winspool.drv/info.c       |  5 ++++-
 dlls/winspool.drv/tests/info.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/dlls/winspool.drv/info.c b/dlls/winspool.drv/info.c
index 54ecb81d3be..8a85dc97c73 100644
--- a/dlls/winspool.drv/info.c
+++ b/dlls/winspool.drv/info.c
@@ -1927,9 +1927,12 @@ BOOL WINAPI IsValidDevmodeW(PDEVMODEW dm, SIZE_T size)
 #undef F_SIZE
     };
     int i;
+    const DWORD offFields = FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields);

if (!dm) return FALSE;
-    if (size < FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields)) return FALSE;
+    if (size < offFields) return FALSE;
+    if (dm->dmSize < offFields || size < dm->dmSize + dm->dmDriverExtra) return FALSE;
+    if (((dm->dmSize - offFields) % 4) && dm->dmDriverExtra) return FALSE;

for (i = 0; i < ARRAY_SIZE(map); i++)
         if ((dm->dmFields & map[i].flag) && size < map[i].size)
diff --git a/dlls/winspool.drv/tests/info.c b/dlls/winspool.drv/tests/info.c
index 92b042e354a..e8beceeb571 100644
--- a/dlls/winspool.drv/tests/info.c
+++ b/dlls/winspool.drv/tests/info.c
@@ -3046,6 +3046,30 @@ static void test_IsValidDevmodeW(void)
         { DM_NUP, FIELD_OFFSET(DEVMODEW, u2.dmNup) + 4, TRUE },

};
+    static const struct
+    {
+        DWORD dmSize;
+        DWORD dmDriverExtra;
+        DWORD bufSize;
+        BOOL ret;
+    } size_test[] =
+    {
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 3, 1, FIELD_OFFSET(DEVMODEW, dmFields) + 4, FALSE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 4, 1, FIELD_OFFSET(DEVMODEW, dmFields) + 8, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 4, 2, FIELD_OFFSET(DEVMODEW, dmFields) + 8, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 4, 3, FIELD_OFFSET(DEVMODEW, dmFields) + 8, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 4, 4, FIELD_OFFSET(DEVMODEW, dmFields) + 8, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 4, 5, FIELD_OFFSET(DEVMODEW, dmFields) + 8, FALSE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 5, 0, FIELD_OFFSET(DEVMODEW, dmFields) + 8, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 5, 1, FIELD_OFFSET(DEVMODEW, dmFields) + 8, FALSE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 12, 1, FIELD_OFFSET(DEVMODEW, dmFields) + 16, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 12, 2, FIELD_OFFSET(DEVMODEW, dmFields) + 16, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 12, 3, FIELD_OFFSET(DEVMODEW, dmFields) + 16, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 12, 4, FIELD_OFFSET(DEVMODEW, dmFields) + 16, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 12, 5, FIELD_OFFSET(DEVMODEW, dmFields) + 16, FALSE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 13, 0, FIELD_OFFSET(DEVMODEW, dmFields) + 16, TRUE },
+        { FIELD_OFFSET(DEVMODEW, dmFields) + 13, 1, FIELD_OFFSET(DEVMODEW, dmFields) + 16, FALSE },
+    };
     DEVMODEW dm;
     int i;
     BOOL ret;
@@ -3065,6 +3089,15 @@ static void test_IsValidDevmodeW(void)
         ret = IsValidDevmodeW(&dm, dm.dmSize);
         ok(ret == test[i].ret, "%d: got %d\n", i, ret);
     }
+
+    dm.dmFields = 0;
+    for (i = 0; i < ARRAY_SIZE(size_test); i++)
+    {
+        dm.dmSize = size_test[i].dmSize;
+        dm.dmDriverExtra = size_test[i].dmDriverExtra;
+        ret = IsValidDevmodeW(&dm, size_test[i].bufSize);
+        ok(ret == size_test[i].ret, "%d: got %d\n", i, ret);
+    }
 }

START_TEST(info)

[Huw Davies (＠huw)]

Huw Davies (@huw) commented about dlls/winspool.drv/info.c:
> #undef F_SIZE
>      };
>      int i;
> -
>
> const DWORD offFields = FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields);
>
> if (!dm) return FALSE;
> - if (size < FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields)) return FALSE;
> - if (size < offFields) return FALSE;
> - if (dm->dmSize < offFields || size < dm->dmSize + dm->dmDriverExtra) return FALSE;
> - if (((dm->dmSize - offFields) % 4) && dm->dmDriverExtra) return FALSE;

This last check doesn't make a huge amount of sense - it may be what Windows ends up doing, but no sane app is going to depend on this.  Could we remove that check and trim the tests so they don't probe this behaviour?

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/winspool.drv/info.c:
> #undef F_SIZE
>      };
>      int i;
> -
>
> const DWORD offFields = FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields);
>
> if (!dm) return FALSE;
> - if (size < FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields)) return FALSE;
> -
>
> if (size < offFields) return FALSE;
> -
>
> if (dm->dmSize < offFields || size < dm->dmSize + dm->dmDriverExtra) return FALSE;
> -
>
> if (((dm->dmSize - offFields) % 4) && dm->dmDriverExtra) return FALSE;
>
> for (i = 0; i < ARRAY_SIZE(map); i++)
>     if ((dm->dmFields & map[i].flag) && size < map[i].size)

This line should be changed to:
```c
if ((dm->dmFields & map[i].flag) && dm->dmSize < map[i].size)
```

[Tingzhong Luo (＠tzluo)]

On Fri Feb 24 09:45:18 2023 +0000, Huw Davies wrote:
> This last check doesn't make a huge amount of sense - it may be what
> Windows ends up doing, but no sane app is going to depend on this.
>  Could we remove that check and trim the tests so they don't probe this behaviour?

The last check is added to make the behavior of this function looks likes  Windows does. if it's not suitable, let's just remove it.