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

Al Viro viro at zeniv.linux.org.uk
Tue Jan 5 16:50:05 UTC 2021

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

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

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;

	// ->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) {
		return NULL;
	// we can safely access inode->i_dentry
        if (hlist_empty(p)) {
		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))
		// hashed + nonrcu really shouldn't be possible
		if (WARN_ON(READ_ONCE(de->d_flags) & DCACE_NONRCU))
        return de;

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

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

More information about the Linux-security-module-archive mailing list