Re: powerpc: Add out of bounds check to crash_shutdown_unregister()

[Suraj Jitindar Singh <sjitindarsingh@gmail.com>]

On 03/05/16 15:00, Michael Ellerman wrote:
> On Thu, 2016-28-04 at 06:17:45 UTC, Suraj Jitindar Singh wrote:
>> When unregistering a crash_shutdown_handle in the function
>> crash_shutdown_unregister() the other handles are shifted down in the
>> array to replace the unregistered handle. The for loop assumes that the
>> last element in the array is null and uses this as the stop condition,
>> however in the case that the last element is not null there is no check
>> to ensure that an out of bounds access is not performed.
> But AFAICS the code ensures that entry will always be NULL. So there's no bug at
> the moment.
>
>> Add a check to terminate the shift operation when CRASH_HANDLER_MAX is
>> reached in order to protect against out of bounds accesses.
> Doing it this way is more robust though. The chance of the NULL terminator being
> corrupted is definitely higher than the code being corrupted, and if the latter
> happens we're probably toast anyway.
>
>> diff --git a/arch/powerpc/kernel/crash.c b/arch/powerpc/kernel/crash.c
>> index 2bb252c..6b267af 100644
>> --- a/arch/powerpc/kernel/crash.c
>> +++ b/arch/powerpc/kernel/crash.c
>> @@ -288,7 +288,7 @@ int crash_shutdown_unregister(crash_shutdown_t handler)
>>  		rc = 1;
>>  	} else {
>>  		/* Shift handles down */
>> -		for (; crash_shutdown_handles[i]; i++)
>> +		for (; crash_shutdown_handles[i] && i < CRASH_HANDLER_MAX; i++)
>>  			crash_shutdown_handles[i] =
>>  				crash_shutdown_handles[i+1];
>>  		rc = 0;
> So if I'm reading it right, with this change we have removed all the code that
> uses the NULL-terminated property of the list.
>
> If so we should also shrink the array to be only CRASH_HANDLER_MAX in size, and
> remove any references to it being NULL terminated.
>
> cheers

Thanks Michael, will change this to reflect that the list is no longer assumed to be null terminated.