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: <6573a0451c4b7_1e7d27294ea@iweiny-mobl.notmuch>
Date:   Fri, 8 Dec 2023 15:01:25 -0800
From:   Ira Weiny <ira.weiny@...el.com>
To:     <dinghao.liu@....edu.cn>, Ira Weiny <ira.weiny@...el.com>
CC:     Dave Jiang <dave.jiang@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        <nvdimm@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init

dinghao.liu@ wrote:
> > Dave Jiang wrote:

[snip]

> > 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)
> > 
> 
> Thanks for pointing out this issue! I rechecked discover_arenas() and found
> that btt_rtt_init() may also trigger a memleak for the same reason as
> btt_freelist_init(). Also, I checked another call trace:
> 
>     btt_init() -> btt_meta_init() -> btt_maplocks_init()
> 
> I think there is a memleak if btt_maplocks_init() succeeds but an error
> happens in btt_init() after btt_meta_init() (e.g., when btt_blk_init()
> returns an error). Therefore, we may need to fix three functions.

Yea I think we need to trace this code better.  This is why devm_ is nice for
memory allocated for the life of the device.

> 
> > 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?
> > 
> 
> Sure. Currently I plan to send 2 patches as follows:
> 1. Using devm_kcalloc() to replace kcalloc() in btt_freelist_init(), 
>    btt_rtt_init(), and btt_maplocks_init(), and removing the corresponding
>    kfree in free_arenas(). I checked some uses of devm_kcalloc() and it
>    seems that we need not to call devm_kfree(). The memory is automatically
>    freed on driver detach, right?

On device put yes.  So if these allocations are scoped to the life of the
device there would be no reason to call devm_kfree() on them at all.  I was not
sure if they got reallocated at some point or not.

> 2. Using the scoped based management for struct btt_sb *super (not a bug,
>    but it could improve the code).

Good!

> 
> I'm not quite sure whether my understanding or bug fixing plan is correct.
> If there are any issues, please correct me, thanks!

The above sounds right.
Ira