Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-PHdr: =?us-ascii?q?9a23=3AfawAthLzMePFp8KUXdmcpTZWNBhigK39O0sv0r?=
 =?us-ascii?q?FitYgXL/X8rarrMEGX3/hxlliBBdydt6obzbKO+4nbGkU4qa6bt34DdJEeHz?=
 =?us-ascii?q?Qksu4x2zIaPcieFEfgJ+TrZSFpVO5LVVti4m3peRMNQJW2aFLduGC94iAPER?=
 =?us-ascii?q?vjKwV1Ov71GonPhMiryuy+4ZLebxlKiTanfb9+MAi9oBnMuMURnYZsMLs6xA?=
 =?us-ascii?q?HTontPdeRWxGdoKkyWkh3h+Mq+/4Nt/jpJtf45+MFOTav1f6IjTbxFFzsmKH?=
 =?us-ascii?q?w65NfqtRbYUwSC4GYXX3gMnRpJBwjF6wz6Xov0vyDnuOdxxDWWMMvrRr0vRz?=
 =?us-ascii?q?+s87lkRwPpiCcfNj427mfXitBrjKlGpB6tvgFzz5LIbI2QMvdxcLndfdcHTm?=
 =?us-ascii?q?RfWMhfWTFKDoelY4cRE+YNOOBVpJT/qVQTtxuzHRSiCv3hyjFIhXH406M13O?=
 =?us-ascii?q?sjHg7a0wItBM4OvXbOodnpKKsfX+K4wa/VxjvDdfNW3jL95ZDVfBA9v/6MRb?=
 =?us-ascii?q?JwftTXyUIyCg3Fi0+fqYjhPzyL1uUGrm+W7/F9WuK0kGMntwFwrSSvxscrkI?=
 =?us-ascii?q?XJgJkVxUre+SV2x4Y1O8S1RUhmatCnCJtdrzyWOoR5T884Q2xkpTw2xqMJtJ?=
 =?us-ascii?q?KlZiQG1ZIqzAPFZfOdaYiH+BfjWf6UITd/mX1qZqqyhw238Ui80u38UdS00E?=
 =?us-ascii?q?pSoipFjNbMsncN2gTP6sedUPt9/1qh2S2V2wDP6uBLPUA0la3BJ54n3rEwjY?=
 =?us-ascii?q?YcvV7GHi/3nEX6lK6WdkM69ei08+nrf7rrq5CGO4J0lw3yKLoil8OhDegiLw?=
 =?us-ascii?q?QCR22b9v691L3n8035WrJKjvgun6nCrZ/aPt8WprK5AgBJ0oYj7AyzDzG90N?=
 =?us-ascii?q?sCh3UHI1VFeAyfg4jzJ17OOOz4Deu4g1m0jDhk3evGMaPhA5jWNXjMjLfhcq?=
 =?us-ascii?q?xg605SzAo808pf64tIBb4bOv78RkjxtNnABB8jLwO02/rnCMl61o4GQWKAHK?=
 =?us-ascii?q?mZMKzPsV+J4OIjOuqMa5EPuDb7Nfcl4+XjjX4glV8Zeqmpw4UYZGqjHvt8IE?=
 =?us-ascii?q?WZfGDsjc0bHWcMoAUyVu7qiEWaWz5Je3myR7485i08CI++DofMWJ6igKed0y?=
 =?us-ascii?q?e8GZ1WZXtLBUyMEXfycIWEXvYMaD+XIsN7lTwET7ehQZc71R6yrA/616ZnLu?=
 =?us-ascii?q?3M9y0ctJLj0sV15uLKmREp6zN7E9md03uMT2FonmIEXjo23KdirkxgzleMz7?=
 =?us-ascii?q?N1g+JXFdNN/fNFSAQ6OoDGz+x8Fd/yXhjNftCTSFapWt+mGy0+Tsotw98SZE?=
 =?us-ascii?q?ZwA9eijhXE3yqwGb8VlqeLCYcy8q3G2nj+Ocd9x2zB1Kk7gFksWtFPOnG+hq?=
 =?us-ascii?q?5j6wjTAJbEk0GYl6asaKQd0zfB9GSdwmqUukFXTgpwXL7bXXAQeETWt8715k?=
 =?us-ascii?q?DcQL+0D7QoLA9BxdSFKqtQZd3jlU9GS+v7ONTCf2KxnH+9BRSPxrOMaormYW?=
 =?us-ascii?q?cd3CLdCEcelQAT5miJNQ4lCyi9uW3eCjtuFVTuY0zw6+Z+rGm3QVMzzwGPd0?=
 =?us-ascii?q?dhzaa6+gYJhfyATPMexqoEtz08qzVwB1u9x8jZC8eEpwZ4eaVcZtQ94E1Z2m?=
 =?us-ascii?q?7DqwN9OZmgJbh4hlECawR3o1/u1xJvB4VEkMgqqm4qzQVrJaKWy1NOai2X3Y?=
 =?us-ascii?q?7uNb3TMWTy4h+vZLDM2l3E09aZ5L0P6PImpFXnpg2pEVAi83p/2dlPz3Sc/o?=
 =?us-ascii?q?nKDBYVUZ/pSEk46h96qKrAYik854Lbz3tsPLK7sj/Hwd0pBe8lxgy8cNdYNa?=
 =?us-ascii?q?OODBXyHNECB8iyNOwqnECkbhcFPO9O76M7IsKmd/SH2K6oO+ZvgSiqjWJZ74?=
 =?us-ascii?q?BhykiM7TZzSvbU35YZxPGVxhCHWy35jFi/qcD3nppEaisOEWWl1CTpBZVcZq?=
 =?us-ascii?q?J3fYkRCGeuJ9e7ycl5h57oCDZk8wuIDkgLyYeSchqbclL50BcYgU8eunG2sT?=
 =?us-ascii?q?CzzzVpnTUktO+U1WrFxOG0MFI7N3JQWW4qrV7qLYH828gXWke1bg5slxa/40?=
 =?us-ascii?q?v+76keoKNhIi/ISERVZSGwKWwkULPm8vKhbsNUoLgvtiYfBOexbEuRTbn+ix?=
 =?us-ascii?q?AaySTmHnZbgjcheGf5lI/+mklBlG+FLHt15EHccMV0yAaXsMfQXtZNzzEGQ2?=
 =?us-ascii?q?9+kjCRCV+iaYr6te6InovO57jtH1mqUYdeJGyyl9uN?=
Subject: Re: [PATCH v4 00/19] LSM: Module stacking for SARA and Landlock
To:     Casey Schaufler <casey@schaufler-ca.com>,
        Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Kees Cook <keescook@chromium.org>
Cc:     LSM <linux-security-module@vger.kernel.org>,
        James Morris <jmorris@namei.org>,
        SE Linux <selinux@tycho.nsa.gov>,
        LKLM <linux-kernel@vger.kernel.org>,
        John Johansen <john.johansen@canonical.com>,
        Paul Moore <paul@paul-moore.com>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= <mic@digikod.net>,
        Salvatore Mesoraca <s.mesoraca16@gmail.com>
References: <e9bfb2d5-d987-55ce-4011-9b32ff745d36@schaufler-ca.com>
 <CAGXu5jK=+FuL+JiSxcHy0i9Ppha=BQYdJW5zvbm1Redbo12wmw@mail.gmail.com>
 <680e6e16-0890-8304-0e8e-6c58966813b5@schaufler-ca.com>
 <CAGXu5j+7FpFDNuOowdHzJ5RXgfETQMv=Fe-EU1gWo+nWK+goZA@mail.gmail.com>
 <39457e79-3816-824b-6b4d-89d21b03f9ce@i-love.sakura.ne.jp>
 <b209d4b8-81a5-30aa-f4d1-9372bbb1b842@schaufler-ca.com>
From:   Stephen Smalley <sds@tycho.nsa.gov>
Message-ID: <a2858469-cf38-7706-52b5-1d5fd5ed43ad@tycho.nsa.gov>
Date:   Mon, 24 Sep 2018 11:01:30 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <b209d4b8-81a5-30aa-f4d1-9372bbb1b842@schaufler-ca.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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