From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Paris Subject: Re: [PATCH 2/3] SELINUX: Make selinux cache VFS RCU walks safe Date: Thu, 21 Apr 2011 20:45:17 -0400 Message-ID: References: <1303431801-10540-1-git-send-email-andi@firstfloor.org> <1303431801-10540-3-git-send-email-andi@firstfloor.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-fsdevel@vger.kernel.org, akpm@linux-foundation.org, torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, npiggin@kernel.dk, shaohua.li@intel.com, sds@tycho.nsa.gov, jmorris@namei.org, linux-security-module@vger.kernel.org, Andi Kleen To: Andi Kleen Return-path: In-Reply-To: <1303431801-10540-3-git-send-email-andi@firstfloor.org> Sender: linux-security-module-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org 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 =3D 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.... -Eric On Thu, Apr 21, 2011 at 8:23 PM, Andi Kleen wrote= : > From: Andi Kleen > > 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 > --- > =A0security/selinux/avc.c =A0 =A0 =A0 =A0 | =A0 48 ++++++++++++++++++= +++++++++++++-------- > =A0security/selinux/hooks.c =A0 =A0 =A0 | =A0 28 +++++++++++---------= -- > =A0security/selinux/include/avc.h | =A0 22 ++++++++++++----- > =A0security/selinux/ss/services.c | =A0 =A02 +- > =A04 files changed, 68 insertions(+), 32 deletions(-) > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > index 9da6420..9163c5f 100644 > --- a/security/selinux/avc.c > +++ b/security/selinux/avc.c > @@ -471,6 +471,7 @@ static void avc_audit_post_callback(struct audit_= buffer *ab, void *a) > =A0* @avd: access vector decisions > =A0* @result: result from avc_has_perm_noaudit > =A0* @a: =A0auxiliary audit data > + * @flags: VFS walk flags > =A0* > =A0* Audit the granting or denial of permissions in accordance > =A0* with the policy. =A0This function is typically called by > @@ -481,9 +482,10 @@ static void avc_audit_post_callback(struct audit= _buffer *ab, void *a) > =A0* be performed under a lock, to allow the lock to be released > =A0* before calling the auditing code. > =A0*/ > -void avc_audit(u32 ssid, u32 tsid, > +int avc_audit(u32 ssid, u32 tsid, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 u16 tclass, u32 requested, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0struct av_decision *avd, int result, str= uct common_audit_data *a) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0struct av_decision *avd, int result, str= uct common_audit_data *a, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned flags) > =A0{ > =A0 =A0 =A0 =A0struct common_audit_data stack_data; > =A0 =A0 =A0 =A0u32 denied, audited; > @@ -515,7 +517,18 @@ void avc_audit(u32 ssid, u32 tsid, > =A0 =A0 =A0 =A0else > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0audited =3D requested & avd->auditallo= w; > =A0 =A0 =A0 =A0if (!audited) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0; > + > + =A0 =A0 =A0 /* > + =A0 =A0 =A0 =A0* When in a RCU walk do the audit on the RCU retry (= until > + =A0 =A0 =A0 =A0* someone makes audit RCU safe) > + =A0 =A0 =A0 =A0* Note this may drop some audits when the situation = changes during > + =A0 =A0 =A0 =A0* retry. However this is logically just as if the op= eration happened > + =A0 =A0 =A0 =A0* a little later. > + =A0 =A0 =A0 =A0*/ > + =A0 =A0 =A0 if (flags & IPERM_FLAG_RCU) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ECHILD; > + > =A0 =A0 =A0 =A0if (!a) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0a =3D &stack_data; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0COMMON_AUDIT_DATA_INIT(a, NONE); > @@ -529,6 +542,7 @@ void avc_audit(u32 ssid, u32 tsid, > =A0 =A0 =A0 =A0a->lsm_pre_audit =3D avc_audit_pre_callback; > =A0 =A0 =A0 =A0a->lsm_post_audit =3D avc_audit_post_callback; > =A0 =A0 =A0 =A0common_lsm_audit(a); > + =A0 =A0 =A0 return 0; > =A0} > > =A0/** > @@ -726,6 +740,7 @@ int avc_ss_reset(u32 seqno) > =A0* @requested: requested permissions, interpreted based on @tclass > =A0* @flags: =A0AVC_STRICT or 0 > =A0* @avd: access vector decisions > + * @vfsflags: VFS walk flags > =A0* > =A0* Check the AVC to determine whether the @requested permissions ar= e granted > =A0* for the SID pair (@ssid, @tsid), interpreting the permissions > @@ -741,7 +756,8 @@ int avc_ss_reset(u32 seqno) > =A0int avc_has_perm_noaudit(u32 ssid, u32 tsid, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u16 tclass, u32 reque= sted, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 unsigned flags, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct av_decision *= in_avd) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct av_decision *= in_avd, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned vfsflags) > =A0{ > =A0 =A0 =A0 =A0struct avc_node *node; > =A0 =A0 =A0 =A0struct av_decision avd_entry, *avd; > @@ -756,6 +772,10 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid, > =A0 =A0 =A0 =A0if (!node) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0rcu_read_unlock(); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Try again later if RCU */ > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (vfsflags & IPERM_FLAG_RCU) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ECHILD; > + > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (in_avd) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0avd =3D in_avd; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else > @@ -793,6 +813,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid, > =A0* @tclass: target security class > =A0* @requested: requested permissions, interpreted based on @tclass > =A0* @auditdata: auxiliary audit data > + * @flags: VFS walk flags > =A0* > =A0* Check the AVC to determine whether the @requested permissions ar= e granted > =A0* for the SID pair (@ssid, @tsid), interpreting the permissions > @@ -802,14 +823,21 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid, > =A0* permissions are granted, -%EACCES if any permissions are denied,= or > =A0* another -errno upon other errors. > =A0*/ > -int avc_has_perm(u32 ssid, u32 tsid, u16 tclass, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u32 requested, struct common_audit_d= ata *auditdata) > +int avc_has_perm_flags(u32 ssid, u32 tsid, u16 tclass, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u32 requested, struct co= mmon_audit_data *auditdata, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned flags) > =A0{ > =A0 =A0 =A0 =A0struct av_decision avd; > - =A0 =A0 =A0 int rc; > - > - =A0 =A0 =A0 rc =3D avc_has_perm_noaudit(ssid, tsid, tclass, request= ed, 0, &avd); > - =A0 =A0 =A0 avc_audit(ssid, tsid, tclass, requested, &avd, rc, audi= tdata); > + =A0 =A0 =A0 int rc, rc2; > + > + =A0 =A0 =A0 rc =3D avc_has_perm_noaudit(ssid, tsid, tclass, request= ed, 0, &avd, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fla= gs); > + =A0 =A0 =A0 if (rc =3D=3D -ECHILD) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc; > + =A0 =A0 =A0 rc2 =3D avc_audit(ssid, tsid, tclass, requested, &avd, = rc, auditdata, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 flags); > + =A0 =A0 =A0 if (rc2 =3D=3D -ECHILD) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc2; > =A0 =A0 =A0 =A0return rc; > =A0} > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index a73f4e4..ea8c755 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -1445,9 +1445,12 @@ static int task_has_capability(struct task_str= uct *tsk, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0BUG(); > =A0 =A0 =A0 =A0} > > - =A0 =A0 =A0 rc =3D avc_has_perm_noaudit(sid, sid, sclass, av, 0, &a= vd); > - =A0 =A0 =A0 if (audit =3D=3D SECURITY_CAP_AUDIT) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 avc_audit(sid, sid, sclass, av, &avd, r= c, &ad); > + =A0 =A0 =A0 rc =3D avc_has_perm_noaudit(sid, sid, sclass, av, 0, &a= vd, 0); > + =A0 =A0 =A0 if (audit =3D=3D SECURITY_CAP_AUDIT) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int rc2 =3D avc_audit(sid, sid, sclass,= av, &avd, rc, &ad, 0); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (rc2 =3D=3D -ECHILD) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc2; > + =A0 =A0 =A0 } > =A0 =A0 =A0 =A0return rc; > =A0} > > @@ -1467,7 +1470,8 @@ static int task_has_system(struct task_struct *= tsk, > =A0static int inode_has_perm(const struct cred *cred, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct inode *inod= e, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u32 perms, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct common_audit= _data *adp) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct common_audit= _data *adp, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 unsigned flags) > =A0{ > =A0 =A0 =A0 =A0struct inode_security_struct *isec; > =A0 =A0 =A0 =A0struct common_audit_data ad; > @@ -1487,7 +1491,7 @@ static int inode_has_perm(const struct cred *cr= ed, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ad.u.fs.inode =3D inode; > =A0 =A0 =A0 =A0} > > - =A0 =A0 =A0 return avc_has_perm(sid, isec->sid, isec->sclass, perms= , adp); > + =A0 =A0 =A0 return avc_has_perm_flags(sid, isec->sid, isec->sclass,= perms, adp, flags); > =A0} > > =A0/* 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, > =A0 =A0 =A0 =A0COMMON_AUDIT_DATA_INIT(&ad, FS); > =A0 =A0 =A0 =A0ad.u.fs.path.mnt =3D mnt; > =A0 =A0 =A0 =A0ad.u.fs.path.dentry =3D dentry; > - =A0 =A0 =A0 return inode_has_perm(cred, inode, av, &ad); > + =A0 =A0 =A0 return inode_has_perm(cred, inode, av, &ad, 0); > =A0} > > =A0/* Check whether a task can use an open file descriptor to > @@ -1540,7 +1544,7 @@ static int file_has_perm(const struct cred *cre= d, > =A0 =A0 =A0 =A0/* av is zero if only checking access to the descripto= r. */ > =A0 =A0 =A0 =A0rc =3D 0; > =A0 =A0 =A0 =A0if (av) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D inode_has_perm(cred, inode, av, = &ad); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D inode_has_perm(cred, inode, av, = &ad, 0); > > =A0out: > =A0 =A0 =A0 =A0return rc; > @@ -2103,7 +2107,7 @@ static inline void flush_unauthorized_files(con= st struct cred *cred, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0file =3D file_priv->fi= le; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0inode =3D file->f_path= =2Edentry->d_inode; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (inode_has_perm(cre= d, inode, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0FILE__READ | FILE__WRITE, NULL)) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0FILE__READ | FILE__WRITE, NULL, 0)) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0drop_t= ty =3D 1; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} > @@ -2649,10 +2653,6 @@ static int selinux_inode_permission(struct ino= de *inode, int mask, unsigned flag > =A0 =A0 =A0 =A0if (!mask) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0; > > - =A0 =A0 =A0 /* May be droppable after audit */ > - =A0 =A0 =A0 if (flags & IPERM_FLAG_RCU) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ECHILD; > - > =A0 =A0 =A0 =A0COMMON_AUDIT_DATA_INIT(&ad, FS); > =A0 =A0 =A0 =A0ad.u.fs.inode =3D inode; > > @@ -2661,7 +2661,7 @@ static int selinux_inode_permission(struct inod= e *inode, int mask, unsigned flag > > =A0 =A0 =A0 =A0perms =3D file_mask_to_av(inode->i_mode, mask); > > - =A0 =A0 =A0 return inode_has_perm(cred, inode, perms, &ad); > + =A0 =A0 =A0 return inode_has_perm(cred, inode, perms, &ad, flags); > =A0} > > =A0static int selinux_inode_setattr(struct dentry *dentry, struct iat= tr *iattr) > @@ -3209,7 +3209,7 @@ static int selinux_dentry_open(struct file *fil= e, const struct cred *cred) > =A0 =A0 =A0 =A0 * new inode label or new policy. > =A0 =A0 =A0 =A0 * This check is not redundant - do not remove. > =A0 =A0 =A0 =A0 */ > - =A0 =A0 =A0 return inode_has_perm(cred, inode, open_file_to_av(file= ), NULL); > + =A0 =A0 =A0 return inode_has_perm(cred, inode, open_file_to_av(file= ), NULL, 0); > =A0} > > =A0/* task security operations */ > diff --git a/security/selinux/include/avc.h b/security/selinux/includ= e/avc.h > index 5615081..65d6e52 100644 > --- a/security/selinux/include/avc.h > +++ b/security/selinux/include/avc.h > @@ -54,21 +54,29 @@ struct avc_cache_stats { > > =A0void __init avc_init(void); > > -void avc_audit(u32 ssid, u32 tsid, > +int avc_audit(u32 ssid, u32 tsid, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 u16 tclass, u32 requested, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct av_decision *avd, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 int result, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0struct common_audit_data *a); > + =A0 =A0 =A0 =A0 =A0 =A0 struct common_audit_data *a, unsigned flags= ); > > =A0#define AVC_STRICT 1 /* Ignore permissive mode. */ > =A0int avc_has_perm_noaudit(u32 ssid, u32 tsid, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u16 tclass, u32 reque= sted, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 unsigned flags, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct av_decision *= avd); > - > -int avc_has_perm(u32 ssid, u32 tsid, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u16 tclass, u32 requested, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct common_audit_data *auditdata)= ; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct av_decision *= avd, unsigned vfsflags); > + > +int avc_has_perm_flags(u32 ssid, u32 tsid, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u16 tclass, u32 requeste= d, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct common_audit_data= *auditdata, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned); > + > +static inline int avc_has_perm(u32 ssid, u32 tsid, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0u16 tcla= ss, u32 requested, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct c= ommon_audit_data *auditdata) > +{ > + =A0 =A0 =A0 return avc_has_perm_flags(ssid, tsid, tclass, requested= , auditdata, 0); > +} > > =A0u32 avc_policy_seqno(void); > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/ser= vices.c > index 6ef4af4..4749202 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2212,7 +2212,7 @@ out_unlock: > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0rc =3D avc_has_perm_noaudit(fromsid, m= ysids[i], > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0SECCLASS_PROCESS, /* kernel value */ > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0PROCESS__TRANSITION, AVC_STRICT, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 NULL); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 NULL, 0); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (!rc) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0mysids2[j++] =3D mysid= s[i]; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cond_resched(); > -- > 1.7.4.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-secur= ity-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at =A0http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-securit= y-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html