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?
