linux-kernel - Re: [PATCH] mfd: syscon: Don't free allocated name for regmap

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87pn10pfod.wl-maz@kernel.org>
Date:   Tue, 16 Feb 2021 08:42:10 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        kernel-team@...roid.com, Lee Jones <lee.jones@...aro.org>,
        Arnd Bergmann <arnd@...db.de>, Suman Anna <s-anna@...com>
Subject: Re: [PATCH] mfd: syscon: Don't free allocated name for regmap_config

Hi Tomi,

On Tue, 16 Feb 2021 06:34:50 +0000,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com> wrote:
> 
> Hi Marc,
> 
> On 03/09/2020 19:02, Marc Zyngier wrote:
> > 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@...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);
> > 
> 
> This patch causes lots of kmemleak reports, for example:

It may trigger some kmemleak reports, but at the time it was written,
this patch avoided some very nasty memory corruption with debugfs. The
problem is that the memory management responsibilities between syscon,
regmap and debugfs are rather obscure.

If you can come up with an accurate description of the life cycle of
syscon_config.name across these 3 subsystems, we may be able to fix it
for good.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.