linux-kernel - RE: [PATCHv2 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1286970199.12782.28.camel@ekuznets-lx-nokia>
Date:	Wed, 13 Oct 2010 15:43:19 +0400
From:	Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@...ia.com>
To:	"ext Varadarajan, Charulatha" <charu@...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>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"Balbi, Felipe" <balbi@...com>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"khilman@...prootsystems.com" <khilman@...prootsystems.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"tero.kristo@...ia.com" <tero.kristo@...ia.com>
Subject: RE: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was
 dereferenced

On Wed, 2010-10-13 at 16:25 +0530, ext Varadarajan, Charulatha wrote:
> 
> > -----Original Message-----
> > From: Evgeny Kuznetsov [mailto:EXT-Eugeny.Kuznetsov@...ia.com]
> > Sent: Wednesday, October 13, 2010 3:35 PM
> > To: tony@...mide.com
> > Cc: linux-omap@...r.kernel.org; linux-kernel@...r.kernel.org; linux-arm-
> > kernel@...ts.infradead.org; Balbi, Felipe; linux@....linux.org.uk;
> > khilman@...prootsystems.com; akpm@...ux-foundation.org; Varadarajan,
> > Charulatha; tero.kristo@...ia.com; ext-eugeny.kuznetsov@...ia.com
> > Subject: [PATCHv2 1/1] omap: Ptr "isr_reg" tracked as NULL was
> > dereferenced
> > 
> > From: Evgeny Kuznetsov <ext-eugeny.kuznetsov@...ia.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 issue.
> 
> Can you elaborate?
"isr_reg" should not be NULL. But if it is NULL then there is kernel
bug. And WARN_ON() used to show it.
I did not see this bug, this is potentially may happen.
> 
> > Warning and exit from function are added in this case.
> > Also compilation check is added for correct architecture
> > configuration.
> > 
> > Signed-off-by: Evgeny Kuznetsov <EXT-Eugeny.Kuznetsov@...ia.com>
> > ---
> >  arch/arm/plat-omap/gpio.c |   18 ++++++++++++++++++
> >  1 files changed, 18 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> > index c05c653..d04913c 100644
> > --- a/arch/arm/plat-omap/gpio.c
> > +++ b/arch/arm/plat-omap/gpio.c
> > @@ -1318,6 +1318,23 @@ 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 !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"
> 
> This is not required. If the architecture is not one of the above
> mentioned, gpio_irq_handler() will not be used/called at all.
This could be removed.

> Also all the possible gpio methods for a given OMAP architecture are
> already considered with "#ifdef"s and (bank->method) checks in
> gpio_irq_handler().
It is not cover all cased, e.g. for OMAP4 arch:
....	
	#if defined(CONFIG_ARCH_OMAP4)
		if (bank->method == METHOD_GPIO_44XX)
			isr_reg = bank->base + OMAP4_GPIO_IRQSTATUS0;
	#endif
.....

If (bank->method != METHOD_GPIO_44XX) then isr_reg will be NULL.
This should not happen, but potentially may have place.

> 
> > +
> > +#endif
> > +
> > +	if (WARN_ON(!isr_reg))
> > +		goto exit;
> 
> For the above mentioned reason, this isr_reg would be non-NULL. Have
> you observed this error anytime?
I did not see this bug, this is potentially may happen.
> 
> Also, the omap-gpio code has similar code spread all over and has to be
> anyway cleaned-up. Is there any reason why gpio_irq_handler() alone is
> addressed in this patch?
Here "isr_reg" is used later in code and may cause oops if it is NULL.
> 
> > +
> >  	while(1) {
> >  		u32 isr_saved, level_mask = 0;
> >  		u32 enabled;
> > @@ -1377,6 +1394,7 @@ static void gpio_irq_handler(unsigned int irq,
> > struct irq_desc *desc)
> >  	configured, we must unmask the bank interrupt only after
> >  	handler(s) are executed in order to avoid spurious bank
> >  	interrupt */
> > +exit:
> >  	if (!unmasked)
> >  		desc->chip->unmask(irq);
> > 
> > --
> > 1.6.3.3
> 


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