[PATCH] powerpc: Add array bounds checking to crash_shutdown_handlers

[PATCH] powerpc: Add array bounds checking to crash_shutdown_handlers

[Suraj Jitindar Singh]

The array crash_shutdown_handles is an array of size CRASH_HANDLER_MAX+1
containing up to CRASH_HANDLER_MAX shutdown_handlers. It is assumed to
be NULL terminated, which it is under normal circumstances. Array
accesses in the functions crash_shutdown_unregister() and
default_machine_crash_shutdown() rely on this NULL termination property
when traversing this list and don't protect again out of bounds accesses.
If the NULL terminator were somehow overwritten these functions could
potentially access out of the bounds of the array.

Shrink the array to size CRASH_HANDLER_MAX and implement explicit array
bounds checking when accessing the elements of the
crash_shutdown_handles[] array in crash_shutdown_unregister() and
default_machine_crash_shutdown().

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 arch/powerpc/kernel/crash.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/crash.c b/arch/powerpc/kernel/crash.c
index 2bb252c..3dc1fad 100644
--- a/arch/powerpc/kernel/crash.c
+++ b/arch/powerpc/kernel/crash.c
@@ -48,8 +48,8 @@ int crashing_cpu = -1;
 static int time_to_dump;
 
 #define CRASH_HANDLER_MAX 3
-/* NULL terminated list of shutdown handles */
-static crash_shutdown_t crash_shutdown_handles[CRASH_HANDLER_MAX+1];
+/* List of shutdown handles */
+static crash_shutdown_t crash_shutdown_handles[CRASH_HANDLER_MAX];
 static DEFINE_SPINLOCK(crash_handlers_lock);
 
 static unsigned long crash_shutdown_buf[JMP_BUF_LEN];
@@ -288,9 +288,14 @@ int crash_shutdown_unregister(crash_shutdown_t handler)
 		rc = 1;
 	} else {
 		/* Shift handles down */
-		for (; crash_shutdown_handles[i]; i++)
+		for (; i < (CRASH_HANDLER_MAX - 1); i++)
 			crash_shutdown_handles[i] =
 				crash_shutdown_handles[i+1];
+		/*
+		 * Reset last entry to NULL now that it has been shifted down,
+		 * this will allow new handles to be added here.
+		 */
+		crash_shutdown_handles[i] = NULL;
 		rc = 0;
 	}
 
@@ -346,7 +351,7 @@ void default_machine_crash_shutdown(struct pt_regs *regs)
 	old_handler = __debugger_fault_handler;
 	__debugger_fault_handler = handle_fault;
 	crash_shutdown_cpu = smp_processor_id();
-	for (i = 0; crash_shutdown_handles[i]; i++) {
+	for (i = 0; crash_shutdown_handles[i] && i < CRASH_HANDLER_MAX; i++) {
 		if (setjmp(crash_shutdown_buf) == 0) {
 			/*
 			 * Insert syncs and delay to ensure
-- 
2.5.0

Re: powerpc: Add array bounds checking to crash_shutdown_handlers

[Michael Ellerman]

On Wed, 2016-11-05 at 00:57:32 UTC, Suraj Jitindar Singh wrote:
> The array crash_shutdown_handles is an array of size CRASH_HANDLER_MAX+1
> containing up to CRASH_HANDLER_MAX shutdown_handlers. It is assumed to
> be NULL terminated, which it is under normal circumstances. Array
> accesses in the functions crash_shutdown_unregister() and
> default_machine_crash_shutdown() rely on this NULL termination property
> when traversing this list and don't protect again out of bounds accesses.
> If the NULL terminator were somehow overwritten these functions could
> potentially access out of the bounds of the array.
> 
> Shrink the array to size CRASH_HANDLER_MAX and implement explicit array
> bounds checking when accessing the elements of the
> crash_shutdown_handles[] array in crash_shutdown_unregister() and
> default_machine_crash_shutdown().
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/1d1451655bad9a6a5fd7a42de6

cheers