2024-03-13 06:46:13

by Duanqiang Wen

[permalink] [raw]
Subject: [PATCH clk] clk: clkdev: add error messages for name exceeding maximum length

if one device register clkdev with dev_id or con_id
greater than maximum length, clkdev_create functions
will not return err, but clk_find functions will not
match the device, it's difficult to identify issues
for developers.So add error messages for dev_id greater
than 20 characters and con_id greater than 16 characters.

Signed-off-by: Duanqiang Wen <[email protected]>
---
drivers/clk/clk.c | 6 ++++++
drivers/clk/clkdev.c | 11 +++++++++++
include/linux/clk-provider.h | 1 +
3 files changed, 18 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2253c154a824..10d3e0d93648 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -295,6 +295,12 @@ struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw)
}
EXPORT_SYMBOL_GPL(clk_hw_get_parent);

+struct device *clk_hw_get_dev(const struct clk_hw *hw)
+{
+ return hw->core->dev;
+}
+EXPORT_SYMBOL_GPL(clk_hw_get_dev);
+
static struct clk_core *__clk_lookup_subtree(const char *name,
struct clk_core *core)
{
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index ee37d0be6877..620dc1e80b48 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -158,6 +158,9 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt,
va_list ap)
{
struct clk_lookup_alloc *cla;
+ struct device *dev;
+
+ dev = clk_hw_get_dev(hw);

cla = kzalloc(sizeof(*cla), GFP_KERNEL);
if (!cla)
@@ -165,11 +168,19 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt,

cla->cl.clk_hw = hw;
if (con_id) {
+ if (strlen(dev_fmt) >= MAX_CON_ID) {
+ pr_err("%s:con_id string cannot be greater than 16 characters\n", dev_fmt);
+ return NULL;
+ }
strscpy(cla->con_id, con_id, sizeof(cla->con_id));
cla->cl.con_id = cla->con_id;
}

if (dev_fmt) {
+ if (strlen(dev_fmt) >= MAX_DEV_ID) {
+ pr_err("%s:dev_id string cannot be greater than 20 characters\n", dev_fmt);
+ return NULL;
+ }
vscnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
cla->cl.dev_id = cla->dev_id;
}
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1293c38ddb7f..88f3e3dde147 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1318,6 +1318,7 @@ int clk_hw_set_parent(struct clk_hw *hw, struct clk_hw *new_parent);
unsigned int __clk_get_enable_count(struct clk *clk);
unsigned long clk_hw_get_rate(const struct clk_hw *hw);
unsigned long clk_hw_get_flags(const struct clk_hw *hw);
+struct device *clk_hw_get_dev(const struct clk_hw *hw);
#define clk_hw_can_set_rate_parent(hw) \
(clk_hw_get_flags((hw)) & CLK_SET_RATE_PARENT)

--
2.27.0



2024-03-13 10:09:24

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH clk] clk: clkdev: add error messages for name exceeding maximum length

On Wed, Mar 13, 2024 at 02:42:52PM +0800, Duanqiang Wen wrote:
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index ee37d0be6877..620dc1e80b48 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -158,6 +158,9 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt,
> va_list ap)
> {
> struct clk_lookup_alloc *cla;
> + struct device *dev;
> +
> + dev = clk_hw_get_dev(hw);

Sorry, but no, clkdev should have minimal dependencies on CCF (it was
designed to be completely independent). I'd prefer not to add this.
Just print the formatted dev_fmt+ap and the con_id when reporting
errors.

>
> cla = kzalloc(sizeof(*cla), GFP_KERNEL);
> if (!cla)
> @@ -165,11 +168,19 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt,
>
> cla->cl.clk_hw = hw;
> if (con_id) {
> + if (strlen(dev_fmt) >= MAX_CON_ID) {

This is wrong (uses dev_fmt not con_id). Also, use sizeof(cla->con_id)
to test against.

> + pr_err("%s:con_id string cannot be greater than 16 characters\n", dev_fmt);

Cleanup?

> + return NULL;
> + }
> strscpy(cla->con_id, con_id, sizeof(cla->con_id));
> cla->cl.con_id = cla->con_id;
> }
>
> if (dev_fmt) {
> + if (strlen(dev_fmt) >= MAX_DEV_ID) {

This is also wrong. The length of the format string does not give any
information on how long the resulting string actually is.

> + pr_err("%s:dev_id string cannot be greater than 20 characters\n", dev_fmt);

Cleanup?

> + return NULL;
> + }
> vscnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);

Using vsnprintf() here and checking whether the return value is larger
than sizeof(cla->dev_id) would be better.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!