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

[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):
> 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;
```