On Mon, Mar 5, 2018 at 11:47 AM, Stephen Smalley <s...@tycho.nsa.gov> 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 <s...@tycho.nsa.gov>
> ---
>  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(&selinux_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(&selinux_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(&selinux_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, &sel_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, 
> &fsi->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 | S_IRUGO | S_IWUGO, 
> MKDEV(MEM_MAJOR, 3));
>         d_add(dentry, inode);
> -       selinux_null.dentry = dentry;
>
> -       dentry = sel_make_dir(sb->s_root, "avc", &sel_last_ino);
> +       dentry = sel_make_dir(sb->s_root, "avc", &fsi->last_ino);
>         if (IS_ERR(dentry)) {
>                 ret = PTR_ERR(dentry);
>                 goto err;
> @@ -1878,7 +1950,7 @@ static int sel_fill_super(struct super_block *sb, void 
> *data, int silent)
>         if (ret)
>                 goto err;
>
> -       dentry = sel_make_dir(sb->s_root, "initial_contexts", &sel_last_ino);
> +       dentry = sel_make_dir(sb->s_root, "initial_contexts", &fsi->last_ino);
>         if (IS_ERR(dentry)) {
>                 ret = PTR_ERR(dentry);
>                 goto err;
> @@ -1888,42 +1960,79 @@ static int sel_fill_super(struct super_block *sb, 
> void *data, int silent)
>         if (ret)
>                 goto err;
>
> -       class_dir = sel_make_dir(sb->s_root, "class", &sel_last_ino);
> -       if (IS_ERR(class_dir)) {
> -               ret = PTR_ERR(class_dir);
> -               class_dir = NULL;
> +       fsi->class_dir = sel_make_dir(sb->s_root, "class", &fsi->last_ino);
> +       if (IS_ERR(fsi->class_dir)) {
> +               ret = PTR_ERR(fsi->class_dir);
> +               fsi->class_dir = NULL;
>                 goto err;
>         }
>
> -       policycap_dir = sel_make_dir(sb->s_root, "policy_capabilities", 
> &sel_last_ino);
> -       if (IS_ERR(policycap_dir)) {
> -               ret = PTR_ERR(policycap_dir);
> -               policycap_dir = NULL;
> +       fsi->policycap_dir = sel_make_dir(sb->s_root, "policy_capabilities",
> +                                         &fsi->last_ino);
> +       if (IS_ERR(fsi->policycap_dir)) {
> +               ret = PTR_ERR(fsi->policycap_dir);
> +               fsi->policycap_dir = NULL;
>                 goto err;
>         }
> +
> +       ret = sel_make_policy_nodes(fsi);
> +       if (ret)
> +               goto err;
>         return 0;
>  err:
>         printk(KERN_ERR "SELinux: %s:  failed while creating inodes\n",
>                 __func__);
> +

Do we want to cleanup fsi/sb->s_fs_info in case of error?

>         return ret;
>  }
>
> +static int selinuxfs_compare(struct super_block *sb, void *p)
> +{
> +       struct selinux_fs_info *fsi = sb->s_fs_info;
> +
> +       return (&selinux_state == fsi->state);
> +}
> +
>  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.

> +static void sel_kill_sb(struct super_block *sb)
> +{
> +       selinux_fs_info_free(sb);
> +       kill_litter_super(sb);
>  }
>
>  static struct file_system_type sel_fs_type = {
>         .name           = "selinuxfs",
>         .mount          = sel_mount,
> -       .kill_sb        = kill_litter_super,
> +       .kill_sb        = sel_kill_sb,
>  };
>
>  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,
> +                                               &null_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?

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 4785ca552d51..ccfa65f6bc17 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2811,6 +2811,13 @@ int security_get_bools(struct selinux_state *state,
>         struct policydb *policydb;
>         int i, rc;
>
> +       if (!state->initialized) {
> +               *len = 0;
> +               *names = NULL;
> +               *values = NULL;
> +               return 0;
> +       }
> +
>         read_lock(&state->ss->policy_rwlock);
>
>         policydb = &state->ss->policydb;
> @@ -3141,6 +3148,12 @@ int security_get_classes(struct selinux_state *state,
>         struct policydb *policydb = &state->ss->policydb;
>         int rc;
>
> +       if (!state->initialized) {
> +               *nclasses = 0;
> +               *classes = NULL;
> +               return 0;
> +       }
> +
>         read_lock(&state->ss->policy_rwlock);
>
>         rc = -ENOMEM;

Both changes in ss/services.c seem like something that should be done
independently of this change, no?

-- 
paul moore
www.paul-moore.com

Reply via email to