I've seen like 4 people change this stuff recently and not one of them got it 100% right. So here are the rules for how to use probes and SEH: * Probe pointers before accessing them. This ensures you're not reading kernel mode memory (and does pretty much nothing else) * Never access user mode pointers outside of SEH. The application controls its address space, so protection, physical memory mapping and contents can change out from under you at any point. For most purposes, MmProbeAndLockPages is the only way to get around this. * Sometimes there is no alternative to copying data to kernel memory. If there is a pointer stored in user mode memory, you need a copy of it because you can't rely on it not changing after your probe (even if you lock down the memory). * In case of an exception you usually want to return the exception code. There are exceptions (heh) to this rule, but if you think you've found one, better be sure and show it with a test. * If you make a pool allocation whose size is directly influenced by user mode input, there's a good chance you'll want to use ExAllocatePoolWithQuotaTag
In particular this means: * Probing something or accessing it once does not save you from using SEH every single time you access the user mode memory * Probing UserModePointer->StructMemberPointer is never correct, you need to make a copy of the struct or that particular member, since the memory can change * The ProbeForReadUnicodeString function is not useful unless you use its return value Hope this helps. Thanks for hanging in there. -T On 2016-09-11 18:13, dchapys...@svn.reactos.org wrote: > Author: dchapyshev > Date: Sun Sep 11 16:13:46 2016 > New Revision: 72655 > > URL: http://svn.reactos.org/svn/reactos?rev=72655&view=rev > Log: > [NtUser] Try to fix tests for user32:EnumDisplaySettings > > Modified: > trunk/reactos/win32ss/user/ntuser/display.c > > Modified: trunk/reactos/win32ss/user/ntuser/display.c > URL: > http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/display.c?rev=72655&r1=72654&r2=72655&view=diff > ============================================================================== > --- trunk/reactos/win32ss/user/ntuser/display.c [iso-8859-1] (original) > +++ trunk/reactos/win32ss/user/ntuser/display.c [iso-8859-1] Sun Sep 11 > 16:13:46 2016 > @@ -443,7 +443,7 @@ > { > /* No device found */ > ERR("No PDEV found!\n"); > - return STATUS_UNSUCCESSFUL; > + return STATUS_INVALID_PARAMETER_1; > } > > *ppdm = ppdev->pdmwDev; > @@ -474,7 +474,7 @@ > { > /* No device found */ > ERR("No device found!\n"); > - return STATUS_UNSUCCESSFUL; > + return STATUS_INVALID_PARAMETER_1; > } > > iFoundMode = 0; > @@ -571,13 +571,18 @@ > > _SEH2_TRY > { > - ProbeForWrite(lpDevMode, sizeof(DEVMODEW), 1); > + ProbeForRead(lpDevMode, sizeof(DEVMODEW), 1); > + > + cbSize = lpDevMode->dmSize; > + cbExtra = lpDevMode->dmDriverExtra; > + > + ProbeForWrite(lpDevMode, cbSize + cbExtra, 1); > } > _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER) > { > _SEH2_YIELD(return _SEH2_GetExceptionCode()); > } > - _SEH2_END > + _SEH2_END; > > if (lpDevMode->dmSize != sizeof(DEVMODEW)) > { > @@ -586,31 +591,30 @@ > > if (pustrDevice) > { > - if (pustrDevice->Buffer == NULL || pustrDevice->Length == 0) > - { > - Status = STATUS_INVALID_PARAMETER_1; > - } > - > /* Initialize destination string */ > RtlInitEmptyUnicodeString(&ustrDevice, awcDevice, sizeof(awcDevice)); > > _SEH2_TRY > { > /* Probe the UNICODE_STRING and the buffer */ > - ProbeForRead(pustrDevice, sizeof(UNICODE_STRING), 1); > - ProbeForRead(pustrDevice->Buffer, pustrDevice->Length, 1); > + ProbeForReadUnicodeString(pustrDevice); > + > + if (!pustrDevice->Length || !pustrDevice->Buffer) > + ExRaiseStatus(STATUS_NO_MEMORY); > + > + ProbeForRead(pustrDevice->Buffer, pustrDevice->Length, > sizeof(UCHAR)); > > /* Copy the string */ > RtlCopyUnicodeString(&ustrDevice, pustrDevice); > } > _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER) > { > - _SEH2_YIELD(return _SEH2_GetExceptionCode()); > - } > - _SEH2_END > + _SEH2_YIELD(return STATUS_INVALID_PARAMETER_1); > + } > + _SEH2_END; > > pustrDevice = &ustrDevice; > - } > + } > > /* Acquire global USER lock */ > UserEnterShared(); > @@ -642,11 +646,6 @@ > /* Copy some information back */ > _SEH2_TRY > { > - ProbeForRead(lpDevMode, sizeof(DEVMODEW), 1); > - cbSize = lpDevMode->dmSize; > - cbExtra = lpDevMode->dmDriverExtra; > - > - ProbeForWrite(lpDevMode, cbSize + cbExtra, 1); > /* Output what we got */ > RtlCopyMemory(lpDevMode, pdm, min(cbSize, pdm->dmSize)); > > @@ -663,13 +662,6 @@ > Status = _SEH2_GetExceptionCode(); > } > _SEH2_END; > - } > - else > - { > - if (Status == STATUS_UNSUCCESSFUL) > - { > - Status = STATUS_INVALID_PARAMETER_1; > - } > } > > return Status; > > _______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
