| Message ID | 20150316044319.23648.82820.stgit@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 16, 2015 at 03:43:19PM +1100, NeilBrown wrote: > Some of dentry_has_perm() is not rcu-safe, so if LOOKUP_RCU > is set in selinux_inode_follow_link(), give up with > -ECHILD. > > It is possible that dentry_has_perm could sometimes complete > in RCU more, in which case the flag could be propagated further > down the stack... It bloody well can. Expand it a bit and you'll see - the nastiness comes from avc_audit() doing return slow_avc_audit(ssid, tsid, tclass, requested, audited, denied, result, a, 0); and passing that 0 to slow_avc_audit(). Pass it MAY_NOT_BLOCK instead and it'll bugger off with -ECHILD in blocking case. Call chain is dentry_has_perm -> inode_has_perm -> avc_has_perm -> avc_audit. Expand those (including avc_audit()) and make slow_avc_audit() get flags & LOOKUP_RCU ? MAY_NOT_BLOCK : 0. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 16 Mar 2015 21:00:35 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote: > On Mon, Mar 16, 2015 at 03:43:19PM +1100, NeilBrown wrote: > > Some of dentry_has_perm() is not rcu-safe, so if LOOKUP_RCU > > is set in selinux_inode_follow_link(), give up with > > -ECHILD. > > > > It is possible that dentry_has_perm could sometimes complete > > in RCU more, in which case the flag could be propagated further > > down the stack... > > It bloody well can. Expand it a bit and you'll see - the nastiness > comes from avc_audit() doing > return slow_avc_audit(ssid, tsid, tclass, > requested, audited, denied, result, > a, 0); > and passing that 0 to slow_avc_audit(). Pass it MAY_NOT_BLOCK instead > and it'll bugger off with -ECHILD in blocking case. > > Call chain is dentry_has_perm -> inode_has_perm -> avc_has_perm -> avc_audit. > Expand those (including avc_audit()) and make slow_avc_audit() get > flags & LOOKUP_RCU ? MAY_NOT_BLOCK : 0. There is more to it than that. avc_has_perm calls avc_has_perm_noaudit which does: rcu_read_lock(); ... if (unlikely(!node)) { node = avc_compute_av(ssid, tsid, tclass, avd); } else ... ... rcu_read_unlock(); and avc_compute_av() does rcu_read_unlock(); security_compute_av(ssid, tsid, tclass, avd); rcu_read_lock(); (yes: unlock, and then lock). so avc_has_perm_noaudit needs to bail out of RCU-walk if node turns out to be NULL. So I either add another 'flags' arg to that, or replace the current one which is unused .... or leave it as someone else's problem :-) NeilBrown
On Fri, Mar 20, 2015 at 03:39:30PM +1100, NeilBrown wrote: > rcu_read_unlock(); > security_compute_av(ssid, tsid, tclass, avd); > rcu_read_lock(); > > (yes: unlock, and then lock). > > so avc_has_perm_noaudit needs to bail out of RCU-walk if node turns out to be > NULL. NFI, but since a) the guts of security_compute_av() are under rwlock (shared), I rather doubt that it could e.g. block b) avc_has_perm_noaudit() is called from selinux_inode_permission(), which is called inside RCU-walk - it's hit on selinux setups in every successful inode_permission() I'd say that it's no worse than it already was. AFAICS, it's a slowpath and we don't want to hold rcu_read_lock() over it to avoid stalls, but if the caller of avc_has_perm_noaudit() used to want rcu_read_lock(), well, we'll just risks stalls -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index e3074e01f058..5d4de8cbfaa6 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2866,6 +2866,8 @@ static int selinux_inode_follow_link(struct dentry *dentry, int flags) { const struct cred *cred = current_cred(); + if (flags & LOOKUP_RCU) + return -ECHILD; return dentry_has_perm(cred, dentry, FILE__READ); }
Some of dentry_has_perm() is not rcu-safe, so if LOOKUP_RCU is set in selinux_inode_follow_link(), give up with -ECHILD. It is possible that dentry_has_perm could sometimes complete in RCU more, in which case the flag could be propagated further down the stack... Signed-off-by: NeilBrown <neilb@suse.de> --- security/selinux/hooks.c | 2 ++ 1 file changed, 2 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html