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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1286524159.24366.126.camel@ekuznets-lx-nokia>
Date:	Fri, 08 Oct 2010 11:49:19 +0400
From:	Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@...ia.com>
To:	ext Kevin Hilman <khilman@...prootsystems.com>, balbi@...com
Cc:	"tony@...mide.com" <tony@...mide.com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"zmc@...ian.net" <zmc@...ian.net>, a.j.buxton@...il.com
Subject: Re: [PATCH 1/1] omap: Ptr "isr_reg" tracked as NULL was
 dereferenced

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/