[PATCH 3/9] LSM: Manage file security blobs

Stephen Smalley sds at tycho.nsa.gov
Tue Oct 31 15:25:51 UTC 2017


On Fri, 2017-10-27 at 14:45 -0700, Casey Schaufler wrote:
> Subject: [PATCH 3/9] LSM: Manage file security blobs
> 
> Move the management of file security blobs from the individual
> security modules to the security infrastructure. The security modules
> using file blobs have been updated accordingly. Modules are required
> to identify the space they need at module initialization. In some
> cases a module no longer needs to supply a blob management hook, in
> which case the hook has been removed.
> 
> Signed-off-by: Casey Schaufler <casey at schaufler-ca.com>
> ---
>  include/linux/lsm_hooks.h           |  1 +
>  security/apparmor/include/context.h |  5 +++++
>  security/apparmor/include/file.h    |  2 +-
>  security/apparmor/lsm.c             | 19 ++++++++--------
>  security/security.c                 | 43
> +++++++++++++++++++++++++++++++++++++
>  security/selinux/hooks.c            | 41 +++++++++----------------
> ----------
>  security/selinux/include/objsec.h   |  5 +++++
>  security/smack/smack.h              |  5 +++++
>  security/smack/smack_lsm.c          | 26 ++++++++--------------
>  9 files changed, 89 insertions(+), 58 deletions(-)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index ee4fcc51fa91..e5d0f1e01b81 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1919,6 +1919,7 @@ struct security_hook_list {
>   */
>  struct lsm_blob_sizes {
>  	int	lbs_cred;
> +	int	lbs_file;
>  };
>  
>  /*
> diff --git a/security/apparmor/include/context.h
> b/security/apparmor/include/context.h
> index 301ab3a0dd04..c6e106a533e8 100644
> --- a/security/apparmor/include/context.h
> +++ b/security/apparmor/include/context.h
> @@ -87,6 +87,11 @@ static inline struct aa_label
> *aa_get_newest_cred_label(const struct cred *cred)
>  	return aa_get_newest_label(aa_cred_raw_label(cred));
>  }
>  
> +static inline struct aa_file_ctx *apparmor_file(const struct file
> *file)
> +{
> +	return file->f_security;
> +}
> +
>  /**
>   * __aa_task_raw_label - retrieve another task's label
>   * @task: task to query  (NOT NULL)
> diff --git a/security/apparmor/include/file.h
> b/security/apparmor/include/file.h
> index 4c2c8ac8842f..b9efe6bc226b 100644
> --- a/security/apparmor/include/file.h
> +++ b/security/apparmor/include/file.h
> @@ -32,7 +32,7 @@ struct path;
>  				 AA_MAY_CHMOD | AA_MAY_CHOWN |
> AA_MAY_LOCK | \
>  				 AA_EXEC_MMAP | AA_MAY_LINK)
>  
> -#define file_ctx(X) ((struct aa_file_ctx *)(X)->f_security)
> +#define file_ctx(X) apparmor_file(X)
>  
>  /* struct aa_file_ctx - the AppArmor context the file was opened in
>   * @lock: lock to update the ctx
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index d80293bde5bf..f2814ba84481 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -402,21 +402,21 @@ static int apparmor_file_open(struct file
> *file, const struct cred *cred)
>  
>  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)
> @@ -1078,6 +1078,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 6fadc3860fb0..4d8e702fa22f 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -37,6 +37,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;
>  
> @@ -83,6 +85,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_fi
> le, 0,
> +						   SLAB_PANIC,
> NULL);
> +	/*
>  	 * The second call to a module specific init function
>  	 * adds hooks to the hook lists and does any other early
>  	 * initializations required.
> @@ -91,6 +100,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;
> @@ -267,6 +277,26 @@ 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)
> +		return 0;
> +
> +	file->f_security = kmem_cache_zalloc(lsm_file_cache,
> GFP_KERNEL);
> +	if (file->f_security == NULL)
> +		return -ENOMEM;
> +	return 0;
>  }
>  
>  /*
> @@ -952,12 +982,25 @@ int security_file_permission(struct file *file,
> int mask)
>  
>  int security_file_alloc(struct file *file)
>  {
> +	int rc = lsm_file_alloc(file);
> +
> +	if (rc)
> +		return rc;
>  	return call_int_hook(file_alloc_security, 0, file);

Suppose that a module's file_alloc_security() hook returns an error. 
What should happen to the blob allocated by lsm_file_alloc()? In
general, callers assumes that security_file_alloc() handles cleanup
internally if it returns an error and do not call security_file_free();
this is also true of other similar alloc hooks I believe.  Further, if
we allow the module file_alloc_security() hooks to perform any
allocation themselves, then we have a similar problem with regard to
cleanup if any one of them fails; to be fully safe, we'd need to call
the file_free_security() hook on the ones that had previously returned
success. Either we need to handle such errors within
security_file_alloc(), or we need to dispense with the ability to
allocate and return an error code from the module's
file_alloc_security(), i.e. make them return void, and probably rename
them to file_init_security() or similar.

>  }
>  
>  void security_file_free(struct file *file)
>  {
> +	void *blob;
> +
> +	if (!lsm_file_cache)
> +		return;
> +
>  	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)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a4d1ec236d4e..28e641f829b2 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -129,7 +129,6 @@ int selinux_enabled = 1;
>  #endif
>  
>  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
> @@ -359,27 +358,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 = file->f_security;
> -	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;
> @@ -1823,7 +1810,7 @@ static int file_has_perm(const struct cred
> *cred,
>  			 struct file *file,
>  			 u32 av)
>  {
> -	struct file_security_struct *fsec = file->f_security;
> +	struct file_security_struct *fsec = selinux_file(file);
>  	struct inode *inode = file_inode(file);
>  	struct common_audit_data ad;
>  	u32 sid = cred_sid(cred);
> @@ -2143,7 +2130,7 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
>  					struct file *file)
>  {
>  	u32 sid = task_sid(to);
> -	struct file_security_struct *fsec = file->f_security;
> +	struct file_security_struct *fsec = selinux_file(file);
>  	struct dentry *dentry = file->f_path.dentry;
>  	struct inode_security_struct *isec;
>  	struct common_audit_data ad;
> @@ -3421,7 +3408,7 @@ static int
> selinux_revalidate_file_permission(struct file *file, int mask)
>  static int selinux_file_permission(struct file *file, int mask)
>  {
>  	struct inode *inode = file_inode(file);
> -	struct file_security_struct *fsec = file->f_security;
> +	struct file_security_struct *fsec = selinux_file(file);
>  	struct inode_security_struct *isec;
>  	u32 sid = current_sid();
>  
> @@ -3443,11 +3430,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.
> @@ -3456,7 +3438,7 @@ static int ioctl_has_perm(const struct cred
> *cred, struct file *file,
>  		u32 requested, u16 cmd)
>  {
>  	struct common_audit_data ad;
> -	struct file_security_struct *fsec = file->f_security;
> +	struct file_security_struct *fsec = selinux_file(file);
>  	struct inode *inode = file_inode(file);
>  	struct inode_security_struct *isec;
>  	struct lsm_ioctlop_audit ioctl;
> @@ -3702,7 +3684,7 @@ static void selinux_file_set_fowner(struct file
> *file)
>  {
>  	struct file_security_struct *fsec;
>  
> -	fsec = file->f_security;
> +	fsec = selinux_file(file);
>  	fsec->fown_sid = current_sid();
>  }
>  
> @@ -3717,7 +3699,7 @@ static int selinux_file_send_sigiotask(struct
> task_struct *tsk,
>  	/* struct fown_struct is never outside the context of a
> struct file */
>  	file = container_of(fown, struct file, f_owner);
>  
> -	fsec = file->f_security;
> +	fsec = selinux_file(file);
>  
>  	if (!signum)
>  		perm = signal_to_av(SIGIO); /* as per
> send_sigio_to_task */
> @@ -3740,7 +3722,7 @@ static int selinux_file_open(struct file *file,
> const struct cred *cred)
>  	struct file_security_struct *fsec;
>  	struct inode_security_struct *isec;
>  
> -	fsec = file->f_security;
> +	fsec = selinux_file(file);
>  	isec = inode_security(file_inode(file));
>  	/*
>  	 * Save inode label and policy sequence number
> @@ -3870,7 +3852,7 @@ static int
> selinux_kernel_module_from_file(struct file *file)
>  	ad.type = LSM_AUDIT_DATA_FILE;
>  	ad.u.file = file;
>  
> -	fsec = file->f_security;
> +	fsec = selinux_file(file);
>  	if (sid != fsec->sid) {
>  		rc = avc_has_perm(sid, fsec->sid, SECCLASS_FD,
> FD__USE, &ad);
>  		if (rc)
> @@ -6215,6 +6197,7 @@ static void selinux_ib_free_security(void
> *ib_sec)
>  
>  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
> = {
> @@ -6285,7 +6268,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),
> @@ -6466,9 +6448,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();
>  
>  	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
> "selinux");
> diff --git a/security/selinux/include/objsec.h
> b/security/selinux/include/objsec.h
> index c0bdb7232f39..504e15ed234f 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -161,4 +161,9 @@ static inline struct task_security_struct
> *selinux_cred(const struct cred *cred)
>  	return cred->security;
>  }
>  
> +static inline struct file_security_struct *selinux_file(const struct
> file *file)
> +{
> +	return file->f_security;
> +}
> +
>  #endif /* _SELINUX_OBJSEC_H_ */
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index ab1d217800e2..d14e8d17eea0 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -361,6 +361,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 ff4e5c632410..a807624aff9a 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1575,25 +1575,13 @@ 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
>   * @cmd: what to do
> @@ -1817,7 +1805,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();
>  }
>  
>  /**
> @@ -1834,6 +1824,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;
> @@ -1846,7 +1837,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))
> @@ -4578,6 +4570,7 @@ static int smack_inode_getsecctx(struct inode
> *inode, void **ctx, u32 *ctxlen)
>  
>  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 =
> {
> @@ -4615,7 +4608,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),
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



More information about the Linux-security-module-archive mailing list