linux-kernel - Re: [PATCH] PM / clock_ops: Fix clock error check in __pm_clk

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: <20150508210557.GC38039@dtor-ws>
Date:	Fri, 8 May 2015 14:05:57 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:	Geert Uytterhoeven <geert+renesas@...der.be>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Grygorii Strashko <Grygorii.Strashko@...aro.org>,
	Kevin Hilman <khilman@...aro.org>,
	Santosh Shilimkar <santosh.shilimkar@...com>,
	Linux PM list <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PM / clock_ops: Fix clock error check in __pm_clk_add()

On Fri, May 08, 2015 at 10:59:04PM +0200, Geert Uytterhoeven wrote:
> On Fri, May 8, 2015 at 7:19 PM, Dmitry Torokhov
> <dmitry.torokhov@...il.com> wrote:
> > On Fri, May 08, 2015 at 10:47:43AM +0200, Geert Uytterhoeven wrote:
> >> In the final iteration of commit 245bd6f6af8a62a2 ("PM / clock_ops: Add
> >> pm_clk_add_clk()"), a refcount increment was added by Grygorii Strashko.
> >> However, the accompanying IS_ERR() check operates on the wrong clock
> >> pointer, which is always zero at this point, i.e. not an error.
> >> This may lead to a NULL pointer dereference later, when __clk_get()
> >> tries to dereference an error pointer.
> >>
> >> Check the passed clock pointer instead to fix this.
> >
> > Frankly I would remove the check altogether. Why do we only check for
> > IS_ERR and not NULL or otherwise validate the pointer? The clk is passed
> 
> __clk_get() does the NULL check.

No, not really. It _handles_ clk being NULL and returns "everything is
fine". In any case it is __clk_get's decision what to do.

I dislike gratuitous checks of arguments passed in. Instead of relying
on APIs refusing grabage we better not pass garbage to these APIs in the
first place. So I'd change it to trust that we are given a usable
pointer and simply do:

	if (!__clk_get(clk)) {
		kfree(ce);
		return -ENOENTl
	}

Thanks.

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