Re: [PATCH v4] proc: Allow pid_revalidate() during LOOKUP_RCU

[Al Viro]

On Tue, Jan 05, 2021 at 05:59:35AM +0000, Al Viro wrote:

> Umm...  I'm rather worried about the side effect you are removing here -
> you are suddenly exposing a bunch of methods in there to RCU mode.
> E.g. is proc_pid_permission() safe with MAY_NOT_BLOCK in the mask?
> generic_permission() call in there is fine, but has_pid_permission()
> doesn't even see the mask.  Is that thing safe in RCU mode?  AFAICS,
> this
> static int selinux_ptrace_access_check(struct task_struct *child,
>                                      unsigned int mode)
> {
>         u32 sid = current_sid();
>         u32 csid = task_sid(child);
> 
>         if (mode & PTRACE_MODE_READ)
>                 return avc_has_perm(&selinux_state,
>                                     sid, csid, SECCLASS_FILE, FILE__READ, NULL);
> 
>         return avc_has_perm(&selinux_state,
>                             sid, csid, SECCLASS_PROCESS, PROCESS__PTRACE, NULL);
> }
> is reachable and IIRC avc_has_perm() should *NOT* be called in RCU mode.
> If nothing else, audit handling needs care...
> 
> And LSM-related stuff is only a part of possible issues here.  It does need
> a careful code audit - you are taking a bunch of methods into the conditions
> they'd never been tested in.  ->permission(), ->get_link(), ->d_revalidate(),
> ->d_hash() and ->d_compare() instances for objects that subtree.  The last
> two are not there in case of anything in /proc/<pid>, but the first 3 very
> much are.

FWIW, after looking through the selinux and smack I started to wonder whether
we really need that "return -ECHILD rather than audit and fail" in case of
->inode_permission().

AFAICS, the reason we need it is that dump_common_audit_data() is not safe
in RCU mode with LSM_AUDIT_DATA_DENTRY and even more so - with
LSM_AUDIT_DATA_INODE (d_find_alias() + dput() there, and dput() can bloody well
block).

LSM_AUDIT_DATA_DENTRY is easy to handle - wrap
                audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
into grabbing/dropping a->u.dentry->d_lock and we are done.  And as for
the LSM_AUDIT_DATA_INODE...  How about this:

/*
 * Caller *MUST* hold rcu_read_lock() and be guaranteed that inode itself
 * will be around until that gets dropped.
 */
struct dentry *d_find_alias_rcu(struct inode *inode)
{
	struct hlist_head *l = &inode->i_dentry;
        struct dentry *de = NULL;

	spin_lock(&inode->i_lock);
	// ->i_dentry and ->i_rcu are colocated, but the latter won't be
	// used without having I_FREEING set, which means no aliases left
	if (inode->i_state & I_FREEING) {
		spin_unlock(&inode->i_lock);
		return NULL;
	}
	// we can safely access inode->i_dentry
        if (hlist_empty(p)) {
		spin_unlock(&inode->i_lock);
		return NULL;
	}
	if (S_ISDIR(inode->i_mode)) {
		de = hlist_entry(l->first, struct dentry, d_u.d_alias);
	} else hlist_for_each_entry(de, l, d_u.d_alias) {
		if (d_unhashed(de))
			continue;
		// hashed + nonrcu really shouldn't be possible
		if (WARN_ON(READ_ONCE(de->d_flags) & DCACE_NONRCU))
			continue;
		break;
	}
	spin_unlock(&inode->i_lock);
        return de;
}

and have
        case LSM_AUDIT_DATA_INODE: {
                struct dentry *dentry;
                struct inode *inode;

		rcu_read_lock();
                inode = a->u.inode;
                dentry = d_find_alias_rcu(inode);
                if (dentry) {
                        audit_log_format(ab, " name=");
			spin_lock(&dentry->d_lock);
                        audit_log_untrustedstring(ab,
                                         dentry->d_name.name);
			spin_unlock(&dentry->d_lock);
                }
                audit_log_format(ab, " dev=");
                audit_log_untrustedstring(ab, inode->i_sb->s_id);
                audit_log_format(ab, " ino=%lu", inode->i_ino);
		rcu_read_unlock();
                break;
        }
in dump_common_audit_data().  Would that be enough to stop bothering
with the entire AVC_NONBLOCKING thing or is there anything else
involved?