Re: [PATCH 06/10] LSM: Infrastructure management of the file security blob

From: Kees Cook
Date: Wed Sep 12 2018 - 20:00:07 EST

Next message: Andy Lutomirski: "Re: [PATCH v6 3/5] seccomp: add a way to get a listener fd from ptrace"
Previous message: Fenghua Yu: "Re: [PATCH] x86/intel_rdt: Show missing resctrl mount options"
In reply to: Casey Schaufler: "[PATCH 06/10] LSM: Infrastructure management of the file security blob"
Next in thread: James Morris: "Re: [PATCH v2 00/10] LSM: Module stacking in support of S.A.R.A and Landlock"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> ---
> 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

Next message: Andy Lutomirski: "Re: [PATCH v6 3/5] seccomp: add a way to get a listener fd from ptrace"
Previous message: Fenghua Yu: "Re: [PATCH] x86/intel_rdt: Show missing resctrl mount options"
In reply to: Casey Schaufler: "[PATCH 06/10] LSM: Infrastructure management of the file security blob"
Next in thread: James Morris: "Re: [PATCH v2 00/10] LSM: Module stacking in support of S.A.R.A and Landlock"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]