[PATCH] regmap: debugfs: potentially duplicate the name string of the config

[PATCH] regmap: debugfs: potentially duplicate the name string of the config

[Dafna Hirschfeld]

In function regmap_debugfs_init the name of the regmap_config is assigned
in a node of regmap_debugfs_early_list to be used later after regmap
is initialized. It is unknown how and when the name is allocated and freed.
Therefore the name should be copied to the node using 'kstrdup_const'.
This fixes an error
"debugfs: Directory 'dummy-' with parent 'regmap' already present!"
where the name was freed in function of_syscon_register before
it was accessed.

Fixes: a52eaeb1898bc (regmap: debugfs: Fix a boot time crash with early regmap init)
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/base/regmap/regmap-debugfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index f58baff2be0a..0845c4a2e33e 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -569,7 +569,7 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
 		if (!node)
 			return;
 		node->map = map;
-		node->name = name;
+		node->name = kstrdup_const(name, GFP_KERNEL);
 		mutex_lock(&regmap_debugfs_early_lock);
 		list_add(&node->link, &regmap_debugfs_early_list);
 		mutex_unlock(&regmap_debugfs_early_lock);
@@ -664,6 +664,7 @@ void regmap_debugfs_exit(struct regmap *map)
 					 link) {
 			if (node->map == map) {
 				list_del(&node->link);
+				kfree_const(node->name);
 				kfree(node);
 			}
 		}
@@ -681,6 +682,7 @@ void regmap_debugfs_initcall(void)
 	list_for_each_entry_safe(node, tmp, &regmap_debugfs_early_list, link) {
 		regmap_debugfs_init(node->map, node->name);
 		list_del(&node->link);
+		kfree_const(node->name);
 		kfree(node);
 	}
 	mutex_unlock(&regmap_debugfs_early_lock);
-- 
2.17.1

Re: [PATCH] regmap: debugfs: potentially duplicate the name string of the config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On Tue, Sep 08, 2020 at 05:28:59PM +0200, Dafna Hirschfeld wrote:
> In function regmap_debugfs_init the name of the regmap_config is assigned
> in a node of regmap_debugfs_early_list to be used later after regmap
> is initialized. It is unknown how and when the name is allocated and freed.
> Therefore the name should be copied to the node using 'kstrdup_const'.

It does not follow that the name should be copied, it equally follows
(and is expected by the API given that the common case is that the name
is a static string assigned at build time) that the caller should not
free the string while the regmap is active.

> This fixes an error
> "debugfs: Directory 'dummy-' with parent 'regmap' already present!"
> where the name was freed in function of_syscon_register before
> it was accessed.

Fix the caller.  It is *very* much unclear to me why a syscon would be
assigning a name for a regmap it creates in the first place.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: debugfs: potentially duplicate the name string of the config

[Dafna Hirschfeld]

Hi, thank you for the fast review,

Am 08.09.20 um 17:35 schrieb Mark Brown:
> On Tue, Sep 08, 2020 at 05:28:59PM +0200, Dafna Hirschfeld wrote:
>> In function regmap_debugfs_init the name of the regmap_config is assigned
>> in a node of regmap_debugfs_early_list to be used later after regmap
>> is initialized. It is unknown how and when the name is allocated and freed.
>> Therefore the name should be copied to the node using 'kstrdup_const'.
> 
> It does not follow that the name should be copied, it equally follows
> (and is expected by the API given that the common case is that the name
> is a static string assigned at build time) that the caller should not
> free the string while the regmap is active.

I see that the same copy is already done in function __regmap_init in drivers/base/regmap/regmap.c
added in patch 8253bb3f82554 "regmap: potentially duplicate the name string stored in regmap"
so I thought I'll do the same.

> 
>> This fixes an error
>> "debugfs: Directory 'dummy-' with parent 'regmap' already present!"
>> where the name was freed in function of_syscon_register before
>> it was accessed.
> 
> Fix the caller.  It is *very* much unclear to me why a syscon would be
> assigning a name for a regmap it creates in the first place.

I'll have a deeper look.

Thanks,
Dafna

> 

Re: [PATCH] regmap: debugfs: potentially duplicate the name string of the config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

On Tue, Sep 08, 2020 at 06:07:56PM +0200, Dafna Hirschfeld wrote:
> Am 08.09.20 um 17:35 schrieb Mark Brown:

> > It does not follow that the name should be copied, it equally follows
> > (and is expected by the API given that the common case is that the name
> > is a static string assigned at build time) that the caller should not
> > free the string while the regmap is active.

> I see that the same copy is already done in function __regmap_init in drivers/base/regmap/regmap.c
> added in patch 8253bb3f82554 "regmap: potentially duplicate the name string stored in regmap"
> so I thought I'll do the same.

Hrm, right.  In that case why not just use that copy we've already
taken?

> > > This fixes an error
> > > "debugfs: Directory 'dummy-' with parent 'regmap' already present!"
> > > where the name was freed in function of_syscon_register before
> > > it was accessed.

> > Fix the caller.  It is *very* much unclear to me why a syscon would be
> > assigning a name for a regmap it creates in the first place.

> I'll have a deeper look.

Whatever happens that caller looks like it shouldn't be creating an
additional name unless I'm missing something about the context.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]