| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <74bc1f9f-cc48-cec9-85f4-3376b66b40fc@ti.com>
Date: Thu, 27 Aug 2020 13:28:21 -0500
From: Suman Anna <s-anna@...com>
To: Marc Zyngier <maz@...nel.org>
CC: Lee Jones <lee.jones@...aro.org>, Arnd Bergmann <arnd@...db.de>,
Grzegorz Jaszczyk <grzegorz.jaszczyk@...aro.org>,
David Lechner <david@...hnology.com>,
Tony Lindgren <tony@...mide.com>,
<linux-kernel@...r.kernel.org>, <linux-omap@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
Roger Quadros <rogerq@...com>, <kernel-team@...roid.com>
Subject: Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with
regmap_config
Hi Marc,
On 8/27/20 9:46 AM, Marc Zyngier wrote:
> 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@...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@...x", 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.
>
Thanks, I missed the subtlety around the debugfs registration.
> 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@...nel.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.
regards
Suman
>
> Fixes: e15d7f2b81d2 ("mfd: syscon: Use a unique name with regmap_config")
> Signed-off-by: Marc Zyngier <maz@...nel.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);
Powered by blists - more mailing lists