[PATCH v4 00/19] LSM: Module stacking for SARA and Landlock

On 09/23/2018 01:09 PM, Casey Schaufler wrote:

On 9/23/2018 8:59 AM, Tetsuo Handa wrote:

quoted

On 2018/09/23 11:43, Kees Cook wrote:

quoted

quoted

quoted

I'm excited about getting this landed!

Soon. Real soon. I hope. I would very much like for
someone from the SELinux camp to chime in, especially on
the selinux_is_enabled() removal.

Agreed.

This patchset from Casey lands before the patchset from Kees, doesn't it?

That is up for negotiation. We may end up combining them.

quoted

OK, a few comments (if I didn't overlook something).

   lsm_early_cred()/lsm_early_task() are called from only __init functions.

True.

quoted

   lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .

Also true.

quoted

   lsm_early_inode() should be avoided because it is not appropriate to
   call panic() when lsm_early_inode() is called after __init phase.

You're correct. In fact, lsm_early_inode() isn't needed at all
until multiple inode using modules are supported.

quoted

   Since all free hooks are called when one of init hooks failed, each
   free hook needs to check whether init hook was called. An example is
   inode_free_security() in security/selinux/hooks.c (but not addressed in
   this patch).

I *think* that selinux_inode_free_security() is safe in this
case because the blob will be zeroed, hence isec->list will
be NULL.

That's not safe - look more closely at what list_empty_careful() tests, 
and then think about what happens when list_del_init() gets called on 
that isec->list.  selinux_inode_free_security() presumes that 
selinux_inode_alloc_security() has been called already.  If you are 
breaking that assumption, you have to fix it.

Is there a reason you can't make inode_alloc_security() return void 
since you moved the allocation to the framework?  Unfortunate that 
inode_init_security name is already in use for another purpose since 
essentially you have reduced these hooks to initialization only.

quoted

   This patchset might fatally prevent LKM-based LSM modules, for LKM-based
   LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot
   be updated upon loading LKM-based LSMs.

LKM based security modules will require dynamically sized blobs.
These can be added to the scheme used here. Each blob would get a
header identifying the modules for which it contains data. When an
LKM is registered if has to declare it's blob space requirements
and gets back the offsets. All alloc operations have to put their
marks in the header. All LKM blob users have to check that the blob
they are looking at has the required data.

module_cred(struct cred *cred) {
	return cred->security + module_blob_sizes.lbs_cred;
}

becomes

module_cred(struct cred *cred) {
	if (blob_includes(module_id))
		return cred->security + module_blob_sizes.lbs_cred;
	return NULL;
}

and the calling code needs to accept a NULL return.
Blobs can never get smaller because readjusting the offsets
isn't going to work, so unloading an LKM security module isn't
going to be as complete as you might like. There may be a way
around this if you unload all the LKM modules, but that's a
special case and there may be dragon lurking in the mist.

quoted

  If security_file_free() is called
   regardless of whether lsm_file_cache is defined, LKM-based LSMs can be
   loaded using current behavior (apart from the fact that legitimate
   interface for appending to security_hook_heads is currently missing).
   How do you plan to handle LKM-based LSMs?

My position all along has been that I don't plan to handle LKM
based LSMs, but that I won't do anything to prevent someone else
from adding them later. I believe that I've done that. Several
designs, including a separate list for dynamically loaded modules
have been proposed. I think some of those would work.

quoted

  include/linux/lsm_hooks.h  |    6 ++----
  security/security.c        |   31 ++++++-------------------------
  security/smack/smack_lsm.c |    8 +++++++-
  3 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 7e8b32f..8014614 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h

@@ -2095,13 +2095,11 @@ static inline void __init yama_add_hooks(void) { }
  static inline void loadpin_add_hooks(void) { };
  #endif
  
-extern int lsm_cred_alloc(struct cred *cred, gfp_t gfp);
  extern int lsm_inode_alloc(struct inode *inode);
  
  #ifdef CONFIG_SECURITY
-void lsm_early_cred(struct cred *cred);
-void lsm_early_inode(struct inode *inode);
-void lsm_early_task(struct task_struct *task);
+void __init lsm_early_cred(struct cred *cred);
+void __init lsm_early_task(struct task_struct *task);
  #endif
  
  #endif /* ! __LINUX_LSM_HOOKS_H */

diff --git a/security/security.c b/security/security.c
index e7c85060..341e8df 100644
--- a/security/security.c
+++ b/security/security.c

@@ -267,7 +267,7 @@ int unregister_lsm_notifier(struct notifier_block *nb)
   *
   * Returns 0, or -ENOMEM if memory can't be allocated.
   */
-int lsm_cred_alloc(struct cred *cred, gfp_t gfp)
+static int lsm_cred_alloc(struct cred *cred, gfp_t gfp)
  {
  	if (blob_sizes.lbs_cred == 0) {
  		cred->security = NULL;

@@ -286,7 +286,7 @@ int lsm_cred_alloc(struct cred *cred, gfp_t gfp)
   *
   * Allocate the cred blob for all the modules if it's not already there
   */
-void lsm_early_cred(struct cred *cred)
+void __init lsm_early_cred(struct cred *cred)
  {
  	int rc;
  

@@ -344,7 +344,7 @@ void __init security_add_blobs(struct lsm_blob_sizes *needed)
   *
   * Returns 0, or -ENOMEM if memory can't be allocated.
   */
-int lsm_file_alloc(struct file *file)
+static int lsm_file_alloc(struct file *file)
  {
  	if (!lsm_file_cache) {
  		file->f_security = NULL;

@@ -379,25 +379,6 @@ int lsm_inode_alloc(struct inode *inode)
  }
  
  /**
- * lsm_early_inode - during initialization allocate a composite inode blob
- * @inode: the inode that needs a blob
- *
- * Allocate the inode blob for all the modules if it's not already there
- */
-void lsm_early_inode(struct inode *inode)
-{
-	int rc;
-
-	if (inode == NULL)
-		panic("%s: NULL inode.\n", __func__);
-	if (inode->i_security != NULL)
-		return;
-	rc = lsm_inode_alloc(inode);
-	if (rc)
-		panic("%s: Early inode alloc failed.\n", __func__);
-}
-
-/**
   * lsm_task_alloc - allocate a composite task blob
   * @task: the task that needs a blob
   *

@@ -466,7 +447,7 @@ int lsm_msg_msg_alloc(struct msg_msg *mp)
   *
   * Allocate the task blob for all the modules if it's not already there
   */
-void lsm_early_task(struct task_struct *task)
+void __init lsm_early_task(struct task_struct *task)
  {
  	int rc;
  

@@ -1202,11 +1183,11 @@ void security_file_free(struct file *file)
  {
  	void *blob;
  
+	call_void_hook(file_free_security, file);
+
  	if (!lsm_file_cache)
  		return;
  
-	call_void_hook(file_free_security, file);
-

Why does this make sense? If the lsm_file_cache isn't
initialized you can't have allocated any file blobs,
no module can have initialized a file blob, hence there
can be nothing for the module to do.

quoted

  	blob = file->f_security;
  	file->f_security = NULL;
  	kmem_cache_free(lsm_file_cache, blob);

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 7843004..b0b4045 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c

@@ -750,6 +750,13 @@ static int smack_set_mnt_opts(struct super_block *sb,
  	if (sp->smk_flags & SMK_SB_INITIALIZED)
  		return 0;
  
+	if (inode->i_security == NULL) {
+		int rc = lsm_inode_alloc(inode);
+
+		if (rc)
+			return rc;
+	}
+
  	if (!smack_privileged(CAP_MAC_ADMIN)) {
  		/*
  		 * Unprivileged mounts don't get to specify Smack values.

@@ -818,7 +825,6 @@ static int smack_set_mnt_opts(struct super_block *sb,
  	/*
  	 * Initialize the root inode.
  	 */
-	lsm_early_inode(inode);
  	init_inode_smack(inode, sp->smk_root);
  
  	if (transmute) {