linux-kernel - Re: clk: rockchip: Checking a kmemdup() call in rockchip_clk_register

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]

Date:   Tue, 15 Oct 2019 22:29:03 +0200
From:   Heiko Stuebner <heiko@...ech.de>
To:     Markus Elfring <Markus.Elfring@....de>
Cc:     linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org,
        kernel-janitors@...r.kernel.org,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Aditya Pakki <pakki001@....edu>, Kangjie Lu <kjlu@....edu>,
        Navid Emamdoost <emamd001@....edu>,
        Stephen McCamant <smccaman@....edu>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: clk: rockchip: Checking a kmemdup() call in rockchip_clk_register_pll()

Am Montag, 14. Oktober 2019, 09:26:41 CEST schrieb Markus Elfring:
> > The other option would be to panic, but the kernel should not
> > panic if other options are available - and continuing with a static
> > pll frequency is less invasive in the error case.
> 
> I would like to point out that this function implementation contains
> the following source code already.
> 
> …
> 	/* name the actual pll */
> 	snprintf(pll_name, sizeof(pll_name), "pll_%s", name);
> 
> 	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> 	if (!pll)
> 		return ERR_PTR(-ENOMEM);
> …
> 
> 
> 
> …
> > +++ b/drivers/clk/rockchip/clk-pll.c
> > @@ -909,14 +909,16 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx,
> …
> > -		pll->rate_count = len;
> >  		pll->rate_table = kmemdup(rate_table,
> >  					pll->rate_count *
> >  					sizeof(struct rockchip_pll_rate_table),
> >  					GFP_KERNEL);
> > -		WARN(!pll->rate_table,
> > -			"%s: could not allocate rate table for %s\n",
> > -			__func__, name);
> > +
> > +		/*
> > +		 * Set num rates to 0 if kmemdup fails. That way the clock
> > +		 * at least can report its rate and stays usable.
> > +		 */
> > +		pll->rate_count = pll->rate_table ? len : 0;
> 
> Can an other error handling strategy make sense occasionally?
>
> 
> …
> 		if (!pll->rate_table) {
> 			clk_unregister(mux_clk);
> 			mux_clk = ERR_PTR(-ENOMEM);
> 			goto err_mux;
> 		}
> …
> 
> 
> Would you like to adjust such exception handling another bit?

Nope.

The big difference is that clocks rely heavily on their names to establish
the clock tree parentship. So the PLL cannot work without the name
but can provide some means of functionality without the rate-table
especially as bootloaders do generally initialize a PLL to some form of
sane frequency.

Heiko