omap: Ptr "isr_reg" tracked as NULL was dereferenced

PATCH Superseded Evgeny Kuznetsov 3 participants 8 messages Oct 5, 2010 – Oct 8, 2010

Subsystems (2)

Arm (32-bit) Arm sub-architectures

lore

Share & tools

lore

Evgeny Kuznetsov

author

2010-10-05 08:42 UTC #0 lore raw

cover omap: Ptr "isr_reg" tracked as NULL was dereferenced

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(-)

Evgeny Kuznetsov

author

2010-10-05 08:46 UTC #1 lore raw

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

From: Evgeny Kuznetsov <[email protected]>

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 <[email protected]>

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

arch/arm/plat-omap/gpio.c +7 -0

index c05c653..7669928 100644
@@ -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;

Signature · 1 line

1.6.3.3

Felipe Balbi

2010-10-05 09:32 UTC #2 lore raw

Hi,

arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1326

Quoted 5 lines · Evgeny Kuznetsov

arch/arm/plat-omap/gpio.c +5 -0

+	if (!isr_reg) {
+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
+				bank->method);
+		BUG();
+		}

this could be simply:

BUG_ON(!isr_reg);

Signature · 1 line

balbi

Evgeny Kuznetsov

author

2010-10-05 09:59 UTC #3 lore raw

Hi,

Yes, but no log in that case.

Regards,
Evgeny

Quoted 13 lines · ext Felipe Balbi

Hi,
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1326
Quoted 5 lines · Evgeny Kuznetsov
arch/arm/plat-omap/gpio.c +5 -0
+	if (!isr_reg) {
+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
+				bank->method);
+		BUG();
+		}
this could be simply:
BUG_ON(!isr_reg);

Felipe Balbi

2010-10-05 11:49 UTC #4 lore raw

Hi,

(don't top post)

Quoted 1 line · Evgeny Kuznetsov

Yes, but no log in that case.

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

Signature · 1 line

balbi

Kevin Hilman

2010-10-05 15:01 UTC #5 lore raw

In reply to Felipe Balbi

Quoted 11 lines · Felipe Balbi

Hi,
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1326
Quoted 5 lines · Evgeny Kuznetsov
arch/arm/plat-omap/gpio.c +5 -0
+	if (!isr_reg) {
+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
+				bank->method);
+		BUG();
+		}
this could be simply:

Quoted 1 line

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.

Quoted 1 line

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. */

Evgeny Kuznetsov

author

2010-10-06 06:37 UTC #6 lore raw

Quoted 13 lines · ext Kevin Hilman

Quoted 11 lines · Felipe Balbi
Hi,
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1326
Quoted 5 lines · Evgeny Kuznetsov
arch/arm/plat-omap/gpio.c +5 -0
+	if (!isr_reg) {
+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
+				bank->method);
+		BUG();
+		}
this could be simply:

Quoted 6 lines

Quoted 1 line
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

Quoted 14 lines

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. */

Evgeny Kuznetsov

author

2010-10-08 07:54 UTC #7 lore raw

Quoted 44 lines · Evgeny Kuznetsov

Quoted 13 lines · ext Kevin Hilman
Quoted 11 lines · Felipe Balbi
Hi,
arch/arm/plat-omap/gpio.c static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) · line 1326
Quoted 5 lines · Evgeny Kuznetsov
arch/arm/plat-omap/gpio.c +5 -0
+	if (!isr_reg) {
+		printk(KERN_ERR "FATAL: Incorrect GPIO method %i\n",
+				bank->method);
+		BUG();
+		}
this could be simply:
Quoted 6 lines
Quoted 1 line
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)

Quoted 8 lines

#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.