From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb1-f195.google.com ([209.85.219.195]:37846 "EHLO mail-yb1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726111AbeIMFG4 (ORCPT ); Thu, 13 Sep 2018 01:06:56 -0400 Received: by mail-yb1-f195.google.com with SMTP id f145-v6so2550758ybg.4 for ; Wed, 12 Sep 2018 17:00:05 -0700 (PDT) Received: from mail-yb1-f173.google.com (mail-yb1-f173.google.com. [209.85.219.173]) by smtp.gmail.com with ESMTPSA id b135-v6sm1809997ywh.24.2018.09.12.17.00.01 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 12 Sep 2018 17:00:01 -0700 (PDT) Received: by mail-yb1-f173.google.com with SMTP id w184-v6so124273ybe.11 for ; Wed, 12 Sep 2018 17:00:01 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <748e4bca-41f1-c43b-a691-bf6b3d910793@schaufler-ca.com> References: <748e4bca-41f1-c43b-a691-bf6b3d910793@schaufler-ca.com> From: Kees Cook Date: Wed, 12 Sep 2018 17:00:00 -0700 Message-ID: Subject: Re: [PATCH 06/10] LSM: Infrastructure management of the file security blob To: Casey Schaufler Cc: LSM , James Morris , LKLM , SE Linux , John Johansen , Tetsuo Handa , Paul Moore , Stephen Smalley , "linux-fsdevel@vger.kernel.org" , Alexey Dobriyan , "Schaufler, Casey" Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler 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 > --- > 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