Re: [PATCH 2/3] clk: samsung: clk-pll: Add support for pll_309

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Logic Error / Out-of-bounds Read when rate_table is empty
> - [Medium] Single-Responsibility Violation: Undocumented architectural refactoring
> --
> 
> --- Patch [2]: [PATCH 2/3] clk: samsung: clk-pll: Add support for pll_309 ---
> commit d62df56ff2e6bf73437e6e25e1335b3fb4571268
> Author: Aiden Isik <[email protected]>
> 
> clk: samsung: clk-pll: Add support for pll_309
> 
> This commit adds support for the pll_309 used in the Exynos5515 SoC, defining
> the PLL characteristics which are handled similarly to pll_531x.
> 
> > diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> > index e74552846ba34..8b1474aeaf0c8 100644
> > --- a/drivers/clk/samsung/clk-pll.c
> > +++ b/drivers/clk/samsung/clk-pll.c
> > @@ -1593,32 +1593,35 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
> >  {
> >  	struct samsung_clk_pll *pll;
> >  	struct clk_init_data init;
> > -	unsigned int len = 0;
> > -	int ret;
> > +	int ret, len;
> >
> > -	if (pll_clk->rate_table) {
> > -		/* find count of rates in rate_table */
> > -		while (pll_clk->rate_table[len].rate != 0)
> > -			len++;
> > -	}
> > -
> > -	pll = kzalloc_flex(*pll, rate_table, len);
> > +	pll = kzalloc_obj(*pll);
> 
> [Severity: Medium]
> This isn't a bug, but the commit message only mentions adding support for the
> new pll_309 type, while this code changes the allocation semantics of the
> rate_table across the entire driver, switching from a flexible array member
> to a separate pointer allocation.
> 
> Could this architectural refactoring be split into a separate patch or
> documented in the commit message?

This was a mistake caused when updating patches against next.
Will fix in v2.

> 
> [ ... ]
> 
> > @@ -1637,7 +1640,7 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
> >  	case pll_a9fracm:
> >  		pll->enable_offs = PLL35XX_ENABLE_SHIFT;
> >  		pll->lock_offs = PLL35XX_LOCK_STAT_SHIFT;
> > -		if (!pll->rate_count)
> > +		if (!pll->rate_table)
> 
> [Severity: High]
> If a clock is registered with an empty rate table containing only the zero
> terminator, kmemdup_array() is called with a count of 0 earlier in this
> function and returns ZERO_SIZE_PTR rather than NULL.
> 
> Since ZERO_SIZE_PTR is not NULL, this check evaluates to false, which
> incorrectly assigns the full operations instead of the safe min_ops fallback.
> 
> If samsung_pll_determine_rate() is subsequently called, it skips its loop
> because rate_count is 0, and attempts to access rate_table[-1]:
> 
> samsung_pll_determine_rate() {
>     ...
>     req->rate = rate_table[i - 1].rate;
> }
> 
> Since rate_table is ZERO_SIZE_PTR, will this result in an out-of-bounds
> memory read?
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
> 

Kind regards,
- Aiden Isik