Make RCU dcache work with CONFIG_SECURITY=y

From: Andi Kleen <[email protected]>

Right now all RCU walks fall back to reference walk when CONFIG_SECURITY is enabled, even though just the standard capability module is active. This is because security_inode_exec_permission unconditionally fails RCU walks.

Move this decision to the low level security module. This requires passing the RCU flags down the security hook. This way at least the capability module and a few easy cases in selinux/smack work with RCU walks with CONFIG_SECURITY=y

Signed-off-by Andi Kleen <[email protected]>

---
 include/linux/security.h   |    2 +-
 security/capability.c      |    2 +-
 security/security.c        |    6 ++----
 security/selinux/hooks.c   |    6 +++++-
 security/smack/smack_lsm.c |    6 +++++-

 5 files changed, 14 insertions(+), 8 deletions(-)

include/linux/security.h +1 -1

index ca02f17..8ce59ef 100644
@@ -1456,7 +1456,7 @@ struct security_operations {
                             struct inode *new_dir, struct dentry *new_dentry);
        int (*inode_readlink) (struct dentry *dentry);
        int (*inode_follow_link) (struct dentry *dentry, struct nameidata *nd);
-       int (*inode_permission) (struct inode *inode, int mask);
+       int (*inode_permission) (struct inode *inode, int mask, unsigned flags);
        int (*inode_setattr)    (struct dentry *dentry, struct iattr *attr);
        int (*inode_getattr) (struct vfsmount *mnt, struct dentry *dentry);
        int (*inode_setxattr) (struct dentry *dentry, const char *name,

security/capability.c +1 -1

index 2984ea4..bbb5115 100644
@@ -181,7 +181,7 @@ static int cap_inode_follow_link(struct dentry *dentry,
        return 0;
  }
 
-static int cap_inode_permission(struct inode *inode, int mask)
+static int cap_inode_permission(struct inode *inode, int mask, unsigned flags)
  {
        return 0;
  }

security/security.c +2 -4

index 1011423..4ba6d4c 100644
@@ -518,16 +518,14 @@ int security_inode_permission(struct inode *inode, int mask)
  {
        if (unlikely(IS_PRIVATE(inode)))
                return 0;
-       return security_ops->inode_permission(inode, mask);
+       return security_ops->inode_permission(inode, mask, 0);
  }
 
  int security_inode_exec_permission(struct inode *inode, unsigned int flags)
  {
        if (unlikely(IS_PRIVATE(inode)))
                return 0;
-       if (flags)
-               return -ECHILD;
-       return security_ops->inode_permission(inode, MAY_EXEC);
+       return security_ops->inode_permission(inode, MAY_EXEC, flags);
  }
 
  int security_inode_setattr(struct dentry *dentry, struct iattr *attr)

security/selinux/hooks.c +5 -1

index f9c3764..a73f4e4 100644
@@ -2635,7 +2635,7 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *na
        return dentry_has_perm(cred, NULL, dentry, FILE__READ);
  }
 
-static int selinux_inode_permission(struct inode *inode, int mask)
+static int selinux_inode_permission(struct inode *inode, int mask, unsigned flags)
  {
        const struct cred *cred = current_cred();
        struct common_audit_data ad;

@@ -2649,6 +2649,10 @@ static int selinux_inode_permission(struct inode *inode, int mask)
        if (!mask)
                return 0;
 
+       /* May be droppable after audit */
+       if (flags & IPERM_FLAG_RCU)
+               return -ECHILD;
+
        COMMON_AUDIT_DATA_INIT(&ad, FS);
        ad.u.fs.inode = inode;
 

security/smack/smack_lsm.c +5 -2

index c6f8fca..400a5d5 100644
@@ -686,7 +686,7 @@ static int smack_inode_rename(struct inode *old_inode,
  *
  * Returns 0 if access is permitted, -EACCES otherwise
  */
-static int smack_inode_permission(struct inode *inode, int mask)
+static int smack_inode_permission(struct inode *inode, int mask, unsigned flags)
  {
        struct smk_audit_info ad;
 

@@ -696,6 +696,10 @@ static int smack_inode_permission(struct inode *inode, int mask)
         */
        if (mask == 0)
                return 0;
+
+       /* May be droppable after audit */
+       if (flags & IPERM_FLAG_RCU)
+               return -ECHILD;
        smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_FS);
        smk_ad_setfield_u_fs_inode(&ad, inode);
        return smk_curacc(smk_of_inode(inode), mask, &ad);
--

1.7.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Andi Kleen <[email protected]>

Right now all RCU walks fall back to reference walk when CONFIG_SECURITY is enabled, even though just the standard capability module is active. This is because security_inode_exec_permission unconditionally fails RCU walks.

Move this decision to the low level security module. This requires passing the RCU flags down the security hook. This way at least the capability module and a few easy cases in selinux/smack work with RCU walks with CONFIG_SECURITY=y

Signed-off-by Andi Kleen <[email protected]>

---
 include/linux/security.h   |    2 +-
 security/capability.c      |    2 +-
 security/security.c        |    6 ++----
 security/selinux/hooks.c   |    6 +++++-
 security/smack/smack_lsm.c |    6 +++++-
 5 files changed, 14 insertions(+), 8 deletions(-)

This seems to miss the hunk in fs/namei.c where the LSM hook is called.

Quoted 1 line

This seems to miss the hunk in fs/namei.c where the LSM hook is called.

namei.c doesn't need changes because it already passes in the flags The changed semantics only "start" in security/security.c

From: Andi Kleen <[email protected]>

Now that the security modules can decide whether they support the dcache RCU walk or not it's possible to make selinux a bit more RCU friendly. selinux already uses RCU for its internal decision cache, so this must be already RCU safe.

This patch makes the VFS RCU walk not retry in selinux if it hit the cache, and only fallback on a cache miss.

I had to add some parameters to pass the state around, otherwise the patch is quite simple.

Signed-off-by Andi Kleen <[email protected]>

---
 security/selinux/avc.c         |   48 +++++++++++++++++++++++++++++++-------- security/selinux/hooks.c       |   28 +++++++++++----------- security/selinux/include/avc.h |   22 ++++++++++++----- security/selinux/ss/services.c |    2 +-

 4 files changed, 68 insertions(+), 32 deletions(-)

security/selinux/avc.c +4 -2

index 9da6420..9163c5f 100644
@@ -471,6 +471,7 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
  * @avd: access vector decisions
  * @result: result from avc_has_perm_noaudit
  * @a:  auxiliary audit data
+ * @flags: VFS walk flags
  *
  * Audit the granting or denial of permissions in accordance
  * with the policy.  This function is typically called by

@@ -481,9 +482,10 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
  * be performed under a lock, to allow the lock to be released
  * before calling the auditing code.
  */
-void avc_audit(u32 ssid, u32 tsid,
+int avc_audit(u32 ssid, u32 tsid,
               u16 tclass, u32 requested,
-              struct av_decision *avd, int result, struct common_audit_data *a)
+              struct av_decision *avd, int result, struct common_audit_data *a,
+              unsigned flags)
  {
        struct common_audit_data stack_data;
        u32 denied, audited;

@@ -515,7 +517,18 @@ void avc_audit(u32 ssid, u32 tsid,

       else
               audited = requested & avd->auditallow; if (!audited)
-               return;
+               return 0;

security/selinux/avc.c +11 -0

+
+       /*
+        * When in a RCU walk do the audit on the RCU retry (until
+        * someone makes audit RCU safe)
+        * Note this may drop some audits when the situation changes during
+        * retry. However this is logically just as if the operation happened
+        * a little later.
+        */
+       if (flags & IPERM_FLAG_RCU)
+               return -ECHILD;
+

       if (!a) {
               a = &stack_data;
               COMMON_AUDIT_DATA_INIT(a, NONE);

security/selinux/avc.c +8 -1

@@ -529,6 +542,7 @@ void avc_audit(u32 ssid, u32 tsid,
        a->lsm_pre_audit = avc_audit_pre_callback;
        a->lsm_post_audit = avc_audit_post_callback;
        common_lsm_audit(a);
+       return 0;
  }
 
  /**

@@ -726,6 +740,7 @@ int avc_ss_reset(u32 seqno)
  * @requested: requested permissions, interpreted based on @tclass
  * @flags:  AVC_STRICT or 0
  * @avd: access vector decisions
+ * @vfsflags: VFS walk flags
  *
  * Check the AVC to determine whether the @requested permissions are granted
  * for the SID pair (@ssid, @tsid), interpreting the permissions

@@ -741,7 +756,8 @@ int avc_ss_reset(u32 seqno)
  int avc_has_perm_noaudit(u32 ssid, u32 tsid,
                         u16 tclass, u32 requested,
                         unsigned flags,
-                        struct av_decision *in_avd)
+                        struct av_decision *in_avd,
+                        unsigned vfsflags)
  {
        struct avc_node *node;
        struct av_decision avd_entry, *avd;

@@ -756,6 +772,10 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
        if (!node) {
                rcu_read_unlock();
 
+               /* Try again later if RCU */
+               if (vfsflags & IPERM_FLAG_RCU)
+                       return -ECHILD;
+
                if (in_avd)
                        avd = in_avd;

               else

security/selinux/avc.c +14 -6

@@ -793,6 +813,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
  * @tclass: target security class
  * @requested: requested permissions, interpreted based on @tclass
  * @auditdata: auxiliary audit data
+ * @flags: VFS walk flags
  *
  * Check the AVC to determine whether the @requested permissions are granted
  * for the SID pair (@ssid, @tsid), interpreting the permissions

@@ -802,14 +823,21 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
  * permissions are granted, -%EACCES if any permissions are denied, or
  * another -errno upon other errors.
  */
-int avc_has_perm(u32 ssid, u32 tsid, u16 tclass,
-                u32 requested, struct common_audit_data *auditdata)
+int avc_has_perm_flags(u32 ssid, u32 tsid, u16 tclass,
+                      u32 requested, struct common_audit_data *auditdata,
+                      unsigned flags)
  {
        struct av_decision avd;
-       int rc;
-
-       rc = avc_has_perm_noaudit(ssid, tsid, tclass, requested, 0, &avd);
-       avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
+       int rc, rc2;
+
+       rc = avc_has_perm_noaudit(ssid, tsid, tclass, requested, 0, &avd,
+                                 flags);
+       if (rc == -ECHILD)
+               return rc;
+       rc2 = avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata,
+                       flags);
+       if (rc2 == -ECHILD)
+               return rc2;
        return rc;
  }
 

security/selinux/hooks.c +11 -7

index a73f4e4..ea8c755 100644
@@ -1445,9 +1445,12 @@ static int task_has_capability(struct task_struct *tsk,
                BUG();
        }
 
-       rc = avc_has_perm_noaudit(sid, sid, sclass, av, 0, &avd);
-       if (audit == SECURITY_CAP_AUDIT)
-               avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
+       rc = avc_has_perm_noaudit(sid, sid, sclass, av, 0, &avd, 0);
+       if (audit == SECURITY_CAP_AUDIT) {
+               int rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad, 0);
+               if (rc2 == -ECHILD)
+                       return rc2;
+       }
        return rc;
  }
 

@@ -1467,7 +1470,8 @@ static int task_has_system(struct task_struct *tsk,
  static int inode_has_perm(const struct cred *cred,
                          struct inode *inode,
                          u32 perms,
-                         struct common_audit_data *adp)
+                         struct common_audit_data *adp,
+                         unsigned flags)
  {
        struct inode_security_struct *isec;
        struct common_audit_data ad;

@@ -1487,7 +1491,7 @@ static int inode_has_perm(const struct cred *cred,
                ad.u.fs.inode = inode;
        }
 
-       return avc_has_perm(sid, isec->sid, isec->sclass, perms, adp);
+       return avc_has_perm_flags(sid, isec->sid, isec->sclass, perms, adp, flags);
  }
 
  /* Same as inode_has_perm, but pass explicit audit data containing

@@ -1504,7 +1508,7 @@ static inline int dentry_has_perm(const struct cred *cred,
        COMMON_AUDIT_DATA_INIT(&ad, FS);
        ad.u.fs.path.mnt = mnt;
        ad.u.fs.path.dentry = dentry;
-       return inode_has_perm(cred, inode, av, &ad);
+       return inode_has_perm(cred, inode, av, &ad, 0);
  }
 
  /* Check whether a task can use an open file descriptor to

@@ -1540,7 +1544,7 @@ static int file_has_perm(const struct cred *cred,
        /* av is zero if only checking access to the descriptor. */
        rc = 0;
        if (av)
-               rc = inode_has_perm(cred, inode, av, &ad);
+               rc = inode_has_perm(cred, inode, av, &ad, 0);
 

 out:
       return rc;

security/selinux/hooks.c +3 -7

@@ -2103,7 +2107,7 @@ static inline void flush_unauthorized_files(const struct cred *cred,
                        file = file_priv->file;
                        inode = file->f_path.dentry->d_inode;
                        if (inode_has_perm(cred, inode,
-                                          FILE__READ | FILE__WRITE, NULL)) {
+                                          FILE__READ | FILE__WRITE, NULL, 0)) {
                                drop_tty = 1;
                        }
                }

@@ -2649,10 +2653,6 @@ static int selinux_inode_permission(struct inode *inode, int mask, unsigned flag
        if (!mask)
                return 0;
 
-       /* May be droppable after audit */
-       if (flags & IPERM_FLAG_RCU)
-               return -ECHILD;
-
        COMMON_AUDIT_DATA_INIT(&ad, FS);
        ad.u.fs.inode = inode;
 

@@ -2661,7 +2661,7 @@ static int selinux_inode_permission(struct inode *inode, int mask, unsigned flag
 
        perms = file_mask_to_av(inode->i_mode, mask);
 
-       return inode_has_perm(cred, inode, perms, &ad);
+       return inode_has_perm(cred, inode, perms, &ad, flags);
  }
 
  static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)

@@ -3209,7 +3209,7 @@ static int selinux_dentry_open(struct file *file, const struct cred *cred)
         * new inode label or new policy.
         * This check is not redundant - do not remove.
         */
-       return inode_has_perm(cred, inode, open_file_to_av(file), NULL);
+       return inode_has_perm(cred, inode, open_file_to_av(file), NULL, 0);
  }
 
  /* task security operations */

security/selinux/include/avc.h +15 -7

index 5615081..65d6e52 100644
@@ -54,21 +54,29 @@ struct avc_cache_stats {
 
  void __init avc_init(void);
 
-void avc_audit(u32 ssid, u32 tsid,
+int avc_audit(u32 ssid, u32 tsid,
               u16 tclass, u32 requested,
               struct av_decision *avd,
               int result,
-              struct common_audit_data *a);
+             struct common_audit_data *a, unsigned flags);
 
  #define AVC_STRICT 1 /* Ignore permissive mode. */
  int avc_has_perm_noaudit(u32 ssid, u32 tsid,
                         u16 tclass, u32 requested,
                         unsigned flags,
-                        struct av_decision *avd);
-
-int avc_has_perm(u32 ssid, u32 tsid,
-                u16 tclass, u32 requested,
-                struct common_audit_data *auditdata);
+                        struct av_decision *avd, unsigned vfsflags);
+
+int avc_has_perm_flags(u32 ssid, u32 tsid,
+                      u16 tclass, u32 requested,
+                      struct common_audit_data *auditdata,
+                      unsigned);
+
+static inline int avc_has_perm(u32 ssid, u32 tsid,
+                              u16 tclass, u32 requested,
+                              struct common_audit_data *auditdata)
+{
+       return avc_has_perm_flags(ssid, tsid, tclass, requested, auditdata, 0);
+}
 
  u32 avc_policy_seqno(void);
 

security/selinux/ss/services.c +0 -0

index 6ef4af4..4749202 100644
@@ -2212,7 +2212,7 @@ out_unlock:
                rc = avc_has_perm_noaudit(fromsid, mysids[i],

                                         SECCLASS_PROCESS, /* kernel value */ PROCESS__TRANSITION, AVC_STRICT,
-                                         NULL);
+                                         NULL, 0);
               if (!rc)
                       mysids2[j++] = mysids[i];
               cond_resched();
--
1.7.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I'll take a close look over the weekend, but I'm pretty sure this is even more strict than it needs to be. I looked at this a while ago and the only RCU unsafe location I could find was in the generic LSM 'audit' code (nothing to do with the audit subsystem). That code can do a d = d_find_alias(); dput(d). I don't think I realized the dput() was not RCU safe at the time. We use it to come up with a name of a dentry that might have caused the denial (although obviously not necessarily the right name)

I could just drop that piece of functionality (and rely on the audit subsystem for the info), but I think I'd rather do it your way. I think I can push your flags a lot deeper than you have pushed them (and remove them in some places you have included them). Let me look over the next day or two....

We found that all .38+ kernels with CONFIG_SECURITY just enables -- but not even any security module active -- are slower than .37. And also they don't really scale on larger machines. CONFIG_SECURITY is a quite common configuration, so this was seen multiple times.

The problem is that with CONFIG_SECURITY every directory permission check will drop out of the RCU walk and redo a bunch of work (and not scale of course), just in case the security module cannot handle it.

This patchkit tries to address this. First it moves the check for RCU walks into the low level security module, so for the CONFIG_SECURITY=y selinux=0 at runtime case you always get full performance. This is an independent patch.

Then it turned out that the two security modules who use the inode_exec_permission hook that impacts dcache walking -- SMACK and selinux -- already use RCU internally. So I added two followon patches that make them not drop out of the RCU walk, as long as they stay in their RCU "fast" path. For selinux this means a cache hit only and no audit event. For smack it means any check as long as auditing is disabled.

I didn't find good test suites for the security modules, so there wasn't a lot of testing on this unfortunately (the selinux one for LTP doesn't seem to work). Some close review of these changes is needed.

On the other hand the VFS changes itself are very straight forward and the 1/1 patch is very straight forward (and a win in itself)

The bottom line is with this patchkit a CONFIG_SECURITY=y kernel has as good VFS performance as a kernel with CONFIG_SECURITY disabled.

I didn't find good test suites for the security modules, so there wasn't a lot of testing on this unfortunately (the selinux one for LTP doesn't seem to work). Some close review of these changes is needed.

On the other hand the VFS changes itself are very straight forward and the 1/1 patch is very straight forward (and a win in itself)

The bottom line is with this patchkit a CONFIG_SECURITY=y kernel has as good VFS performance as a kernel with CONFIG_SECURITY disabled.

Quoted 6 lines · Andi Kleen

I didn't find good test suites for the security modules, so there wasn't a lot of testing on this unfortunately (the selinux one for LTP doesn't seem to work). Some close review of these changes is needed.

Quoted 6 lines

On the other hand the VFS changes itself are very straight forward and the 1/1 patch is very straight forward (and a win in itself)

The bottom line is with this patchkit a CONFIG_SECURITY=y kernel has as good VFS performance as a kernel with CONFIG_SECURITY disabled.

Gaah. My immediate reaction to the patch-series was "This is great, I was really hoping we could get all those annoying cases sorted out, and I'll queue them for the next merge window".

Having then actually read through the patches a bit more, I then got convinced that at least the first patch should probably be applied right away and be marked for stable, since it looks pretty damn obvious to me, and it might already on its own fix the performance regression for some configurations (although realistically I guess few enough people really do the "selinux=0" thing, so the big advantage is making easier to backport the other patches later if we don't do them now).

Comments? I'd really like to see/hear feedback like "yeah, this looks really obviously safe" vs "yeah, looks good, but I really don't feel very comfortable with it" from the security people.

Quoted 16 lines · Linus Torvalds

Quoted 11 lines · Andi Kleen

I didn't find good test suites for the security modules, so there wasn't a lot of testing on this unfortunately (the selinux one for LTP doesn't seem to work). Some close review of these changes is needed.

On the other hand the VFS changes itself are very straight forward and the 1/1 patch is very straight forward (and a win in itself)

The bottom line is with this patchkit a CONFIG_SECURITY=y kernel has as good VFS performance as a kernel with CONFIG_SECURITY disabled.

Gaah. My immediate reaction to the patch-series was "This is great, I was really hoping we could get all those annoying cases sorted out, and I'll queue them for the next merge window".

Quoted 8 lines

Having then actually read through the patches a bit more, I then got convinced that at least the first patch should probably be applied right away and be marked for stable, since it looks pretty damn obvious to me, and it might already on its own fix the performance regression for some configurations (although realistically I guess few enough people really do the "selinux=0" thing, so the big advantage is making easier to backport the other patches later if we don't do them now).

Yes I agree. The first patch is (nearly) a no-brainer and already has significant benefits. I would like to see it in .39.

Quoted 3 lines

Comments? I'd really like to see/hear feedback like "yeah, this looks really obviously safe" vs "yeah, looks good, but I really don't feel very comfortable with it" from the security people.

Especially SMACK review is needed.

Or maybe selinux only for now,
already got one ack for that.

(BTW I have some doubts on the locking in smack in general, but that's a separate issue -- see other thread)

-Andi

Quoted 6 lines · Andi Kleen

I didn't find good test suites for the security modules, so there wasn't a lot of testing on this unfortunately (the selinux one for LTP doesn't seem to work). Some close review of these changes is needed.

Quoted 6 lines

On the other hand the VFS changes itself are very straight forward and the 1/1 patch is very straight forward (and a win in itself)

The bottom line is with this patchkit a CONFIG_SECURITY=y kernel has as good VFS performance as a kernel with CONFIG_SECURITY disabled.

Gaah. My immediate reaction to the patch-series was "This is great, I was really hoping we could get all those annoying cases sorted out, and I'll queue them for the next merge window".

Having then actually read through the patches a bit more, I then got convinced that at least the first patch should probably be applied right away and be marked for stable, since it looks pretty damn obvious to me, and it might already on its own fix the performance regression for some configurations (although realistically I guess few enough people really do the "selinux=0" thing, so the big advantage is making easier to backport the other patches later if we don't do them now).

And now I'm vacillating about the two later patches too. They look fine to me, but I really have _zero_ familiarity with selinux and smack internals, so unlike the first patch, I can't go "that looks like the obviously right thing, and it clearly catches all the RCU cases".

The "we can't use all the nifty RCU pathwalk in the config that most distros ship by default" is clearly a performance regression, and has meant that it's not been really showing its real advantages for most people. So in that sense, it's a regression fix and thus valid even though we're pretty late in the -rc series.

But at the same time, it's also a bit scary.

Comments? I'd really like to see/hear feedback like "yeah, this looks really obviously safe" vs "yeah, looks good, but I really don't feel very comfortable with it" from the security people.

From an SELinux PoV (And I feel semi confident about SMACK) patch 1/3

Quoted 4 lines · Linus Torvalds

Comments? I'd really like to see/hear feedback like "yeah, this looks really obviously safe" vs "yeah, looks good, but I really don't feel very comfortable with it" from the security people.

From an SELinux PoV (And I feel semi confident about SMACK) patch 1/3 can and should go in as soon as you want.  2/3 looked safe on first glance, but I think I can make it smaller and better.  I'll try to get a version of 2/3 on list in the next couple of days.  My first thought was that it should probably go in via my SELinux tree next window......