Re: [PATCH] PM / clock_ops: Fix clock error check in __pm_clk_add()

[Dmitry Torokhov]

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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/