| Message ID | 20241225060600.3094154-1-zhoubinbin@loongson.cn (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: clk-loongson2: Fix the number count of clk provider | expand |
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);
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);
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);
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
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);
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(-)