[PATCH v2] clk: clk-loongson2: Fix the number count of clk provider

[PATCH v2] clk: clk-loongson2: Fix the number count of clk provider

[Binbin Zhou]

Since commit 02fb4f008433 ("clk: clk-loongson2: Fix potential buffer
overflow in flexible-array member access"), the clk provider register is
failed.

The count of `clks_num` is shown below:

	for (p = data; p->name; p++)
		clks_num++;

In fact, `clks_num` represents the number of SoC clocks and should be
expressed as the maximum value of the clock binding id in use (p->id + 1).

Now we fix it to avoid the following error when trying to register a clk
provider:

[ 13.409595] of_clk_hw_onecell_get: invalid index 17

Cc: stable@vger.kernel.org
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Fixes: 02fb4f008433 ("clk: clk-loongson2: Fix potential buffer overflow in flexible-array member access")
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
V2:
- Add Gustavo A. R. Silva to cc list;
- Populate the onecell data with -ENOENT error pointers to avoid
  returning NULL, for it is a valid clock.

Link to V1:
https://lore.kernel.org/all/20241225060600.3094154-1-zhoubinbin@loongson.cn/

 drivers/clk/clk-loongson2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-loongson2.c b/drivers/clk/clk-loongson2.c
index 6bf51d5a49a1..9c240a2308f5 100644
--- a/drivers/clk/clk-loongson2.c
+++ b/drivers/clk/clk-loongson2.c
@@ -294,7 +294,7 @@ static int loongson2_clk_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	for (p = data; p->name; p++)
-		clks_num++;
+		clks_num = max(clks_num, p->id + 1);
 
 	clp = devm_kzalloc(dev, struct_size(clp, clk_data.hws, clks_num),
 			   GFP_KERNEL);
@@ -309,6 +309,9 @@ static int loongson2_clk_probe(struct platform_device *pdev)
 	clp->clk_data.num = clks_num;
 	clp->dev = dev;
 
+	/* Avoid returning NULL for unused id */
+	memset_p((void **)&clp->clk_data.hws, ERR_PTR(-ENOENT), clks_num);
+
 	for (i = 0; i < clks_num; i++) {
 		p = &data[i];
 		switch (p->type) {
-- 
2.43.5

Re: [PATCH v2] clk: clk-loongson2: Fix the number count of clk provider

[Stephen Boyd]

Quoting Binbin Zhou (2025-01-09 03:30:04)
> diff --git a/drivers/clk/clk-loongson2.c b/drivers/clk/clk-loongson2.c
> index 6bf51d5a49a1..9c240a2308f5 100644
> --- a/drivers/clk/clk-loongson2.c
> +++ b/drivers/clk/clk-loongson2.c
> @@ -294,7 +294,7 @@ static int loongson2_clk_probe(struct platform_device *pdev)
>                 return -EINVAL;
>  
>         for (p = data; p->name; p++)
> -               clks_num++;
> +               clks_num = max(clks_num, p->id + 1);
>  
>         clp = devm_kzalloc(dev, struct_size(clp, clk_data.hws, clks_num),
>                            GFP_KERNEL);
> @@ -309,6 +309,9 @@ static int loongson2_clk_probe(struct platform_device *pdev)
>         clp->clk_data.num = clks_num;
>         clp->dev = dev;
>  
> +       /* Avoid returning NULL for unused id */
> +       memset_p((void **)&clp->clk_data.hws, ERR_PTR(-ENOENT), clks_num);

This looks wrong. It's already an array of pointers, i.e. the type is
'struct clk_hw *[]' or 'struct clk_hw **' so we shouldn't need to take
the address of it. Should it be

	memset_p((void **)clkp->clk_data.hws, ERR_PTR(-ENOENT), clks_num);

? It's unfortunate that we have to cast here, but I guess this is the
best way we can indicate that the type should be an array of pointers.

Re: [PATCH v2] clk: clk-loongson2: Fix the number count of clk provider

[Binbin Zhou]

On Tue, Jan 14, 2025 at 2:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Binbin Zhou (2025-01-09 03:30:04)
> > diff --git a/drivers/clk/clk-loongson2.c b/drivers/clk/clk-loongson2.c
> > index 6bf51d5a49a1..9c240a2308f5 100644
> > --- a/drivers/clk/clk-loongson2.c
> > +++ b/drivers/clk/clk-loongson2.c
> > @@ -294,7 +294,7 @@ static int loongson2_clk_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >
> >         for (p = data; p->name; p++)
> > -               clks_num++;
> > +               clks_num = max(clks_num, p->id + 1);
> >
> >         clp = devm_kzalloc(dev, struct_size(clp, clk_data.hws, clks_num),
> >                            GFP_KERNEL);
> > @@ -309,6 +309,9 @@ static int loongson2_clk_probe(struct platform_device *pdev)
> >         clp->clk_data.num = clks_num;
> >         clp->dev = dev;
> >
> > +       /* Avoid returning NULL for unused id */
> > +       memset_p((void **)&clp->clk_data.hws, ERR_PTR(-ENOENT), clks_num);
>
> This looks wrong. It's already an array of pointers, i.e. the type is
> 'struct clk_hw *[]' or 'struct clk_hw **' so we shouldn't need to take
> the address of it. Should it be
>
>         memset_p((void **)clkp->clk_data.hws, ERR_PTR(-ENOENT), clks_num);

I'm very sorry, it was a cheap clerical error and I'll fix it right away.
>
> ? It's unfortunate that we have to cast here, but I guess this is the
> best way we can indicate that the type should be an array of pointers.

-- 
Thanks.
Binbin