From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 63D5AC77B60 for ; Tue, 28 Mar 2023 07:47:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232263AbjC1Hrc (ORCPT ); Tue, 28 Mar 2023 03:47:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33046 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230093AbjC1Hrb (ORCPT ); Tue, 28 Mar 2023 03:47:31 -0400 Received: from frasgout12.his.huawei.com (frasgout12.his.huawei.com [14.137.139.154]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 582C61A8; Tue, 28 Mar 2023 00:47:29 -0700 (PDT) Received: from mail02.huawei.com (unknown [172.18.147.227]) by frasgout12.his.huawei.com (SkyGuard) with ESMTP id 4Pm1jq5DGmz9v7Y6; Tue, 28 Mar 2023 15:37:35 +0800 (CST) Received: from roberto-ThinkStation-P620 (unknown [10.204.63.22]) by APP1 (Coremail) with SMTP id LxC2BwBn6QRkmyJkwsvWAQ--.64076S2; Tue, 28 Mar 2023 08:46:57 +0100 (CET) Message-ID: <1e08006f9011efa48deaf656c358ca3d438b9768.camel@huaweicloud.com> Subject: Re: [PATCH v8 4/6] security: Allow all LSMs to provide xattrs for inode_init_security hook From: Roberto Sassu To: Paul Moore Cc: mark@fasheh.com, jlbec@evilplan.org, joseph.qi@linux.alibaba.com, zohar@linux.ibm.com, dmitry.kasatkin@gmail.com, jmorris@namei.org, serge@hallyn.com, stephen.smalley.work@gmail.com, eparis@parisplace.org, casey@schaufler-ca.com, ocfs2-devel@oss.oracle.com, reiserfs-devel@vger.kernel.org, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, linux-kernel@vger.kernel.org, keescook@chromium.org, nicolas.bouchinet@clip-os.org, Roberto Sassu Date: Tue, 28 Mar 2023 09:46:41 +0200 In-Reply-To: References: <20230314081720.4158676-1-roberto.sassu@huaweicloud.com> <20230314081720.4158676-5-roberto.sassu@huaweicloud.com> <939e6c88662ad90b963993c4cc1b702083e74a7a.camel@huaweicloud.com> <8b63d00d8ac3f686e51889ea4fc8d83f8ecb300d.camel@huaweicloud.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.36.5-0ubuntu1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: LxC2BwBn6QRkmyJkwsvWAQ--.64076S2 X-Coremail-Antispam: 1UD129KBjvJXoW3Xw1UKw4ftFWDCF15KFyxuFg_yoWfKF48pr WUJ3Wjkr4DJFyUCrySqa18uw1agrWrGrWUXrn8JryUAr1qyr1xJry0kr45uFykXrW8Jr1q qr4jyr47urn8A3DanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUkjb4IE77IF4wAFF20E14v26ryj6rWUM7CY07I20VC2zVCF04k2 6cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4 vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7Cj xVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv67AKxVWUJVW8JwA2z4x0Y4vEx4A2jsIEc7CjxV AFwI0_Gr0_Gr1UM2AIxVAIcxkEcVAq07x20xvEncxIr21l5I8CrVACY4xI64kE6c02F40E x7xfMcIj6xIIjxv20xvE14v26r1j6r18McIj6I8E87Iv67AKxVWUJVW8JwAm72CE4IkC6x 0Yz7v_Jr0_Gr1lF7xvr2IY64vIr41lFIxGxcIEc7CjxVA2Y2ka0xkIwI1l42xK82IYc2Ij 64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG67AKxVWUJVWUGwC20s026x 8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r4a6rW5MIIYrxkI7VAKI48JMIIF0xvE 2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r4j6F4UMIIF0xvE42 xK8VAvwI8IcIk0rVWrZr1j6s0DMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2jsIE c7CjxVAFwI0_Gr0_Gr1UYxBIdaVFxhVjvjDU0xZFpf9x07UZ18PUUUUU= X-CM-SenderInfo: purev21wro2thvvxqx5xdzvxpfor3voofrz/1tbiAQAKBF1jj4s6twADs7 X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-integrity@vger.kernel.org On Mon, 2023-03-27 at 17:02 -0400, Paul Moore wrote: > On Mon, Mar 27, 2023 at 3:30 AM Roberto Sassu > wrote: > > On Fri, 2023-03-24 at 17:39 -0400, Paul Moore wrote: > > > On Fri, Mar 24, 2023 at 9:26 AM Roberto Sassu > > > wrote: > > > > On Fri, 2023-03-24 at 11:18 +0100, Roberto Sassu wrote: > > > > > On Thu, 2023-03-23 at 20:09 -0400, Paul Moore wrote: > > > > > > On Tue, Mar 14, 2023 at 4:19 AM Roberto Sassu > > > > > > wrote: > > > > > > > From: Roberto Sassu > > > > > > > > > > > > > > Currently, security_inode_init_security() supports only one LSM providing > > > > > > > an xattr and EVM calculating the HMAC on that xattr, plus other inode > > > > > > > metadata. > > > > > > > > > > > > > > Allow all LSMs to provide one or multiple xattrs, by extending the security > > > > > > > blob reservation mechanism. Introduce the new lbs_xattr field of the > > > > > > > lsm_blob_sizes structure, so that each LSM can specify how many xattrs it > > > > > > > needs, and the LSM infrastructure knows how many xattr slots it should > > > > > > > allocate. > > > > > > > > > > > > > > Dynamically allocate the xattrs array to be populated by LSMs with the > > > > > > > inode_init_security hook, and pass it to the latter instead of the > > > > > > > name/value/len triple. Update the documentation accordingly, and fix the > > > > > > > description of the xattr name, as it is not allocated anymore. > > > > > > > > > > > > > > Since the LSM infrastructure, at initialization time, updates the number of > > > > > > > the requested xattrs provided by each LSM with a corresponding offset in > > > > > > > the security blob (in this case the xattr array), it makes straightforward > > > > > > > for an LSM to access the right position in the xattr array. > > > > > > > > > > > > > > There is still the issue that an LSM might not fill the xattr, even if it > > > > > > > requests it (legitimate case, for example it might have been loaded but not > > > > > > > initialized with a policy). Since users of the xattr array (e.g. the > > > > > > > initxattrs() callbacks) detect the end of the xattr array by checking if > > > > > > > the xattr name is NULL, not filling an xattr would cause those users to > > > > > > > stop scanning xattrs prematurely. > > > > > > > > > > > > > > Solve that issue by introducing security_check_compact_filled_xattrs(), > > > > > > > which does a basic check of the xattr array (if the xattr name is filled, > > > > > > > the xattr value should be too, and viceversa), and compacts the xattr array > > > > > > > by removing the holes. > > > > > > > > > > > > > > An alternative solution would be to let users of the xattr array know the > > > > > > > number of elements of that array, so that they don't have to check the > > > > > > > termination. However, this seems more invasive, compared to a simple move > > > > > > > of few array elements. > > > > > > > > > > > > > > security_check_compact_filled_xattrs() also determines how many xattrs in > > > > > > > the xattr array have been filled. If there is none, skip > > > > > > > evm_inode_init_security() and initxattrs(). Skipping the former also avoids > > > > > > > EVM to crash the kernel, as it is expecting a filled xattr. > > > > > > > > > > > > > > Finally, adapt both SELinux and Smack to use the new definition of the > > > > > > > inode_init_security hook, and to correctly fill the designated slots in the > > > > > > > xattr array. For Smack, reserve space for the other defined xattrs although > > > > > > > they are not set yet in smack_inode_init_security(). > > > > > > > > > > > > > > Reported-by: Nicolas Bouchinet (EVM crash) > > > > > > > Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/ > > > > > > > Signed-off-by: Roberto Sassu > > > > > > > Reviewed-by: Casey Schaufler > > > > > > > Reviewed-by: Mimi Zohar > > > > > > > --- > > > > > > > include/linux/lsm_hook_defs.h | 3 +- > > > > > > > include/linux/lsm_hooks.h | 1 + > > > > > > > security/security.c | 119 +++++++++++++++++++++++++++++----- > > > > > > > security/selinux/hooks.c | 19 ++++-- > > > > > > > security/smack/smack_lsm.c | 33 ++++++---- > > > > > > > 5 files changed, 137 insertions(+), 38 deletions(-) > > > > > > ... > > > > > > > > > > @@ -1604,33 +1654,66 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, > > > > > > > const struct qstr *qstr, > > > > > > > const initxattrs initxattrs, void *fs_data) > > > > > > > { > > > > > > > - struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1]; > > > > > > > - struct xattr *lsm_xattr, *evm_xattr, *xattr; > > > > > > > - int ret; > > > > > > > + struct security_hook_list *P; > > > > > > > + struct xattr *new_xattrs; > > > > > > > + struct xattr *xattr; > > > > > > > + int ret = -EOPNOTSUPP, num_filled_xattrs = 0; > > > > > > > > > > > > > > if (unlikely(IS_PRIVATE(inode))) > > > > > > > return 0; > > > > > > > > > > > > > > + if (!blob_sizes.lbs_xattr) > > > > > > > + return 0; > > > > > > > + > > > > > > > if (!initxattrs) > > > > > > > return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, > > > > > > > - dir, qstr, NULL, NULL, NULL); > > > > > > > - memset(new_xattrs, 0, sizeof(new_xattrs)); > > > > > > > - lsm_xattr = new_xattrs; > > > > > > > - ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr, > > > > > > > - &lsm_xattr->name, > > > > > > > - &lsm_xattr->value, > > > > > > > - &lsm_xattr->value_len); > > > > > > > - if (ret) > > > > > > > + dir, qstr, NULL); > > > > > > > + /* Allocate +1 for EVM and +1 as terminator. */ > > > > > > > + new_xattrs = kcalloc(blob_sizes.lbs_xattr + 2, sizeof(*new_xattrs), > > > > > > > + GFP_NOFS); > > > > > > > + if (!new_xattrs) > > > > > > > + return -ENOMEM; > > > > > > > + > > > > > > > + hlist_for_each_entry(P, &security_hook_heads.inode_init_security, > > > > > > > + list) { > > > > > > > + ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs); > > > > > > > + if (ret && ret != -EOPNOTSUPP) > > > > > > > + goto out; > > > > > > > + /* > > > > > > > + * As documented in lsm_hooks.h, -EOPNOTSUPP in this context > > > > > > > + * means that the LSM is not willing to provide an xattr, not > > > > > > > + * that it wants to signal an error. Thus, continue to invoke > > > > > > > + * the remaining LSMs. > > > > > > > + */ > > > > > > > + if (ret == -EOPNOTSUPP) > > > > > > > + continue; > > > > > > > + /* > > > > > > > + * As the number of xattrs reserved by LSMs is not directly > > > > > > > + * available, directly use the total number blob_sizes.lbs_xattr > > > > > > > + * to keep the code simple, while being not the most efficient > > > > > > > + * way. > > > > > > > + */ > > > > > > > > > > > > Is there a good reason why the LSM can't return the number of xattrs > > > > > > it is adding to the xattr array? It seems like it should be fairly > > > > > > trivial for the individual LSMs to determine and it could save a lot > > > > > > of work. However, given we're at v8 on this patchset I'm sure I'm > > > > > > missing something obvious, can you help me understand why the idea > > > > > > above is crazy stupid? ;) > > > > > > > > Much simple answer. Yes, LSMs could return the number of xattrs set, > > > > but security_check_compact_filled_xattrs() also needs to know from > > > > which offset (the lbs_xattr of each LSM) it should start compacting. > > > > > > > > Example: suppose that you have three LSMs with: > > > > > > > > LSM#1: lbs_xattr 1 > > > > LSM#2: lbs_xattr 2 (disabled) > > > > LSM#3: lbs_xattr 1 > > > > > > > > The current compaction interval is: already compacted xattrs - end of > > > > new_xattr array. > > > > > > > > When the security_inode_init_security() loop calls LSM#3, the > > > > compaction interval is: 1 - 2 (LSM#2 returns 0), which clearly isn't > > > > right. The correct compaction interval should be: 3 - 4. > > > > > > > > Going to the end of new_xattrs is an approximation, but it ensures > > > > that security_check_compact_filled_xattrs() reaches the xattr set by > > > > LSM#3. > > > > > > > > The alternative I was mentioning of passing num_filled_xattrs to LSMs > > > > goes again in the direction of doing on-the-fly compaction, while LSMs > > > > are more familiar with using the lbs_* fields. > > > > > > I guess I was thinking of the case where the LSM layer, i.e. > > > security_inode_init_security(), allocates an xattr array like it does > > > now based on the maximum number of xattrs possible using the > > > lsm_blob_sizes values and passes a pointer to the individual LSMs > > > which is incremented based on how many xattrs are created by the > > > individual LSMs. Here is some *very* rough pseudo code: > > > > > > int security_inode_init_security(...) > > > { > > > > > > /* allocate an xattr array */ > > > xattrs = kcalloc(blob_sizes, sizeof(*xattrs), GFP_BLAH); > > > > > > /* loop on the lsms */ > > > xa_cnt = 0; > > > while (lsm_hooks) { > > > rc = call_hook(lsm_hook, &xattrs[xa_cnt]); > > > if (rc > 0) > > > xa_cnt += rc; > > > } > > > > > > /* evm magic */ > > > evm_inode_init_security(...) > > > } > > > > > > Does that work? Am I missing something? > > > > Oh, unfortunately not. EVM needs to see all xattrs (when it is moved to > > the LSM infrastructure). > > Okay, that's fair, but we could still pass the full xattrs array and a > reference to the current count which could be both read and updated by > the individual LSMs, right? Yes, we could do. > The issue is that the separate compaction stage is not something we > want to have to do if we can avoid it. Maybe we're stuck with it, but > I'm not yet convinced that we can't make some minor changes to the > LSMs to avoid the compaction step. I liked more the idea that LSMs do what they are most familiar with, get an offset in a security blob or, in this case, a starting slot in the new_xattrs array, and write there. v3 had the lsm_find_xattr_slot() helper, to get the starting slot, but somehow I find it less intuitive. Ok, if you prefer to avoid the compaction stage, I will rewrite this patch. Thanks Roberto