[PATCH] powerpc: Add out of bounds check to crash_shutdown_unregister()

[PATCH] powerpc: Add out of bounds check to crash_shutdown_unregister()

[Suraj Jitindar Singh]

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.

Add a check to terminate the shift operation when CRASH_HANDLER_MAX is
reached in order to protect against out of bounds accesses.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 arch/powerpc/kernel/crash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
-- 
2.5.0

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

[Balbir Singh]

On 28/04/16 16:17, 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.
> 
> Add a check to terminate the shift operation when CRASH_HANDLER_MAX is
> reached in order to protect against out of bounds accesses.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  arch/powerpc/kernel/crash.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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;
> 

with i = CRASH_HANDLER_MAX-1 we could end up with crash_shutdown_handles[i+1] already out of bounds
I think you need to check that i+1 does not overflow

Balbir

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

[Suraj Jitindar Singh]

On Thu, 28 Apr 2016 16:55:13 +1000
Balbir Singh <bsingharora@gmail.com> wrote:

> On 28/04/16 16:17, 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.
> > 
> > Add a check to terminate the shift operation when CRASH_HANDLER_MAX
> > is reached in order to protect against out of bounds accesses.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  arch/powerpc/kernel/crash.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 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;
> >   
> 
> with i = CRASH_HANDLER_MAX-1 we could end up with
> crash_shutdown_handles[i+1] already out of bounds I think you need to
> check that i+1 does not overflow
> 
> Balbir

Thanks for taking a look Balbir, the size of crash_shutdown_handles is
actually CRASH_HANDLER_MAX+1.

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

[Balbir Singh]

> 
> Thanks for taking a look Balbir, the size of crash_shutdown_handles is
> actually CRASH_HANDLER_MAX+1.
> 

Acked-by: Balbir Singh <bsingharora@gmail.com>

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

[Michael Ellerman]

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

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

[Suraj Jitindar Singh]

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.