From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from goalie.tycho.ncsc.mil (goalie.infosec.tycho.ncsc.mil [144.51.242.250]) by tarius.tycho.ncsc.mil (8.14.4/8.14.4) with ESMTP id w8OKY017006582 for ; Mon, 24 Sep 2018 16:34:00 -0400 To: Tetsuo Handa , Kees Cook Cc: LSM , James Morris , SE Linux , LKLM , John Johansen , Paul Moore , Stephen Smalley , "linux-fsdevel@vger.kernel.org" , Alexey Dobriyan , =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= , Salvatore Mesoraca References: <680e6e16-0890-8304-0e8e-6c58966813b5@schaufler-ca.com> <39457e79-3816-824b-6b4d-89d21b03f9ce@i-love.sakura.ne.jp> <3350edec-c233-9428-73b0-58d582d1e3b9@schaufler-ca.com> From: Casey Schaufler Message-ID: <28e4ca6d-e86c-ee1f-ead8-f03d1ccd7d54@schaufler-ca.com> Date: Mon, 24 Sep 2018 13:33:51 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Subject: Re: [PATCH v4 00/19] LSM: Module stacking for SARA and Landlock List-Id: "Security-Enhanced Linux \(SELinux\) mailing list" List-Post: List-Help: On 9/24/2018 10:53 AM, Tetsuo Handa wrote: > On 2018/09/25 2:16, Casey Schaufler wrote: >>> 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? > Indeed. AKARI can as well use security_sk_free() using address of > "struct sock" as a key. > >> 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. > LKM based LSMs are too late for updating blob_sizes.* fields. That is true with the code in this patch set. As I mentioned, changing the blob handling to include a header with real use information would be required. > Even if they could, they after all have to somehow check whether > corresponding init hook was called. That's checking for NULL. Right. >>>>> @@ -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. >> > The free hook might be used for freeing resources which were not allocated > by alloc hook. Yama is using task_free hook without task_alloc hook. > Someone might want to use file_free hook without file_alloc hook. OK, you're correct. Checking for an initialized kmem_cache isn't appropriate.