Re: [PATCH 1/2] selinux: wrap selinuxfs state

[Paul Moore]

On Wed, Mar 14, 2018 at 11:44 AM, Stephen Smalley
 wrote:
> On Tue, Mar 13, 2018, 12:18 PM Paul Moore  wrote:
>> On Mon, Mar 5, 2018 at 11:47 AM, Stephen Smalley 
>> wrote:

...

>> >  static struct dentry *sel_mount(struct file_system_type *fs_type,
>> >   int flags, const char *dev_name, void *data)
>> >  {
>> > -   return mount_single(fs_type, flags, data, sel_fill_super);
>> > +   int (*fill_super)(struct super_block *, void *, int) =
>> > sel_fill_super;
>> > +   struct super_block *s;
>> > +   int error;
>> > +
>> > +   s = sget(fs_type, selinuxfs_compare, set_anon_super, flags,
>> > NULL);
>> > +   if (IS_ERR(s))
>> > +   return ERR_CAST(s);
>> > +   if (!s->s_root) {
>> > +   error = fill_super(s, data, flags & MS_SILENT ? 1 : 0);
>> > +   if (error) {
>> > +   deactivate_locked_super(s);
>> > +   return ERR_PTR(error);
>> > +   }
>> > +   s->s_flags |= MS_ACTIVE;
>> > +   }
>> > +   return dget(s->s_root);
>> > +}
>>
>> Why is mount_single() no longer desirable here?  The only difference I
>> can see is the call to do_remount_sb().  If the do_remount_sb() call
>> is problematic we should handle that in a separate patch and not bury
>> it here.
>
> This is laying the groundwork for supporting multiple distinct selinux fs
> super blocks. That said, we could likely defer this part of the patch until
> namespaces are introduced.

I suspected it was something like that.  Let's keep the mount_single()
call for now.

>> >  struct vfsmount *selinuxfs_mount;
>> > +struct path selinux_null;
>> >
>> >  static int __init init_sel_fs(void)
>> >  {
>> > +   struct qstr null_name = QSTR_INIT(NULL_FILE_NAME,
>> > + sizeof(NULL_FILE_NAME)-1);
>> > int err;
>> >
>> > if (!selinux_enabled)
>> > @@ -1945,6 +2054,13 @@ static int __init init_sel_fs(void)
>> > err = PTR_ERR(selinuxfs_mount);
>> > selinuxfs_mount = NULL;
>> > }
>> > +   selinux_null.dentry =
>> > d_hash_and_lookup(selinux_null.mnt->mnt_root,
>> > +   _name);
>> > +   if (IS_ERR(selinux_null.dentry)) {
>> > +   pr_err("selinuxfs:  could not lookup null!\n");
>> > +   err = PTR_ERR(selinux_null.dentry);
>> > +   selinux_null.dentry = NULL;
>> > +   }
>>
>> I'm getting a very strong feeling that I'm missing something important
>> here, but on quick inspection it doesn't appear we ever use the value
>> stored in selinux_null, and I'm really lost as to why we ever make it
>> available in include/security.h ... can we simply get rid of it?
>
> It is used by flush_unauthorized_files() in hooks.c.

Yep, there it is.  How did I miss that?  Evidently I was wrong when I
thought I knew how to use grep ...

-- 
paul moore
www.paul-moore.com

×

Re: [PATCH 1/2] selinux: wrap selinuxfs state

[Stephen Smalley]

On Tue, Mar 13, 2018, 12:18 PM Paul Moore  wrote:

> On Mon, Mar 5, 2018 at 11:47 AM, Stephen Smalley 
> wrote:
> > Move global selinuxfs state to a per-instance structure
> (selinux_fs_info),
> > and include a pointer to the selinux_state in this structure.
> > Pass this selinux_state to all security server operations, thereby
> > ensuring that each selinuxfs instance presents a view of and acts
> > as an interface to a particular selinux_state instance.
> >
> > This change should have no effect on SELinux behavior or APIs
> > (userspace or LSM).  It merely wraps the selinuxfs global state,
> > links it to a particular selinux_state (currently always the single
> > global selinux_state) and uses that state for all operations.
> >
> > Signed-off-by: Stephen Smalley 
> > ---
> >  security/selinux/selinuxfs.c   | 472
> +
> >  security/selinux/ss/services.c |  13 ++
> >  2 files changed, 307 insertions(+), 178 deletions(-)
>
> ...
>
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index 0dbd5fd6a396..1a32e93ba7b9 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -94,11 +122,13 @@ static unsigned long sel_last_ino = SEL_INO_NEXT -
> 1;
> >  static ssize_t sel_read_enforce(struct file *filp, char __user *buf,
> > size_t count, loff_t *ppos)
> >  {
> > +   struct selinux_fs_info *fsi = file_inode(filp)->i_sb->s_fs_info;
> > +   struct selinux_state *state = fsi->state;
> > char tmpbuf[TMPBUFLEN];
> > ssize_t length;
> >
> > length = scnprintf(tmpbuf, TMPBUFLEN, "%d",
> > -  enforcing_enabled(_state));
> > +  enforcing_enabled(state));
>
> Since we only use state once, it seems like we could just use
> fsi->state without problem.
>
> > @@ -186,7 +218,9 @@ static const struct file_operations
> sel_handle_unknown_ops = {
> >
> >  static int sel_open_handle_status(struct inode *inode, struct file
> *filp)
> >  {
> > -   struct page*status =
> selinux_kernel_status_page(_state);
> > +   struct selinux_fs_info *fsi = file_inode(filp)->i_sb->s_fs_info;
> > +   struct selinux_state *state = fsi->state;
> > +   struct page*status = selinux_kernel_status_page(state);
>
> Once again, why do we need state instead of fsi->state?
>
> > if (!status)
> > return -ENOMEM;
> > @@ -242,6 +276,8 @@ static ssize_t sel_write_disable(struct file *file,
> const char __user *buf,
> >  size_t count, loff_t *ppos)
> >
> >  {
> > +   struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info;
> > +   struct selinux_state *state = fsi->state;
> > char *page;
> > ssize_t length;
> > int new_value;
> > @@ -262,7 +298,7 @@ static ssize_t sel_write_disable(struct file *file,
> const char __user *buf,
> > goto out;
> >
> > if (new_value) {
> > -   length = selinux_disable(_state);
> > +   length = selinux_disable(state);
>
> Same as above.
>
> I think if we end up having to reference it more than once, go ahead
> and add another variable to the stack, otherwise just stick with
> fsi->state.  Go ahead and apply this logic to the rest of this patch.
>
> > @@ -1808,8 +1872,11 @@ static struct dentry *sel_make_dir(struct dentry
> *dir, const char *name,
> > return dentry;
> >  }
> >
> > +#define NULL_FILE_NAME "null"
> > +
> >  static int sel_fill_super(struct super_block *sb, void *data, int
> silent)
> >  {
> > +   struct selinux_fs_info *fsi;
> > int ret;
> > struct dentry *dentry;
> > struct inode *inode;
> > @@ -1837,14 +1904,20 @@ static int sel_fill_super(struct super_block
> *sb, void *data, int silent)
> > S_IWUGO},
> > /* last one */ {""}
> > };
> > +
> > +   ret = selinux_fs_info_create(sb);
> > +   if (ret)
> > +   goto err;
> > +
> > ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files);
> > if (ret)
> > goto err;
> >
> > -   bool_dir = sel_make_dir(sb->s_root, BOOL_DIR_NAME,
> _last_ino);
> > -   if (IS_ERR(bool_dir)) {
> > -   ret = PTR_ERR(bool_dir);
> > -   bool_dir = NULL;
> > +   fsi = sb->s_fs_info;
> > +   fsi->bool_dir = sel_make_dir(sb->s_root, BOOL_DIR_NAME,
> >last_ino);
> > +   if (IS_ERR(fsi->bool_dir)) {
> > +   ret = PTR_ERR(fsi->bool_dir);
> > +   fsi->bool_dir = NULL;
> > goto err;
> > }
> >
> > @@ -1858,7 +1931,7 @@ static int sel_fill_super(struct super_block *sb,
> void *data, int silent)
> > if (!inode)
> > goto err;
> >
> > -   inode->i_ino = ++sel_last_ino;
> > +   inode->i_ino = 

Re: [PATCH 1/2] selinux: wrap selinuxfs state

[Paul Moore]

On Mon, Mar 5, 2018 at 11:47 AM, Stephen Smalley  wrote:
> Move global selinuxfs state to a per-instance structure (selinux_fs_info),
> and include a pointer to the selinux_state in this structure.
> Pass this selinux_state to all security server operations, thereby
> ensuring that each selinuxfs instance presents a view of and acts
> as an interface to a particular selinux_state instance.
>
> This change should have no effect on SELinux behavior or APIs
> (userspace or LSM).  It merely wraps the selinuxfs global state,
> links it to a particular selinux_state (currently always the single
> global selinux_state) and uses that state for all operations.
>
> Signed-off-by: Stephen Smalley 
> ---
>  security/selinux/selinuxfs.c   | 472 
> +
>  security/selinux/ss/services.c |  13 ++
>  2 files changed, 307 insertions(+), 178 deletions(-)

...

> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 0dbd5fd6a396..1a32e93ba7b9 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -94,11 +122,13 @@ static unsigned long sel_last_ino = SEL_INO_NEXT - 1;
>  static ssize_t sel_read_enforce(struct file *filp, char __user *buf,
> size_t count, loff_t *ppos)
>  {
> +   struct selinux_fs_info *fsi = file_inode(filp)->i_sb->s_fs_info;
> +   struct selinux_state *state = fsi->state;
> char tmpbuf[TMPBUFLEN];
> ssize_t length;
>
> length = scnprintf(tmpbuf, TMPBUFLEN, "%d",
> -  enforcing_enabled(_state));
> +  enforcing_enabled(state));

Since we only use state once, it seems like we could just use
fsi->state without problem.

> @@ -186,7 +218,9 @@ static const struct file_operations 
> sel_handle_unknown_ops = {
>
>  static int sel_open_handle_status(struct inode *inode, struct file *filp)
>  {
> -   struct page*status = selinux_kernel_status_page(_state);
> +   struct selinux_fs_info *fsi = file_inode(filp)->i_sb->s_fs_info;
> +   struct selinux_state *state = fsi->state;
> +   struct page*status = selinux_kernel_status_page(state);

Once again, why do we need state instead of fsi->state?

> if (!status)
> return -ENOMEM;
> @@ -242,6 +276,8 @@ static ssize_t sel_write_disable(struct file *file, const 
> char __user *buf,
>  size_t count, loff_t *ppos)
>
>  {
> +   struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info;
> +   struct selinux_state *state = fsi->state;
> char *page;
> ssize_t length;
> int new_value;
> @@ -262,7 +298,7 @@ static ssize_t sel_write_disable(struct file *file, const 
> char __user *buf,
> goto out;
>
> if (new_value) {
> -   length = selinux_disable(_state);
> +   length = selinux_disable(state);

Same as above.

I think if we end up having to reference it more than once, go ahead
and add another variable to the stack, otherwise just stick with
fsi->state.  Go ahead and apply this logic to the rest of this patch.

> @@ -1808,8 +1872,11 @@ static struct dentry *sel_make_dir(struct dentry *dir, 
> const char *name,
> return dentry;
>  }
>
> +#define NULL_FILE_NAME "null"
> +
>  static int sel_fill_super(struct super_block *sb, void *data, int silent)
>  {
> +   struct selinux_fs_info *fsi;
> int ret;
> struct dentry *dentry;
> struct inode *inode;
> @@ -1837,14 +1904,20 @@ static int sel_fill_super(struct super_block *sb, 
> void *data, int silent)
> S_IWUGO},
> /* last one */ {""}
> };
> +
> +   ret = selinux_fs_info_create(sb);
> +   if (ret)
> +   goto err;
> +
> ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files);
> if (ret)
> goto err;
>
> -   bool_dir = sel_make_dir(sb->s_root, BOOL_DIR_NAME, _last_ino);
> -   if (IS_ERR(bool_dir)) {
> -   ret = PTR_ERR(bool_dir);
> -   bool_dir = NULL;
> +   fsi = sb->s_fs_info;
> +   fsi->bool_dir = sel_make_dir(sb->s_root, BOOL_DIR_NAME, 
> >last_ino);
> +   if (IS_ERR(fsi->bool_dir)) {
> +   ret = PTR_ERR(fsi->bool_dir);
> +   fsi->bool_dir = NULL;
> goto err;
> }
>
> @@ -1858,7 +1931,7 @@ static int sel_fill_super(struct super_block *sb, void 
> *data, int silent)
> if (!inode)
> goto err;
>
> -   inode->i_ino = ++sel_last_ino;
> +   inode->i_ino = ++fsi->last_ino;
> isec = (struct inode_security_struct *)inode->i_security;
> isec->sid = SECINITSID_DEVNULL;
> isec->sclass = SECCLASS_CHR_FILE;
> @@ -1866,9 +1939,8 @@ static int sel_fill_super(struct super_block *sb, void 
> *data, int silent)
>
> init_special_inode(inode, S_IFCHR | 

Re: [PATCH 1/2] selinux: wrap selinuxfs state

[James Morris]

On Mon, 5 Mar 2018, Stephen Smalley wrote:

> Move global selinuxfs state to a per-instance structure (selinux_fs_info),
> and include a pointer to the selinux_state in this structure.
> Pass this selinux_state to all security server operations, thereby
> ensuring that each selinuxfs instance presents a view of and acts
> as an interface to a particular selinux_state instance.
> 
> This change should have no effect on SELinux behavior or APIs
> (userspace or LSM).  It merely wraps the selinuxfs global state,
> links it to a particular selinux_state (currently always the single
> global selinux_state) and uses that state for all operations.
> 
> Signed-off-by: Stephen Smalley 


Reviewed-by: James Morris 


-- 
James Morris

[PATCH 1/2] selinux: wrap selinuxfs state

[Stephen Smalley]

Move global selinuxfs state to a per-instance structure (selinux_fs_info),
and include a pointer to the selinux_state in this structure.
Pass this selinux_state to all security server operations, thereby
ensuring that each selinuxfs instance presents a view of and acts
as an interface to a particular selinux_state instance.

This change should have no effect on SELinux behavior or APIs
(userspace or LSM).  It merely wraps the selinuxfs global state,
links it to a particular selinux_state (currently always the single
global selinux_state) and uses that state for all operations.

Signed-off-by: Stephen Smalley 
---
 security/selinux/selinuxfs.c   | 472 +
 security/selinux/ss/services.c |  13 ++
 2 files changed, 307 insertions(+), 178 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 0dbd5fd6a396..1a32e93ba7b9 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -41,23 +42,6 @@
 #include "objsec.h"
 #include "conditional.h"
 
-static DEFINE_MUTEX(sel_mutex);
-
-/* global data for booleans */
-static struct dentry *bool_dir;
-static int bool_num;
-static char **bool_pending_names;
-static int *bool_pending_values;
-
-/* global data for classes */
-static struct dentry *class_dir;
-static unsigned long last_class_ino;
-
-static char policy_opened;
-
-/* global data for policy capabilities */
-static struct dentry *policycap_dir;
-
 enum sel_inos {
SEL_ROOT_INO = 2,
SEL_LOAD,   /* load policy */
@@ -82,7 +66,51 @@ enum sel_inos {
SEL_INO_NEXT,   /* The next inode number to use */
 };
 
-static unsigned long sel_last_ino = SEL_INO_NEXT - 1;
+struct selinux_fs_info {
+   struct dentry *bool_dir;
+   unsigned int bool_num;
+   char **bool_pending_names;
+   unsigned int *bool_pending_values;
+   struct dentry *class_dir;
+   unsigned long last_class_ino;
+   bool policy_opened;
+   struct dentry *policycap_dir;
+   struct mutex mutex;
+   unsigned long last_ino;
+   struct selinux_state *state;
+   struct super_block *sb;
+};
+
+static int selinux_fs_info_create(struct super_block *sb)
+{
+   struct selinux_fs_info *fsi;
+
+   fsi = kzalloc(sizeof(*fsi), GFP_KERNEL);
+   if (!fsi)
+   return -ENOMEM;
+
+   mutex_init(>mutex);
+   fsi->last_ino = SEL_INO_NEXT - 1;
+   fsi->state = _state;
+   fsi->sb = sb;
+   sb->s_fs_info = fsi;
+   return 0;
+}
+
+static void selinux_fs_info_free(struct super_block *sb)
+{
+   struct selinux_fs_info *fsi = sb->s_fs_info;
+   int i;
+
+   if (fsi) {
+   for (i = 0; i < fsi->bool_num; i++)
+   kfree(fsi->bool_pending_names[i]);
+   kfree(fsi->bool_pending_names);
+   kfree(fsi->bool_pending_values);
+   }
+   kfree(sb->s_fs_info);
+   sb->s_fs_info = NULL;
+}
 
 #define SEL_INITCON_INO_OFFSET 0x0100
 #define SEL_BOOL_INO_OFFSET0x0200
@@ -94,11 +122,13 @@ static unsigned long sel_last_ino = SEL_INO_NEXT - 1;
 static ssize_t sel_read_enforce(struct file *filp, char __user *buf,
size_t count, loff_t *ppos)
 {
+   struct selinux_fs_info *fsi = file_inode(filp)->i_sb->s_fs_info;
+   struct selinux_state *state = fsi->state;
char tmpbuf[TMPBUFLEN];
ssize_t length;
 
length = scnprintf(tmpbuf, TMPBUFLEN, "%d",
-  enforcing_enabled(_state));
+  enforcing_enabled(state));
return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
 }
 
@@ -107,6 +137,8 @@ static ssize_t sel_write_enforce(struct file *file, const 
char __user *buf,
 size_t count, loff_t *ppos)
 
 {
+   struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info;
+   struct selinux_state *state = fsi->state;
char *page = NULL;
ssize_t length;
int old_value, new_value;
@@ -128,8 +160,7 @@ static ssize_t sel_write_enforce(struct file *file, const 
char __user *buf,
 
new_value = !!new_value;
 
-   old_value = enforcing_enabled(_state);
-
+   old_value = enforcing_enabled(state);
if (new_value != old_value) {
length = avc_has_perm(current_sid(), SECINITSID_SECURITY,
  SECCLASS_SECURITY, SECURITY__SETENFORCE,
@@ -141,12 +172,11 @@ static ssize_t sel_write_enforce(struct file *file, const 
char __user *buf,
new_value, old_value,
from_kuid(_user_ns, audit_get_loginuid(current)),
audit_get_sessionid(current));
-   enforcing_set(_state, new_value);
+   enforcing_set(state, new_value);
if (new_value)