Message-ID: <3350edec-c233-9428-73b0-58d582d1e3b9@schaufler-ca.com>
Date:   Mon, 24 Sep 2018 10:16:52 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     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>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        "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 9/23/2018 6:53 PM, Tetsuo Handa wrote:
> On 2018/09/24 2:09, Casey Schaufler wrote:
>>>   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.
>>
> OK.
>
>>>   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.
> Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs
> might use security blobs for only a few objects. For example, AKARI uses
> inode security blob for remembering whether source address/port of an
> accept()ed socket was already checked, only during accept() operation and
> first socket operation on the accept()ed socket. Thus, there is no need
> to waste memory by assigning blobs for all inode objects.

The first question is why use an inode blob? Shouldn't you
be using a socket blob for this socket based information?

If you only want information part of the time you can declare
a pointer sized blob and manage what hangs off that as you will.
I personally think that the added complexity of conditional
blob management is more pain than it's worth, but if you want
a really big blob, but only on occasion, I could see doing it.

>> 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 LKM-based LSMs who want to use security blobs have to check for
> NULL return, they might choose "not using infrastructure managed
> security blobs" and "using locally hashed blobs associated with
> object's address" (like AKARI does).

I can't see how a check for NULL could possibly be a bigger
hassle than doing your own locally hashed blobs.

>
>>>  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.
> Though AKARI is not using security_file_free(), some of LKM-based LSMs
> might want to use it. If file_free_security hook is called unconditionally,
> such LKM-based LSMs can be registered/unregistered, without worrying about
> inability to shrink sizes for blobs.

The infrastructure wouldn't call unregistered hooks, so any module
that allocates additional memory attached to a blob is going to have
to deal with freeing that when it unregisters. Aside from that unregistration
should be a (not so) small matter of locking.

>
>>> @@ -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.
>>
> For modules (not limited to LKM-based LSMs) which want to use
> file blobs for only a few objects and avoid wasting memory by
> allocating file blobs to all file objects.
>
> Infrastructure based blob management fits well for LSM modules
> which want to assign blobs to all objects (like SELinux). But
> forcing infrastructure based blob management can become a huge
> waste of memory for LSM modules which want to assign blobs to
> only a few objects. Unconditionally calling file_free_security
> hook (as with other hooks) preserves a room for allowing the
> latter type of LSM modules without using infrastructure based
> blob management.

There is a hypothetical issue here, but that would require abuse
of the infrastructure. Having a file_free_security hook that doesn't
free a security blob allocated by file_alloc_security may coincidentaly
be useful, but that's not the intent of the hook.

Powered by blists - more mailing lists