linux-kernel - Re: [PATCH 5/6] eventfs: get rid of dentry pointers without refcounts

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: Wed, 31 Jan 2024 10:08:37 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Al Viro <viro@...iv.linux.org.uk>, Masami Hiramatsu <mhiramat@...nel.org>, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] eventfs: get rid of dentry pointers without refcounts

On Wed, 31 Jan 2024 at 05:14, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> If you also notice, tracefs only allows mkdir/rmdir to be assigned to
> one directory. Once it is assigned, no other directories can have mkdir
> rmdir functionality.

I think that does limit the damage, but it's not clear that it is actually safe.

Because you don't hold the inode lock, somebody could come in and do a
mkdir inside the other one that is being removed, ie something like

 - thread 1 does took the inode lock, called ->rmdir

 - it then drops the inode lock (both parent and the directory that is
getting removed) and gets the event lock

 - but thread 2 can come in between that inode lock drop and event lock

Notice: thread 2 comes in where there is *no* locking. Nada. Zilch.

This is what worries me.

But it does help that it's all only in *one* directory.  At least
another mkdir cannot happen *inside* the one that is going away while
the locks are not held. So the good news is that it does mean that
there's only one point that is being protected.

But I do worry about things like this (in vfs_rmdir()):

        inode_lock(dentry->d_inode);

        error = -EBUSY;
        if (is_local_mountpoint(dentry) ||
            (dentry->d_inode->i_flags & S_KERNEL_FILE))
                goto out;

        error = security_inode_rmdir(dir, dentry);
        if (error)
                goto out;

        error = dir->i_op->rmdir(dir, dentry);
        if (error)
                goto out;

notice how it does that "is this a mount point" test atomically wrt
the rmdir before it is allowed to proceed.

And I do think that the inode lock is what also protects it from
concurrent mounts. So now what happens when that "thread 2" above
comes in while there is *no* locking, and mounts something there?

Now, I'm not saying this is a huge problem. But it's very much an
example of a thing that *could* be a problem. Dropping locks in the
middle is just very scary.

               Linus