* [PATCH 1/2] selinux: avoid silent denials in permissive mode under RCU walk
@ 2018-12-12 15:10 Stephen Smalley
2018-12-12 15:10 ` [PATCH 2/2] selinux: stop passing MAY_NOT_BLOCK to the AVC upon follow_link Stephen Smalley
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Smalley @ 2018-12-12 15:10 UTC (permalink / raw)
To: selinux; +Cc: paul, bmktuwien, linux-fsdevel, viro, Stephen Smalley
commit 0dc1ba24f7fff6 ("SELINUX: Make selinux cache VFS RCU walks safe")
results in no audit messages at all if in permissive mode because the
cache is updated during the rcu walk and thus no denial occurs on
the subsequent ref walk. Fix this by not updating the cache when
performing a non-blocking permission check. This only affects search
and symlink read checks during rcu walk.
Fixes: 0dc1ba24f7fff6 ("SELINUX: Make selinux cache VFS RCU walks safe")
Reported-by: BMK <bmktuwien@gmail.com>
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
security/selinux/avc.c | 23 +++++++++++++++++++++--
security/selinux/hooks.c | 4 +++-
security/selinux/include/avc.h | 1 +
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 635e5c1e3e48..5de18a6d5c3f 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -838,6 +838,7 @@ int __init avc_add_callback(int (*callback)(u32 event), u32 events)
* @ssid,@tsid,@tclass : identifier of an AVC entry
* @seqno : sequence number when decision was made
* @xpd: extended_perms_decision to be added to the node
+ * @flags: the AVC_* flags, e.g. AVC_NONBLOCKING, AVC_EXTENDED_PERMS, or 0.
*
* if a valid AVC entry doesn't exist,this function returns -ENOENT.
* if kmalloc() called internal returns NULL, this function returns -ENOMEM.
@@ -856,6 +857,23 @@ static int avc_update_node(struct selinux_avc *avc,
struct hlist_head *head;
spinlock_t *lock;
+ /*
+ * If we are in a non-blocking code path, e.g. VFS RCU walk,
+ * then we must not add permissions to a cache entry
+ * because we cannot safely audit the denial. Otherwise,
+ * during the subsequent blocking retry (e.g. VFS ref walk), we
+ * will find the permissions already granted in the cache entry
+ * and won't audit anything at all, leading to silent denials in
+ * permissive mode that only appear when in enforcing mode.
+ *
+ * See the corresponding handling in slow_avc_audit(), and the
+ * logic in selinux_inode_follow_link and selinux_inode_permission
+ * for the VFS MAY_NOT_BLOCK flag, which is transliterated into
+ * AVC_NONBLOCKING for avc_has_perm_noaudit().
+ */
+ if (flags & AVC_NONBLOCKING)
+ return 0;
+
node = avc_alloc_node(avc);
if (!node) {
rc = -ENOMEM;
@@ -1115,7 +1133,7 @@ int avc_has_extended_perms(struct selinux_state *state,
* @tsid: target security identifier
* @tclass: target security class
* @requested: requested permissions, interpreted based on @tclass
- * @flags: AVC_STRICT or 0
+ * @flags: AVC_STRICT, AVC_NONBLOCKING, or 0
* @avd: access vector decisions
*
* Check the AVC to determine whether the @requested permissions are granted
@@ -1199,7 +1217,8 @@ int avc_has_perm_flags(struct selinux_state *state,
struct av_decision avd;
int rc, rc2;
- rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested, 0,
+ rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested,
+ (flags & MAY_NOT_BLOCK) ? AVC_NONBLOCKING : 0,
&avd);
rc2 = avc_audit(state, ssid, tsid, tclass, requested, &avd, rc,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7ce012d9ec51..9b05f84808d9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3196,7 +3196,9 @@ static int selinux_inode_permission(struct inode *inode, int mask)
return PTR_ERR(isec);
rc = avc_has_perm_noaudit(&selinux_state,
- sid, isec->sid, isec->sclass, perms, 0, &avd);
+ sid, isec->sid, isec->sclass, perms,
+ (flags & MAY_NOT_BLOCK) ? AVC_NONBLOCKING : 0,
+ &avd);
audited = avc_audit_required(perms, &avd, rc,
from_access ? FILE__AUDIT_ACCESS : 0,
&denied);
diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
index ef899bcfd2cb..74ea50977c20 100644
--- a/security/selinux/include/avc.h
+++ b/security/selinux/include/avc.h
@@ -142,6 +142,7 @@ static inline int avc_audit(struct selinux_state *state,
#define AVC_STRICT 1 /* Ignore permissive mode. */
#define AVC_EXTENDED_PERMS 2 /* update extended permissions */
+#define AVC_NONBLOCKING 4 /* non blocking */
int avc_has_perm_noaudit(struct selinux_state *state,
u32 ssid, u32 tsid,
u16 tclass, u32 requested,
--
2.19.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] selinux: stop passing MAY_NOT_BLOCK to the AVC upon follow_link
2018-12-12 15:10 [PATCH 1/2] selinux: avoid silent denials in permissive mode under RCU walk Stephen Smalley
@ 2018-12-12 15:10 ` Stephen Smalley
2019-01-11 1:37 ` Paul Moore
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Smalley @ 2018-12-12 15:10 UTC (permalink / raw)
To: selinux; +Cc: paul, bmktuwien, linux-fsdevel, viro, Stephen Smalley
commit bda0be7ad9948 ("security: make inode_follow_link RCU-walk aware")
switched selinux_inode_follow_link() to use avc_has_perm_flags() and
pass down the MAY_NOT_BLOCK flag if called during RCU walk. However,
the only test of MAY_NOT_BLOCK occurs during slow_avc_audit()
and only if passing an inode as audit data (LSM_AUDIT_DATA_INODE). Since
selinux_inode_follow_link() passes a dentry directly, passing MAY_NOT_BLOCK
here serves no purpose. Switch selinux_inode_follow_link() to use
avc_has_perm() and drop avc_has_perm_flags() since there are no other
users.
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
security/selinux/avc.c | 24 ++----------------------
security/selinux/hooks.c | 5 ++---
security/selinux/include/avc.h | 5 -----
3 files changed, 4 insertions(+), 30 deletions(-)
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 5de18a6d5c3f..9b63d8ee1687 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -867,9 +867,8 @@ static int avc_update_node(struct selinux_avc *avc,
* permissive mode that only appear when in enforcing mode.
*
* See the corresponding handling in slow_avc_audit(), and the
- * logic in selinux_inode_follow_link and selinux_inode_permission
- * for the VFS MAY_NOT_BLOCK flag, which is transliterated into
- * AVC_NONBLOCKING for avc_has_perm_noaudit().
+ * logic in selinux_inode_permission for the MAY_NOT_BLOCK flag,
+ * which is transliterated into AVC_NONBLOCKING.
*/
if (flags & AVC_NONBLOCKING)
return 0;
@@ -1209,25 +1208,6 @@ int avc_has_perm(struct selinux_state *state, u32 ssid, u32 tsid, u16 tclass,
return rc;
}
-int avc_has_perm_flags(struct selinux_state *state,
- u32 ssid, u32 tsid, u16 tclass, u32 requested,
- struct common_audit_data *auditdata,
- int flags)
-{
- struct av_decision avd;
- int rc, rc2;
-
- rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested,
- (flags & MAY_NOT_BLOCK) ? AVC_NONBLOCKING : 0,
- &avd);
-
- rc2 = avc_audit(state, ssid, tsid, tclass, requested, &avd, rc,
- auditdata, flags);
- if (rc2)
- return rc2;
- return rc;
-}
-
u32 avc_policy_seqno(struct selinux_state *state)
{
return state->avc->avc_cache.latest_notif;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9b05f84808d9..f012d2eb159e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3139,9 +3139,8 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode,
if (IS_ERR(isec))
return PTR_ERR(isec);
- return avc_has_perm_flags(&selinux_state,
- sid, isec->sid, isec->sclass, FILE__READ, &ad,
- rcu ? MAY_NOT_BLOCK : 0);
+ return avc_has_perm(&selinux_state,
+ sid, isec->sid, isec->sclass, FILE__READ, &ad);
}
static noinline int audit_inode_permission(struct inode *inode,
diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
index 74ea50977c20..7be0e1e90e8b 100644
--- a/security/selinux/include/avc.h
+++ b/security/selinux/include/avc.h
@@ -153,11 +153,6 @@ int avc_has_perm(struct selinux_state *state,
u32 ssid, u32 tsid,
u16 tclass, u32 requested,
struct common_audit_data *auditdata);
-int avc_has_perm_flags(struct selinux_state *state,
- u32 ssid, u32 tsid,
- u16 tclass, u32 requested,
- struct common_audit_data *auditdata,
- int flags);
int avc_has_extended_perms(struct selinux_state *state,
u32 ssid, u32 tsid, u16 tclass, u32 requested,
--
2.19.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] selinux: stop passing MAY_NOT_BLOCK to the AVC upon follow_link
2018-12-12 15:10 ` [PATCH 2/2] selinux: stop passing MAY_NOT_BLOCK to the AVC upon follow_link Stephen Smalley
@ 2019-01-11 1:37 ` Paul Moore
0 siblings, 0 replies; 3+ messages in thread
From: Paul Moore @ 2019-01-11 1:37 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, bmktuwien, linux-fsdevel, viro
On Wed, Dec 12, 2018 at 10:08 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> commit bda0be7ad9948 ("security: make inode_follow_link RCU-walk aware")
> switched selinux_inode_follow_link() to use avc_has_perm_flags() and
> pass down the MAY_NOT_BLOCK flag if called during RCU walk. However,
> the only test of MAY_NOT_BLOCK occurs during slow_avc_audit()
> and only if passing an inode as audit data (LSM_AUDIT_DATA_INODE). Since
> selinux_inode_follow_link() passes a dentry directly, passing MAY_NOT_BLOCK
> here serves no purpose. Switch selinux_inode_follow_link() to use
> avc_has_perm() and drop avc_has_perm_flags() since there are no other
> users.
>
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/avc.c | 24 ++----------------------
> security/selinux/hooks.c | 5 ++---
> security/selinux/include/avc.h | 5 -----
> 3 files changed, 4 insertions(+), 30 deletions(-)
I just merged both 1/2 and 2/2 into selinux/next, thanks Stephen.
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 5de18a6d5c3f..9b63d8ee1687 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -867,9 +867,8 @@ static int avc_update_node(struct selinux_avc *avc,
> * permissive mode that only appear when in enforcing mode.
> *
> * See the corresponding handling in slow_avc_audit(), and the
> - * logic in selinux_inode_follow_link and selinux_inode_permission
> - * for the VFS MAY_NOT_BLOCK flag, which is transliterated into
> - * AVC_NONBLOCKING for avc_has_perm_noaudit().
> + * logic in selinux_inode_permission for the MAY_NOT_BLOCK flag,
> + * which is transliterated into AVC_NONBLOCKING.
> */
> if (flags & AVC_NONBLOCKING)
> return 0;
> @@ -1209,25 +1208,6 @@ int avc_has_perm(struct selinux_state *state, u32 ssid, u32 tsid, u16 tclass,
> return rc;
> }
>
> -int avc_has_perm_flags(struct selinux_state *state,
> - u32 ssid, u32 tsid, u16 tclass, u32 requested,
> - struct common_audit_data *auditdata,
> - int flags)
> -{
> - struct av_decision avd;
> - int rc, rc2;
> -
> - rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested,
> - (flags & MAY_NOT_BLOCK) ? AVC_NONBLOCKING : 0,
> - &avd);
> -
> - rc2 = avc_audit(state, ssid, tsid, tclass, requested, &avd, rc,
> - auditdata, flags);
> - if (rc2)
> - return rc2;
> - return rc;
> -}
> -
> u32 avc_policy_seqno(struct selinux_state *state)
> {
> return state->avc->avc_cache.latest_notif;
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9b05f84808d9..f012d2eb159e 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3139,9 +3139,8 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode,
> if (IS_ERR(isec))
> return PTR_ERR(isec);
>
> - return avc_has_perm_flags(&selinux_state,
> - sid, isec->sid, isec->sclass, FILE__READ, &ad,
> - rcu ? MAY_NOT_BLOCK : 0);
> + return avc_has_perm(&selinux_state,
> + sid, isec->sid, isec->sclass, FILE__READ, &ad);
> }
>
> static noinline int audit_inode_permission(struct inode *inode,
> diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
> index 74ea50977c20..7be0e1e90e8b 100644
> --- a/security/selinux/include/avc.h
> +++ b/security/selinux/include/avc.h
> @@ -153,11 +153,6 @@ int avc_has_perm(struct selinux_state *state,
> u32 ssid, u32 tsid,
> u16 tclass, u32 requested,
> struct common_audit_data *auditdata);
> -int avc_has_perm_flags(struct selinux_state *state,
> - u32 ssid, u32 tsid,
> - u16 tclass, u32 requested,
> - struct common_audit_data *auditdata,
> - int flags);
>
> int avc_has_extended_perms(struct selinux_state *state,
> u32 ssid, u32 tsid, u16 tclass, u32 requested,
> --
> 2.19.2
>
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-01-11 1:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-12 15:10 [PATCH 1/2] selinux: avoid silent denials in permissive mode under RCU walk Stephen Smalley
2018-12-12 15:10 ` [PATCH 2/2] selinux: stop passing MAY_NOT_BLOCK to the AVC upon follow_link Stephen Smalley
2019-01-11 1:37 ` Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.