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?

[ ... ]

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