mm: double-free in cgwb_bdi_init

[Dmitry Vyukov]

Hello,

syzkaller hit the following report on linux-next
eb60f01302b24ce93108414e2c4c673cb7cd6e05:

BUG: Double free or freeing an invalid pointer
CPU: 0 PID: 15931 Comm: syz-executor2 Not tainted 4.10.0-rc7-next-20170207 #1
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011

Call Trace:
kfree+0xd3/0x250 mm/slab.c:3819
cgwb_bdi_init mm/backing-dev.c:764 [inline]
bdi_init+0xbf5/0xed0 mm/backing-dev.c:788
bdi_setup_and_register+0x70/0x100 mm/backing-dev.c:929
v9fs_session_init+0x17b/0x1a00 fs/9p/v9fs.c:335
v9fs_mount+0x81/0x830 fs/9p/vfs_super.c:130
mount_fs+0x97/0x2e0 fs/super.c:1223
vfs_kern_mount.part.24+0xc6/0x430 fs/namespace.c:976
vfs_kern_mount fs/namespace.c:2509 [inline]
do_new_mount fs/namespace.c:2512 [inline]
do_mount+0x426/0x2ec0 fs/namespace.c:2834
SYSC_mount fs/namespace.c:3050 [inline]
SyS_mount+0xab/0x120 fs/namespace.c:3027
entry_SYSCALL_64_fastpath+0x1f/0xc2

Object at ffff8801d1c30340, in cache kmalloc-32 size: 32
Allocated:
PID = 15931
[<ffffffff8193e1d6>] kzalloc include/linux/slab.h:638 [inline]
[<ffffffff8193e1d6>] cgwb_bdi_init mm/backing-dev.c:758 [inline]
[<ffffffff8193e1d6>] bdi_init+0x346/0xed0 mm/backing-dev.c:788
[<ffffffff8193f5e0>] bdi_setup_and_register+0x70/0x100 mm/backing-dev.c:929
[<ffffffff8209815b>] v9fs_session_init+0x17b/0x1a00 fs/9p/v9fs.c:335
[<ffffffff82086101>] v9fs_mount+0x81/0x830 fs/9p/vfs_super.c:130
[<ffffffff81a92ff7>] mount_fs+0x97/0x2e0 fs/super.c:1223
[<ffffffff81b0c036>] vfs_kern_mount.part.24+0xc6/0x430 fs/namespace.c:976
[<ffffffff81b16c56>] vfs_kern_mount fs/namespace.c:2509 [inline]
[<ffffffff81b16c56>] do_new_mount fs/namespace.c:2512 [inline]
[<ffffffff81b16c56>] do_mount+0x426/0x2ec0 fs/namespace.c:2834
[<ffffffff81b1a23b>] SYSC_mount fs/namespace.c:3050 [inline]
[<ffffffff81b1a23b>] SyS_mount+0xab/0x120 fs/namespace.c:3027

Freed:
PID = 15931
[<ffffffff81a360d3>] kfree+0xd3/0x250 mm/slab.c:3819
[<ffffffff8193ea97>] wb_congested_put include/linux/backing-dev.h:440 [inline]
[<ffffffff8193ea97>] wb_init mm/backing-dev.c:337 [inline]
[<ffffffff8193ea97>] cgwb_bdi_init mm/backing-dev.c:762 [inline]
[<ffffffff8193ea97>] bdi_init+0xc07/0xed0 mm/backing-dev.c:788
[<ffffffff8193f5e0>] bdi_setup_and_register+0x70/0x100 mm/backing-dev.c:929
[<ffffffff8209815b>] v9fs_session_init+0x17b/0x1a00 fs/9p/v9fs.c:335
[<ffffffff82086101>] v9fs_mount+0x81/0x830 fs/9p/vfs_super.c:130
[<ffffffff81a92ff7>] mount_fs+0x97/0x2e0 fs/super.c:1223
[<ffffffff81b0c036>] vfs_kern_mount.part.24+0xc6/0x430 fs/namespace.c:976
[<ffffffff81b16c56>] vfs_kern_mount fs/namespace.c:2509 [inline]
[<ffffffff81b16c56>] do_new_mount fs/namespace.c:2512 [inline]
[<ffffffff81b16c56>] do_mount+0x426/0x2ec0 fs/namespace.c:2834
[<ffffffff81b1a23b>] SYSC_mount fs/namespace.c:3050 [inline]
[<ffffffff81b1a23b>] SyS_mount+0xab/0x120 fs/namespace.c:3027
[<ffffffff844ca541>] entry_SYSCALL_64_fastpath+0x1f/0xc2


It all happens in the context on a single syscall. Also right before
that there was a bunch of allocation failures:

https://gist.githubusercontent.com/dvyukov/a840e280871136fc9654833e59970342/raw/385864d7584a4575ca5b9e2cc70815b9516b6598/gistfile1.txt

So this looks like a straight double-free on error path.

[Tejun Heo]

When !CONFIG_CGROUP_WRITEBACK, bdi has single bdi_writeback_congested
at bdi->wb_congested. cgwb_bdi_init() allocates it with kzalloc() and
doesn't do further initialization. This usually works fine as the
reference count gets bumped to 1 by wb_init() and the put from
wb_exit() releases it.

However, when wb_init() fails, it puts the wb base ref automatically
freeing the wb and the explicit kfree() in cgwb_bdi_init() error path
ends up trying to free the same pointer the second time causing a
double-free.

Fix it by explicitly initilizing the refcnt to 1 and putting the base
ref from cgwb_bdi_destroy().

Signed-off-by: Tejun Heo <t...@kernel.org>
Reported-by: Dmitry Vyukov <dvy...@google.com>
Fixes: a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in bdi_writeback")
Cc: sta...@vger.kernel.org # v4.2+
---
Hello,

ISTR seeing another fix for this bug but can't find it right now. If
I'm imagining things, please apply this one. If not, either one is
fine.

Thanks.

mm/backing-dev.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 3bfed5ab..61b3407 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -758,15 +758,20 @@ static int cgwb_bdi_init(struct backing_dev_info *bdi)
if (!bdi->wb_congested)
return -ENOMEM;

+ atomic_set(&bdi->wb_congested->refcnt, 1);
+
err = wb_init(&bdi->wb, bdi, 1, GFP_KERNEL);
if (err) {
- kfree(bdi->wb_congested);
+ wb_congested_put(bdi->wb_congested);
return err;
}
return 0;
}

-static void cgwb_bdi_destroy(struct backing_dev_info *bdi) { }
+static void cgwb_bdi_destroy(struct backing_dev_info *bdi)
+{
+ wb_congested_put(bdi->wb_congested);
+}

#endif /* CONFIG_CGROUP_WRITEBACK */

[Jens Axboe]

On 02/08/2017 01:19 PM, Tejun Heo wrote:
> When !CONFIG_CGROUP_WRITEBACK, bdi has single bdi_writeback_congested
> at bdi->wb_congested. cgwb_bdi_init() allocates it with kzalloc() and
> doesn't do further initialization. This usually works fine as the
> reference count gets bumped to 1 by wb_init() and the put from
> wb_exit() releases it.
>
> However, when wb_init() fails, it puts the wb base ref automatically
> freeing the wb and the explicit kfree() in cgwb_bdi_init() error path
> ends up trying to free the same pointer the second time causing a
> double-free.
>
> Fix it by explicitly initilizing the refcnt to 1 and putting the base
> ref from cgwb_bdi_destroy().

Queued up for 4.11.

--
Jens Axboe