2023-01-10 12:04:19

by Li Chen

[permalink] [raw]
Subject: [PATCH] clk: create write_enable file to control clk rate write and other dangerous ops permission

From: Li Chen <[email protected]>

It's common requirement for bsp debug/test to change clk rate from userspace.

Currently, we must define CLKOCK_ALLOW_WRITE_DEBUGFS then re-compile kernel
to allow this feature. Let's replace it with a "write_enable" file to
allow enable it at runtime.

Signed-off-by: Li Chen <[email protected]>
Change-Id: Ic4bf94c572c24f6979c2b7aea042fec654370220
---
drivers/clk/clk.c | 39 ++++++++++-----------------------------
1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e62552a75f08..668f691bf67a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -89,6 +89,7 @@ struct clk_core {
struct hlist_node debug_node;
#endif
struct kref ref;
+ bool write_enable;
};

#define CREATE_TRACE_POINTS
@@ -3263,8 +3264,6 @@ static int clk_dump_show(struct seq_file *s, void *data)
}
DEFINE_SHOW_ATTRIBUTE(clk_dump);

-#undef CLOCK_ALLOW_WRITE_DEBUGFS
-#ifdef CLOCK_ALLOW_WRITE_DEBUGFS
/*
* This can be dangerous, therefore don't provide any real compile time
* configuration option for this feature.
@@ -3275,6 +3274,9 @@ static int clk_rate_set(void *data, u64 val)
struct clk_core *core = data;
int ret;

+ if (!core->write_enable)
+ return -EACCES;
+
clk_prepare_lock();
ret = clk_core_set_rate_nolock(core, val);
clk_prepare_unlock();
@@ -3289,6 +3291,9 @@ static int clk_prepare_enable_set(void *data, u64 val)
struct clk_core *core = data;
int ret = 0;

+ if (!core->write_enable)
+ return -EACCES;
+
if (val)
ret = clk_prepare_enable(core->hw->clk);
else
@@ -3301,6 +3306,9 @@ static int clk_prepare_enable_get(void *data, u64 *val)
{
struct clk_core *core = data;

+ if (!core->write_enable)
+ return -EACCES;
+
*val = core->enable_count && core->prepare_count;
return 0;
}
@@ -3308,11 +3316,6 @@ static int clk_prepare_enable_get(void *data, u64 *val)
DEFINE_DEBUGFS_ATTRIBUTE(clk_prepare_enable_fops, clk_prepare_enable_get,
clk_prepare_enable_set, "%llu\n");

-#else
-#define clk_rate_set NULL
-#define clk_rate_mode 0444
-#endif
-
static int clk_rate_get(void *data, u64 *val)
{
struct clk_core *core = data;
@@ -3426,7 +3429,6 @@ static int current_parent_show(struct seq_file *s, void *data)
}
DEFINE_SHOW_ATTRIBUTE(current_parent);

-#ifdef CLOCK_ALLOW_WRITE_DEBUGFS
static ssize_t current_parent_write(struct file *file, const char __user *ubuf,
size_t count, loff_t *ppos)
{
@@ -3460,7 +3462,6 @@ static const struct file_operations current_parent_rw_fops = {
.llseek = seq_lseek,
.release = single_release,
};
-#endif

static int clk_duty_cycle_show(struct seq_file *s, void *data)
{
@@ -3524,7 +3525,6 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
debugfs_create_u32("clk_notifier_count", 0444, root, &core->notifier_count);
debugfs_create_file("clk_duty_cycle", 0444, root, core,
&clk_duty_cycle_fops);
-#ifdef CLOCK_ALLOW_WRITE_DEBUGFS
debugfs_create_file("clk_prepare_enable", 0644, root, core,
&clk_prepare_enable_fops);

@@ -3532,7 +3532,6 @@ static void clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
debugfs_create_file("clk_parent", 0644, root, core,
&current_parent_rw_fops);
else
-#endif
if (core->num_parents > 0)
debugfs_create_file("clk_parent", 0444, root, core,
&current_parent_fops);
@@ -3592,24 +3591,6 @@ static int __init clk_debug_init(void)
{
struct clk_core *core;

-#ifdef CLOCK_ALLOW_WRITE_DEBUGFS
- pr_warn("\n");
- pr_warn("********************************************************************\n");
- pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
- pr_warn("** **\n");
- pr_warn("** WRITEABLE clk DebugFS SUPPORT HAS BEEN ENABLED IN THIS KERNEL **\n");
- pr_warn("** **\n");
- pr_warn("** This means that this kernel is built to expose clk operations **\n");
- pr_warn("** such as parent or rate setting, enabling, disabling, etc. **\n");
- pr_warn("** to userspace, which may compromise security on your system. **\n");
- pr_warn("** **\n");
- pr_warn("** If you see this message and you are not debugging the **\n");
- pr_warn("** kernel, report this immediately to your vendor! **\n");
- pr_warn("** **\n");
- pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
- pr_warn("********************************************************************\n");
-#endif
-
rootdir = debugfs_create_dir("clk", NULL);

debugfs_create_file("clk_summary", 0444, rootdir, &all_lists,
--
2.34.1


2023-01-11 19:11:20

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH] clk: create write_enable file to control clk rate write and other dangerous ops permission

Quoting Li Chen (2023-01-10 03:45:39)
> From: Li Chen <[email protected]>
>
> It's common requirement for bsp debug/test to change clk rate from userspace.
>
> Currently, we must define CLKOCK_ALLOW_WRITE_DEBUGFS then re-compile kernel
> to allow this feature. Let's replace it with a "write_enable" file to
> allow enable it at runtime.

Nak. This design is intentional.

2023-01-12 03:09:14

by Li Chen

[permalink] [raw]
Subject: Re: [PATCH] clk: create write_enable file to control clk rate write and other dangerous ops permission

Hi Stephen,
---- On Thu, 12 Jan 2023 02:37:29 +0800 Stephen Boyd wrote ---
> Quoting Li Chen (2023-01-10 03:45:39)
> > From: Li Chen [email protected]>
> >
> > It's common requirement for bsp debug/test to change clk rate from userspace.
> >
> > Currently, we must define CLKOCK_ALLOW_WRITE_DEBUGFS then re-compile kernel
> > to allow this feature. Let's replace it with a "write_enable" file to
> > allow enable it at runtime.
>
> Nak. This design is intentional.

Thanks for your reply. Got it. But sometimes re-compile is somewhat low efficient. Is it acceptable to provide
a write_enable to clk_core(not enable by default), and allow clk driver to enable it inside clk_ops->init
via clk_hw->core->write_enable = 1?

Regards,
Li

2023-01-12 19:45:58

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH] clk: create write_enable file to control clk rate write and other dangerous ops permission

Quoting Li Chen (2023-01-11 19:05:34)
> Hi Stephen,
> ---- On Thu, 12 Jan 2023 02:37:29 +0800 Stephen Boyd wrote ---
> > Quoting Li Chen (2023-01-10 03:45:39)
> > > From: Li Chen [email protected]>
> > >
> > > It's common requirement for bsp debug/test to change clk rate from userspace.
> > >
> > > Currently, we must define CLKOCK_ALLOW_WRITE_DEBUGFS then re-compile kernel
> > > to allow this feature. Let's replace it with a "write_enable" file to
> > > allow enable it at runtime.
> >
> > Nak. This design is intentional.
>
> Thanks for your reply. Got it. But sometimes re-compile is somewhat low efficient. Is it acceptable to provide
> a write_enable to clk_core(not enable by default), and allow clk driver to enable it inside clk_ops->init
> via clk_hw->core->write_enable = 1?
>

No