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

[PATCH] 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

Fixes: 02fb4f008433 ("clk: clk-loongson2: Fix potential buffer overflow in flexible-array member access")
Cc: stable@vger.kernel.org
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/clk/clk-loongson2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-loongson2.c b/drivers/clk/clk-loongson2.c
index 6bf51d5a49a1..b1b2038acd0b 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);
-- 
2.43.5

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

[Stephen Boyd]

Quoting Binbin Zhou (2024-12-24 22:05:59)
> 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
> 
> Fixes: 02fb4f008433 ("clk: clk-loongson2: Fix potential buffer overflow in flexible-array member access")
> Cc: stable@vger.kernel.org
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---

It's common practice to Cc the author of a patch in Fixes. Please do it
next time.

>  drivers/clk/clk-loongson2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-loongson2.c b/drivers/clk/clk-loongson2.c
> index 6bf51d5a49a1..b1b2038acd0b 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);

NULL is a valid clk. Either fill the onecell data with -ENOENT error
pointers, or stop using it and implement a custom version of
of_clk_hw_onecell_get() that doesn't allow invalid clks to be requested
from this provider.

>  
>         clp = devm_kzalloc(dev, struct_size(clp, clk_data.hws, clks_num),
>                            GFP_KERNEL);

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

[Binbin Zhou]

Hi Stephen:

Thanks for your review.

On Wed, Jan 8, 2025 at 5:25 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Binbin Zhou (2024-12-24 22:05:59)
> > 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
> >
> > Fixes: 02fb4f008433 ("clk: clk-loongson2: Fix potential buffer overflow in flexible-array member access")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
>
> It's common practice to Cc the author of a patch in Fixes. Please do it
> next time.

Oh, sorry it's my fault, I will do it next time.
>
> >  drivers/clk/clk-loongson2.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk-loongson2.c b/drivers/clk/clk-loongson2.c
> > index 6bf51d5a49a1..b1b2038acd0b 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);
>
> NULL is a valid clk. Either fill the onecell data with -ENOENT error
> pointers, or stop using it and implement a custom version of
> of_clk_hw_onecell_get() that doesn't allow invalid clks to be requested
> from this provider.

Emm...
Just in case, how about setting all items to ERR_PTR(-ENOENT) before
assigning them.
This is shown below:

               while (--clk_num >= 0)
                         clp->clk_data.hws[clk_num] = ERR_PTR(-ENOENT);
>
> >
> >         clp = devm_kzalloc(dev, struct_size(clp, clk_data.hws, clks_num),
> >                            GFP_KERNEL);



-- 
Thanks.
Binbin

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

[Stephen Boyd]

Quoting Binbin Zhou (2025-01-07 17:41:43)
> On Wed, Jan 8, 2025 at 5:25 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > Quoting Binbin Zhou (2024-12-24 22:05:59)
> > > diff --git a/drivers/clk/clk-loongson2.c b/drivers/clk/clk-loongson2.c
> > > index 6bf51d5a49a1..b1b2038acd0b 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);
> >
> > NULL is a valid clk. Either fill the onecell data with -ENOENT error
> > pointers, or stop using it and implement a custom version of
> > of_clk_hw_onecell_get() that doesn't allow invalid clks to be requested
> > from this provider.
> 
> Emm...
> Just in case, how about setting all items to ERR_PTR(-ENOENT) before
> assigning them.
> This is shown below:
> 
>                while (--clk_num >= 0)
>                          clp->clk_data.hws[clk_num] = ERR_PTR(-ENOENT);

Or something like:

	memset_p(&clk->clk_data.hws, ERR_PTR(-ENOENT), clk_num);

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

[Binbin Zhou]

Hi Stephen:

Thanks for your comments.

On Thu, Jan 9, 2025 at 3:20 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Binbin Zhou (2025-01-07 17:41:43)
> > On Wed, Jan 8, 2025 at 5:25 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > > Quoting Binbin Zhou (2024-12-24 22:05:59)
> > > > diff --git a/drivers/clk/clk-loongson2.c b/drivers/clk/clk-loongson2.c
> > > > index 6bf51d5a49a1..b1b2038acd0b 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);
> > >
> > > NULL is a valid clk. Either fill the onecell data with -ENOENT error
> > > pointers, or stop using it and implement a custom version of
> > > of_clk_hw_onecell_get() that doesn't allow invalid clks to be requested
> > > from this provider.
> >
> > Emm...
> > Just in case, how about setting all items to ERR_PTR(-ENOENT) before
> > assigning them.
> > This is shown below:
> >
> >                while (--clk_num >= 0)
> >                          clp->clk_data.hws[clk_num] = ERR_PTR(-ENOENT);
>
> Or something like:
>
>         memset_p(&clk->clk_data.hws, ERR_PTR(-ENOENT), clk_num);

Indeed, it looks better and cleaner.
I'll update in V2 soon.

--
Thanks.
Binbin