On 2020-08-27 21:32, Suman Anna wrote:

Hi Marc,

+ Mark Brown

On 8/27/20 3:06 PM, Marc Zyngier wrote:

quoted

Hi Suman,

On 2020-08-27 19:28, Suman Anna wrote:

quoted

Hi Marc,

On 8/27/20 9:46 AM, Marc Zyngier wrote:

[...]

quoted

quoted

quoted

This patch triggers some illegal memory accesses when debugfs is
enabled, as regmap does rely on config->name to be persistent
when the debugfs registration is deferred via 
regmap_debugfs_early_list
(__regmap_init() -> regmap_attach_dev() -> 
regmap_debugfs_init()...),
leading to a KASAN splat on demand.

Thanks, I missed the subtlety around the debugfs registration.

quoted

I came up with the following patch that solves the issue for me.

Thanks,

        M.

From fd3f5f2bf72df53be18d13914fe349a34f81f16b Mon Sep 17 00:00:00 
2001
From: Marc Zyngier <maz@kernel.org>
Date: Thu, 27 Aug 2020 14:45:34 +0100
Subject: [PATCH] mfd: syscon: Don't free allocated name for 
regmap_config

The name allocated for the regmap_config structure is freed
pretty early, right after the registration of the MMIO region.

Unfortunately, that doesn't follow the life cycle that debugfs
expects, as it can access the name field long after the free
has occured.

Move the free on the error path, and keep it forever otherwise.

Hmm, this is exactly what I was trying to avoid. The regmap_init does 
duplicate
the name into map->name if config->name is given, and the regmap 
debugfs makes
another copy of its own into debugfs_name when actually registered. 
If the rules
for regmap_init is that the config->name should be persistent, then I 
guess we
have no choice but to go with the below fix.

Does something like below help?

diff --git a/drivers/base/regmap/regmap.c 

b/drivers/base/regmap/regmap.c
index e93700af7e6e..96d8a0161c89 100644

--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c

@@ -1137,7 +1137,7 @@ struct regmap *__regmap_init(struct device 

*dev,
                if (ret != 0)
                        goto err_regcache;
        } else {
-               regmap_debugfs_init(map, config->name);
+               regmap_debugfs_init(map, map->name);

But there are couple of other places in regmap code that uses 
config->name, but
those won't be exercised with the syscon code.

Is config->name always the same as map->name? If so, why don't you 
just
pass map once and for all? Is the lifetime of map->name the same as
that of config->name?

map->name is created (kstrdup_const) from config->name if not NULL, so 
above
replacement should be exactly equivalent, map is filled in 
_regmap_init. But it
does make the regmap_debugfs_init callsites in the file look 
dissimilar.

quoted

My worry with this approach is that we start changing stuff in a rush,
and this would IMHO deserve a thorough investigation of whether this
change is actually safe.

I'd rather take the safe approach of either keeping the memory around
until we clearly understand what the implications are (and probably
this should involve the regmap maintainer), or to revert this patch
until we figure out the actual life cycle of the various names.

Yeah, agreed. Let's see what Mark suggests.

Mark,
Can you clarify the lifecycle expectations on the config->name and do 
you have
any suggestions here?

Have we reached a conclusion here? Can we get a fix in mainline?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel