[PATCH 0/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[PATCH 0/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

Hi,

Here is patch which fixes bug in /arch/arm/plat-omap/gpio.c file.
Pointer which may have NULL value in some cases (depend on kernel
configuration and GPIO method) is dereferenced later in code.
If pointer is NULL it mean some kernel bug.

Thanks,
Best Regards,
Evgeny

Evgeny Kuznetsov (1):
  omap: Ptr "isr_reg" tracked as NULL was dereferenced

 arch/arm/plat-omap/gpio.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

[PATCH 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@nokia.com>

Value of "isr_reg" pointer is depend on configuration and GPIO method.
Potentially it may have NULL value and it is dereferenced later 
in code. If pointer is NULL there is some kernel bug.
Log line and 'BUG' macro are added to halt code execution in this case.

Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@nokia.com>
---
 arch/arm/plat-omap/gpio.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index c05c653..7669928 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -1318,6 +1318,13 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	if (bank->method == METHOD_GPIO_44XX)
 		isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
 #endif
+
+	if (!isr_reg) {
+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
+				bank->method);
+		BUG();
+		}
+
 	while(1) {
 		u32 isr_saved, level_mask = 0;
 		u32 enabled;
-- 
1.6.3.3

Re: [PATCH 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Felipe Balbi]

Hi,

On Tue, Oct 05, 2010 at 03:42:10AM -0500, Evgeny Kuznetsov wrote:
>+	if (!isr_reg) {
>+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
>+				bank->method);
>+		BUG();
>+		}

this could be simply:

BUG_ON(!isr_reg);

-- 
balbi

Re: [PATCH 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

Hi,

Yes, but no log in that case.

Regards,
Evgeny

On Tue, 2010-10-05 at 12:32 +0300, ext Felipe Balbi wrote:
> Hi,
> 
> On Tue, Oct 05, 2010 at 03:42:10AM -0500, Evgeny Kuznetsov wrote:
> >+	if (!isr_reg) {
> >+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
> >+				bank->method);
> >+		BUG();
> >+		}
> 
> this could be simply:
> 
> BUG_ON(!isr_reg);
> 

Re: [PATCH 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Felipe Balbi]

Hi,

(don't top post)

On Tue, Oct 05, 2010 at 04:55:19AM -0500, Evgeny Kuznetsov wrote:
>Yes, but no log in that case.

what do you call the stack dump BUG() shows ??

-- 
balbi

Re: [PATCH 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Tue, Oct 05, 2010 at 03:42:10AM -0500, Evgeny Kuznetsov wrote:
>>+	if (!isr_reg) {
>>+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
>>+				bank->method);
>>+		BUG();
>>+		}
>
> this could be simply:
>
> BUG_ON(!isr_reg);

WARN_ON is better.

A BUG() will panic the kernel and stop everything.  This is not an error
condition that should prevent the entire kernel from running.

>From asm-generic/bug.h:

/*
 * Don't use BUG() or BUG_ON() unless there's really no way out; one
 * example might be detecting data structure corruption in the middle
 * of an operation that can't be backed out of.  If the (sub)system
 * can somehow continue operating, perhaps with reduced functionality,
 * it's probably not BUG-worthy.
 *
 * If you're tempted to BUG(), think again:  is completely giving up
 * really the *only* solution?  There are usually better options, where
 * users don't need to reboot ASAP and can mostly shut down cleanly.
 */

Re: [PATCH 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

On Tue, 2010-10-05 at 08:01 -0700, ext Kevin Hilman wrote:
> Felipe Balbi <balbi@ti.com> writes:
> 
> > Hi,
> >
> > On Tue, Oct 05, 2010 at 03:42:10AM -0500, Evgeny Kuznetsov wrote:
> >>+	if (!isr_reg) {
> >>+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
> >>+				bank->method);
> >>+		BUG();
> >>+		}
> >
> > this could be simply:
> >
> > BUG_ON(!isr_reg);
> 
> WARN_ON is better.
> 
> A BUG() will panic the kernel and stop everything.  This is not an error
> condition that should prevent the entire kernel from running.

'isr_reg' is dereferenced later in code:
	...
	isr_saved = isr = __raw_readl(isr_reg) & enabled;
	...
So this will stop kernel anyway.
I just hoped to help in understanding of issue by log line. WARN_ON
could be used for this.

As a variant compilation error could be added, to prevent situation when
kernel is incorrectly configured.
E.g.:
#if !defined(CONFIG_ARCH_OMAP1) &&		
	!defined(CONFIG_ARCH_OMAP15XX) &&	
	!defined(CONFIG_ARCH_OMAP16XX) &&	
	!defined(CONFIG_ARCH_OMAP730) &&	
	!defined(CONFIG_ARCH_OMAP850) &&	
	!defined(CONFIG_ARCH_OMAP2) &&	
	!defined(CONFIG_ARCH_OMAP3) &&	
	!defined(CONFIG_ARCH_OMAP4)

#error "Incorrect arch configuration"
#endif

But there are still cases when 'isr_reg' could have NULL value (if
'bank->method' is not equal to configured one).

Regards,
Evgeny
> 
> From asm-generic/bug.h:
> 
> /*
>  * Don't use BUG() or BUG_ON() unless there's really no way out; one
>  * example might be detecting data structure corruption in the middle
>  * of an operation that can't be backed out of.  If the (sub)system
>  * can somehow continue operating, perhaps with reduced functionality,
>  * it's probably not BUG-worthy.
>  *
>  * If you're tempted to BUG(), think again:  is completely giving up
>  * really the *only* solution?  There are usually better options, where
>  * users don't need to reboot ASAP and can mostly shut down cleanly.
>  */

Re: [PATCH 1/1] omap: Ptr "isr_reg" tracked as NULL was dereferenced

[Evgeny Kuznetsov]

On Wed, 2010-10-06 at 10:33 +0400, Evgeny Kuznetsov wrote:
> On Tue, 2010-10-05 at 08:01 -0700, ext Kevin Hilman wrote:
> > Felipe Balbi <balbi@ti.com> writes:
> > 
> > > Hi,
> > >
> > > On Tue, Oct 05, 2010 at 03:42:10AM -0500, Evgeny Kuznetsov wrote:
> > >>+	if (!isr_reg) {
> > >>+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
> > >>+				bank->method);
> > >>+		BUG();
> > >>+		}
> > >
> > > this could be simply:
> > >
> > > BUG_ON(!isr_reg);
> > 
> > WARN_ON is better.
> > 
> > A BUG() will panic the kernel and stop everything.  This is not an error
> > condition that should prevent the entire kernel from running.
> 



> 'isr_reg' is dereferenced later in code:
> 	...
> 	isr_saved = isr = __raw_readl(isr_reg) & enabled;
> 	...
> So this will stop kernel anyway.
> I just hoped to help in understanding of issue by log line. WARN_ON
> could be used for this.
> 
> As a variant compilation error could be added, to prevent situation when
> kernel is incorrectly configured.
> E.g.:
> #if !defined(CONFIG_ARCH_OMAP1) &&		
> 	!defined(CONFIG_ARCH_OMAP15XX) &&	
> 	!defined(CONFIG_ARCH_OMAP16XX) &&	
> 	!defined(CONFIG_ARCH_OMAP730) &&	
> 	!defined(CONFIG_ARCH_OMAP850) &&	
> 	!defined(CONFIG_ARCH_OMAP2) &&	
> 	!defined(CONFIG_ARCH_OMAP3) &&	
> 	!defined(CONFIG_ARCH_OMAP4)
> 
> #error "Incorrect arch configuration"
> #endif
> 
> But there are still cases when 'isr_reg' could have NULL value (if
> 'bank->method' is not equal to configured one).
> 
> Regards,
> Evgeny

If 'isr_reg' is NULL then interrupt could not be handled. We may unmask
the GPIO bank interrupt to continue handle GPIO interrupts for other
lines. And exit handler to prevent kernel oops since 'isr_reg' is
dereferenced later in code(see my message above).

if (WARN_ON(!isr_reg)) {
	desc->chip->unmask(irq);
	return;
	}

One thing I warn about that we could not clear edge sensitive
interrupts:
	_enable_gpio_irqbank(bank, isr_saved & ~level_mask, 0);
	_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
	_enable_gpio_irqbank(bank, isr_saved & ~level_mask, 1);


What do you think?

Evgeny.