linux-kernel - Re: [syzbot] memory leak in xas

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: <Ys14hjgqMTudMCtv@casper.infradead.org>
Date:   Tue, 12 Jul 2022 14:35:02 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        syzbot <syzbot+a785d07959bc94837d51@...kaller.appspotmail.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        syzkaller-bugs@...glegroups.com, Zach O'Keefe <zokeefe@...gle.com>,
        Yang Shi <shy828301@...il.com>,
        Liam Howlett <liam.howlett@...cle.com>
Subject: Re: [syzbot] memory leak in xas_create

On Tue, Jul 12, 2022 at 01:57:59PM +0100, Matthew Wilcox wrote:
> > So I assumed the nodes stored in the xas object, which is local to the
> > collapse_file() function.
> 
> Yes, that's a reasonable thing to think, but it's actually not how
> it works.  When we allocate a node in xas_create(), we put it straight
> into the tree without storing it in xas->xa_alloc.  We may then end
> up not using it, but the node isn't leaked because it's in the tree.
> 
> If the GFP_NOWAIT allocation fails (it didn't in these stack traces),
> we call xas_nomem(), which sees an -ENOMEM, allocates a node and stores
> it in xas->xa_alloc; then we go round the loop again where xas_create()
> will take the node from xas->xa_alloc.  But the backtraces here don't
> implicate xas_nomem().

There is actually a leak here, but it's not the one that's been found.

        do {
                xas_lock_irq(&xas);
                xas_create_range(&xas);
                if (!xas_error(&xas))
                        break;
                xas_unlock_irq(&xas);
                if (!xas_nomem(&xas, GFP_KERNEL)) {
                        result = SCAN_FAIL;
                        goto out;
                }
        } while (1);

If xas_create() fails, it sets xas error to -ENOMEM.  So we unlock the
xarray lock and call xas_nomem().  xas_nomem() sees the error is -ENOMEM
and allocates a node with GFP_KERNEL, putting the node in xas->xa_alloc.
We re-take the spinlock and call xas_create() again.  If we still need
a node, xas_alloc() will take the node stored in xas->xa_alloc.  If we
raced and don't need a node, xas_create_range() succeeds and we break out,
failing to free the xas->xa_alloc node.

We should call xas_destroy() at the out: label.  However, this does not
explain what is going on, and will not fix what is going on, since any
nodes allocated during xas_create_range() will be stored safely in the
tree and will not be freed by xas_destroy().