linux-kernel - Re: dcache_readdir NULL inode oops

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]

Date:   Fri, 30 Nov 2018 16:08:52 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Jan Glauber <Jan.Glauber@...ium.com>,
        Will Deacon <will.deacon@....com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jslaby@...e.com" <jslaby@...e.com>
Subject: Re: dcache_readdir NULL inode oops

On Fri, Nov 30, 2018 at 09:16:49AM -0600, Eric W. Biederman wrote:
> >> > +       inode_lock(parent->d_inode);
> >> >         dentry->d_fsdata = NULL;
> >> >         drop_nlink(dentry->d_inode);
> >> >         d_delete(dentry);
> >> > +       inode_unlock(parent->d_inode);
> >> > +
> >> >         dput(dentry);   /* d_alloc_name() in devpts_pty_new() */
> >> >  }
> >
> > This feels right but getting some feedback from others would be good.
> 
> This is going to be special at least because we are not coming through
> the normal unlink path and we are manipulating the dcache.
> 
> This looks plausible.  If this is whats going on then we have had this
> bug for a very long time.  I will see if I can make some time.
> 
> It looks like in the general case everything is serialized by the
> devpts_mutex.  I wonder if just changing the order of operations
> here would be enough.
> 
> AKA: drop_nlink d_delete then dentry->d_fsdata.  Ugh d_fsdata is not
> implicated so that won't help here.

It certainly won't.  The thing is, this
                if (!dir_emit(ctx, next->d_name.name, next->d_name.len,
                              d_inode(next)->i_ino, dt_type(d_inode(next))))
in dcache_readdir() obviously can block, so all we can hold over it is
blocking locks.  Which we do - specifically, ->i_rwsem on our directory.

It's actually worse than missing inode_lock() - consider the effects
of mount --bind /mnt/foo /dev/pts/42.  What happens when that thing
goes away?  Right, a lost mount...

I'll resurrect the "kernel-internal rm -rf done right" series and
post it; devpts is not the only place suffering such problem (binfmt_misc,
etc.)