[PATCH v5 0/1] MR2262: winspool: Check dmSize in IsValidDevmodeW
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
From: Tingzhong Luo <luotingzhong(a)uniontech.com> --- 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) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2262
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?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2262#note_25377
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:
if ((dm->dmFields & map[i].flag) && dm->dmSize < map[i].size)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2262#note_25390
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2262#note_25395
participants (4)
-
Huw Davies (@huw) -
Piotr Caban (@piotr) -
Tingzhong Luo -
Tingzhong Luo (@tzluo)