Re: [PATCH AUTOSEL 5.8 14/29] regmap: debugfs: Fix handling of name string for debugfs init delays

2020-10-04 Thread Sasha Levin

On Tue, Sep 29, 2020 at 08:33:34AM +, Charles Keepax wrote:

On Mon, Sep 28, 2020 at 09:30:11PM -0400, Sasha Levin wrote:

From: Charles Keepax 

[ Upstream commit 94cc89eb8fa5039fcb6e3e3d50f929ddcccee095 ]

In regmap_debugfs_init the initialisation of the debugfs is delayed
if the root node isn't ready yet. Most callers of regmap_debugfs_init
pass the name from the regmap_config, which is considered temporary
ie. may be unallocated after the regmap_init call returns. This leads
to a potential use after free, where config->name has been freed by
the time it is used in regmap_debugfs_initcall.



Afraid this patch had some issues if you are back porting it you
definitely need to take these two patches as well:

commit 1d512ee861b80da63cbc501b973c53131aa22f29
regmap: debugfs: Fix more error path regressions


Looks like 1d512ee861b is queued for the merge window even though it's a
bugfix for this release?

I'm going to drop this patch.

--
Thanks,
Sasha


Re: [PATCH AUTOSEL 5.8 14/29] regmap: debugfs: Fix handling of name string for debugfs init delays

2020-09-29 Thread Charles Keepax
On Mon, Sep 28, 2020 at 09:30:11PM -0400, Sasha Levin wrote:
> From: Charles Keepax 
> 
> [ Upstream commit 94cc89eb8fa5039fcb6e3e3d50f929ddcccee095 ]
> 
> In regmap_debugfs_init the initialisation of the debugfs is delayed
> if the root node isn't ready yet. Most callers of regmap_debugfs_init
> pass the name from the regmap_config, which is considered temporary
> ie. may be unallocated after the regmap_init call returns. This leads
> to a potential use after free, where config->name has been freed by
> the time it is used in regmap_debugfs_initcall.
> 

Afraid this patch had some issues if you are back porting it you
definitely need to take these two patches as well:

commit 1d512ee861b80da63cbc501b973c53131aa22f29
regmap: debugfs: Fix more error path regressions

commit d36cb0205f034e943aa29e35b59c6a441f0056b5
regmap: debugfs: Add back in erroneously removed initialisation of ret

Thanks,
Charles


[PATCH AUTOSEL 5.8 14/29] regmap: debugfs: Fix handling of name string for debugfs init delays

2020-09-28 Thread Sasha Levin
From: Charles Keepax 

[ Upstream commit 94cc89eb8fa5039fcb6e3e3d50f929ddcccee095 ]

In regmap_debugfs_init the initialisation of the debugfs is delayed
if the root node isn't ready yet. Most callers of regmap_debugfs_init
pass the name from the regmap_config, which is considered temporary
ie. may be unallocated after the regmap_init call returns. This leads
to a potential use after free, where config->name has been freed by
the time it is used in regmap_debugfs_initcall.

This situation can be seen on Zynq, where the architecture init_irq
callback registers a syscon device, using a local variable for the
regmap_config. As init_irq is very early in the platform bring up the
regmap debugfs root isn't ready yet. Although this doesn't crash it
does result in the debugfs entry not having the correct name.

Regmap already sets map->name from config->name on the regmap_init
path and the fact that a separate field is used to pass the name
to regmap_debugfs_init appears to be an artifact of the debugfs
name being added before the map name. As such this patch updates
regmap_debugfs_init to use map->name, which is already duplicated from
the config avoiding the issue.

This does however leave two lose ends, both regmap_attach_dev and
regmap_reinit_cache can be called after a regmap is registered and
would have had the effect of applying a new name to the debugfs
entries. In both of these cases it was chosen to update the map
name. In the case of regmap_attach_dev there are 3 users that
currently use this function to update the name, thus doing so avoids
changes for those users and it seems reasonable that attaching
a device would want to set the name of the map. In the case of
regmap_reinit_cache the primary use-case appears to be devices that
need some register access to identify the device (for example devices
in the same family) and then update the cache to match the exact
hardware. Whilst no users do currently update the name here, given the
use-case it seemed reasonable the name might want to be updated once
the device is better identified.

Signed-off-by: Charles Keepax 
Link: 
https://lore.kernel.org/r/20200917120828.12987-1-ckee...@opensource.cirrus.com
Signed-off-by: Mark Brown 
Signed-off-by: Sasha Levin 
---
 drivers/base/regmap/internal.h   |  4 +--
 drivers/base/regmap/regmap-debugfs.c |  7 ++---
 drivers/base/regmap/regmap.c | 44 +---
 3 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 3d80c4b43f720..e0ff8e90ebdcf 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -217,7 +217,7 @@ struct regmap_field {
 
 #ifdef CONFIG_DEBUG_FS
 extern void regmap_debugfs_initcall(void);
-extern void regmap_debugfs_init(struct regmap *map, const char *name);
+extern void regmap_debugfs_init(struct regmap *map);
 extern void regmap_debugfs_exit(struct regmap *map);
 
 static inline void regmap_debugfs_disable(struct regmap *map)
@@ -227,7 +227,7 @@ static inline void regmap_debugfs_disable(struct regmap 
*map)
 
 #else
 static inline void regmap_debugfs_initcall(void) { }
-static inline void regmap_debugfs_init(struct regmap *map, const char *name) { 
}
+static inline void regmap_debugfs_init(struct regmap *map) { }
 static inline void regmap_debugfs_exit(struct regmap *map) { }
 static inline void regmap_debugfs_disable(struct regmap *map) { }
 #endif
diff --git a/drivers/base/regmap/regmap-debugfs.c 
b/drivers/base/regmap/regmap-debugfs.c
index f58baff2be0af..b6d63ef16b442 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -17,7 +17,6 @@
 
 struct regmap_debugfs_node {
struct regmap *map;
-   const char *name;
struct list_head link;
 };
 
@@ -544,11 +543,12 @@ static const struct file_operations 
regmap_cache_bypass_fops = {
.write = regmap_cache_bypass_write_file,
 };
 
-void regmap_debugfs_init(struct regmap *map, const char *name)
+void regmap_debugfs_init(struct regmap *map)
 {
struct rb_node *next;
struct regmap_range_node *range_node;
const char *devname = "dummy";
+   const char *name = map->name;
 
/*
 * Userspace can initiate reads from the hardware over debugfs.
@@ -569,7 +569,6 @@ void regmap_debugfs_init(struct regmap *map, const char 
*name)
if (!node)
return;
node->map = map;
-   node->name = name;
mutex_lock(_debugfs_early_lock);
list_add(>link, _debugfs_early_list);
mutex_unlock(_debugfs_early_lock);
@@ -679,7 +678,7 @@ void regmap_debugfs_initcall(void)
 
mutex_lock(_debugfs_early_lock);
list_for_each_entry_safe(node, tmp, _debugfs_early_list, link) {
-   regmap_debugfs_init(node->map, node->name);
+   regmap_debugfs_init(node->map);
list_del(>link);