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

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

[Li Chen]

From: Li Chen <lchen@ambarella.com>

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 <lchen@ambarella.com>
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

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

[Stephen Boyd]

Quoting Li Chen (2023-01-10 03:45:39)
> From: Li Chen <lchen@ambarella.com>
> 
> 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.

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

[Li Chen]

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 lchen@ambarella.com>
 > > 
 > > 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

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

[Stephen Boyd]

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 lchen@ambarella.com>
>  > > 
>  > > 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