Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <748e4bca-41f1-c43b-a691-bf6b3d910793@schaufler-ca.com>
References: <cbf848ae-a1ac-c5ab-f23b-bc9217fceebe@schaufler-ca.com> <748e4bca-41f1-c43b-a691-bf6b3d910793@schaufler-ca.com>
From:   Kees Cook <keescook@chromium.org>
Date:   Wed, 12 Sep 2018 17:00:00 -0700
Message-ID: <CAGXu5j+WwT=eFv8upvSHUUHq23TQJ3HvXy=ASZOwsQRR88Wg4w@mail.gmail.com>
Subject: Re: [PATCH 06/10] LSM: Infrastructure management of the file security blob
To:     Casey Schaufler <casey@schaufler-ca.com>
Cc:     LSM <linux-security-module@vger.kernel.org>,
        James Morris <jmorris@namei.org>,
        LKLM <linux-kernel@vger.kernel.org>,
        SE Linux <selinux@tycho.nsa.gov>,
        John Johansen <john.johansen@canonical.com>,
        Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Paul Moore <paul@paul-moore.com>,
        Stephen Smalley <sds@tycho.nsa.gov>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        "Schaufler, Casey" <casey.schaufler@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> Move management of the file->f_security blob out of the
> individual security modules and into the infrastructure.
> The modules no longer allocate or free the data, instead
> they tell the infrastructure how much space they require.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  include/linux/lsm_hooks.h  |  1 +
>  security/apparmor/lsm.c    | 19 +++++++-------
>  security/security.c        | 54 +++++++++++++++++++++++++++++++++++---
>  security/selinux/hooks.c   | 25 ++----------------
>  security/smack/smack.h     |  5 ++++
>  security/smack/smack_lsm.c | 26 +++++++-----------
>  6 files changed, 78 insertions(+), 52 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 0bef312efd45..167ffbd4d0c0 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2029,6 +2029,7 @@ struct security_hook_list {
>   */
>  struct lsm_blob_sizes {
>         int     lbs_cred;
> +       int     lbs_file;
>  };
>
>  /*
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index c2566aaa138e..15716b6ff860 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -431,21 +431,21 @@ static int apparmor_file_open(struct file *file)
>
>  static int apparmor_file_alloc_security(struct file *file)
>  {
> -       int error = 0;
> -
> -       /* freed by apparmor_file_free_security */
> +       struct aa_file_ctx *ctx = file_ctx(file);
>         struct aa_label *label = begin_current_label_crit_section();
> -       file->f_security = aa_alloc_file_ctx(label, GFP_KERNEL);
> -       if (!file_ctx(file))
> -               error = -ENOMEM;
> -       end_current_label_crit_section(label);
>
> -       return error;
> +       spin_lock_init(&ctx->lock);
> +       rcu_assign_pointer(ctx->label, aa_get_label(label));
> +       end_current_label_crit_section(label);
> +       return 0;
>  }
>
>  static void apparmor_file_free_security(struct file *file)
>  {
> -       aa_free_file_ctx(file_ctx(file));
> +       struct aa_file_ctx *ctx = file_ctx(file);
> +
> +       if (ctx)
> +               aa_put_label(rcu_access_pointer(ctx->label));
>  }
>
>  static int common_file_perm(const char *op, struct file *file, u32 mask)
> @@ -1131,6 +1131,7 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
>   */
>  struct lsm_blob_sizes apparmor_blob_sizes = {
>         .lbs_cred = sizeof(struct aa_task_ctx *),
> +       .lbs_file = sizeof(struct aa_file_ctx),
>  };
>
>  static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> diff --git a/security/security.c b/security/security.c
> index ff7df14f6db1..5430cae73cf6 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -40,6 +40,8 @@
>  struct security_hook_heads security_hook_heads __lsm_ro_after_init;
>  static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
>
> +static struct kmem_cache *lsm_file_cache;
> +
>  char *lsm_names;
>  static struct lsm_blob_sizes blob_sizes;
>
> @@ -92,6 +94,13 @@ int __init security_init(void)
>          */
>         do_security_initcalls();
>
> +       /*
> +        * Create any kmem_caches needed for blobs
> +        */
> +       if (blob_sizes.lbs_file)
> +               lsm_file_cache = kmem_cache_create("lsm_file_cache",
> +                                                  blob_sizes.lbs_file, 0,
> +                                                  SLAB_PANIC, NULL);
>         /*
>          * The second call to a module specific init function
>          * adds hooks to the hook lists and does any other early
> @@ -101,6 +110,7 @@ int __init security_init(void)
>
>  #ifdef CONFIG_SECURITY_LSM_DEBUG
>         pr_info("LSM: cred blob size       = %d\n", blob_sizes.lbs_cred);
> +       pr_info("LSM: file blob size       = %d\n", blob_sizes.lbs_file);
>  #endif
>
>         return 0;
> @@ -277,6 +287,28 @@ static void __init lsm_set_size(int *need, int *lbs)
>  void __init security_add_blobs(struct lsm_blob_sizes *needed)
>  {
>         lsm_set_size(&needed->lbs_cred, &blob_sizes.lbs_cred);
> +       lsm_set_size(&needed->lbs_file, &blob_sizes.lbs_file);
> +}
> +
> +/**
> + * lsm_file_alloc - allocate a composite file blob
> + * @file: the file that needs a blob
> + *
> + * Allocate the file blob for all the modules
> + *
> + * Returns 0, or -ENOMEM if memory can't be allocated.
> + */
> +int lsm_file_alloc(struct file *file)
> +{
> +       if (!lsm_file_cache) {
> +               file->f_security = NULL;
> +               return 0;
> +       }
> +
> +       file->f_security = kmem_cache_zalloc(lsm_file_cache, GFP_KERNEL);
> +       if (file->f_security == NULL)
> +               return -ENOMEM;
> +       return 0;
>  }
>
>  /*
> @@ -962,12 +994,28 @@ int security_file_permission(struct file *file, int mask)
>
>  int security_file_alloc(struct file *file)
>  {
> -       return call_int_hook(file_alloc_security, 0, file);
> +       int rc = lsm_file_alloc(file);
> +
> +       if (rc)
> +               return rc;
> +       rc = call_int_hook(file_alloc_security, 0, file);
> +       if (unlikely(rc))
> +               security_file_free(file);
> +       return rc;
>  }
>
>  void security_file_free(struct file *file)
>  {
> +       void *blob;
> +
> +       if (!lsm_file_cache)
> +               return;

Should this be WARN_ON?

> +
>         call_void_hook(file_free_security, file);
> +
> +       blob = file->f_security;
> +       file->f_security = NULL;
> +       kmem_cache_free(lsm_file_cache, blob);
>  }
>
>  int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> @@ -1085,7 +1133,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
>                 return rc;
>
>         rc = call_int_hook(cred_alloc_blank, 0, cred, gfp);
> -       if (rc)
> +       if (unlikely(rc))
>                 security_cred_free(cred);
>         return rc;
>  }
> @@ -1106,7 +1154,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp)
>                 return rc;
>
>         rc = call_int_hook(cred_prepare, 0, new, old, gfp);
> -       if (rc)
> +       if (unlikely(rc))
>                 security_cred_free(new);
>         return rc;
>  }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 94b3123c237b..3468b4592036 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -149,7 +149,6 @@ static int __init checkreqprot_setup(char *str)
>  __setup("checkreqprot=", checkreqprot_setup);
>
>  static struct kmem_cache *sel_inode_cache;
> -static struct kmem_cache *file_security_cache;
>
>  /**
>   * selinux_secmark_enabled - Check to see if SECMARK is currently enabled
> @@ -381,27 +380,15 @@ static void inode_free_security(struct inode *inode)
>
>  static int file_alloc_security(struct file *file)
>  {
> -       struct file_security_struct *fsec;
> +       struct file_security_struct *fsec = selinux_file(file);
>         u32 sid = current_sid();
>
> -       fsec = kmem_cache_zalloc(file_security_cache, GFP_KERNEL);
> -       if (!fsec)
> -               return -ENOMEM;
> -
>         fsec->sid = sid;
>         fsec->fown_sid = sid;
> -       file->f_security = fsec;
>
>         return 0;
>  }
>
> -static void file_free_security(struct file *file)
> -{
> -       struct file_security_struct *fsec = selinux_file(file);
> -       file->f_security = NULL;
> -       kmem_cache_free(file_security_cache, fsec);
> -}
> -
>  static int superblock_alloc_security(struct super_block *sb)
>  {
>         struct superblock_security_struct *sbsec;
> @@ -3558,11 +3545,6 @@ static int selinux_file_alloc_security(struct file *file)
>         return file_alloc_security(file);
>  }
>
> -static void selinux_file_free_security(struct file *file)
> -{
> -       file_free_security(file);
> -}
> -
>  /*
>   * Check whether a task has the ioctl permission and cmd
>   * operation to an inode.
> @@ -6857,6 +6839,7 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
>
>  struct lsm_blob_sizes selinux_blob_sizes = {
>         .lbs_cred = sizeof(struct task_security_struct),
> +       .lbs_file = sizeof(struct file_security_struct),
>  };
>
>  static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> @@ -6927,7 +6910,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>
>         LSM_HOOK_INIT(file_permission, selinux_file_permission),
>         LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
> -       LSM_HOOK_INIT(file_free_security, selinux_file_free_security),
>         LSM_HOOK_INIT(file_ioctl, selinux_file_ioctl),
>         LSM_HOOK_INIT(mmap_file, selinux_mmap_file),
>         LSM_HOOK_INIT(mmap_addr, selinux_mmap_addr),
> @@ -7130,9 +7112,6 @@ static __init int selinux_init(void)
>         sel_inode_cache = kmem_cache_create("selinux_inode_security",
>                                             sizeof(struct inode_security_struct),
>                                             0, SLAB_PANIC, NULL);
> -       file_security_cache = kmem_cache_create("selinux_file_security",
> -                                           sizeof(struct file_security_struct),
> -                                           0, SLAB_PANIC, NULL);
>         avc_init();
>
>         avtab_cache_init();
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 0c6dce446825..043525a52e94 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -362,6 +362,11 @@ static inline struct task_smack *smack_cred(const struct cred *cred)
>         return cred->security;
>  }
>
> +static inline struct smack_known **smack_file(const struct file *file)
> +{
> +       return file->f_security;
> +}
> +
>  /*
>   * Is the directory transmuting?
>   */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index a06ea8aa89c4..d1430341798f 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1571,24 +1571,12 @@ static void smack_inode_getsecid(struct inode *inode, u32 *secid)
>   */
>  static int smack_file_alloc_security(struct file *file)
>  {
> -       struct smack_known *skp = smk_of_current();
> +       struct smack_known **blob = smack_file(file);
>
> -       file->f_security = skp;
> +       *blob = smk_of_current();
>         return 0;
>  }
>
> -/**
> - * smack_file_free_security - clear a file security blob
> - * @file: the object
> - *
> - * The security blob for a file is a pointer to the master
> - * label list, so no memory is freed.
> - */
> -static void smack_file_free_security(struct file *file)
> -{
> -       file->f_security = NULL;
> -}
> -
>  /**
>   * smack_file_ioctl - Smack check on ioctls
>   * @file: the object
> @@ -1813,7 +1801,9 @@ static int smack_mmap_file(struct file *file,
>   */
>  static void smack_file_set_fowner(struct file *file)
>  {
> -       file->f_security = smk_of_current();
> +       struct smack_known **blob = smack_file(file);
> +
> +       *blob = smk_of_current();
>  }

Is this supposed to be part of a separate patch to prepare smack for
the infrastructure change?

Otherwise, seems good.

-Kees

>
>  /**
> @@ -1830,6 +1820,7 @@ static void smack_file_set_fowner(struct file *file)
>  static int smack_file_send_sigiotask(struct task_struct *tsk,
>                                      struct fown_struct *fown, int signum)
>  {
> +       struct smack_known **blob;
>         struct smack_known *skp;
>         struct smack_known *tkp = smk_of_task(smack_cred(tsk->cred));
>         struct file *file;
> @@ -1842,7 +1833,8 @@ static int smack_file_send_sigiotask(struct task_struct *tsk,
>         file = container_of(fown, struct file, f_owner);
>
>         /* we don't log here as rc can be overriden */
> -       skp = file->f_security;
> +       blob = smack_file(file);
> +       skp = *blob;
>         rc = smk_access(skp, tkp, MAY_DELIVER, NULL);
>         rc = smk_bu_note("sigiotask", skp, tkp, MAY_DELIVER, rc);
>         if (rc != 0 && has_capability(tsk, CAP_MAC_OVERRIDE))
> @@ -4626,6 +4618,7 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
>
>  struct lsm_blob_sizes smack_blob_sizes = {
>         .lbs_cred = sizeof(struct task_smack),
> +       .lbs_file = sizeof(struct smack_known *),
>  };
>
>  static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> @@ -4663,7 +4656,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(inode_getsecid, smack_inode_getsecid),
>
>         LSM_HOOK_INIT(file_alloc_security, smack_file_alloc_security),
> -       LSM_HOOK_INIT(file_free_security, smack_file_free_security),
>         LSM_HOOK_INIT(file_ioctl, smack_file_ioctl),
>         LSM_HOOK_INIT(file_lock, smack_file_lock),
>         LSM_HOOK_INIT(file_fcntl, smack_file_fcntl),
> --
> 2.17.1
>
>


-- 
Kees Cook
Pixel Security