Hello.
As I posted before in selinux list,
I found big overhead of SELinux in read/write on some CPUs,
and trying tuning.
There were discussion in previous threads.
Part 1:
http://marc.info/?t=118845343400001&r=1&w=2
Part 2:
http://marc.info/?t=118880749800004&r=1&w=2
I would like to RFC again about this topic.
1. Background
Look at benchmark result below.
lmbench simple read/write.
Big overhead exists especially on SH(SuperH) arch.
1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
Base SELinux Overhead(%)
Simple read 1.10 1.24 12.3
Simple write 1.00 1.14 14.0
* Base: kernel compiled without SELinux support
2) Result for SH(SH4, SH7751R), kernel 2.6.22
Base SELinux Overhead(%)
Simple read 2.39 5.49 130.5
Simple write 2.07 5.10 146.6
# This result is a little different from previous threads,
# because I changed some kernel configs.
Overhead more than 100%
I also found about 70-90% overhead in ARM.
2. About patch
I found a overhead in selinux_file_permission function.
This is a function that is called in read/write calls,
and does SELinux permission check.
SELinux checks permission both in open and read/write time.
Stephen Smalley sugessted that we can usually skip permission check
in selinux_file_permission.
By this patch,
permission check in selinux_file_permssion is done only when
- sid of task has changed after file open
- sid of inode has changed after file open
- policy load or boolean change happen after file open
To detect these changes,
I added entries in file_security struct and saving these values at file open.
And to save sid of inode at the time of file open,
I had to add new LSM hook in __dentry_open function.
3. Benchmark after applying this patch
1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
Base SELinux Overhead(%)
Simple read 1.10 1.12 1.6(Before 12.3)
Simple write 1.00 1.03 3.6(Before 14.0)
2) Result for SH(SH4, SH7751R), kernel 2.6.22
Base SELinux Overhead(%)
Simple read 2.39 2.65 11.1(Before 130.5)
Simple write 2.07 2.28 10.5(Before 146.6)
Performance has improved a lot.
I want comments from community.
Signed-off-by: Yuichi Nakamura<[email protected]>
---
fs/open.c | 5 +++
include/linux/security.h | 11 +++++++
security/selinux/avc.c | 5 ++-
security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++++++++++-
security/selinux/include/objsec.h | 3 ++
5 files changed, 76 insertions(+), 2 deletions(-)
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
--- linux-2.6.22.orig/security/selinux/avc.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/avc.c 2007-09-06 14:33:35.000000000 +0900
@@ -123,6 +123,7 @@ DEFINE_PER_CPU(struct avc_cache_stats, a
#endif
static struct avc_cache avc_cache;
+u32 policy_seqno = 0;
static struct avc_callback_node *avc_callbacks;
static struct kmem_cache *avc_node_cachep;
@@ -431,8 +432,10 @@ static int avc_latest_notif_update(int s
ret = -EAGAIN;
}
} else {
- if (seqno > avc_cache.latest_notif)
+ if (seqno > avc_cache.latest_notif) {
avc_cache.latest_notif = seqno;
+ policy_seqno = seqno;
+ }
}
spin_unlock_irqrestore(¬if_lock, flag);
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
--- linux-2.6.22.orig/security/selinux/hooks.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/hooks.c 2007-09-06 16:08:36.000000000 +0900
@@ -84,6 +84,7 @@
extern unsigned int policydb_loaded_version;
extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
extern int selinux_compat_net;
+extern u32 policy_seqno;
#ifdef CONFIG_SECURITY_SELINUX_DEVELOP
int selinux_enforcing = 0;
@@ -220,6 +221,8 @@ static int file_alloc_security(struct fi
fsec->file = file;
fsec->sid = tsec->sid;
+ fsec->tsid = tsec->sid;
+ fsec->pseqno = policy_seqno;
fsec->fown_sid = tsec->sid;
file->f_security = fsec;
@@ -2458,7 +2461,7 @@ static int selinux_inode_listsecurity(st
/* file security operations */
-static int selinux_file_permission(struct file *file, int mask)
+static int do_selinux_file_permission(struct file *file, int mask)
{
int rc;
struct inode *inode = file->f_path.dentry->d_inode;
@@ -2480,6 +2483,43 @@ static int selinux_file_permission(struc
return selinux_netlbl_inode_permission(inode, mask);
}
+static int selinux_file_permission(struct file *file, int mask)
+{
+ struct inode *inode = file->f_path.dentry->d_inode;
+ struct task_security_struct *tsec = current->security;
+ struct file_security_struct *fsec = file->f_security;
+ struct inode_security_struct *isec = inode->i_security;
+
+ if (!mask) {
+ /* No permission to check. Existence test. */
+ return 0;
+ }
+
+ if (tsec->sid != fsec->tsid) {
+ if (tsec->sid != fsec->sid) {
+ struct vfsmount *mnt = file->f_path.mnt;
+ struct dentry *dentry = file->f_path.dentry;
+ struct avc_audit_data ad;
+ int rc;
+ AVC_AUDIT_DATA_INIT(&ad, FS);
+ ad.u.fs.mnt = mnt;
+ ad.u.fs.dentry = dentry;
+ rc = avc_has_perm(tsec->sid, fsec->sid,
+ SECCLASS_FD,
+ FD__USE,
+ &ad);
+ if (rc)
+ return rc;
+ }
+ return do_selinux_file_permission(file, mask);
+ }
+
+ if (fsec->isid == isec->sid && fsec->pseqno == policy_seqno)
+ return selinux_netlbl_inode_permission(inode, mask);
+
+ return do_selinux_file_permission(file, mask);
+}
+
static int selinux_file_alloc_security(struct file *file)
{
return file_alloc_security(file);
@@ -2715,6 +2755,16 @@ static int selinux_file_receive(struct f
return file_has_perm(current, file, file_to_av(file));
}
+static int selinux_dentry_open(struct file *file, int flags)
+{
+ struct file_security_struct *fsec;
+ struct inode_security_struct *isec;
+ fsec = file->f_security;
+ isec = file->f_path.dentry->d_inode->i_security;
+ fsec->isid = isec->sid;
+ return 0;
+}
+
/* task security operations */
static int selinux_task_create(unsigned long clone_flags)
@@ -4780,6 +4830,8 @@ static struct security_operations selinu
.file_send_sigiotask = selinux_file_send_sigiotask,
.file_receive = selinux_file_receive,
+ .dentry_open = selinux_dentry_open,
+
.task_create = selinux_task_create,
.task_alloc_security = selinux_task_alloc_security,
.task_free_security = selinux_task_free_security,
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
--- linux-2.6.22.orig/security/selinux/include/objsec.h 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/objsec.h 2007-09-06 14:58:11.000000000 +0900
@@ -53,6 +53,9 @@ struct file_security_struct {
struct file *file; /* back pointer to file object */
u32 sid; /* SID of open file description */
u32 fown_sid; /* SID of file owner (for SIGIO) */
+ u32 tsid; /* SID of task at the time of file open*/
+ u32 isid; /* SID of inode at the time of file open */
+ u32 pseqno; /* Policy seqno at the time of file open */
};
struct superblock_security_struct {
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/fs/open.c linux-2.6.22/fs/open.c
--- linux-2.6.22.orig/fs/open.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/fs/open.c 2007-09-06 15:12:29.000000000 +0900
@@ -698,6 +698,11 @@ static struct file *__dentry_open(struct
if (!open && f->f_op)
open = f->f_op->open;
+
+ error = security_dentry_open(f, flags);
+ if (error)
+ goto cleanup_all;
+
if (open) {
error = open(inode, f);
if (error)
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
--- linux-2.6.22.orig/include/linux/security.h 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/include/linux/security.h 2007-09-06 15:22:39.000000000 +0900
@@ -503,6 +503,11 @@ struct request_sock;
* @file contains the file structure being received.
* Return 0 if permission is granted.
*
+ * Security hook for dentry
+ *
+ * @dentry_open
+ * Check permission or get additional information before opening dentry.
+ *
* Security hooks for task operations.
*
* @task_create:
@@ -1253,6 +1258,7 @@ struct security_operations {
int (*file_send_sigiotask) (struct task_struct * tsk,
struct fown_struct * fown, int sig);
int (*file_receive) (struct file * file);
+ int (*dentry_open) (struct file *file, int flags);
int (*task_create) (unsigned long clone_flags);
int (*task_alloc_security) (struct task_struct * p);
@@ -1854,6 +1860,11 @@ static inline int security_file_receive
return security_ops->file_receive (file);
}
+static inline int security_dentry_open (struct file *file, int flags)
+{
+ return security_ops->dentry_open (file, flags);
+}
+
static inline int security_task_create (unsigned long clone_flags)
{
return security_ops->task_create (clone_flags);
Regards,
--
Yuichi Nakamura
Hitachi Software Engineering Co., Ltd.
Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
SELinux Policy Editor: http://seedit.sourceforge.net/
On Thu, 2007-09-06 at 16:27 +0900, Yuichi Nakamura wrote:
> Hello.
>
> As I posted before in selinux list,
> I found big overhead of SELinux in read/write on some CPUs,
> and trying tuning.
> There were discussion in previous threads.
> Part 1:
> http://marc.info/?t=118845343400001&r=1&w=2
> Part 2:
> http://marc.info/?t=118880749800004&r=1&w=2
>
> I would like to RFC again about this topic.
Thanks for your work on this, as this is clearly an important area to
improve.
> 1. Background
> Look at benchmark result below.
> lmbench simple read/write.
> Big overhead exists especially on SH(SuperH) arch.
>
> 1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
> Base SELinux Overhead(%)
> Simple read 1.10 1.24 12.3
> Simple write 1.00 1.14 14.0
> * Base: kernel compiled without SELinux support
>
> 2) Result for SH(SH4, SH7751R), kernel 2.6.22
> Base SELinux Overhead(%)
> Simple read 2.39 5.49 130.5
> Simple write 2.07 5.10 146.6
> # This result is a little different from previous threads,
> # because I changed some kernel configs.
>
> Overhead more than 100%
> I also found about 70-90% overhead in ARM.
>
> 2. About patch
> I found a overhead in selinux_file_permission function.
> This is a function that is called in read/write calls,
> and does SELinux permission check.
> SELinux checks permission both in open and read/write time.
> Stephen Smalley sugessted that we can usually skip permission check
> in selinux_file_permission.
> By this patch,
> permission check in selinux_file_permssion is done only when
> - sid of task has changed after file open
> - sid of inode has changed after file open
> - policy load or boolean change happen after file open
>
> To detect these changes,
> I added entries in file_security struct and saving these values at file open.
>
> And to save sid of inode at the time of file open,
> I had to add new LSM hook in __dentry_open function.
>
> 3. Benchmark after applying this patch
> 1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
> Base SELinux Overhead(%)
> Simple read 1.10 1.12 1.6(Before 12.3)
> Simple write 1.00 1.03 3.6(Before 14.0)
>
> 2) Result for SH(SH4, SH7751R), kernel 2.6.22
> Base SELinux Overhead(%)
> Simple read 2.39 2.65 11.1(Before 130.5)
> Simple write 2.07 2.28 10.5(Before 146.6)
>
> Performance has improved a lot.
> I want comments from community.
>
> Signed-off-by: Yuichi Nakamura<[email protected]>
> ---
> fs/open.c | 5 +++
> include/linux/security.h | 11 +++++++
> security/selinux/avc.c | 5 ++-
> security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++++++++++-
> security/selinux/include/objsec.h | 3 ++
> 5 files changed, 76 insertions(+), 2 deletions(-)
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
> --- linux-2.6.22.orig/security/selinux/avc.c 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/avc.c 2007-09-06 14:33:35.000000000 +0900
> @@ -123,6 +123,7 @@ DEFINE_PER_CPU(struct avc_cache_stats, a
> #endif
>
> static struct avc_cache avc_cache;
> +u32 policy_seqno = 0;
> static struct avc_callback_node *avc_callbacks;
> static struct kmem_cache *avc_node_cachep;
>
> @@ -431,8 +432,10 @@ static int avc_latest_notif_update(int s
> ret = -EAGAIN;
> }
> } else {
> - if (seqno > avc_cache.latest_notif)
> + if (seqno > avc_cache.latest_notif) {
> avc_cache.latest_notif = seqno;
> + policy_seqno = seqno;
> + }
I would have just provided an avc interface for obtaining the seqno,
e.g.
u32 avc_policy_seqno(void)
{
return avc_cache.latest_notif;
}
> }
> spin_unlock_irqrestore(¬if_lock, flag);
>
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
> --- linux-2.6.22.orig/security/selinux/hooks.c 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/hooks.c 2007-09-06 16:08:36.000000000 +0900
> @@ -84,6 +84,7 @@
> extern unsigned int policydb_loaded_version;
> extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
> extern int selinux_compat_net;
> +extern u32 policy_seqno;
I think that they frown upon extern declarations in .c files (versus
in .h files), so we don't want to add more - we ultimately should clean
up what we presently have.
>
> #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
> int selinux_enforcing = 0;
> @@ -220,6 +221,8 @@ static int file_alloc_security(struct fi
>
> fsec->file = file;
> fsec->sid = tsec->sid;
> + fsec->tsid = tsec->sid;
I'm not sure why we need the separate field here, as fsec->sid already
holds the allocating task SID and doesn't change.
> + fsec->pseqno = policy_seqno;
Not sure that you want to set the seqno here versus from your new hook,
as it could conceivably change in between.
> fsec->fown_sid = tsec->sid;
> file->f_security = fsec;
>
> @@ -2458,7 +2461,7 @@ static int selinux_inode_listsecurity(st
>
> /* file security operations */
>
> -static int selinux_file_permission(struct file *file, int mask)
> +static int do_selinux_file_permission(struct file *file, int mask)
Might want to rename for clarity, e.g.
selinux_revalidate_file_permission or
selinux_file_permission_slow (slow path) or
something similar.
> {
> int rc;
> struct inode *inode = file->f_path.dentry->d_inode;
> @@ -2480,6 +2483,43 @@ static int selinux_file_permission(struc
> return selinux_netlbl_inode_permission(inode, mask);
> }
>
> +static int selinux_file_permission(struct file *file, int mask)
> +{
> + struct inode *inode = file->f_path.dentry->d_inode;
> + struct task_security_struct *tsec = current->security;
> + struct file_security_struct *fsec = file->f_security;
> + struct inode_security_struct *isec = inode->i_security;
> +
> + if (!mask) {
> + /* No permission to check. Existence test. */
> + return 0;
> + }
> +
> + if (tsec->sid != fsec->tsid) {
> + if (tsec->sid != fsec->sid) {
> + struct vfsmount *mnt = file->f_path.mnt;
> + struct dentry *dentry = file->f_path.dentry;
> + struct avc_audit_data ad;
> + int rc;
> + AVC_AUDIT_DATA_INIT(&ad, FS);
> + ad.u.fs.mnt = mnt;
> + ad.u.fs.dentry = dentry;
> + rc = avc_has_perm(tsec->sid, fsec->sid,
> + SECCLASS_FD,
> + FD__USE,
> + &ad);
> + if (rc)
> + return rc;
> + }
Why inline the FD_USE check here given that you are falling back to the
full function call anyway? I also don't see why you separate this from
the rest of the comparison - I'd just make it something like:
if (tsec->sid == fsec->sid && isec->sid == fsec->isid &&
avc_seqno() == fsec->seqno)
return selinux_netlbl_inode_permission(inode, mask);
return selinux_revalidate_file_permission(file, mask);
> static int selinux_file_alloc_security(struct file *file)
> {
> return file_alloc_security(file);
> @@ -2715,6 +2755,16 @@ static int selinux_file_receive(struct f
> return file_has_perm(current, file, file_to_av(file));
> }
>
> +static int selinux_dentry_open(struct file *file, int flags)
> +{
> + struct file_security_struct *fsec;
> + struct inode_security_struct *isec;
> + fsec = file->f_security;
> + isec = file->f_path.dentry->d_inode->i_security;
> + fsec->isid = isec->sid;
Set the seqno here too. Ideally, it would come straight from the AVC
entry that was used for the open-time check, but that is a bit more
invasive and there will always be a small window there.
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
> --- linux-2.6.22.orig/security/selinux/include/objsec.h 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/include/objsec.h 2007-09-06 14:58:11.000000000 +0900
> @@ -53,6 +53,9 @@ struct file_security_struct {
> struct file *file; /* back pointer to file object */
> u32 sid; /* SID of open file description */
> u32 fown_sid; /* SID of file owner (for SIGIO) */
> + u32 tsid; /* SID of task at the time of file open*/
> + u32 isid; /* SID of inode at the time of file open */
> + u32 pseqno; /* Policy seqno at the time of file open */
No need for tsid above I think.
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
> --- linux-2.6.22.orig/include/linux/security.h 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/include/linux/security.h 2007-09-06 15:22:39.000000000 +0900
> @@ -503,6 +503,11 @@ struct request_sock;
> * @file contains the file structure being received.
> * Return 0 if permission is granted.
> *
> + * Security hook for dentry
> + *
> + * @dentry_open
> + * Check permission or get additional information before opening dentry.
> + *
> * Security hooks for task operations.
> *
> * @task_create:
> @@ -1253,6 +1258,7 @@ struct security_operations {
> int (*file_send_sigiotask) (struct task_struct * tsk,
> struct fown_struct * fown, int sig);
> int (*file_receive) (struct file * file);
> + int (*dentry_open) (struct file *file, int flags);
>
> int (*task_create) (unsigned long clone_flags);
> int (*task_alloc_security) (struct task_struct * p);
> @@ -1854,6 +1860,11 @@ static inline int security_file_receive
> return security_ops->file_receive (file);
> }
>
> +static inline int security_dentry_open (struct file *file, int flags)
> +{
> + return security_ops->dentry_open (file, flags);
> +}
> +
> static inline int security_task_create (unsigned long clone_flags)
> {
> return security_ops->task_create (clone_flags);
Need to also provide the stub definition in the #else case (SECURITY=n)
and a stub function for the dummy security module.
Thanks.
--
Stephen Smalley
National Security Agency
On Thu, 06 Sep 2007 09:47:15 -0400
Stephen Smalley wrote:
<snip>
> >
> > @@ -431,8 +432,10 @@ static int avc_latest_notif_update(int s
> > ret = -EAGAIN;
> > }
> > } else {
> > - if (seqno > avc_cache.latest_notif)
> > + if (seqno > avc_cache.latest_notif) {
> > avc_cache.latest_notif = seqno;
> > + policy_seqno = seqno;
> > + }
>
> I would have just provided an avc interface for obtaining the seqno,
> e.g.
> u32 avc_policy_seqno(void)
> {
> return avc_cache.latest_notif;
> }
Fixed.
>
> > }
> > spin_unlock_irqrestore(¬if_lock, flag);
> >
> > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
> > --- linux-2.6.22.orig/security/selinux/hooks.c 2007-07-09 08:32:17.000000000 +0900
> > +++ linux-2.6.22/security/selinux/hooks.c 2007-09-06 16:08:36.000000000 +0900
> > @@ -84,6 +84,7 @@
> > extern unsigned int policydb_loaded_version;
> > extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
> > extern int selinux_compat_net;
> > +extern u32 policy_seqno;
>
> I think that they frown upon extern declarations in .c files (versus
> in .h files), so we don't want to add more - we ultimately should clean
> up what we presently have.
Removed policy_seqno.
>
> >
> > #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
> > int selinux_enforcing = 0;
> > @@ -220,6 +221,8 @@ static int file_alloc_security(struct fi
> >
> > fsec->file = file;
> > fsec->sid = tsec->sid;
> > + fsec->tsid = tsec->sid;
>
> I'm not sure why we need the separate field here, as fsec->sid already
> holds the allocating task SID and doesn't change.
I see.
removed fsec->tsid.
> > + fsec->pseqno = policy_seqno;
>
> Not sure that you want to set the seqno here versus from your new hook,
> as it could conceivably change in between.
Fixed, pseqno is set in selinux_dentry_open.
> > fsec->fown_sid = tsec->sid;
> > file->f_security = fsec;
> >
> > @@ -2458,7 +2461,7 @@ static int selinux_inode_listsecurity(st
> >
> > /* file security operations */
> >
> > -static int selinux_file_permission(struct file *file, int mask)
> > +static int do_selinux_file_permission(struct file *file, int mask)
>
> Might want to rename for clarity, e.g.
> selinux_revalidate_file_permission or
> selinux_file_permission_slow (slow path) or
> something similar.
Renamed to selinux_revalidate_file_permission.
>
> > {
> > int rc;
> > struct inode *inode = file->f_path.dentry->d_inode;
> > @@ -2480,6 +2483,43 @@ static int selinux_file_permission(struc
> > return selinux_netlbl_inode_permission(inode, mask);
> > }
> >
> > +static int selinux_file_permission(struct file *file, int mask)
> > +{
> > + struct inode *inode = file->f_path.dentry->d_inode;
> > + struct task_security_struct *tsec = current->security;
> > + struct file_security_struct *fsec = file->f_security;
> > + struct inode_security_struct *isec = inode->i_security;
> > +
> > + if (!mask) {
> > + /* No permission to check. Existence test. */
> > + return 0;
> > + }
> > +
> > + if (tsec->sid != fsec->tsid) {
> > + if (tsec->sid != fsec->sid) {
> > + struct vfsmount *mnt = file->f_path.mnt;
> > + struct dentry *dentry = file->f_path.dentry;
> > + struct avc_audit_data ad;
> > + int rc;
> > + AVC_AUDIT_DATA_INIT(&ad, FS);
> > + ad.u.fs.mnt = mnt;
> > + ad.u.fs.dentry = dentry;
> > + rc = avc_has_perm(tsec->sid, fsec->sid,
> > + SECCLASS_FD,
> > + FD__USE,
> > + &ad);
> > + if (rc)
> > + return rc;
> > + }
>
> Why inline the FD_USE check here given that you are falling back to the
> full function call anyway? I also don't see why you separate this from
> the rest of the comparison - I'd just make it something like:
> if (tsec->sid == fsec->sid && isec->sid == fsec->isid &&
> avc_seqno() == fsec->seqno)
> return selinux_netlbl_inode_permission(inode, mask);
> return selinux_revalidate_file_permission(file, mask);
Thanks, I missed that.
FD_USE check is called in file_permission..
Fixed like you pointed out.
> > static int selinux_file_alloc_security(struct file *file)
> > {
> > return file_alloc_security(file);
> > @@ -2715,6 +2755,16 @@ static int selinux_file_receive(struct f
> > return file_has_perm(current, file, file_to_av(file));
> > }
> >
> > +static int selinux_dentry_open(struct file *file, int flags)
> > +{
> > + struct file_security_struct *fsec;
> > + struct inode_security_struct *isec;
> > + fsec = file->f_security;
> > + isec = file->f_path.dentry->d_inode->i_security;
> > + fsec->isid = isec->sid;
>
> Set the seqno here too. Ideally, it would come straight from the AVC
> entry that was used for the open-time check, but that is a bit more
> invasive and there will always be a small window there.
Fixed, setting pseqno now.
> > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
> > --- linux-2.6.22.orig/security/selinux/include/objsec.h 2007-07-09 08:32:17.000000000 +0900
> > +++ linux-2.6.22/security/selinux/include/objsec.h 2007-09-06 14:58:11.000000000 +0900
> > @@ -53,6 +53,9 @@ struct file_security_struct {
> > struct file *file; /* back pointer to file object */
> > u32 sid; /* SID of open file description */
> > u32 fown_sid; /* SID of file owner (for SIGIO) */
> > + u32 tsid; /* SID of task at the time of file open*/
> > + u32 isid; /* SID of inode at the time of file open */
> > + u32 pseqno; /* Policy seqno at the time of file open */
>
> No need for tsid above I think.
Removed tsid.
> > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
> > --- linux-2.6.22.orig/include/linux/security.h 2007-07-09 08:32:17.000000000 +0900
> > +++ linux-2.6.22/include/linux/security.h 2007-09-06 15:22:39.000000000 +0900
> > @@ -503,6 +503,11 @@ struct request_sock;
> > * @file contains the file structure being received.
> > * Return 0 if permission is granted.
> > *
> > + * Security hook for dentry
> > + *
> > + * @dentry_open
> > + * Check permission or get additional information before opening dentry.
> > + *
> > * Security hooks for task operations.
> > *
> > * @task_create:
> > @@ -1253,6 +1258,7 @@ struct security_operations {
> > int (*file_send_sigiotask) (struct task_struct * tsk,
> > struct fown_struct * fown, int sig);
> > int (*file_receive) (struct file * file);
> > + int (*dentry_open) (struct file *file, int flags);
> >
> > int (*task_create) (unsigned long clone_flags);
> > int (*task_alloc_security) (struct task_struct * p);
> > @@ -1854,6 +1860,11 @@ static inline int security_file_receive
> > return security_ops->file_receive (file);
> > }
> >
> > +static inline int security_dentry_open (struct file *file, int flags)
> > +{
> > + return security_ops->dentry_open (file, flags);
> > +}
> > +
> > static inline int security_task_create (unsigned long clone_flags)
> > {
> > return security_ops->task_create (clone_flags);
>
> Need to also provide the stub definition in the #else case (SECURITY=n)
> and a stub function for the dummy security module.
Fixed.
>
> Thanks.
>
> --
> Stephen Smalley
> National Security Agency
Next is updated patch.
Signed-off-by: Yuichi Nakamura<[email protected]>
---
fs/open.c | 5 +++++
include/linux/security.h | 16 ++++++++++++++++
security/selinux/avc.c | 5 +++++
security/selinux/hooks.c | 36 +++++++++++++++++++++++++++++++++++-
security/selinux/include/avc.h | 2 ++
security/selinux/include/objsec.h | 2 ++
6 files changed, 65 insertions(+), 1 deletion(-)
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
--- linux-2.6.22.orig/security/selinux/avc.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/avc.c 2007-09-10 09:56:22.000000000 +0900
@@ -913,3 +913,8 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
return rc;
}
+
+u32 avc_policy_seqno(void)
+{
+ return avc_cache.latest_notif;
+}
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
--- linux-2.6.22.orig/security/selinux/hooks.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/hooks.c 2007-09-10 10:11:13.000000000 +0900
@@ -14,6 +14,8 @@
* <[email protected]>
* Copyright (C) 2006 Hewlett-Packard Development Company, L.P.
* Paul Moore, <[email protected]>
+ * Copyright (C) 2007 Hitachi Software Engineering Co., Ltd.
+ * Yuichi Nakamura <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2,
@@ -2458,7 +2460,7 @@ static int selinux_inode_listsecurity(st
/* file security operations */
-static int selinux_file_permission(struct file *file, int mask)
+static int selinux_revalidate_file_permission(struct file *file, int mask)
{
int rc;
struct inode *inode = file->f_path.dentry->d_inode;
@@ -2480,6 +2482,25 @@ static int selinux_file_permission(struc
return selinux_netlbl_inode_permission(inode, mask);
}
+static int selinux_file_permission(struct file *file, int mask)
+{
+ struct inode *inode = file->f_path.dentry->d_inode;
+ struct task_security_struct *tsec = current->security;
+ struct file_security_struct *fsec = file->f_security;
+ struct inode_security_struct *isec = inode->i_security;
+
+ if (!mask) {
+ /* No permission to check. Existence test. */
+ return 0;
+ }
+
+ if (tsec->sid == fsec->sid && fsec->isid == isec->sid
+ && fsec->pseqno == avc_policy_seqno())
+ return selinux_netlbl_inode_permission(inode, mask);
+
+ return selinux_revalidate_file_permission(file, mask);
+}
+
static int selinux_file_alloc_security(struct file *file)
{
return file_alloc_security(file);
@@ -2715,6 +2736,17 @@ static int selinux_file_receive(struct f
return file_has_perm(current, file, file_to_av(file));
}
+static int selinux_dentry_open(struct file *file, int flags)
+{
+ struct file_security_struct *fsec;
+ struct inode_security_struct *isec;
+ fsec = file->f_security;
+ isec = file->f_path.dentry->d_inode->i_security;
+ fsec->isid = isec->sid;
+ fsec->pseqno = avc_policy_seqno();
+ return 0;
+}
+
/* task security operations */
static int selinux_task_create(unsigned long clone_flags)
@@ -4780,6 +4812,8 @@ static struct security_operations selinu
.file_send_sigiotask = selinux_file_send_sigiotask,
.file_receive = selinux_file_receive,
+ .dentry_open = selinux_dentry_open,
+
.task_create = selinux_task_create,
.task_alloc_security = selinux_task_alloc_security,
.task_free_security = selinux_task_free_security,
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/avc.h linux-2.6.22/security/selinux/include/avc.h
--- linux-2.6.22.orig/security/selinux/include/avc.h 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/avc.h 2007-09-10 09:56:22.000000000 +0900
@@ -110,6 +110,8 @@ int avc_has_perm(u32 ssid, u32 tsid,
u16 tclass, u32 requested,
struct avc_audit_data *auditdata);
+u32 avc_policy_seqno(void);
+
#define AVC_CALLBACK_GRANT 1
#define AVC_CALLBACK_TRY_REVOKE 2
#define AVC_CALLBACK_REVOKE 4
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
--- linux-2.6.22.orig/security/selinux/include/objsec.h 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/objsec.h 2007-09-10 09:56:22.000000000 +0900
@@ -53,6 +53,8 @@ struct file_security_struct {
struct file *file; /* back pointer to file object */
u32 sid; /* SID of open file description */
u32 fown_sid; /* SID of file owner (for SIGIO) */
+ u32 isid; /* SID of inode at the time of file open */
+ u32 pseqno; /* Policy seqno at the time of file open */
};
struct superblock_security_struct {
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/fs/open.c linux-2.6.22/fs/open.c
--- linux-2.6.22.orig/fs/open.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/fs/open.c 2007-09-10 09:56:22.000000000 +0900
@@ -698,6 +698,11 @@ static struct file *__dentry_open(struct
if (!open && f->f_op)
open = f->f_op->open;
+
+ error = security_dentry_open(f, flags);
+ if (error)
+ goto cleanup_all;
+
if (open) {
error = open(inode, f);
if (error)
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
--- linux-2.6.22.orig/include/linux/security.h 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/include/linux/security.h 2007-09-10 09:56:22.000000000 +0900
@@ -503,6 +503,11 @@ struct request_sock;
* @file contains the file structure being received.
* Return 0 if permission is granted.
*
+ * Security hook for dentry
+ *
+ * @dentry_open
+ * Check permission or get additional information before opening dentry.
+ *
* Security hooks for task operations.
*
* @task_create:
@@ -1253,6 +1258,7 @@ struct security_operations {
int (*file_send_sigiotask) (struct task_struct * tsk,
struct fown_struct * fown, int sig);
int (*file_receive) (struct file * file);
+ int (*dentry_open) (struct file *file, int flags);
int (*task_create) (unsigned long clone_flags);
int (*task_alloc_security) (struct task_struct * p);
@@ -1854,6 +1860,11 @@ static inline int security_file_receive
return security_ops->file_receive (file);
}
+static inline int security_dentry_open (struct file *file, int flags)
+{
+ return security_ops->dentry_open (file, flags);
+}
+
static inline int security_task_create (unsigned long clone_flags)
{
return security_ops->task_create (clone_flags);
@@ -2529,6 +2540,11 @@ static inline int security_file_receive
return 0;
}
+static inline int security_dentry_open (struct file *file, int flags)
+{
+ return 0;
+}
+
static inline int security_task_create (unsigned long clone_flags)
{
return 0;
Regards,
--
Yuichi Nakamura
Hitachi Software Engineering Co., Ltd.
Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
SELinux Policy Editor: http://seedit.sourceforge.net/
On Mon, 2007-09-10 at 10:31 +0900, Yuichi Nakamura wrote:
> Next is updated patch.
Thanks.
Please include the short description of the patch though when
re-submitting.
> Signed-off-by: Yuichi Nakamura<[email protected]>
> ---
> fs/open.c | 5 +++++
> include/linux/security.h | 16 ++++++++++++++++
> security/selinux/avc.c | 5 +++++
> security/selinux/hooks.c | 36 +++++++++++++++++++++++++++++++++++-
> security/selinux/include/avc.h | 2 ++
> security/selinux/include/objsec.h | 2 ++
> 6 files changed, 65 insertions(+), 1 deletion(-)
Still missing the necessary changes to security/dummy.c (add
dummy_dentry_open() and update security_fixup_ops()). For
CONFIG_SECURITY=y but SELinux disabled.
Also, have you re-run your benchmarks with this version of the patch?
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
> --- linux-2.6.22.orig/security/selinux/avc.c 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/avc.c 2007-09-10 09:56:22.000000000 +0900
> @@ -913,3 +913,8 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
> avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
> return rc;
> }
> +
> +u32 avc_policy_seqno(void)
> +{
> + return avc_cache.latest_notif;
> +}
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
> --- linux-2.6.22.orig/security/selinux/hooks.c 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/hooks.c 2007-09-10 10:11:13.000000000 +0900
> @@ -14,6 +14,8 @@
> * <[email protected]>
> * Copyright (C) 2006 Hewlett-Packard Development Company, L.P.
> * Paul Moore, <[email protected]>
> + * Copyright (C) 2007 Hitachi Software Engineering Co., Ltd.
> + * Yuichi Nakamura <[email protected]>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2,
> @@ -2458,7 +2460,7 @@ static int selinux_inode_listsecurity(st
>
> /* file security operations */
>
> -static int selinux_file_permission(struct file *file, int mask)
> +static int selinux_revalidate_file_permission(struct file *file, int mask)
> {
> int rc;
> struct inode *inode = file->f_path.dentry->d_inode;
> @@ -2480,6 +2482,25 @@ static int selinux_file_permission(struc
> return selinux_netlbl_inode_permission(inode, mask);
> }
>
> +static int selinux_file_permission(struct file *file, int mask)
> +{
> + struct inode *inode = file->f_path.dentry->d_inode;
> + struct task_security_struct *tsec = current->security;
> + struct file_security_struct *fsec = file->f_security;
> + struct inode_security_struct *isec = inode->i_security;
> +
> + if (!mask) {
> + /* No permission to check. Existence test. */
> + return 0;
> + }
> +
> + if (tsec->sid == fsec->sid && fsec->isid == isec->sid
> + && fsec->pseqno == avc_policy_seqno())
> + return selinux_netlbl_inode_permission(inode, mask);
> +
> + return selinux_revalidate_file_permission(file, mask);
> +}
> +
> static int selinux_file_alloc_security(struct file *file)
> {
> return file_alloc_security(file);
> @@ -2715,6 +2736,17 @@ static int selinux_file_receive(struct f
> return file_has_perm(current, file, file_to_av(file));
> }
>
> +static int selinux_dentry_open(struct file *file, int flags)
> +{
> + struct file_security_struct *fsec;
> + struct inode_security_struct *isec;
> + fsec = file->f_security;
> + isec = file->f_path.dentry->d_inode->i_security;
> + fsec->isid = isec->sid;
> + fsec->pseqno = avc_policy_seqno();
> + return 0;
> +}
> +
> /* task security operations */
>
> static int selinux_task_create(unsigned long clone_flags)
> @@ -4780,6 +4812,8 @@ static struct security_operations selinu
> .file_send_sigiotask = selinux_file_send_sigiotask,
> .file_receive = selinux_file_receive,
>
> + .dentry_open = selinux_dentry_open,
> +
> .task_create = selinux_task_create,
> .task_alloc_security = selinux_task_alloc_security,
> .task_free_security = selinux_task_free_security,
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/avc.h linux-2.6.22/security/selinux/include/avc.h
> --- linux-2.6.22.orig/security/selinux/include/avc.h 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/include/avc.h 2007-09-10 09:56:22.000000000 +0900
> @@ -110,6 +110,8 @@ int avc_has_perm(u32 ssid, u32 tsid,
> u16 tclass, u32 requested,
> struct avc_audit_data *auditdata);
>
> +u32 avc_policy_seqno(void);
> +
> #define AVC_CALLBACK_GRANT 1
> #define AVC_CALLBACK_TRY_REVOKE 2
> #define AVC_CALLBACK_REVOKE 4
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
> --- linux-2.6.22.orig/security/selinux/include/objsec.h 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/include/objsec.h 2007-09-10 09:56:22.000000000 +0900
> @@ -53,6 +53,8 @@ struct file_security_struct {
> struct file *file; /* back pointer to file object */
> u32 sid; /* SID of open file description */
> u32 fown_sid; /* SID of file owner (for SIGIO) */
> + u32 isid; /* SID of inode at the time of file open */
> + u32 pseqno; /* Policy seqno at the time of file open */
> };
>
> struct superblock_security_struct {
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/fs/open.c linux-2.6.22/fs/open.c
> --- linux-2.6.22.orig/fs/open.c 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/fs/open.c 2007-09-10 09:56:22.000000000 +0900
> @@ -698,6 +698,11 @@ static struct file *__dentry_open(struct
>
> if (!open && f->f_op)
> open = f->f_op->open;
> +
> + error = security_dentry_open(f, flags);
> + if (error)
> + goto cleanup_all;
> +
> if (open) {
> error = open(inode, f);
> if (error)
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
> --- linux-2.6.22.orig/include/linux/security.h 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/include/linux/security.h 2007-09-10 09:56:22.000000000 +0900
> @@ -503,6 +503,11 @@ struct request_sock;
> * @file contains the file structure being received.
> * Return 0 if permission is granted.
> *
> + * Security hook for dentry
> + *
> + * @dentry_open
> + * Check permission or get additional information before opening dentry.
> + *
> * Security hooks for task operations.
> *
> * @task_create:
> @@ -1253,6 +1258,7 @@ struct security_operations {
> int (*file_send_sigiotask) (struct task_struct * tsk,
> struct fown_struct * fown, int sig);
> int (*file_receive) (struct file * file);
> + int (*dentry_open) (struct file *file, int flags);
>
> int (*task_create) (unsigned long clone_flags);
> int (*task_alloc_security) (struct task_struct * p);
> @@ -1854,6 +1860,11 @@ static inline int security_file_receive
> return security_ops->file_receive (file);
> }
>
> +static inline int security_dentry_open (struct file *file, int flags)
> +{
> + return security_ops->dentry_open (file, flags);
> +}
> +
> static inline int security_task_create (unsigned long clone_flags)
> {
> return security_ops->task_create (clone_flags);
> @@ -2529,6 +2540,11 @@ static inline int security_file_receive
> return 0;
> }
>
> +static inline int security_dentry_open (struct file *file, int flags)
> +{
> + return 0;
> +}
> +
> static inline int security_task_create (unsigned long clone_flags)
> {
> return 0;
>
> Regards,
--
Stephen Smalley
National Security Agency
Hi.
Stephen Smalley pointed out possibility of race condition
in off-list discussion.
Stephen Smalley said:
> One other observation about the patch: it presently leaves open a
> (small) race window in which the file could get relabeled or policy gets
> reloaded between the time of the normal permission check (from
> selinux_inode_permission) and the time you copy the inode SID and policy
> seqno to the file security struct. In which case you might end up never
> revalidating access upon read/write even though conditions changed since
> the open-time permission check. Not sure how to cleanly fix in a
> lock-free manner, and adding locks here will only make matters worse.
To fix that, permission has to be checked in selinux_dentry_open.
Therefore, in open, number of permission checks increased.
As shown in benchmark result below, it does not affect open/close
performance so much.
Following is benchmark result.
* Benchmark
lmbench simple read,write,open/close.
* Before tuning
1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
Base SELinux Overhead(%)
Simple read 1.10 1.24 12.3
Simple write 1.02 1.14 14.0
open/close 5.97 7.45 24.9
* Base: kernel compiled without SELinux support
2) Result for SH(SH4, SH7751R), kernel 2.6.22
Base SELinux Overhead(%)
Simple read 2.39 5.49 130.5
Simple write 2.07 5.10 146.6
open/close 32.6 62.8 93.0
* After tuning
1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
Base SELinux Overhead(%)
Simple read 1.10 1.13 2.3(Before 12.3)
Simple write 1.02 1.024 0.6(Before 14.0)
open/close 5.97 7.48 25.3(Before 24.9)
* Base: kernel compiled without SELinux support
2) Result for SH(SH4, SH7751R), kernel 2.6.22
Base SELinux Overhead(%)
Simple read 2.39 2.63 10.4(Before 130.5)
Simple write 2.07 2.34 13.1(Before 146.6)
open/close 32.6 58.7 80.2(before 93.0)
Next is a patch.
* Description of patch
This patch improves performance of read/write in SELinux.
It improves performance by skipping permission check in
selinux_file_permission. Permission is only checked when
sid change or policy load is detected after file open.
To detect sid change, new LSM hook securiy_dentry_open is added.
Signed-off-by: Yuichi Nakamura<[email protected]>
---
fs/open.c | 5 ++++
include/linux/security.h | 16 ++++++++++++++
security/dummy.c | 6 +++++
security/selinux/avc.c | 5 ++++
security/selinux/hooks.c | 43 +++++++++++++++++++++++++++++++++++++-
security/selinux/include/avc.h | 2 +
security/selinux/include/objsec.h | 2 +
7 files changed, 78 insertions(+), 1 deletion(-)
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
--- linux-2.6.22.orig/security/selinux/avc.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/avc.c 2007-09-12 08:24:27.000000000 +0900
@@ -913,3 +913,8 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
return rc;
}
+
+u32 avc_policy_seqno(void)
+{
+ return avc_cache.latest_notif;
+}
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
--- linux-2.6.22.orig/security/selinux/hooks.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/hooks.c 2007-09-12 08:42:49.000000000 +0900
@@ -14,6 +14,8 @@
* <[email protected]>
* Copyright (C) 2006 Hewlett-Packard Development Company, L.P.
* Paul Moore, <[email protected]>
+ * Copyright (C) 2007 Hitachi Software Engineering Co., Ltd.
+ * Yuichi Nakamura <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2,
@@ -80,6 +82,7 @@
#define XATTR_SELINUX_SUFFIX "selinux"
#define XATTR_NAME_SELINUX XATTR_SECURITY_PREFIX XATTR_SELINUX_SUFFIX
+#define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])
extern unsigned int policydb_loaded_version;
extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
@@ -2458,7 +2461,7 @@ static int selinux_inode_listsecurity(st
/* file security operations */
-static int selinux_file_permission(struct file *file, int mask)
+static int selinux_revalidate_file_permission(struct file *file, int mask)
{
int rc;
struct inode *inode = file->f_path.dentry->d_inode;
@@ -2480,6 +2483,25 @@ static int selinux_file_permission(struc
return selinux_netlbl_inode_permission(inode, mask);
}
+static int selinux_file_permission(struct file *file, int mask)
+{
+ struct inode *inode = file->f_path.dentry->d_inode;
+ struct task_security_struct *tsec = current->security;
+ struct file_security_struct *fsec = file->f_security;
+ struct inode_security_struct *isec = inode->i_security;
+
+ if (!mask) {
+ /* No permission to check. Existence test. */
+ return 0;
+ }
+
+ if (tsec->sid == fsec->sid && fsec->isid == isec->sid
+ && fsec->pseqno == avc_policy_seqno())
+ return selinux_netlbl_inode_permission(inode, mask);
+
+ return selinux_revalidate_file_permission(file, mask);
+}
+
static int selinux_file_alloc_security(struct file *file)
{
return file_alloc_security(file);
@@ -2715,6 +2737,23 @@ static int selinux_file_receive(struct f
return file_has_perm(current, file, file_to_av(file));
}
+static int selinux_dentry_open(struct file *file)
+{
+ struct file_security_struct *fsec;
+ struct inode *inode;
+ struct inode_security_struct *isec;
+ inode = file->f_path.dentry->d_inode;
+ fsec = file->f_security;
+ isec = inode->i_security;
+ fsec->isid = isec->sid;
+ fsec->pseqno = avc_policy_seqno();
+
+ /*Permission has to be rechecked here.
+ Policy load of inode sid can happen between
+ may_open and selinux_dentry_open.*/
+ return inode_has_perm(current, inode, file_to_av(file), NULL);
+}
+
/* task security operations */
static int selinux_task_create(unsigned long clone_flags)
@@ -4780,6 +4819,8 @@ static struct security_operations selinu
.file_send_sigiotask = selinux_file_send_sigiotask,
.file_receive = selinux_file_receive,
+ .dentry_open = selinux_dentry_open,
+
.task_create = selinux_task_create,
.task_alloc_security = selinux_task_alloc_security,
.task_free_security = selinux_task_free_security,
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/avc.h linux-2.6.22/security/selinux/include/avc.h
--- linux-2.6.22.orig/security/selinux/include/avc.h 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/avc.h 2007-09-12 08:24:27.000000000 +0900
@@ -110,6 +110,8 @@ int avc_has_perm(u32 ssid, u32 tsid,
u16 tclass, u32 requested,
struct avc_audit_data *auditdata);
+u32 avc_policy_seqno(void);
+
#define AVC_CALLBACK_GRANT 1
#define AVC_CALLBACK_TRY_REVOKE 2
#define AVC_CALLBACK_REVOKE 4
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
--- linux-2.6.22.orig/security/selinux/include/objsec.h 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/objsec.h 2007-09-12 08:24:27.000000000 +0900
@@ -53,6 +53,8 @@ struct file_security_struct {
struct file *file; /* back pointer to file object */
u32 sid; /* SID of open file description */
u32 fown_sid; /* SID of file owner (for SIGIO) */
+ u32 isid; /* SID of inode at the time of file open */
+ u32 pseqno; /* Policy seqno at the time of file open */
};
struct superblock_security_struct {
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/fs/open.c linux-2.6.22/fs/open.c
--- linux-2.6.22.orig/fs/open.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/fs/open.c 2007-09-12 08:31:24.000000000 +0900
@@ -696,8 +696,13 @@ static struct file *__dentry_open(struct
f->f_op = fops_get(inode->i_fop);
file_move(f, &inode->i_sb->s_files);
+ error = security_dentry_open(f);
+ if (error)
+ goto cleanup_all;
+
if (!open && f->f_op)
open = f->f_op->open;
+
if (open) {
error = open(inode, f);
if (error)
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
--- linux-2.6.22.orig/include/linux/security.h 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/include/linux/security.h 2007-09-12 08:30:16.000000000 +0900
@@ -503,6 +503,11 @@ struct request_sock;
* @file contains the file structure being received.
* Return 0 if permission is granted.
*
+ * Security hook for dentry
+ *
+ * @dentry_open
+ * Check permission or get additional information before opening dentry.
+ *
* Security hooks for task operations.
*
* @task_create:
@@ -1253,6 +1258,7 @@ struct security_operations {
int (*file_send_sigiotask) (struct task_struct * tsk,
struct fown_struct * fown, int sig);
int (*file_receive) (struct file * file);
+ int (*dentry_open) (struct file *file);
int (*task_create) (unsigned long clone_flags);
int (*task_alloc_security) (struct task_struct * p);
@@ -1854,6 +1860,11 @@ static inline int security_file_receive
return security_ops->file_receive (file);
}
+static inline int security_dentry_open (struct file *file)
+{
+ return security_ops->dentry_open (file);
+}
+
static inline int security_task_create (unsigned long clone_flags)
{
return security_ops->task_create (clone_flags);
@@ -2529,6 +2540,11 @@ static inline int security_file_receive
return 0;
}
+static inline int security_dentry_open (struct file *file)
+{
+ return 0;
+}
+
static inline int security_task_create (unsigned long clone_flags)
{
return 0;
--- linux-2.6.22.orig/security/dummy.c 2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/dummy.c 2007-09-12 08:29:45.000000000 +0900
@@ -459,6 +459,11 @@ static int dummy_file_receive (struct fi
return 0;
}
+static int dummy_dentry_open (struct file *file)
+{
+ return 0;
+}
+
static int dummy_task_create (unsigned long clone_flags)
{
return 0;
@@ -1029,6 +1034,7 @@ void security_fixup_ops (struct security
set_to_dummy_if_null(ops, file_set_fowner);
set_to_dummy_if_null(ops, file_send_sigiotask);
set_to_dummy_if_null(ops, file_receive);
+ set_to_dummy_if_null(ops, dentry_open);
set_to_dummy_if_null(ops, task_create);
set_to_dummy_if_null(ops, task_alloc_security);
set_to_dummy_if_null(ops, task_free_security);
Regards,
--
Yuichi Nakamura
Hitachi Software Engineering Co., Ltd.
Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
SELinux Policy Editor: http://seedit.sourceforge.net/
On Wed, 2007-09-12 at 17:51 +0900, Yuichi Nakamura wrote:
> Hi.
>
> Stephen Smalley pointed out possibility of race condition
> in off-list discussion.
> Stephen Smalley said:
> > One other observation about the patch: it presently leaves open a
> > (small) race window in which the file could get relabeled or policy gets
> > reloaded between the time of the normal permission check (from
> > selinux_inode_permission) and the time you copy the inode SID and policy
> > seqno to the file security struct. In which case you might end up never
> > revalidating access upon read/write even though conditions changed since
> > the open-time permission check. Not sure how to cleanly fix in a
> > lock-free manner, and adding locks here will only make matters worse.
>
> To fix that, permission has to be checked in selinux_dentry_open.
> Therefore, in open, number of permission checks increased.
> As shown in benchmark result below, it does not affect open/close
> performance so much.
>
> Following is benchmark result.
> * Benchmark
> lmbench simple read,write,open/close.
>
> * Before tuning
> 1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
> Base SELinux Overhead(%)
> Simple read 1.10 1.24 12.3
> Simple write 1.02 1.14 14.0
> open/close 5.97 7.45 24.9
> * Base: kernel compiled without SELinux support
>
> 2) Result for SH(SH4, SH7751R), kernel 2.6.22
> Base SELinux Overhead(%)
> Simple read 2.39 5.49 130.5
> Simple write 2.07 5.10 146.6
> open/close 32.6 62.8 93.0
>
> * After tuning
> 1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
> Base SELinux Overhead(%)
> Simple read 1.10 1.13 2.3(Before 12.3)
> Simple write 1.02 1.024 0.6(Before 14.0)
> open/close 5.97 7.48 25.3(Before 24.9)
> * Base: kernel compiled without SELinux support
>
> 2) Result for SH(SH4, SH7751R), kernel 2.6.22
> Base SELinux Overhead(%)
> Simple read 2.39 2.63 10.4(Before 130.5)
> Simple write 2.07 2.34 13.1(Before 146.6)
> open/close 32.6 58.7 80.2(before 93.0)
>
> Next is a patch.
Thanks, a few comments below.
>
> * Description of patch
> This patch improves performance of read/write in SELinux.
> It improves performance by skipping permission check in
> selinux_file_permission. Permission is only checked when
> sid change or policy load is detected after file open.
> To detect sid change, new LSM hook securiy_dentry_open is added.
I think I'd reword this a little, e.g.
It reduces the selinux overhead on read/write by only revalidating
permissions in selinux_file_permission if the task or inode labels have
changed or the policy has changed since the open-time check. A new LSM
hook, security_dentry_open, is added to capture the necessary state at
open time to allow this optimization.
>
> Signed-off-by: Yuichi Nakamura<[email protected]>
> ---
> fs/open.c | 5 ++++
> include/linux/security.h | 16 ++++++++++++++
> security/dummy.c | 6 +++++
> security/selinux/avc.c | 5 ++++
> security/selinux/hooks.c | 43 +++++++++++++++++++++++++++++++++++++-
> security/selinux/include/avc.h | 2 +
> security/selinux/include/objsec.h | 2 +
> 7 files changed, 78 insertions(+), 1 deletion(-)
<snip>
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
> --- linux-2.6.22.orig/security/selinux/hooks.c 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/hooks.c 2007-09-12 08:42:49.000000000 +0900
> @@ -80,6 +82,7 @@
>
> #define XATTR_SELINUX_SUFFIX "selinux"
> #define XATTR_NAME_SELINUX XATTR_SECURITY_PREFIX XATTR_SELINUX_SUFFIX
> +#define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])
Leftover from prior version of the patch, no longer needed.
<snip>
> @@ -2715,6 +2737,23 @@ static int selinux_file_receive(struct f
> return file_has_perm(current, file, file_to_av(file));
> }
>
> +static int selinux_dentry_open(struct file *file)
> +{
> + struct file_security_struct *fsec;
> + struct inode *inode;
> + struct inode_security_struct *isec;
> + inode = file->f_path.dentry->d_inode;
> + fsec = file->f_security;
> + isec = inode->i_security;
I'd add a comment here, e.g.
/*
* Save inode label and policy sequence number
* at open-time so that selinux_file_permission
* can determine whether revalidation is necessary.
* Task label is already saved in the file security
* struct as its SID.
*/
> + fsec->isid = isec->sid;
> + fsec->pseqno = avc_policy_seqno();
> +
> + /*Permission has to be rechecked here.
> + Policy load of inode sid can happen between
> + may_open and selinux_dentry_open.*/
Typo in the comment (s/of/or/), coding style isn't right for a
multi-line comment, and likely needs clarification, e.g.
/*
* Since the inode label or policy seqno may have changed
* between the selinux_inode_permission check and the saving
* of state above, recheck that access is still permitted.
* Otherwise, access might never be revalidated against the
* new inode label or new policy.
* This check is not redundant - do not remove.
*/
> + return inode_has_perm(current, inode, file_to_av(file), NULL);
> +}
> +
> /* task security operations */
>
> static int selinux_task_create(unsigned long clone_flags)
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/fs/open.c linux-2.6.22/fs/open.c
> --- linux-2.6.22.orig/fs/open.c 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/fs/open.c 2007-09-12 08:31:24.000000000 +0900
> @@ -696,8 +696,13 @@ static struct file *__dentry_open(struct
> f->f_op = fops_get(inode->i_fop);
> file_move(f, &inode->i_sb->s_files);
>
> + error = security_dentry_open(f);
> + if (error)
> + goto cleanup_all;
> +
> if (!open && f->f_op)
> open = f->f_op->open;
> +
Extraneous whitespace leftover from prior version of the patch.
> if (open) {
> error = open(inode, f);
> if (error)
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
> --- linux-2.6.22.orig/include/linux/security.h 2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/include/linux/security.h 2007-09-12 08:30:16.000000000 +0900
> @@ -503,6 +503,11 @@ struct request_sock;
> * @file contains the file structure being received.
> * Return 0 if permission is granted.
> *
> + * Security hook for dentry
> + *
> + * @dentry_open
> + * Check permission or get additional information before opening dentry.
> + *
More precisely, "Save open-time permission checking state for later use
upon file_permission, and recheck access if anything has changed since
inode_permission."
--
Stephen Smalley
National Security Agency
On Thu, 13 Sep 2007 08:58:32 -0400
Stephen Smalley wrote:
> On Wed, 2007-09-12 at 17:51 +0900, Yuichi Nakamura wrote:
<snip>
> Thanks, a few comments below.
Thanks for comments!
> >
> > * Description of patch
> > This patch improves performance of read/write in SELinux.
> > It improves performance by skipping permission check in
> > selinux_file_permission. Permission is only checked when
> > sid change or policy load is detected after file open.
> > To detect sid change, new LSM hook securiy_dentry_open is added.
>
> I think I'd reword this a little, e.g.
>
> It reduces the selinux overhead on read/write by only revalidating
> permissions in selinux_file_permission if the task or inode labels have
> changed or the policy has changed since the open-time check. A new LSM
> hook, security_dentry_open, is added to capture the necessary state at
> open time to allow this optimization.
I see, I will use that.
> >
> > Signed-off-by: Yuichi Nakamura<[email protected]>
> > ---
> > fs/open.c | 5 ++++
> > include/linux/security.h | 16 ++++++++++++++
> > security/dummy.c | 6 +++++
> > security/selinux/avc.c | 5 ++++
> > security/selinux/hooks.c | 43 +++++++++++++++++++++++++++++++++++++-
> > security/selinux/include/avc.h | 2 +
> > security/selinux/include/objsec.h | 2 +
> > 7 files changed, 78 insertions(+), 1 deletion(-)
> <snip>
> > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
> > --- linux-2.6.22.orig/security/selinux/hooks.c 2007-07-09 08:32:17.000000000 +0900
> > +++ linux-2.6.22/security/selinux/hooks.c 2007-09-12 08:42:49.000000000 +0900
> > @@ -80,6 +82,7 @@
> >
> > #define XATTR_SELINUX_SUFFIX "selinux"
> > #define XATTR_NAME_SELINUX XATTR_SECURITY_PREFIX XATTR_SELINUX_SUFFIX
> > +#define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])
>
> Leftover from prior version of the patch, no longer needed.
Fixed.
>
> <snip>
> > @@ -2715,6 +2737,23 @@ static int selinux_file_receive(struct f
> > return file_has_perm(current, file, file_to_av(file));
> > }
> >
> > +static int selinux_dentry_open(struct file *file)
> > +{
> > + struct file_security_struct *fsec;
> > + struct inode *inode;
> > + struct inode_security_struct *isec;
> > + inode = file->f_path.dentry->d_inode;
> > + fsec = file->f_security;
> > + isec = inode->i_security;
>
> I'd add a comment here, e.g.
> /*
> * Save inode label and policy sequence number
> * at open-time so that selinux_file_permission
> * can determine whether revalidation is necessary.
> * Task label is already saved in the file security
> * struct as its SID.
> */
Fixed.
>
> > + fsec->isid = isec->sid;
> > + fsec->pseqno = avc_policy_seqno();
> > +
> > + /*Permission has to be rechecked here.
> > + Policy load of inode sid can happen between
> > + may_open and selinux_dentry_open.*/
>
> Typo in the comment (s/of/or/), coding style isn't right for a
> multi-line comment, and likely needs clarification, e.g.
> /*
> * Since the inode label or policy seqno may have changed
> * between the selinux_inode_permission check and the saving
> * of state above, recheck that access is still permitted.
> * Otherwise, access might never be revalidated against the
> * new inode label or new policy.
> * This check is not redundant - do not remove.
> */
Fixed.
>
> > + return inode_has_perm(current, inode, file_to_av(file), NULL);
> > +}
> > +
> > /* task security operations */
> >
> > static int selinux_task_create(unsigned long clone_flags)
>
> > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/fs/open.c linux-2.6.22/fs/open.c
> > --- linux-2.6.22.orig/fs/open.c 2007-07-09 08:32:17.000000000 +0900
> > +++ linux-2.6.22/fs/open.c 2007-09-12 08:31:24.000000000 +0900
> > @@ -696,8 +696,13 @@ static struct file *__dentry_open(struct
> > f->f_op = fops_get(inode->i_fop);
> > file_move(f, &inode->i_sb->s_files);
> >
> > + error = security_dentry_open(f);
> > + if (error)
> > + goto cleanup_all;
> > +
> > if (!open && f->f_op)
> > open = f->f_op->open;
> > +
>
> Extraneous whitespace leftover from prior version of the patch.
Fixed.
>
> > if (open) {
> > error = open(inode, f);
> > if (error)
> > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
> > --- linux-2.6.22.orig/include/linux/security.h 2007-07-09 08:32:17.000000000 +0900
> > +++ linux-2.6.22/include/linux/security.h 2007-09-12 08:30:16.000000000 +0900
> > @@ -503,6 +503,11 @@ struct request_sock;
> > * @file contains the file structure being received.
> > * Return 0 if permission is granted.
> > *
> > + * Security hook for dentry
> > + *
> > + * @dentry_open
> > + * Check permission or get additional information before opening dentry.
> > + *
>
> More precisely, "Save open-time permission checking state for later use
> upon file_permission, and recheck access if anything has changed since
> inode_permission."
Fixed.
> --
> Stephen Smalley
> National Security Agency
I would like to send patch in next e-mail in new thread.
Regards,
--
Yuichi Nakamura
Hitachi Software Engineering Co., Ltd.
Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
SELinux Policy Editor: http://seedit.sourceforge.net/