Message-ID: <a2858469-cf38-7706-52b5-1d5fd5ed43ad@tycho.nsa.gov>
Date:   Mon, 24 Sep 2018 11:01:30 -0400
From:   Stephen Smalley <sds@...ho.nsa.gov>
To:     Casey Schaufler <casey@...aufler-ca.com>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Kees Cook <keescook@...omium.org>
Cc:     LSM <linux-security-module@...r.kernel.org>,
        James Morris <jmorris@...ei.org>,
        SE Linux <selinux@...ho.nsa.gov>,
        LKLM <linux-kernel@...r.kernel.org>,
        John Johansen <john.johansen@...onical.com>,
        Paul Moore <paul@...l-moore.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Mickaël Salaün <mic@...ikod.net>,
        Salvatore Mesoraca <s.mesoraca16@...il.com>
Subject: Re: [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:
>> On 2018/09/23 11:43, Kees Cook wrote:
>>>>> 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.
> 
>> OK, a few comments (if I didn't overlook something).
>>
>>    lsm_early_cred()/lsm_early_task() are called from only __init functions.
> 
> True.
> 
>>    lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .
> 
> Also true.
> 
>>    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.
> 
>>    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.

> 
>>    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.
> 
>>   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.
> 
>>   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.
> 
>>   	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) {
> 

Powered by blists - more mailing lists