Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config

[Marc Zyngier <maz@kernel.org>]

Hi all,

On 2020-07-27 22:10, Suman Anna wrote:
> The DT node full name is currently being used in regmap_config
> which in turn is used to create the regmap debugfs directories.
> This name however is not guaranteed to be unique and the regmap
> debugfs registration can fail in the cases where the syscon nodes
> have the same unit-address but are present in different DT node
> hierarchies. Replace this logic using the syscon reg resource
> address instead (inspired from logic used while creating platform
> devices) to ensure a unique name is given for each syscon.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> Hi Arnd,
> Lee is looking for your review on this patch. Can you please
> review and provide your comments.
> 
> This is a resend of the patch that was posted previously, rebased
> now onto latest kernel.
> 
> v2: https://patchwork.kernel.org/patch/11353355/
>  - Fix build warning reported by kbuild test bot
> v1: https://patchwork.kernel.org/patch/11346363/
> 
>  drivers/mfd/syscon.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 3a97816d0cba..75859e492984 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -101,12 +101,14 @@ static struct syscon *of_syscon_register(struct
> device_node *np, bool check_clk)
>  		}
>  	}
> 
> -	syscon_config.name = of_node_full_name(np);
> +	syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", np,
> +				       (u64)res.start);
>  	syscon_config.reg_stride = reg_io_width;
>  	syscon_config.val_bits = reg_io_width * 8;
>  	syscon_config.max_register = resource_size(&res) - reg_io_width;
> 
>  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> +	kfree(syscon_config.name);
>  	if (IS_ERR(regmap)) {
>  		pr_err("regmap init failed\n");
>  		ret = PTR_ERR(regmap);

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.

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.

Fixes: e15d7f2b81d2 ("mfd: syscon: Use a unique name with 
regmap_config")
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
  drivers/mfd/syscon.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 75859e492984..7a660411c562 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -108,7 +108,6 @@ static struct syscon *of_syscon_register(struct 
device_node *np, bool check_clk)
  	syscon_config.max_register = resource_size(&res) - reg_io_width;

  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
-	kfree(syscon_config.name);
  	if (IS_ERR(regmap)) {
  		pr_err("regmap init failed\n");
  		ret = PTR_ERR(regmap);
@@ -145,6 +144,7 @@ static struct syscon *of_syscon_register(struct 
device_node *np, bool check_clk)
  	regmap_exit(regmap);
  err_regmap:
  	iounmap(base);
+	kfree(syscon_config.name);
  err_map:
  	kfree(syscon);
  	return ERR_PTR(ret);
-- 
2.27.0


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