linux-kernel - Re: general protection fault in kernfs_kill

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

Date:   Fri, 20 Apr 2018 09:31:58 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Eric Biggers <ebiggers3@...il.com>,
        Al Viro <viro@...IV.linux.org.uk>,
        syzbot <syzbot+151de3f2be6b40ac8026@...kaller.appspotmail.com>,
        gregkh@...uxfoundation.org, kstewart@...uxfoundation.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        pombredanne@...b.com, syzkaller-bugs@...glegroups.com,
        tglx@...utronix.de, linux-fsdevel@...r.kernel.org
Subject: Re: general protection fault in kernfs_kill_sb

On Fri 20-04-18 14:29:39, Tetsuo Handa wrote:
> Eric Biggers wrote:
> > But, there is still a related bug: when mounting sysfs, if register_shrinker()
> > fails in sget_userns(), then kernfs_kill_sb() gets called, which frees the
> > 'struct kernfs_super_info'.  But, the 'struct kernfs_super_info' is also freed
> > in kernfs_mount_ns() by:
> > 
> >         sb = sget_userns(fs_type, kernfs_test_super, kernfs_set_super, flags,
> >                          &init_user_ns, info);
> >         if (IS_ERR(sb) || sb->s_fs_info != info)
> >                 kfree(info);
> >         if (IS_ERR(sb))
> >                 return ERR_CAST(sb);
> > 
> > I guess the problem is that sget_userns() shouldn't take ownership of the 'info'
> > if it returns an error -- but, it actually does if register_shrinker() fails,
> > resulting in a double free.
> > 
> > Here is a reproducer and the KASAN splat.  This is on Linus' tree (87ef12027b9b)
> > with vfs/for-linus merged in.
> 
> I'm waiting for response from Michal Hocko regarding
> http://lkml.kernel.org/r/201804111909.EGC64586.QSFLFJFOVHOOtM@I-love.SAKURA.ne.jp .

I didn't plan to respond util all the Al's concerns with the existing
scheme are resolved. This is not an urgent thing to fix so better fix it
properly. Your API change is kinda ugly so it would be preferable to do
it properly as suggested by Al. Maybe that will be more work but my
understanding is that the resulting code would be better. If that is not
the case then I do not really have any fundamental objection to your
patch except it is ugly.
-- 
Michal Hocko
SUSE Labs