On Tue, Jan 5, 2021 at 12:00 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> We are not guaranteed the locking environment that would prevent
> dentry getting renamed right under us. And it's possible for
> old long name to be freed after rename, leading to UAF here.
This whole thing isn't important enough to get the dentry lock. It's
more of a hint than anything else.
Why isn't the fix to just use READ_ONCE() of the name pointer, and do
it under RCU?
That's what dentry_name() does for the much more complex case of
actually even following parent data for a depth up to 4, much less
just a single name.
So instead of
spin_lock(&dentry->d_lock);
audit_log_untrustedstring(ab, dentry->d_name.name);
spin_unlock(&dentry->d_lock);
why not
rcu_read_lock();
audit_log_untrustedstring(ab,
READ_ONCE(dentry->d_name.name));
rcu_read_unlock();
which looks a lot more in line with the other dentry path functions.
Maybe even have this as part of fs/d_path.c and try to get rid of
magic internal dentry name knowledge from the audit code?
Linus
