linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
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>
Subject: Re: [PATCH 06/10] LSM: Infrastructure management of the file security blob
Date: Wed, 12 Sep 2018 17:00:00 -0700	[thread overview]
Message-ID: <CAGXu5j+WwT=eFv8upvSHUUHq23TQJ3HvXy=ASZOwsQRR88Wg4w@mail.gmail.com> (raw)
In-Reply-To: <748e4bca-41f1-c43b-a691-bf6b3d910793@schaufler-ca.com>

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

  reply	other threads:[~2018-09-13  5:06 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-11 16:26 [PATCH v2 00/10] LSM: Module stacking in support of S.A.R.A and Landlock Casey Schaufler
2018-09-11 16:41 ` [PATCH 01/10] procfs: add smack subdir to attrs Casey Schaufler
2018-09-12 22:57   ` Kees Cook
2018-09-11 16:41 ` [PATCH 02/10] Smack: Abstract use of cred security blob Casey Schaufler
2018-09-12 23:04   ` Kees Cook
2018-09-11 16:41 ` [PATCH 03/10] SELinux: " Casey Schaufler
2018-09-12 23:10   ` Kees Cook
2018-09-11 16:41 ` [PATCH 04/10] LSM: Infrastructure management of the " Casey Schaufler
2018-09-12 23:53   ` Kees Cook
2018-09-13 19:01     ` Casey Schaufler
2018-09-13 21:12       ` Kees Cook
2018-09-11 16:41 ` [PATCH 05/10] SELinux: Abstract use of file " Casey Schaufler
2018-09-12 23:54   ` Kees Cook
2018-09-11 16:42 ` [PATCH 06/10] LSM: Infrastructure management of the " Casey Schaufler
2018-09-13  0:00   ` Kees Cook [this message]
2018-09-11 16:42 ` [PATCH 07/10] SELinux: Abstract use of inode " Casey Schaufler
2018-09-13  0:23   ` Kees Cook
2018-09-11 16:42 ` [PATCH 08/10] Smack: " Casey Schaufler
2018-09-13  0:24   ` Kees Cook
2018-09-11 16:42 ` [PATCH 09/10] LSM: Infrastructure management of the inode security Casey Schaufler
2018-09-13  0:30   ` Kees Cook
2018-09-11 16:42 ` [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock Casey Schaufler
2018-09-13  4:19   ` Kees Cook
2018-09-13 13:16     ` Paul Moore
2018-09-13 15:19       ` Kees Cook
2018-09-13 19:12         ` Paul Moore
2018-09-13 20:58           ` Jordan Glover
2018-09-13 21:50             ` Paul Moore
2018-09-13 22:04               ` Jordan Glover
2018-09-13 23:01               ` Casey Schaufler
2018-09-13 21:01           ` Kees Cook
2018-09-13 21:38             ` Paul Moore
2018-09-13 21:51               ` Kees Cook
2018-09-13 23:06                 ` Kees Cook
2018-09-13 23:32                   ` John Johansen
2018-09-13 23:51                     ` Kees Cook
2018-09-14  0:03                       ` Casey Schaufler
2018-09-14  0:06                         ` Kees Cook
2018-09-13 23:51                   ` Casey Schaufler
2018-09-13 23:57                     ` Kees Cook
2018-09-14  0:08                       ` Casey Schaufler
2018-09-14  0:19                         ` Kees Cook
2018-09-14 15:57                           ` Casey Schaufler
2018-09-14 20:05                             ` Kees Cook
2018-09-14 20:47                               ` Casey Schaufler
2018-09-14 18:18                         ` James Morris
2018-09-14 18:23                           ` John Johansen
2018-09-14  0:03                     ` Kees Cook
2018-09-14  2:42                 ` Paul Moore
2018-09-11 20:43 ` [PATCH v2 00/10] LSM: Module stacking in support of S.A.R.A and Landlock James Morris
2018-09-12 21:29 ` James Morris
2018-09-16 16:54   ` Salvatore Mesoraca
2018-09-16 17:25     ` Casey Schaufler
2018-09-16 17:45       ` Salvatore Mesoraca
2018-09-18  7:44   ` Mickaël Salaün
2018-09-18 15:23     ` Casey Schaufler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGXu5j+WwT=eFv8upvSHUUHq23TQJ3HvXy=ASZOwsQRR88Wg4w@mail.gmail.com' \
    --to=keescook@chromium.org \
    --cc=adobriyan@gmail.com \
    --cc=casey.schaufler@intel.com \
    --cc=casey@schaufler-ca.com \
    --cc=jmorris@namei.org \
    --cc=john.johansen@canonical.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@tycho.nsa.gov \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).