[PATCH v9 0/2] 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`.

--
  v9: winspool: Check the alignment of dmSize in IsValidDevmodeW()
      winspool: Check dmSize in IsValidDevmodeW
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2262

[PATCH v9 1/2] winspool: Check dmSize in IsValidDevmodeW

[Tingzhong Luo]

From: Tingzhong Luo <luotingzhong(a)uniontech.com>

Co-authored-by: Piotr Caban <piotr(a)codeweavers.com>
---
 dlls/winspool.drv/info.c       |  4 +++-
 dlls/winspool.drv/tests/info.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/dlls/winspool.drv/info.c b/dlls/winspool.drv/info.c
index 54ecb81d3be..d41a17be06b 100644
--- a/dlls/winspool.drv/info.c
+++ b/dlls/winspool.drv/info.c
@@ -1927,9 +1927,11 @@ 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;
 
     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..3dfbd529bcc 100644
--- a/dlls/winspool.drv/tests/info.c
+++ b/dlls/winspool.drv/tests/info.c
@@ -3046,6 +3046,26 @@ 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) + 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 },
+    };
     DEVMODEW dm;
     int i;
     BOOL ret;
@@ -3065,6 +3085,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)
-- 
GitLab


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

[PATCH v9 2/2] winspool: Check the alignment of dmSize in IsValidDevmodeW()

[Piotr Caban]

From: Piotr Caban <piotr(a)codeweavers.com>

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

diff --git a/dlls/winspool.drv/info.c b/dlls/winspool.drv/info.c
index d41a17be06b..df7ad4ddeb2 100644
--- a/dlls/winspool.drv/info.c
+++ b/dlls/winspool.drv/info.c
@@ -1934,7 +1934,7 @@ BOOL WINAPI IsValidDevmodeW(PDEVMODEW dm, SIZE_T size)
     if (dm->dmSize < offFields || size < dm->dmSize + dm->dmDriverExtra) return FALSE;
 
     for (i = 0; i < ARRAY_SIZE(map); i++)
-        if ((dm->dmFields & map[i].flag) && size < map[i].size)
+        if ((dm->dmFields & map[i].flag) && dm->dmSize < map[i].size)
             return FALSE;
 
     return TRUE;
diff --git a/dlls/winspool.drv/tests/info.c b/dlls/winspool.drv/tests/info.c
index 3dfbd529bcc..52849277c4c 100644
--- a/dlls/winspool.drv/tests/info.c
+++ b/dlls/winspool.drv/tests/info.c
@@ -3084,6 +3084,8 @@ static void test_IsValidDevmodeW(void)
         dm.dmFields = test[i].dmFields;
         ret = IsValidDevmodeW(&dm, dm.dmSize);
         ok(ret == test[i].ret, "%d: got %d\n", i, ret);
+        ret = IsValidDevmodeW(&dm, dm.dmSize + 4);
+        ok(ret == test[i].ret, "%d: got %d\n", i, ret);
     }
 
     dm.dmFields = 0;
-- 
GitLab

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

[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);
> Let's use `fields_off` instead of `offFields`.

Also, you don't need to `Co-authored-by:` tag in the commit message.

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

[Huw Davies (＠huw)]

Huw Davies (@huw) commented about dlls/winspool.drv/info.c:
> if (dm->dmSize < offFields || size < dm->dmSize + dm->dmDriverExtra) return FALSE;
>
> for (i = 0; i < ARRAY_SIZE(map); i++)
> -        if ((dm->dmFields & map[i].flag) && size < map[i].size)
> +        if ((dm->dmFields & map[i].flag) && dm->dmSize < map[i].size)

Let's just squash these two commits together.

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