linux-kernel - Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <65722f2a94d68_1c7b6229452@iweiny-mobl.notmuch>
Date:   Thu, 7 Dec 2023 12:46:34 -0800
From:   Ira Weiny <ira.weiny@...el.com>
To:     Dave Jiang <dave.jiang@...el.com>,
        Dinghao Liu <dinghao.liu@....edu.cn>
CC:     Vishal Verma <vishal.l.verma@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Ira Weiny <ira.weiny@...el.com>, <nvdimm@...ts.linux.dev>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init

Dave Jiang wrote:
> 

[snip]

First off thanks for the patch.  This code seems to have a few things to
clean up.

> 
> On 12/6/23 20:43, Dinghao Liu wrote:
> > When an error happens in btt_freelist_init(), its caller
> > discover_arenas() will directly free arena, which makes
> > arena->freelist allocated in btt_freelist_init() a leaked
> > memory. Fix this by freeing arena->freelist in all error
> > handling paths of btt_freelist_init().
> > 
> > Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
> > Signed-off-by: Dinghao Liu <dinghao.liu@....edu.cn>
> 
> How about use the new scope based resource management and we can avoid the goto mess altogether?
> https://lwn.net/Articles/934679/
> 

The freelist is returned as part of arena.  I've not traced both paths of
btt_freelist_init() completely but devm_kcalloc() looks like a better
solution here because this memory needs to live past the function scope.

That said, this patch does not completely fix freelist from leaking in the
following error path.

	discover_arenas()
		btt_freelist_init() -> ok (memory allocated)
		btt_rtt_init() -> fail
			goto out;
			(leak because arena is not yet on btt->arena_list)
		OR
		btt_maplocks_init() -> fail
			goto out;
			(leak because arena is not yet on btt->arena_list)

This error could be fixed by adding to arena_list earlier but devm_*()
also takes care of this without having to worry about that logic.

On normal operation all of this memory can be free'ed with the
corresponding devm_kfree() and/or devm_add_action_*() calls if arenas come
and go.  I'm not sure off the top of my head.

In addition, looking at this code.  discover_arenas() could make use of
the scoped based management for struct btt_sb *super!

Dinghao would you be willing to submit a series of 2 or 3 patches to fix
the above issues?

Thanks!
Ira