linux-kernel - Re: NULL pointer deref in put_fs

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20191106210646.GA1495@gandi.net>
Date:   Wed, 6 Nov 2019 22:06:46 +0100
From:   Thibaut Sautereau <thibaut@...tereau.fr>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Christian Brauner <christian.brauner@...ntu.com>,
        dhowells@...hat.com, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Christian Brauner <christian@...uner.io>,
        cgroups@...r.kernel.org
Subject: Re: NULL pointer deref in put_fs_context with unprivileged LXC

On Wed, Nov 06, 2019 at 07:24:07AM +0000, Al Viro wrote:
> On Tue, Nov 05, 2019 at 09:58:30PM +0100, Thibaut Sautereau wrote:
> 
> > > > 	BUG: kernel NULL pointer dereference, address: 0000000000000043
> 
> ERR_PTR(something)->d_sb, most likely.
> 
> > > > 	493		if (fc->root) {
> > > > 	494			sb = fc->root->d_sb;
> > > > 	495			dput(fc->root);
> > > > 	496			fc->root = NULL;
> > > > 	497			deactivate_super(sb);
> > > > 	498		}
> 
> > 	fs_context: DEBUG: fc->root = fffffffffffffff3
> > 	fs_context: DEBUG: fc->source = cgroup2
> 
> Yup.  That'd be ERR_PTR(-13), i.e. ERR_PTR(-EACCES).  Most likely
> from
>                 nsdentry = kernfs_node_dentry(cgrp->kn, sb);
>                 dput(fc->root);
>                 fc->root = nsdentry;
>                 if (IS_ERR(nsdentry)) {
>                         ret = PTR_ERR(nsdentry);
>                         deactivate_locked_super(sb);
>                 }
> 
> in cgroup_do_get_tree().  As a quick test, try to add fc->root = NULL;
> next to that deactivate_locked_super(sb); inside the if (IS_ERR(...))
> body and see if it helps; it's not the best way to fix it (I'd rather
> go for
>                 if (IS_ERR(nsdentry)) {
>                         ret = PTR_ERR(nsdentry);
>                         deactivate_locked_super(sb);
> 			nsdentry = NULL;
>                 }
>                 fc->root = nsdentry;
> ), but it would serve to verify that this is the source of that crap.

Yes, you're absolutely right. Your first suggestion fixes the bug, as
well as your second one. Thanks!

By the way, I had just finished the bisection, confirming that
71d883c37e8d ("cgroup_do_mount(): massage calling conventions") brought
the issue.

Do you want me to send a patch or are you dealing with that?

-- 
Thibaut