Handling PnP root driver power IRPs requires that a device object must come up
with a device extension to determine whether it is a function driver and if so,
handle the IRP accordingly.
CORE-18989
- Add missing ExAllocatePool NULL checks.
- Fix order of KeBugCheckEx parameters for PNP_DETECTED_FATAL_ERROR.
- The Controller and Peripheral numbers are zero-based, so if the caller
wants to inspect controller (or peripheral) zero, let it be so!
The original code was treating controller number zero for enumerating
controllers of a given class within the different buses, which is
wrong. See the diff'ed trace below.
Tested with Windows' videoprt.sys VideoPortGetDeviceData().
```diff
IoQueryDeviceDescription()
BusType: 0xB093C224 (0)
BusNumber: 0xB093C228 (0)
ControllerType: 0xF9D01030 (19)
ControllerNumber: 0xF9D01038 (0)
PeripheralType: 0x00000000 (4294967295)
PeripheralNumber: 0x00000000 (4294967295)
CalloutRoutine: 0xF9CF74E4
Context: 0xF9D5A340
--> Query: 0xF9D5A22C
IopQueryBusDescription(Query: 0xF9D5A22C)
RootKey: '\REGISTRY\MACHINE\HARDWARE\DESCRIPTION\SYSTEM'
RootKeyHandle: 0x00000598
KeyIsRoot: TRUE
Bus: 0xF9D5A290 (4294967295)
Seen: 'CentralProcessor'
Seen: 'FloatingPointProcessor'
Seen: 'MultifunctionAdapter'
SubRootRegName: '\REGISTRY\MACHINE\HARDWARE\DESCRIPTION\SYSTEM\MultifunctionAdapter'
IopQueryBusDescription(Query: 0xF9D5A22C)
RootKey: '\REGISTRY\MACHINE\HARDWARE\DESCRIPTION\SYSTEM\MultifunctionAdapter'
RootKeyHandle: 0x00000590
KeyIsRoot: FALSE
Bus: 0xF9D5A290 (4294967295)
Seen: '0'
SubRootRegName: '\REGISTRY\MACHINE\HARDWARE\DESCRIPTION\SYSTEM\MultifunctionAdapter\0'
Getting bus value: 'Identifier'
Getting bus value: 'Configuration Data'
Getting bus value: 'Component Information'
--> Getting device on Bus #0 : '\REGISTRY\MACHINE\HARDWARE\DESCRIPTION\SYSTEM\MultifunctionAdapter\0'
IopQueryDeviceDescription(Query: 0xF9D5A22C)
RootKey: '\REGISTRY\MACHINE\HARDWARE\DESCRIPTION\SYSTEM\MultifunctionAdapter\0'
RootKeyHandle: 0x00000590
Bus: 0
- Enumerating controllers in '\REGISTRY\MACHINE\HARDWARE\DESCRIPTION\SYSTEM\MultifunctionAdapter\0\DisplayController'...
+ Getting controller #0
+ Retrieving controller '\REGISTRY\MACHINE\HARDWARE\DESCRIPTION\SYSTEM\MultifunctionAdapter\0\DisplayController\0'
```
Based on a commit by Vadim Galyant:
5ef5c11e7f
Also fix a minor type conversion warning. CORE-18963 CORE-17977
Co-authored-by: Vadim Galyant <vgal@rambler.ru>
We should compare against DeviceObject as DeviceInstance is never NULL.
Fix a resource leak as well. The bug CORE-18983 seems to lay somewhere
else though, I just stumbled upon this one while researching it.
Note there is a BSOD in the PnP manager on reboot after the driver
installation failure, but it seems it was uncovered by the fix
as opposed to caused by it.
- Refactor most of the code, since there's quite some stuff that don't make much sense.
For instance ImpersonationLevel is basically the requested impersonation level a
server asks for. PsImpersonateClient doesn't explicitly say that SecurityAnonymous
and SecurityIdentification are not allowed. If the server was to give such levels
it simply means it doesn't want to impersonate the client.
Another thing that doesn't make much sense is that we check if the client is
associated with an anonymous token, then avoid impersonating regular anonymous
tokens that weren't created by the system. Only system can create such tokens
and an anonymous token basically means a token with hidden security info.
- Check that the server is within the same client logon session.
- If the server is granted the SeImpersonatePrivilege privilege, allow impersonation
regardless of the conditions we want to check for.
- Update the documentation and code comments.
As it currently stands the PsImpersonateClient routine does the following approach.
If impersonation couldn't be granted to a client the routine will make a copy
of the client's access token. As it makes a copy of the said token PsImpersonateClient
will reference the copied token after impersonation info have been filled out.
In the same code path we are assigning the desired level for impersonation to thread
impersonation info.
This is wrong for two reasons:
- On a copy situation the SeCopyClientToken routine holds a reference as the object
has been created. Referencing it at the bottom of the PsImpersonateClient routine
will make it that the token is referenced twice and whenever a server stops
impersonation the token still has an extra reference count which keeps the token
still alive in object database and memory space.
- If client impersonation is not possible the thread impersonation info should
have been assigned SecurityIdentification level to further indicate that the
actual impersonation of the thread is not currently in force but instead we
are assigning the impersonation level that is supplied by the caller. For instance
if the requested level is SecurityDelegation but impersonation is not possible
the level will be assigned that of SecurityDelegation yet the token has an
impersonation level of SecurityIdentification. This could lead to erratic behaviors
as well as potential impersonation escalation.
Fix the aforementioned issues by avoiding a double reference and properly assign
the impersonation level to SecurityIdentification if the server is not able to
impersonate the target client.
- Add the missing privileges to the SYSTEM privileges which might be needed,
notably SeUndockPrivilege, SeManageVolumePrivilege, SeCreateGlobalPrivilege and
SeImpersonatePrivilege.
Specifically SeImpersonatePrivilege is important here because with it we
allow system components of the core OS to perform certain system tasks.
- Declare the Groups array with a maximum of 3 elements in SepCreateSystemProcessToken
and 1 element in SepCreateSystemAnonymousLogonToken respectively, because previously
this array was oversized with most of free space left as a waste.
- Avoid hardcoding the size value of the Privilege array, instead initialize it
by hand and compute the exact number of elements with RTL_NUMBER_OF.
- Fix whitespace; add SAL annotations, doxygen documentation...
- Deduplicate the array of description strings corresponding to
IO_QUERY_DEVICE_DATA_FORMAT.
- Unhardcode the "[3]" into 'IoQueryDeviceMaxData': the maximum number
of device data queried.
- Wrap most of the code into a new private routine, SepOpenThreadToken.
And properly fail gracefully if we fail to open a thread's token instead of just keeping going.
- Do not use the same thread object that we have referenced in NtOpenThreadTokenEx
to do a copy of the access token in case we can't open it directly.
Instead we must reference a new object with full access, solely used for
the purpose to do our required operations.
- Add debug prints
CORE-18986
Removing any disabled privileges or groups in the middle of token dynamic
part allocation can pose problems. During the operation of making an access
token as effective, we are toying with the privileges and groups arrays
of the token.
After that we are allocating the dynamic part and set EndMem (the end tail
of the memory part) to that dynamic part, previously it was set to the
variable part. As a matter of fact we are making the token effective in
the middle where EndMem still points to VariablePart, thus DynamicPart
will end up with memory pool blocks butchered in the pool list.
Another problem, albeit not related to the DynamicPart corruption, is that
the code starts iterating over the UserAndGroups array from 0, which is
the actual user. One cannot simply remove the user from the array, so we
have to start looping right from the groups.
Move the token effective code part at the end of the SepDuplicateToken
function, which fixes the random pool corruptions caused by the butchered
DynamicPart.
CORE-18986
CORE-18962
- Deduplicate a while-loop by adding one more recursive call.
- Add IopMapDetectedDeviceId() helper function with a structure
in order to reduce hardcoded constants and checks.
- Do not allocate a new stack, if the thread already has a large one. This prevents the function from freeing a large stack as a normal stack and subsequently leaking system PTEs.
- Fix the check for failure of PsConvertToGuiThread (test eax, not rax, for being negative, because by default rax is zero extended from eax, not sign extended). This fixes an infinite loop on failure.
The data has to be written into ObjectTypeInfo based on the return length,
not only what is provided by the input buffer length. Fix suggested by
Hermès.
On a x86 system aligning the return length pointer to a 4-byte boundary
works best since pointers in general are 4-byte aligned for x86 systems.
However, what happens on a AMD64 system is that we still align this pointer
to 4-byte, ObjectTypeInfo is a 8-byte pointer and we might write into
the return length past the 4-byte boundary.
If one were to allocate a pool of memory with that length and query all
the object types info and free the said pool of memory thereafter, the
system will crash with BAD_POOL_HEADER because ObQueryTypeInfo overwrote
the return length past the 4-byte boundary length therefore leading up with
corrupted memory blocks in the pool header.
This symptom of BAD_POOL_HEADER happens exactly the same in Windows Server
2003 x64 Edition. Newer versions of Windows like 10 aren't affected.
But, Windows has another bug where they are using MaximumLength for the
calculation of the needed length to be returned to caller. MaximumLength
does not guarantee you that it includes the NULL-terminator in the length
and that potentially leads to a buffer overrun.
Also annotate the ObQueryTypeInfo function with SAL2.
https://processhacker.sourceforge.io/doc/object_8c_source.html (read the
comment in KphObjectTypeInformation).
Second parameter is optional, so mark it as such and check whether it was passed. Fixes a sporadic 0x24 bugcheck caused by access violation when running ReactOS on NTFS volume with WinXP ntfs.sys.
Finally handlers are - unlike except blocks - not part of the function they are in, but separate functions, which are called during unwind. PSEH implements them on GCC using nested functions. While "return" from a finally handler is allowed with native SEH, it's handled by the compiler through an extra unwinding operation using _local_unwind, WHICH IS NOT SUPPORTED BY PSEH! With PSEH, returning from a finally handler does not return from the function, instead it will only return from the finally handler and the function will continue below the finally handler as if there was no return at all. To fix this, the return is removed and an additional success check is added.
Also use _SEH_VOLATILE to make sure the variable assignment is not optimized away by the compiler and add zero out the result parameters on error.
... that would otherwise cause a debugger re-entry.
Also use KdbPuts/Printf instead of KdpDprintf that won't be available
once KDBG is moved out of it.
... since the original ones are internal to the kernel and won't be
available once KDBG is moved out of it.
Use these functions in the pager/prompt support.
The built string can be:
°°Kernel Debugger: Serial port found: COM1 (Port 0x000003F8) BaudRate 115200°°°°
(with ° representing the \r and \n in the message)
and you can verify that this is more than 80 characters in total.
CORE-17627
When closing a file, fastfat zeroes it out from ValidDataLength up to the end of the file.
The ValidDataLength field is updated when the file content is actually written to disk.
There is currently a race between the file-close path and the page out path, leading to potential file corruptions when the zeroing happens after the memory has been flushed to disk.
Fix this by actually flushing the file to disk when unmapping files, with file lock acquired. This way, the FS driver cannot zero out the tail of the file while we're actually flushing it to disk.
This one was more subtle because the prompt (KdIoReadLine) functionality
makes a call-back to KDBG own command history getter function KdbGetHistoryEntry.
It is planned for this to become a registered optional callback pointer.
This is done in preparation for moving all this functionality in a
separate KDTERM "KD Terminal Driver" DLL.
Additionally:
- Flush the terminal input before sending ANSI escape sequences.
- In KDBG pager, always use the correct reading-key function (the
same used also for reading keys for a line of user input), and not
the simplistic two-call KdbpGetCharSerial + KdbpTryGetCharSerial
that would split the \r \n across calls.
- Call KdbpGetCommandLineSettings() in KdbInitialize() at BootPhase 0,
which is indirectly called by KdDebuggerInitialize0(). And fix its
command-line parsing too.
Rename KdbpReadCommand as KdIoReadLine. Extract the last-command
repetition functionality out of KdIoReadLine and put it where it
belongs: only in the KDBG command main loop KdbpCliMainLoop.
Use this function instead of KdpDprintf(), otherwise, we send them to
**ALL** the display providers, including for example dmesg. Replaying
the listing with dmesg would then cause the terminal to misbehave later.
For example, it would send the answer of a "Query Device Attributes"
command, as the response to a query for terminal size...
Addendum to commit 84e32e4e.
Explain more accurately what's going on regarding the returned string
and the inaccurate claims made in the official DbgPrompt documentation
in MSDN. (Has been verified by looking through the traffic in WinDbg
debugging of Windows and ReactOS.)
Of course, now that we **correctly** set the LoadSymbools setting,
we attempt loading symbols at BootPhase 0 and everything goes awry!
So introduce that hack to fallback to our old behaviour.
A proper fix (and removal of the hack) will be done in future commits.
Addendum to commit de892d5b.
The boot options get stripped of their optional command switch '/'
(and replaced by whitspace separation) by the NT loader. Also, forbid
the presence of space between the optional '=' character following
(NO)LOADSYMBOLS.
In addition, fix the default initialization of LoadSymbols in KdbSymInit():
we cannot rely on MmNumberOfPhysicalPages in BootPhase 0 since at this point,
the Memory Manager hasn't been initialized and this variable is not yet set.
(We are called by KdInitSystem(0) -> KdDebuggerInitialize0 at kernel init.)
It gets initialized later on between BootPhase 0 and 1.
Also display a nice KDBG signon showing the status of symbols loading.
LoadSymbols was reset to its default value whenever KdbSymInit() was
called, thus we would e.g. load symbols even if /NOLOADSYMBOLS or
/LOADSYMBOLS=NO were specified at the command line.
CORE-17470
+ KdpDebugLogInit: Add resources cleanup in failure code paths.
Fix, in an NT-compatible manner, how (and when) the KD/KDBG BootPhase >=2
initialization steps are performed.
These are necessary for any functionality KDBG needs, that would depend
on the NT I/O Manager and the storage and filesystem stacks to be running.
This includes, creating the debug log file, and for KDBG, loading its
KDBinit initialization file.
As a result, file debug logging is fixed.
The old ReactOS-specific (NT-incompatible) callback we did in the middle
of IoInitSystem() is removed, in favor of a runtime mechanism that should
work on Windows as well.
The idea for this new mechanism is loosely inspired by the TDL4 rootkit,
see http://blog.w4kfu.com/public/tdl4_article/draft_tdl4article.html
but contrary to it, a specific hook is used instead, as well as the
technique of driver reinitialization:
https://web.archive.org/web/20211021050515/https://driverentry.com.br/en/blog/?p=261
Its rationale is as follows:
We want to be able to perform I/O-related initialization (starting a
logger thread for file log debugging, loading KDBinit file for KDBG,
etc.). A good place for this would be as early as possible, once the
I/O Manager has started the storage and the boot filesystem drivers.
Here is an overview of the initialization steps of the NT Kernel and
Executive:
----
KiSystemStartup(KeLoaderBlock)
if (Cpu == 0) KdInitSystem(0, KeLoaderBlock);
KiSwitchToBootStack() -> KiSystemStartupBootStack()
-> KiInitializeKernel() -> ExpInitializeExecutive(Cpu, KeLoaderBlock)
(NOTE: Any unexpected debugger break will call KdInitSystem(0, NULL); )
KdInitSystem(0, LoaderBlock) -> KdDebuggerInitialize0(LoaderBlock);
ExpInitializeExecutive(Cpu == 0): ExpInitializationPhase = 0;
HalInitSystem(0, KeLoaderBlock); <-- Sets HalInitPnpDriver callback.
...
PsInitSystem(LoaderBlock)
PsCreateSystemThread(Phase1Initialization)
Phase1Initialization(Discard): ExpInitializationPhase = 1;
HalInitSystem(1, KeLoaderBlock);
...
Early initialization of Ob, Ex, Ke.
KdInitSystem(1, KeLoaderBlock);
...
KdDebuggerInitialize1(LoaderBlock);
...
IoInitSystem(LoaderBlock);
...
----
As we can see, KdDebuggerInitialize1() is the last KD initialization
routine the kernel calls, and is called *before* the I/O Manager starts.
Thus, direct Nt/ZwCreateFile ... calls done there would fail. Also,
we want to do the I/O initialization as soon as possible. There does
not seem to be any exported way to be notified about the I/O manager
initialization steps... that is, unless we somehow become a driver and
insert ourselves in the flow!
Since we are not a regular driver, we need to invoke IoCreateDriver()
to create one. However, remember that we are currently running *before*
IoInitSystem(), the I/O subsystem is not initialized yet. Due to this,
calling IoCreateDriver(), much like any other IO functions, would lead
to a crash, because it calls
ObCreateObject(..., IoDriverObjectType, ...), and IoDriverObjectType
is non-initialized yet (it's NULL).
The chosen solution is to hook a "known" exported callback: namely, the
HalInitPnpDriver() callback (it initializes the "HAL Root Bus Driver").
It is set very early on by the HAL via the HalInitSystem(0, ...) call,
and is called early on by IoInitSystem() before any driver is loaded,
but after the I/O Manager has been minimally set up so that new drivers
can be created.
When the hook: KdpInitDriver() is called, we create our driver with
IoCreateDriver(), specifying its entrypoint KdpDriverEntry(), then
restore and call the original HalInitPnpDriver() callback.
Another possible unexplored alternative, could be to insert ourselves
in the KeLoaderBlock->LoadOrderListHead boot modules list, or in the
KeLoaderBlock->BootDriverListHead boot-driver list. (Note that while
we may be able to do this, because boot-drivers are resident in memory,
much like we are, we cannot insert ourselves in the system-driver list
however, since those drivers are expected to come from PE image files.)
Once the KdpDriverEntry() driver entrypoint is called, we register
KdpDriverReinit() for re-initialization with the I/O Manager, in order
to provide more initialization points. KdpDriverReinit() calls the KD
providers at BootPhase >= 2, and schedules further reinitializations
(at most 3 more) if any of the providers request so.
CORE-10749
The dmesg command is now available even if screen output is disabled.
Co-authored-by: Hermès Bélusca-Maïto <hermes.belusca-maito@reactos.org>
- KdbSymInit() in kdb_symbols.c only initializes symbols implementation
support.
- The rest of KdbInitialize gets moved into kdb_cli.c and initializes
the KDBG debugger itself.
- Move KdbDebugPrint to kdb_cli.c as well.
Access check is an expensive operation, that is, whenever an access to an
object is performed an access check has to be done to ensure the access
can be allowed to the calling thread who attempts to access such object.
Currently SepAnalyzeAcesFromDacl allocates a block of pool memory for
access check rights, nagging the Memory Manager like a desperate naughty
creep. So instead initialize the access rights as a simple variable in
SepAccessCheck and pass it out as an address to SepAnalyzeAcesFromDacl so
that the function will fill it up with access rights. This helps with
performance, avoiding wasting a few bits of memory just to hold these
access rights.
In addition to that, add a few asserts and fix the copyright header on
both se.h and accesschk.c, to reflect the Coding Style rules.
This mutes a lot of debug spam that fills up the debugger when an access
check fails because a requestor doesn't have enough privileges to access
an object.
MmLoadSystemImage has a PUNICODE_STRING NamePrefix parameter which is
currently unused in ReactOS. When the kernel loads the crash dump
storage stack drivers, the drivers will be loaded with MmLoadSystemImage
with a "dump_" or "hiber_" (for hibernation, which uses crash dump
stack too) prefix. This change adds in the prefix support, and is
supposed to push crash dump support forward.
CORE-376
- Use SAL2 annotations.
- KdSendPacket(): Validate DEBUG_IO API call.
- KdReceivePacket(): Take the LengthOfStringRead into account; use
KdbpReadCommand() to read the input, so that correct line edition
is available (backspace, etc.)
- Don't read anything and return immediately if the buffer size is zero.
- Allow the controlling keys (up/down arrows, backspace) even if the
buffer is full! (especially backspace if too much has been written).
- Return the number of characters stored, not counting the NULL terminator.
Currently the failure code path doesn't do any kind of cleanup against
the hive that was being linked to master. The cleanup is pretty
straightforward as you just simply close the hive file handles and free
the registry kernel structures.
CORE-5772
CORE-17263
CORE-13559
Log debug output of the lazy flusher as much information as possible so that we can examine how does the lazy flusher behave, since it didn't work for almost a decade. Verbose debugging will be disabled once we're confident enough the registry implementation in ReactOS is rock solid.
Whenever ReactOS finishes its operations onto the registry and unlocks it, a lazy flush is invoked to do an eventual flushing of the registry to the backing storage of the system. Except that... lazy flushing never comes into place.
This is because whenever CmpLazyFlush is called that sets up a timer which needs to expire in order to trigger the lazy flusher engine worker. However, registry locking/unlocking is a frequent occurrence, mainly when on desktop. Therefore as a matter of fact, CmpLazyFlush keeps removing and inserting the timer and the lazy flusher will never kick in that way.
Ironically the lazy flusher actually does the flushing when on USETUP installation phase because during text-mode setup installation in ReactOS the frequency of registry operations is actually less so the timer has the opportunity to expire and fire up the flusher.
In addition to that, we must queue a lazy flush when marking cells as dirty because such dirty data has to be flushed down to the media storage of the system. Of course, the real place where lazy flushing operation is done should be in a subset helper like HvMarkDirty that marks parts of a hive as dirty but since we do not have that, we'll be lazy flushing the registry during cells dirty marking instead for now.
CORE-18303
Addendum to commit de81021ba.
Otherwise, we get the following build error:
\ntoskrnl\kd64\kddata.c(532,5): error: initializer element is not a compile-time constant
PtrToUL64(RtlpBreakWithStatusInstruction),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
\ntoskrnl\kd64\kddata.c(526,26): note: expanded from macro 'PtrToUL64'
#define PtrToUL64(x) ((ULPTR64)(x))
^~~~~~~~~~~~
If you ask why there are two sets of functions that do the same, it's
because this file (and the kdmain.c) will very soon some day be moved to
a transport dll, outside the kernel, and it will need these functions.
See this command's documentation:
https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/-dbgprint
and the section "DbgPrint buffer and the debugger"
https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/reading-and-filtering-debugging-messages#dbgprint-buffer-and-the-debugger
for more details.
- Loosely implement the function, based on our existing circular printout
buffers in kdio.c.
- Enable its usage in the KdpPrint() and KdpPrompt() functions.
Notice that this function will *only* capture the strings being sent **to**
the debugger, and not the strings the debugger itself produce. (This means
that we cannot use the KdPrintCircularBuffer as a replacement for our
KDBG dmesg one, for example...)
How to test:
Run ReactOS under WinDbg, and use the !dbgprint command to view the
buffer. You can also use the Memory Window, place yourself at the
address pointed by KdPrintCircularBuffer and KdPrintWritePointer, and
read its contents.
What you should observe:
Prior notice: The circular buffer in debug builds of ReactOS and Windows
is 0x8000 bytes large. In release builds, its size is down to 0x1000.
1- When you start e.g. the 2nd-stage GUI installation of ReactOS, going
past the initial "devices installation" and letting it stabilize on
the Welcome page, break into WinDbg and run the !dbgprint command. You
should notice that the end of its output is weirdly truncated, compared
to what has been actually emitted to the debug output. Comparing this
with the actual contents of the circular buffer (via Memory Window),
shows that the buffer contents is actually correct.
2- Copy all the text that has been output by the !dbgprint command and
paste it in an editor; count the number of all characters appearing +
newlines (only CR or LF), and observe that this number is "mysteriously"
equal to 16384 == 0x4000.
3- Continue running ReactOS installation for a little while, breaking back
back into WinDbg and looking at !dbgprint again. Its output seems to be
still stopping at the same place as before (but the actual buffer memory
contents shows otherwise). Continue running ROS installation, and break
into the debugger when ROS is about to restart. You should now observe
that the dbgprint buffer rolled over:
dd nt!KdPrintRolloverCount shows 1.
Carefully analysing the output of !dbgprint, however, you will notice
that it looks a bit garbage-y: the first part of the output is actually
truncated after 16384 characters, then you get a second part of the
buffer showing what ReactOS was printing while shutting down. Then
you get again what was shown at the top of the !dbgprint output.
(Of course, comparing with the actual contents of the circular buffer
in memory shows that its contents are fine...)
The reason of these strange observations, is because there is an intrinsic
bug in the !dbgprint command implementation (in kdexts.dll). Essentially,
it displays the contents of the circular buffer in two single dprintf()
calls: one for the "older" (bottom) part of the buffer:
[WritePointer, EndOfBuffer]
and one for the "newer" (upper) part of the buffer:
[CircularBuffer, WritePointer[ .
The first aspect of the bug (causing observation 3), is that those two
parts are not necessarily NULL-terminated strings (especially after
rollover), so for example, displaying the upper part of the buffer, will
potentially also display part of the buffer's bottom part.
The second aspect of the bug (explaining observations 1 and 2), is due
to the implementation of the dprintf() function (callback in dbgenv.dll).
There, it uses a fixed-sized buffer of size 0x4000 == 16384 characters.
Since the output of the circular buffer is not done by little chunks,
but by the two large parts, if any of those are larger than 0x4000 they
get truncated on display.
(This last observation is confirmed in a completely different context by
https://community.osr.com/discussion/112439/dprintf-s-max-string-length .)
But the underlying GCC stupidity is still there (15 years later).
However, enable it only in 32-bit GCC builds, not in 64-bits nor with MSVC.
See commit b9cd3f2d9 (r25845) for some details.
GCC is indeed still incapable of casting 32-bit pointers up to 64-bits,
when static-initializing arrays (**outside** a function) without emitting
the error:
"error: initializer element is not constant"
(which might somehow indicate it actually tries to generate executable
code for casting the pointers, instead of doing it at compile-time).
Going down the rabbit hole, other stupidities show up:
Our PVOID64 type and the related POINTER_64 (in 32-bit archs), or the
PVOID32 and POINTER_32 (in 64-bit archs), are all silently broken in
GCC builds, because the pointer size attributes __ptr64 and __ptr32,
which are originally MSVC-specific, are defined to nothing in _mingw.h.
(And similarly for the __uptr and __sptr sign-extension attributes.)
Clang and other sane ompilers has since then implemented those (enabled
with -fms-extensions), but not GCC. The closest thing that could exist
for GCC is to do:
#define __ptr64 __attribute__((mode(DI)))
in order to get a 64-bit-sized pointer type with
typedef void* __ptr64 PVOID64;
but even this does not work, with the error:
"error: invalid pointer mode 'DI'"
Choose the correct element of the KiUnexpectedRange array,
depending on the interrupt vector, the same way as here:
a2c6af0da4/ntoskrnl/ke/amd64/except.c (L77)
And guard KeConnectInterrupt() execution with dispatcher lock.
CORE-14922
- Line-wrapping is enabled with 'ESC[?7h' (the '?' was forgotten).
Notice that the following reference also shows it wrong:
https://www.cse.psu.edu/~kxc104/class/cse472/09s/hw/hw7/vt100ansi.htm
- Terminal type is actually queried with 'ESC Z' (VT52-compatible), or
with 'ESC[c' (VT100-compatible). The antediluvian CTRL-E ('\x05')
control code gives instead a user-configurable (usually empty) string,
so it's not reliable.
Also, we don't really care about the returned result, we just need to
know that one has been sent.
Cross-checked with online documentation:
* "Digital VT100 User Guide" (EK-VT100-UG-001) (1st edition, August 1978,
reviewed November 1978).
* https://www.real-world-systems.com/docs/ANSIcode.html
* https://geoffg.net/Downloads/Terminal/TerminalEscapeCodes.pdf
* https://invisible-island.net/xterm/ctlseqs/ctlseqs.html
* https://en.wikipedia.org/wiki/Enquiry_character
- Retrieve the size of the *controlling terminal* with escape sequences
only when it's a serial one: serial output is enabled *and* KDSERIAL
is set (i.e. user input through serial). See code for the actual logic
(the corresponding truth table is left as an exercise for the reader).
- Fix also a "Buffer" vs. "InBuffer" mismatch, that caused the whole
code to fail.
- For fallback terminal size values, use meaningful ones when SCREEN
is instead the controlling "terminal" (based on full-size BOOTVID
values).
- When echoing read characters during line-cooking, do direct output by
using KdpDprintf() instead of going through the heavy KdbpPrint() function.
This fixes some input artifacts like: 1. extra slowdowns due to
querying terminal size at each keypress, and 2. getting obnoxious
"--- Press q to abort ..." prompts in the middle of typing a long
comamnd because you are at the very bottom of the screen.
- Remove KdbInit() macro and directly use KdbpCliInit() (since the place
where it was used was already within an #ifdef KDBG block).
- Declare KdpKdbgInit() only when KDBG is defined, move its definition
into kdio.c and remove the legacy wrappers/kdbg.c file.
And in KdbInitialize(), set KdpInitRoutine directly to the former,
instead of using the KdpKdbgInit indirection.
- Don't reset KdComPortInUse in KdpDebugLogInit().
- Minor refactorings: KdpSerialDebugPrint -> KdpSerialPrint and make it
static; argument name "Message" -> "String", "StringLength" -> "Length".
What we have:
- Maximum number of pagefiles: 16
- Minimum pagefile size: 256 pages (1 MB when page size = 4096 bytes)
- Maximum pagefile size:
* 32-bit platforms: (1024 * 1024 - 1) pages (~ 4095 MB)
* x86 with PAE support: same size as for AMD x64
* x64 platform: (4 * 1024 * 1024 * 1024 - 1) pages (~ 16 TB)
* IA64 platform: (8 * 1024 * 1024 * 1024 - 1) pages (~ 32 TB)
Those are the values as supported and verified by the NT kernel.
Now, user-mode programs (including SMSS.EXE) have different opinions
on these, namely, they consider estimates directly in MB, respectively:
4095 MB, (16 * 1024 * 1024) MB, and (32 * 1024 * 1024) MB
(verified on Win2k3 and Win7 32 and 64 bits).
Also here, the minimum pagefile size is set to 2 MB.
Starting Windows 8+ (and 10), those values change slightly, and are
still not fully synchronized between NTOS:MM and SMSS. Finally, while
(x86 PAE and) AMD64 and ARM64 seem to share the maximum pagefile
size limit, 32-bit ARMv7 appears to use different limits than regular
x86 (2 GB instead of 4).
Please keep those values as they are for NT compatibility!
See the following references:
https://www.geoffchappell.com/studies/windows/km/ntoskrnl/api/mm/modwrite/create.htmhttps://techcommunity.microsoft.com/t5/ask-the-performance-team/what-is-the-page-file-for-anyway/ba-p/372608
+ Manual extraction of the values from different NT 6.2,6.3,10 builds.
[SMSS] Fill out in particular the x86-specific case for PAE.
[NTOS:MM] Some cleanup in the NtCreatePagingFile() code, namely:
- Clarify some comments;
- Validate the lower and upper bounds of the Minimum and Maximum sizes
(based on Windows behaviour as explained by Geoff + manual tests).
- Open the pagefile in case-insensitive;
- Simplify the loop that finds an existing matching pagefile;
- Simplify some failure exit paths;
- Add a "Missing validation steps TODO" comment block explaining the
existing code-hole.
The "failed to grant access rights" message isn't enough to understand what kind of access rights haven't been granted and why. Dumping information of the captured security descriptor, the ACL and its ACEs with mask rights and token SIDs should be enough to understand the reason of the failure in question.
debug.c will serve as a centralized facility for security debugging routines and everything related to that. This file will be expanded with further debug functions for the Security subsystem if needed.
Return TRUE instead of NTSTATUS code which has a value of FALSE and may confuse caller.
Fixes sporadic 0x7B bugcheck when booting from corrupted NTFS volume using WinXP ntfs.sys.
PsIdleProcess and PsInitialSystemProcess share the same handle table. This
leads ObGetProcessHandleCount() to report the same number of handles
when called on those system processes, when being enumerated by
NtQuerySystemInformation(SystemProcessInformation).
Instead, just return 0 for the handle count of the Idle process in SystemProcessInformation.
This is not done in ObGetProcessHandleCount(), since a separate
NtQueryInformationProcess(ProcessHandleCount) for the idle process should return
a non-zero value.
CORE-16577
Not only primary group assignation was broken but new dynamic length calculation is also broken. The length of the captured SID is not taken into account so the new dynamic length gets only the size of the default ACL present in an access token.
Therefore, the condition is always FALSE and the code never jumps to the STATUS_ALLOTTED_SPACE_EXCEEDED branch because the length will always be small than the charged dynamic length.
Addendum to 86bde3c.
With current master, what happens is that when someone wants to assign a new primary group SID for an access token, it results in an instant page fault because the primary group variable doesn't get assigned the dynamic part's address.
So the primary group variable gets an address which is basically a representation of the ACL size, hence the said address is bogus and it's where the page fault kicks in.
CORE-18249
Since we were charging the pool quota after the VAD insertion,
if the quota charge failed, the VAD would still have been inserted.
This commit attempts to resolve this issue by charging quota
before inserting the VAD thus allowing the quota charge to fail early.
Addendum to 884356a0. CORE-18028
On current master, ReactOS faces these problems:
- ObCreateObject charges both paged and non paged pool a size of TOKEN structure, not the actual dynamic contents of WHAT IS inside a token. For paged pool charge the size is that of the dynamic area (primary group + default DACL if any). This is basically what DynamicCharged is for.
For the non paged pool charge, the actual charge is that of TOKEN structure upon creation. On duplication and filtering however, the paged pool charge size is that of the inherited dynamic charged space from an existing token whereas the non paged pool size is that of the calculated token body
length for the new duplicated/filtered token. On current master, we're literally cheating the kernel by charging the wrong amount of quota not taking into account the dynamic contents which they come from UM.
- Both DynamicCharged and DynamicAvailable are not fully handled (DynamicAvailable is pretty much poorly handled with some cases still to be taking into account). DynamicCharged is barely handled, like at all.
- As a result of these two points above, NtSetInformationToken doesn't check when the caller wants to set up a new default token DACL or primary group if the newly DACL or the said group exceeds the dynamic charged boundary. So what happens is that I'm going to act like a smug bastard fat politician and whack
the primary group and DACL of an token however I want to, because why in the hell not? In reality no, the kernel has to punish whoever attempts to do that, although we currently don't.
- The dynamic area (aka DynamicPart) only picks up the default DACL but not the primary group as well. Generally the dynamic part is composed of primary group and default DACL, if provided.
In addition to that, we aren't returning the dynamic charged and available area in token statistics. SepComputeAvailableDynamicSpace helper is here to accommodate that. Apparently Windows is calculating the dynamic available area rather than just querying the DynamicAvailable field directly from the token.
My theory regarding this is like the following: on Windows both TokenDefaultDacl and TokenPrimaryGroup classes are barely used by the system components during startup (LSASS provides both a DACL and primary group when calling NtCreateToken anyway). In fact DynamicAvailable is 0 during token creation, duplication and filtering when inspecting a token with WinDBG. So
if an application wants to query token statistics that application will face a dynamic available space of 0.
On ObpChargeQuotaForObject function, the kernel will either charge the default object type charges or the specified information charges obtained from ObCreateObject API call. What happens is that if a paged pool charge is specified on ObCreateObject call the kernel will charge that
but when an object is about to be de-allocated, the amount of quota to return back to the system is the amounting of the paged pool charge specified previously by the ObCreateObject call plus the amounting of the security descriptor charge (see oblife.c / line 98).
This will result in a fatal crash with a bugcheck of QUOTA_UNDERFLOW because we are returning quota with bits of it that was never charged and that's SecurityDescriptorCharge. A QUOTA_UNDERFLOW bugcheck occurs in two following scenarios:
-- When installing Virtualbox Guest Additions and prompting the installer to reboot the system for you
-- When logging off and on back to the system and then you restart the system normally
This bug has been discovered whilst working on #4555 PR.
TokenGroupsAndPrivileges is the younger sister of two TokenGroups and TokenPrivileges classes. In its purpose there's no huge substantial differences apart that this class comes with its own structure, TOKEN_GROUPS_AND_PRIVILEGES, and that this structure comes with extra information.
In NtQueryInformationToken function, remove the useless and redundant NULL check for two primary reasons. First, DefaultQueryInfoBufferCheck already does the necessary probing validation checks and second, ReturnLength must NEVER be NULL!
If the caller does not respect the calling rules of NtQueryInformationToken, the caller is expected to be miserably punished.
NtQueryInformationToken is by far the only system call in NT where ReturnLength simply cannot be optional. On Windows this parameter is always probed and an argument to NULL directly leads to an access violation exception.
This is due to the fact of how tokens work, as its information contents (token user, owner, primary group, et al) are dynamic and can vary throughout over time in memory.
What happens on current ReactOS master however is that ReturnLength is only probed if the parameter is not NULL. On a NULL case scenario the probing checks succeed and NtQueryInformationToken fails later. For this, just get rid of CompleteProbing
parameter and opt in for a bit mask flag based approach, with ICIF_FORCE_RETURN_LENGTH_PROBE being set on DefaultQueryInfoBufferCheck which NtQueryInformationToken calls it to do sanity checks.
In addition to that...
- Document the ICIF probe helpers
- Annotate the ICIF prope helpers with SAL
- With the riddance of CompleteProbing and adoption of flags based approach, add ICIF_PROBE_READ_WRITE and ICIF_PROBE_READ flags alongside with ICIF_FORCE_RETURN_LENGTH_PROBE
The current state of Security manager's code is kind of a mess. Mainly, there's code scattered around places where they shouldn't belong and token implementation (token.c) is already of a bloat in itself as it is. The file has over 6k lines and it's subject to grow exponentially with improvements, features, whatever that is.
With that being said, the token implementation code in the kernel will be split accordingly and rest of the code moved to appropriate places. The new layout will look as follows (excluding the already existing files):
- client.c (Client security implementation code)
- objtype.c (Object type list implementation code -- more code related to object types will be put here when I'm going to implement object type access checks in the future)
- subject.c (Subject security context support)
The token implementation in the kernel will be split in 4 distinct files as shown:
- token.c (Base token support routines)
- tokenlif.c (Life management of a token object -- that is Duplication, Creation and Filtering)
- tokencls.c (Token Query/Set Information Classes support)
- tokenadj.c (Token privileges/groups adjusting support)
In addition to that, tidy up the internal header and reorganize it as well.
Fix MiInsertSharedUserPageVad to not charge the system process pool quota.
Even though PsChargeProcessNonPagedPoolQuota itself checks if the process specified is the system process, this doesn't work here as we're too early into boot for the kernel to know what the system process is.
This commit fully implements the inner logic of KeSaveFloatingPointState and KeRestoreFloatingPointState routines. On ReactOS we're currently simply doing a FNSAVE operation whereas on Windows it is a lot more than that.
On Windows Server 2003 the logic more or less goes like this. In order to save the FPU state the NPX state of the current thread has to be checked first, that is, if NPX is loaded and currently charged for the current thread then the system will acquire the NPX registers actively present. From that point it performs either a FNSAVE or FXSAVE
if FX is actually supported. Otherwise the control word and MXCsr registers are obtained.
FXSAVE/FNSAVE operation is done solely if the FX save area is held up in a pool allocation. Pool allocation occurs if it's been found out that the NPX IRQL of the thread is not the same as the current thread which from where it determines if the interrupt level is APC then allocate some pool memory and hold the save area there, otherwise
the save area in question is grabbed from the current processor control region. If NPX is not loaded for the current thread then the FPU state is obtained from the NPX frame.
In our case we'll be doing something way simpler. Only do a FXSAVE/FNSAVE directly of the FPU state registers, in this way we are simplifying the code and the actual logic of Save/Restore mechanism.
This is needed to store FPU state information when saving or restoring the floating point state of a system due to a call to KeSaveFloatingPointState or KeRestoreFloatingPointState.
- Add a check for correct PDO before doing anything
- Process the request only for started devices
- Send the request synchronously during the start sequence
This makes Windows' i8042prt.sys work on ReactOS.
Addendum to cf0bc1c132
The problem is that EndMem is changed to point to the DynamicPart of
the token, but the code after that expects it to still point into the
VariablePart instead.
Problem fixed by moving the insertion of RestrictedSids much sooner
(where the original ones are also being copied).
Shared locking must be used on the source token as it is accessed for
reading only. This fixes in particular the kmtest SeTokenFiltering that
would hang otherwise on a (wrong) exclusive locking.
- SepPerformTokenFiltering(): Always shared-lock the source token.
Its callers (NtFilterToken and SeFilterToken) just need to sanitize and
put the parameters in correct form before calling this helper function.
- Sync comments in NtFilterToken() with SeFilterToken().
This function is either called inter-kernel (in which case, all
parameters must be valid, and if not, we have to bugcheck), or, it
is called with **captured** parameters (from NtFilterToken) and those
latter ones are now expected to be valid and reside in kernel-mode.
Finally, data copied between token structures reside in kernel-mode
only and again are expected to be valid (if not, we bugcheck).
- The ACL is however not validated when the function is called within
kernel mode and no capture is actually being done.
- Simplify aspects of the function (returning early when possible).
If inserting the allocated VAD fails, MiMapViewOfDataSection will make no attempt to free the allocated VAD. Nor will it call MiDereferenceControlArea(ControlArea); like other failure return paths. This commit fixes this behavior.
Co-authored-by: Hermès BÉLUSCA - MAÏTO <hermes.belusca-maito@reactos.org>
We are allocating blocks of pool memory for a security descriptor with its own specific tag, TAG_SEC_QUERY, so just use it when freeing when releasing the descriptor as well (aka freeing the said pool).
There are two fundamental problems when it comes to access checks in ReactOS. First, the internal function SepAccessCheck which is the heart and brain of the whole access checks logic of the kernel warrants access to the calling thread of a process to an object even though access could not be given.
This can potentially leave security issues as we literally leave objects to be touched indiscriminately by anyone regardless of their ACEs in the DACL of a security descriptor. Second, the current access check code doesn't take into account the fact that an access token can have restricted SIDs. In such scenario we must perform additional access checks by iterating over the restricted SIDs of the primary token by comparing the SID equality and see if the group can be granted certain rights based on the ACE policy that represents the same SID.
Part of SepAccessCheck's code logic will be split for a separate private kernel routine, SepAnalyzeAcesFromDacl. The reasons for this are primarily two -- such code is subject to grow eventually as we'll support different type ACEs and handle them accordingly -- and we avoid further code duplicates. On Windows Server 2003 there are 5 different type of ACEs that are supported for access checks:
- ACCESS_DENIED_ACE_TYPE (supported by ReactOS)
- ACCESS_ALLOWED_ACE_TYPE (supported by ReactOS)
- ACCESS_DENIED_OBJECT_ACE_TYPE
- ACCESS_ALLOWED_OBJECT_ACE_TYPE
- ACCESS_ALLOWED_COMPOUND_ACE_TYPE
This gives the opportunity for us to have a semi serious kernel where security of objects are are taken into account, rather than giving access to everyone.
CORE-9174
CORE-9175
CORE-9184
CORE-14520
SepSidInTokenEx function already provides the necessary mechanism to handle scenario where a token has restricted SIDs or a principal SID is given to the call. There's no reason to have these redundant ASSERTs anymore.
In addition to that make sure if the SID is not a restricted and if that SID is the first element on the array and it's enabled, this is the primary user.
This function will be used to retrieve a security identifier from a valid access control entry in the kernel. Mostly and exclusively used within access checks related code and such.
Implement the correct start-stop sequence for resource rebalancing
without the actual rebalancing. Also move IoInvalidateDeviceState
processing into the enumeration thread as it should be.
CORE-17519
Implement initial token debug code. For now debug information that is being tracked are: process image file name, process and thread client IDs and token creation method. More specific debug code can be added later only if needed.
As for the token creation method, this follows the same principle as on Windows where the creation method is defined by a value denoting the first letter of the said method of creation. That is, 0xC is for token creation, 0xD is for token duplication and 0xF is for token filtering. The debug field names are taken from Windows PDB symbols for WinDBG debug extension support purposes. The names must not be changed!
- Add a new cmboot.h header to isolate the boot-support definitions
shared with the NT/ReactOS bootloader.
- Move CmpFreeDriverList() to cmboot.c so that we can use it for
cleanup paths in the NT/ReactOS bootloader.
- CmpFindControlSet(): Directly build the control set name in UNICODE,
instead of doing an ANSI->UNICODE conversion.
- Directly assign the CurrentControlSet\Services constant string,
instead of going the route of init-empty-string + append-string.
This is possible since that string is not modified later.
- Remove ASSERT(FALSE), replacing them with correct failure handling.
- Add cleanup paths in CmpAddDriverToList().
- Simplify and fix CmpFreeDriverList(): it's the full DriverNode
that needs to be freed; not the LIST_ENTRY pointer.
- Add other validity checks:
* Registry value types and data sizes;
* For multi-strings, verify that they are NULL-terminated.
* For (multi-)strings, check whether they are NULL-terminated before
optionally removing their trailing NULL character from the count.
Check also whether they are of zero-length and take appropriate
action where necessary.
- Add CmpIsDriverInList() for future usage in CMBOOT compiled in
bootloader mode.
- Add SAL annotations and Doxygen documentation.
- Add debug traces.
- Formatting / code style fixes.
** TODO: Fix SafeBoot support **
