[PATCH v2 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`.

--
  v2: winspool: Check dmSize in IsValidDevmodeW
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2262

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

[Tingzhong Luo]

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

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

diff --git a/dlls/winspool.drv/info.c b/dlls/winspool.drv/info.c
index 54ecb81d3be..26e662d7ffb 100644
--- a/dlls/winspool.drv/info.c
+++ b/dlls/winspool.drv/info.c
@@ -1929,6 +1929,7 @@ BOOL WINAPI IsValidDevmodeW(PDEVMODEW dm, SIZE_T size)
     int i;
 
     if (!dm) return FALSE;
+    if (!dm->dmSize || size < dm->dmSize) return FALSE;
     if (size < FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields)) return FALSE;
 
     for (i = 0; i < ARRAY_SIZE(map); i++)
diff --git a/dlls/winspool.drv/tests/info.c b/dlls/winspool.drv/tests/info.c
index 92b042e354a..6fb279dca1e 100644
--- a/dlls/winspool.drv/tests/info.c
+++ b/dlls/winspool.drv/tests/info.c
@@ -3065,6 +3065,13 @@ static void test_IsValidDevmodeW(void)
         ret = IsValidDevmodeW(&dm, dm.dmSize);
         ok(ret == test[i].ret, "%d: got %d\n", i, ret);
     }
+
+    dm.dmSize = 0;
+    ret = IsValidDevmodeW(&dm, FIELD_OFFSET(DEVMODEW, u2.dmNup) + 4);
+    ok(!ret, "%d: got %d\n", i, ret); /* failed if dm.dmSize is zero */
+    dm.dmSize = FIELD_OFFSET(DEVMODEW, u2.dmNup) + 6;
+    ret = IsValidDevmodeW(&dm, FIELD_OFFSET(DEVMODEW, u2.dmNup) + 4);
+    ok(!ret, "%d: got %d\n", i, ret); /* failed if dm.dmSize greater than bufSize */
 }
 
 START_TEST(info)
-- 
GitLab

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

[Tingzhong Luo (＠tzluo)]

On Thu Feb 23 11:27:14 2023 +0000, Tingzhong Luo wrote:
> changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/2262/diffs?diff_id=34177&start_sha=fbc8db57519f514774e9f494c5da17249d2d6f52#2f93c6576b6b77d6ed08bb09657bb49f1c328a7f_3070_3070)
> Removed the blank line.

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

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/winspool.drv/info.c:
> int i;
>
> if (!dm) return FALSE;
> +    if (!dm->dmSize || size < dm->dmSize) return FALSE;

It's not safe to check `dm->dmSize` field before validating that `size` is big enough so we can read it. Also there needs to be enough room for `dm->dmSize + dm->dmDriverExtra`. Current implementation will also behave incorrectly if `dm->dmSize` is 1 (it should also fail in this case).

I think about something along these lines (not tested):
```c
if (!dm) return FALSE;
if (size < FIELD_OFFSET(DEVMODEW, dmFields)) return FALSE;
if (dm->dmSize < FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields) || dm->dmSize + dm->dmDriverExtra > size) return FALSE;

for (i = 0; i < ARRAY_SIZE(map); i++)
    if ((dm->dmFields & map[i].flag) && dm->dmSize < map[i].size)
        return FALSE;

return TRUE;
```

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

[Tingzhong Luo (＠tzluo)]

On Thu Feb 23 12:16:53 2023 +0000, Piotr Caban wrote:
> It's not safe to check `dm->dmSize` field before validating that `size`
> is big enough so we can read it. Also there needs to be enough room for
> `dm->dmSize + dm->dmDriverExtra`. Current implementation will also
> behave incorrectly if `dm->dmSize` is 1 (it should also fail in this case).
> I think about something along these lines (not tested):
> ```c
> if (!dm) return FALSE;
> if (size < FIELD_OFFSET(DEVMODEW, dmFields)) return FALSE;
> if (dm->dmSize < FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields)
> || dm->dmSize + dm->dmDriverExtra > size) return FALSE;
> for (i = 0; i < ARRAY_SIZE(map); i++)
>     if ((dm->dmFields & map[i].flag) && dm->dmSize < map[i].size)
>         return FALSE;
> return TRUE;
> ```
> Yep, this implementation is incorrect when `dm.dmSize` is one. Then I try to enumerate the combination of `dmSize` and `dmDriverExtra` in unit test:

```c
memset(&dm, 0, sizeof(dm));
dm.dmFields = 0;
for (i = 0; i < 50; i++)
{
    dm.dmSize = FIELD_OFFSET(DEVMODEW, dmFields) + i;
    for (dm.dmDriverExtra = 0; dm.dmDriverExtra < 100; dm.dmDriverExtra++)
    {
        ret = IsValidDevmodeW(&dm, 200);
        ok(!ret, "dmSize %d, dmDriverExtra %d, ret %d\n", dm.dmSize, dm.dmDriverExtra, ret);
    }
}
```

Then run it in Windows 10: 

```
info.c:3076: Test failed: dmSize 76, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 76, dmDriverExtra 1, ret 1
...
info.c:3076: Test failed: dmSize 76, dmDriverExtra 99, ret 1
info.c:3076: Test failed: dmSize 77, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 78, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 79, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 80, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 80, dmDriverExtra 1, ret 1
info.c:3076: Test failed: dmSize 80, dmDriverExtra 2, ret 1
...
info.c:3076: Test failed: dmSize 80, dmDriverExtra 98, ret 1
info.c:3076: Test failed: dmSize 80, dmDriverExtra 99, ret 1
info.c:3076: Test failed: dmSize 81, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 82, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 83, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 84, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 84, dmDriverExtra 1, ret 1
info.c:3076: Test failed: dmSize 84, dmDriverExtra 2, ret 1
...
info.c:3076: Test failed: dmSize 84, dmDriverExtra 98, ret 1
info.c:3076: Test failed: dmSize 84, dmDriverExtra 99, ret 1
info.c:3076: Test failed: dmSize 85, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 86, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 87, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 88, dmDriverExtra 0, ret 1
,,,
info.c:3076: Test failed: dmSize 113, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 114, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 115, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 116, dmDriverExtra 0, ret 1
info.c:3076: Test failed: dmSize 116, dmDriverExtra 1, ret 1
info.c:3076: Test failed: dmSize 116, dmDriverExtra 2, ret 1
...
info.c:3076: Test failed: dmSize 116, dmDriverExtra 83, ret 1
info.c:3076: Test failed: dmSize 116, dmDriverExtra 84, ret 1
...
```

The result is interesting, when `dmSize` is less than 76 (value of `FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields)`), `IsValidDevmodeW()` should return false. Then it only receive `dmDriverExtra` every four times. So the implementation to match the behavior of Windows might looks like this:

```c
    const DWORD offFields = FIELD_OFFSET(DEVMODEW, dmFields) + sizeof(dm->dmFields);

    if (!dm) 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;
```

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