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

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

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.