From 18064031f970efb1636b9fa95e231ba85e39c376 Mon Sep 17 00:00:00 2001 From: Aleksey Bragin Date: Thu, 27 Sep 2007 13:07:43 +0000 Subject: [PATCH] - Fix multiple bugs in RtlWalkFrameChain and secure it against any possibility of a bugcheck while walking the stack. - Fix bugs in RtlUnwind and RtlExceptionDispatch which assumed the DPC stack size was 4KB instead of 12KB. - Fix multiple bugs in RtlpGetStackLimits and seure it against bugchecks. Properly detect DPC or invalid stacks. - PsConvertToGuiThread should acquire a guarded region, not a critical section, to stop all APCs. - Fix bug in bugzilla reporting which was making things crash. - Unlock address space before raising to HIGH_LEVEL in KeBugCheck. - Display blue screen at APC_LEVEL, to avoid the assertion in procobj.c when trying to attach to csrss. This should fix the recursive bugchecking when the GUI is up, and also take down the GUI properly. The fix is a hack. - Fix bogus implementation of IoGetStackLimits and make it work properly. - Make MmCreateKernelStack return the base of the stack, not the limit, and fix all callers appropriately. - Remove svn:needs-lock properties of various files, whose contents either changes too often or whose contents is definately clean. Bugreports and information - by Alex. svn path=/trunk/; revision=29244 --- reactos/lib/rtl/exception.c | 108 +++++++++++++++++------------- reactos/ntoskrnl/io/iomgr/util.c | 36 ++++++++-- reactos/ntoskrnl/ke/bug.c | 19 +++--- reactos/ntoskrnl/ke/i386/kiinit.c | 2 +- reactos/ntoskrnl/ke/thrdobj.c | 7 +- reactos/ntoskrnl/mm/procsup.c | 5 +- reactos/ntoskrnl/ps/win32.c | 11 ++- reactos/ntoskrnl/rtl/libsupp.c | 39 ++++++++--- 8 files changed, 141 insertions(+), 86 deletions(-) diff --git a/reactos/lib/rtl/exception.c b/reactos/lib/rtl/exception.c index 5a110f93541..b320b75ca6f 100644 --- a/reactos/lib/rtl/exception.c +++ b/reactos/lib/rtl/exception.c @@ -122,12 +122,11 @@ RtlWalkFrameChain(OUT PVOID *Callers, IN ULONG Count, IN ULONG Flags) { - PULONG Stack, NewStack; + ULONG_PTR Stack, NewStack, StackBegin, StackEnd; ULONG Eip; - ULONG_PTR StackBegin, StackEnd; BOOLEAN Result, StopSearch = FALSE; ULONG i = 0; - + /* Get current EBP */ #if defined(_M_IX86) #if defined __GNUC__ @@ -136,16 +135,16 @@ RtlWalkFrameChain(OUT PVOID *Callers, __asm mov Stack, ebp #endif #elif defined(_M_MIPS) - __asm__("move $sp, %0" : "=r" (Stack) : ); + __asm__("move $sp, %0" : "=r" (Stack) : ); #elif defined(_M_PPC) __asm__("mr %0,1" : "=r" (Stack) : ); #else #error Unknown architecture #endif - + /* Set it as the stack begin limit as well */ StackBegin = (ULONG_PTR)Stack; - + /* Check if we're called for non-logging mode */ if (!Flags) { @@ -155,52 +154,65 @@ RtlWalkFrameChain(OUT PVOID *Callers, &StackEnd); if (!Result) return 0; } - - /* Loop the frames */ - for (i = 0; i < Count; i++) + + /* Use a SEH block for maximum protection */ + _SEH_TRY { - /* Check if we're past the stack */ - if ((ULONG_PTR)Stack >= StackEnd) break; - - /* Check if this is the first entry */ -#if 0 - if (!i) + /* Loop the frames */ + for (i = 0; i < Count; i++) { - if ((ULONG_PTR)Stack != StackBegin) break; + /* + * Leave if we're past the stack, + * if we're before the stack, + * or if we've reached ourselves. + */ + if ((Stack >= StackEnd) || + (!i ? (Stack < StackBegin) : (Stack <= StackBegin)) || + ((StackEnd - Stack) < (2 * sizeof(ULONG_PTR)))) + { + /* We're done or hit a bad address */ + break; + } + + /* Get new stack and EIP */ + NewStack = *(PULONG_PTR)Stack; + Eip = *(PULONG_PTR)(Stack + sizeof(ULONG_PTR)); + + /* Check if the new pointer is above the oldone and past the end */ + if (!((Stack < NewStack) && (NewStack < StackEnd))) + { + /* Stop searching after this entry */ + StopSearch = TRUE; + } + + /* Also make sure that the EIP isn't a stack address */ + if ((StackBegin < Eip) && (Eip < StackEnd)) break; + + /* Check if we reached a user-mode address */ + if (!(Flags) && !(Eip & 0x80000000)) break; + + /* Save this frame */ + Callers[i] = (PVOID)Eip; + + /* Check if we should continue */ + if (StopSearch) + { + /* Return the next index */ + i++; + break; + } + + /* Move to the next stack */ + Stack = NewStack; } - else - { - if ((ULONG_PTR)Stack == StackBegin) break; - } -#endif - - /* Make sure there's enough frames */ - if ((StackEnd - (ULONG_PTR)Stack) < (2 * sizeof(ULONG_PTR))) break; - - /* Get new stack and EIP */ - NewStack = (PULONG)Stack[0]; - Eip = Stack[1]; - - /* Check if the new pointer is above the oldone and past the end */ - if (!((Stack < NewStack) && ((ULONG_PTR)NewStack < StackEnd))) - { - /* Stop searching after this entry */ - StopSearch = TRUE; - } - - /* Also make sure that the EIP isn't a stack address */ - if ((StackBegin < Eip) && (Eip < StackEnd)) break; - - /* Save this frame */ - Callers[i] = (PVOID)Eip; - - /* Check if we should continue */ - if (StopSearch) break; - - /* Move to the next stack */ - Stack = NewStack; } - + _SEH_HANDLE + { + /* No index */ + i = 0; + } + _SEH_END; + /* Return frames parsed */ return i; } diff --git a/reactos/ntoskrnl/io/iomgr/util.c b/reactos/ntoskrnl/io/iomgr/util.c index ca1becc8eee..6f151e82cc5 100644 --- a/reactos/ntoskrnl/io/iomgr/util.c +++ b/reactos/ntoskrnl/io/iomgr/util.c @@ -12,6 +12,11 @@ #define NDEBUG #include +VOID +NTAPI +RtlpGetStackLimits(PULONG_PTR StackBase, + PULONG_PTR StackLimit); + /* DATA **********************************************************************/ KSPIN_LOCK CancelSpinLock; @@ -48,10 +53,33 @@ NTAPI IoGetStackLimits(OUT PULONG LowLimit, OUT PULONG HighLimit) { - /* Return the limits from the TEB... this is wrong! */ - DPRINT1("FIXME: IoGetStackLimits returning B*LLSHIT!\n"); - *LowLimit = (ULONG)NtCurrentTeb()->Tib.StackLimit; - *HighLimit = (ULONG)NtCurrentTeb()->Tib.StackBase; + PKPRCB Prcb = KeGetCurrentPrcb(); + ULONG_PTR DpcStack = (ULONG_PTR)(Prcb->DpcStack); + volatile ULONG_PTR StackAddress; + + /* Save our stack address so we always know it's valid */ + StackAddress = (ULONG_PTR)(&StackAddress); + + /* Get stack values */ + RtlpGetStackLimits(LowLimit, HighLimit); + + /* Check if we're outside the stack */ + if ((StackAddress < *LowLimit) || (StackAddress > *HighLimit)) + { + /* Check if we may be in a DPC */ + if (KeGetCurrentIrql() >= DISPATCH_LEVEL) + { + /* Check if we really are in a DPC */ + if ((Prcb->DpcRoutineActive) && + (StackAddress <= DpcStack) && + (StackAddress >= DpcStack - KERNEL_STACK_SIZE)) + { + /* Use the DPC stack limits */ + *HighLimit = DpcStack; + *LowLimit = DpcStack - KERNEL_STACK_SIZE; + } + } + } } /* diff --git a/reactos/ntoskrnl/ke/bug.c b/reactos/ntoskrnl/ke/bug.c index e514095f2c3..186932a2cdf 100644 --- a/reactos/ntoskrnl/ke/bug.c +++ b/reactos/ntoskrnl/ke/bug.c @@ -222,9 +222,7 @@ KeRosDumpTriageForBugZillaReport(VOID) "BIOS Version: %wZ\n" "Video BIOS Date: %wZ\n" "Video BIOS Version: %wZ\n" - "Memory: %d\n" - "NT Build Number: %lx\n" - "NT Build Lab: %s\n", + "Memory: %d\n", KeProcessorArchitecture, KeFeatureBits, KiFastSystemCallDisable, @@ -251,9 +249,7 @@ KeRosDumpTriageForBugZillaReport(VOID) &KeRosBiosVersion, &KeRosVideoBiosDate, &KeRosVideoBiosVersion, - MmStats.NrTotalPages * PAGE_SIZE, - NtBuildNumber, - NtBuildLab); + MmStats.NrTotalPages * PAGE_SIZE); #endif } @@ -640,7 +636,6 @@ KeBugCheckWithTf(IN ULONG BugCheckCode, PVOID DriverBase; PLDR_DATA_TABLE_ENTRY LdrEntry; PULONG_PTR HardErrorParameters; - KIRQL OldIrql; #ifdef CONFIG_SMP LONG i = 0; #endif @@ -976,15 +971,15 @@ KeBugCheckWithTf(IN ULONG BugCheckCode, } } - /* Raise IRQL to HIGH_LEVEL */ - _disable(); - KeRaiseIrql(HIGH_LEVEL, &OldIrql); - /* ROS HACK: Unlock the Kernel Address Space if we own it */ if (KernelAddressSpaceLock.Owner == KeGetCurrentThread()) { MmUnlockAddressSpace(MmGetKernelAddressSpace()); } + + /* Raise IRQL to HIGH_LEVEL */ + _disable(); + KfRaiseIrql(HIGH_LEVEL); /* Avoid recursion */ if (!InterlockedDecrement((PLONG)&KeBugCheckCount)) @@ -1006,11 +1001,13 @@ KeBugCheckWithTf(IN ULONG BugCheckCode, #endif /* Display the BSOD */ + KfLowerIrql(APC_LEVEL); // This is a nastier hack than any ever before KiDisplayBlueScreen(MessageId, IsHardError, HardErrCaption, HardErrMessage, AnsiName); + KfRaiseIrql(HIGH_LEVEL); /* Check if the debugger is disabled but we can enable it */ if (!(KdDebuggerEnabled) && !(KdPitchDebugger)) diff --git a/reactos/ntoskrnl/ke/i386/kiinit.c b/reactos/ntoskrnl/ke/i386/kiinit.c index 50c6f8a399e..a8e9b1ff65b 100644 --- a/reactos/ntoskrnl/ke/i386/kiinit.c +++ b/reactos/ntoskrnl/ke/i386/kiinit.c @@ -582,7 +582,7 @@ KiInitializeKernel(IN PKPROCESS InitProcess, /* Allocate the DPC Stack */ DpcStack = MmCreateKernelStack(FALSE, 0); if (!DpcStack) KeBugCheckEx(NO_PAGES_AVAILABLE, 1, 0, 0, 0); - Prcb->DpcStack = (PVOID)((ULONG_PTR)DpcStack + KERNEL_STACK_SIZE); + Prcb->DpcStack = DpcStack; /* Allocate the IOPM save area. */ Ki386IopmSaveArea = ExAllocatePoolWithTag(PagedPool, diff --git a/reactos/ntoskrnl/ke/thrdobj.c b/reactos/ntoskrnl/ke/thrdobj.c index f782591f2bb..6c39bc9b1d4 100644 --- a/reactos/ntoskrnl/ke/thrdobj.c +++ b/reactos/ntoskrnl/ke/thrdobj.c @@ -797,8 +797,7 @@ KeInitThread(IN OUT PKTHREAD Thread, if (!KernelStack) { /* We don't, allocate one */ - KernelStack = (PVOID)((ULONG_PTR)MmCreateKernelStack(FALSE, 0) + - KERNEL_STACK_SIZE); + KernelStack = MmCreateKernelStack(FALSE, 0); if (!KernelStack) return STATUS_INSUFFICIENT_RESOURCES; /* Remember for later */ @@ -806,8 +805,8 @@ KeInitThread(IN OUT PKTHREAD Thread, } /* Set the Thread Stacks */ - Thread->InitialStack = (PCHAR)KernelStack; - Thread->StackBase = (PCHAR)KernelStack; + Thread->InitialStack = KernelStack; + Thread->StackBase = KernelStack; Thread->StackLimit = (ULONG_PTR)KernelStack - KERNEL_STACK_SIZE; Thread->KernelStackResident = TRUE; diff --git a/reactos/ntoskrnl/mm/procsup.c b/reactos/ntoskrnl/mm/procsup.c index 876535e3fe3..67b360f38b8 100644 --- a/reactos/ntoskrnl/mm/procsup.c +++ b/reactos/ntoskrnl/mm/procsup.c @@ -242,8 +242,9 @@ MmCreateKernelStack(BOOLEAN GuiStack, KEBUGCHECK(0); } - /* Return the stack */ - return KernelStack; + /* Return the stack base */ + return (PVOID)((ULONG_PTR)KernelStack + + (GuiStack ? KERNEL_LARGE_STACK_SIZE : KERNEL_STACK_SIZE)); } /* diff --git a/reactos/ntoskrnl/ps/win32.c b/reactos/ntoskrnl/ps/win32.c index ac0805dd370..668da922ff5 100644 --- a/reactos/ntoskrnl/ps/win32.c +++ b/reactos/ntoskrnl/ps/win32.c @@ -52,8 +52,7 @@ PsConvertToGuiThread(VOID) if (!Thread->Tcb.LargeStack) { /* We don't create one */ - NewStack = (ULONG_PTR)MmCreateKernelStack(TRUE, 0) + - KERNEL_LARGE_STACK_SIZE; + NewStack = (ULONG_PTR)MmCreateKernelStack(TRUE, 0); if (!NewStack) { /* Panic in user-mode */ @@ -61,15 +60,15 @@ PsConvertToGuiThread(VOID) return STATUS_NO_MEMORY; } - /* We're about to switch stacks. Enter a critical region */ - KeEnterCriticalRegion(); + /* We're about to switch stacks. Enter a guarded region */ + KeEnterGuardedRegion(); /* Switch stacks */ OldStack = KeSwitchKernelStack((PVOID)NewStack, (PVOID)(NewStack - KERNEL_STACK_SIZE)); - /* Leave the critical region */ - KeLeaveCriticalRegion(); + /* Leave the guarded region */ + KeLeaveGuardedRegion(); /* Delete the old stack */ MmDeleteKernelStack(OldStack, FALSE); diff --git a/reactos/ntoskrnl/rtl/libsupp.c b/reactos/ntoskrnl/rtl/libsupp.c index 1d1fedeac53..412f39b8bc8 100644 --- a/reactos/ntoskrnl/rtl/libsupp.c +++ b/reactos/ntoskrnl/rtl/libsupp.c @@ -197,11 +197,11 @@ RtlpHandleDpcStackException(IN PEXCEPTION_REGISTRATION_RECORD RegistrationFrame, /* Check if we are in a DPC and the stack matches */ if ((Prcb->DpcRoutineActive) && (RegistrationFrameEnd <= DpcStack) && - ((ULONG_PTR)RegistrationFrame >= DpcStack - 4096)) + ((ULONG_PTR)RegistrationFrame >= DpcStack - KERNEL_STACK_SIZE)) { /* Update the limits to the DPC Stack's */ *StackHigh = DpcStack; - *StackLow = DpcStack - 4096; + *StackLow = DpcStack - KERNEL_STACK_SIZE; return TRUE; } } @@ -218,18 +218,37 @@ RtlpCaptureStackLimits(IN ULONG_PTR Ebp, { PKTHREAD Thread = KeGetCurrentThread(); - /* FIXME: Super native implementation */ - + /* Don't even try at ISR level or later */ + if (KeGetCurrentIrql() > DISPATCH_LEVEL) return FALSE; + /* Start with defaults */ *StackBegin = Thread->StackLimit; *StackEnd = (ULONG_PTR)Thread->StackBase; - - /* Check if we seem to be on the DPC stack */ - if ((*StackBegin > Ebp) || (Ebp > *StackEnd)) + + /* Check if EBP is inside the stack */ + if ((*StackBegin <= Ebp) && (Ebp <= *StackEnd)) { - /* FIXME: TODO */ - //ASSERT(FALSE); - DPRINT1("Stacks: %p %p %p\n", Ebp, *StackBegin, *StackEnd); + /* Then make the stack start at EBP */ + *StackBegin = Ebp; + } + else + { + /* Now we're going to assume we're on the DPC stack */ + *StackEnd = (ULONG_PTR)(KeGetPcr()->Prcb->DpcStack); + *StackBegin = *StackEnd - KERNEL_STACK_SIZE; + + /* Check if we seem to be on the DPC stack */ + if ((*StackEnd) && (*StackBegin < Ebp) && (Ebp <= *StackEnd)) + { + /* We're on the DPC stack */ + *StackBegin = Ebp; + } + else + { + /* We're somewhere else entirely... use EBP for safety */ + *StackBegin = Ebp; + *StackEnd = (ULONG_PTR)PAGE_ALIGN(*StackBegin); + } } /* Return success */