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 4BB5AC7EE43 for ; Sat, 10 Jun 2023 07:10:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230173AbjFJHKg (ORCPT ); Sat, 10 Jun 2023 03:10:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34818 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229598AbjFJHKf (ORCPT ); Sat, 10 Jun 2023 03:10:35 -0400 Received: from frasgout12.his.huawei.com (unknown [14.137.139.154]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 409C33A9B; Sat, 10 Jun 2023 00:10:33 -0700 (PDT) Received: from mail02.huawei.com (unknown [172.18.147.227]) by frasgout12.his.huawei.com (SkyGuard) with ESMTP id 4QdTLc3Hx7z9xFrh; Sat, 10 Jun 2023 14:58:32 +0800 (CST) Received: from [10.81.219.229] (unknown [10.81.219.229]) by APP2 (Coremail) with SMTP id GxC2BwCXPjXBIYRk43ofAw--.4331S2; Sat, 10 Jun 2023 08:10:05 +0100 (CET) Message-ID: <809521fb-044b-fb5d-d7a5-654e633d41aa@huaweicloud.com> Date: Sat, 10 Jun 2023 09:09:51 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: [PATCH v11 2/4] smack: Set the SMACK64TRANSMUTE xattr in smack_inode_init_security() Content-Language: en-US To: Mimi Zohar , dmitry.kasatkin@gmail.com, paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com, stephen.smalley.work@gmail.com, eparis@parisplace.org, casey@schaufler-ca.com Cc: linux-kernel@vger.kernel.org, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, bpf@vger.kernel.org, kpsingh@kernel.org, keescook@chromium.org, nicolas.bouchinet@clip-os.org, Roberto Sassu References: <20230603191518.1397490-1-roberto.sassu@huaweicloud.com> <20230603191518.1397490-3-roberto.sassu@huaweicloud.com> <5f8d8e67a7803ede8847d30ffe3204723b4ac7a7.camel@linux.ibm.com> From: Roberto Sassu In-Reply-To: <5f8d8e67a7803ede8847d30ffe3204723b4ac7a7.camel@linux.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CM-TRANSID: GxC2BwCXPjXBIYRk43ofAw--.4331S2 X-Coremail-Antispam: 1UD129KBjvJXoW3Gw43ZrW3Xr1rAw4xCF47CFg_yoW7ZFWrpF WUt3W3KFs5tFnrGrWSyF45W34S9a1rKr4UWr9xWrZav3ZrJw1xKFWvqr1Y9F17Xrykuw1F qF4jqry3XFn0y37anT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvIb4IE77IF4wAFF20E14v26ryj6rWUM7CY07I20VC2zVCF04k2 6cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4 vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7Cj xVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv67AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxV AFwI0_Gr0_Gr1UM2AIxVAIcxkEcVAq07x20xvEncxIr21l5I8CrVACY4xI64kE6c02F40E x7xfMcIj6xIIjxv20xvE14v26r1Y6r17McIj6I8E87Iv67AKxVWUJVW8JwAm72CE4IkC6x 0Yz7v_Jr0_Gr1lF7xvr2IY64vIr41lFIxGxcIEc7CjxVA2Y2ka0xkIwI1lc7I2V7IY0VAS 07AlzVAYIcxG8wCF04k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8JwC20s026c 02F40E14v26r1j6r18MI8I3I0E7480Y4vE14v26r106r1rMI8E67AF67kF1VAFwI0_GFv_ WrylIxkGc2Ij64vIr41lIxAIcVC0I7IYx2IY67AKxVWUJVWUCwCI42IY6xIIjxv20xvEc7 CjxVAFwI0_Gr0_Cr1lIxAIcVCF04k26cxKx2IYs7xG6rWUJVWrZr1UMIIF0xvEx4A2jsIE 14v26r1j6r4UMIIF0xvEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UYxBIdaVFxhVjvjDU0xZFpf 9x07UWwZcUUUUU= X-CM-SenderInfo: purev21wro2thvvxqx5xdzvxpfor3voofrz/1tbiAQAEBF1jj45+QAAAsA X-CFilter-Loop: Reflected Precedence: bulk List-ID: On 6/9/2023 9:35 PM, Mimi Zohar wrote: > Hi Roberto, > > On Sat, 2023-06-03 at 21:15 +0200, Roberto Sassu wrote: >> From: Roberto Sassu >> >> With the newly added ability of LSMs to supply multiple xattrs, set >> SMACK64TRASMUTE in smack_inode_init_security(), instead of d_instantiate(). >> Do it by incrementing SMACK_INODE_INIT_XATTRS to 2 and by calling >> lsm_get_xattr_slot() a second time, if the transmuting conditions are met. >> >> The LSM infrastructure passes all xattrs provided by LSMs to the >> filesystems through the initxattrs() callback, so that filesystems can >> store xattrs in the disk. >> >> After the change, the SMK_INODE_TRANSMUTE inode flag is always set by >> d_instantiate() after fetching SMACK64TRANSMUTE from the disk. Before it >> was done by smack_inode_post_setxattr() as result of the __vfs_setxattr() >> call. >> >> Removing __vfs_setxattr() also prevents invalidating the EVM HMAC, by >> adding a new xattr without checking and updating the existing HMAC. >> >> Signed-off-by: Roberto Sassu > > Just a few comments/nits inline. > >> --- >> security/smack/smack.h | 2 +- >> security/smack/smack_lsm.c | 43 +++++++++++++++++++++++--------------- >> 2 files changed, 27 insertions(+), 18 deletions(-) >> >> diff --git a/security/smack/smack.h b/security/smack/smack.h >> index aa15ff56ed6..041688e5a77 100644 >> --- a/security/smack/smack.h >> +++ b/security/smack/smack.h >> @@ -128,7 +128,7 @@ struct task_smack { >> >> #define SMK_INODE_INSTANT 0x01 /* inode is instantiated */ >> #define SMK_INODE_TRANSMUTE 0x02 /* directory is transmuting */ >> -#define SMK_INODE_CHANGED 0x04 /* smack was transmuted */ >> +#define SMK_INODE_CHANGED 0x04 /* smack was transmuted (unused) */ >> #define SMK_INODE_IMPURE 0x08 /* involved in an impure transaction */ >> >> /* >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index a1c30275692..b67d901ee74 100644 >> --- a/security/smack/smack_lsm.c >> +++ b/security/smack/smack_lsm.c >> @@ -52,7 +52,14 @@ >> #define SMK_RECEIVING 1 >> #define SMK_SENDING 2 >> >> -#define SMACK_INODE_INIT_XATTRS 1 >> +/* >> + * Smack uses multiple xattrs. >> + * SMACK64 - for access control, >> + * SMACK64TRANSMUTE - label initialization, >> + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, >> + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP >> + */ >> +#define SMACK_INODE_INIT_XATTRS 2 >> >> #ifdef SMACK_IPV6_PORT_LABELING >> static DEFINE_MUTEX(smack_ipv6_lock); >> @@ -935,7 +942,6 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >> struct xattr *xattrs, int *xattr_count) >> { >> struct task_smack *tsp = smack_cred(current_cred()); >> - struct inode_smack *issp = smack_inode(inode); >> struct smack_known *skp = smk_of_task(tsp); >> struct smack_known *isp = smk_of_inode(inode); >> struct smack_known *dsp = smk_of_inode(dir); >> @@ -963,6 +969,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >> if ((tsp->smk_task == tsp->smk_transmuted) || >> (may > 0 && ((may & MAY_TRANSMUTE) != 0) && >> smk_inode_transmutable(dir))) { >> + struct xattr *xattr_transmute; >> + > > Variables should be defined at the beginning of the function. Casey asked to declare the variable in this block. > Is there a reason for beginning the function with "if (xattr) {" > instead "if (!xattr) return 0;". This causes unnecessary indenting. I revisited this part and made few fixes: https://lore.kernel.org/linux-security-module/20230607123612.2791303-1-roberto.sassu@huaweicloud.com/ Patch 3 should address your concern. >> /* >> * The caller of smack_dentry_create_files_as() >> * should have overridden the current cred, so the >> @@ -971,7 +979,16 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >> */ >> if (tsp->smk_task != tsp->smk_transmuted) >> isp = dsp; >> - issp->smk_flags |= SMK_INODE_CHANGED; >> + xattr_transmute = lsm_get_xattr_slot(xattrs, xattr_count); >> + if (xattr_transmute) { >> + xattr_transmute->value = kmemdup(TRANS_TRUE, >> + TRANS_TRUE_SIZE, GFP_NOFS); > > script/checkpatch --strict complains here. Thanks, I didn't know about it. It seems that they are more stylistic things. Probably, not worth to respin the patch set just for those (unless you prefer I do it). Thanks Roberto >> + if (xattr_transmute->value == NULL) >> + return -ENOMEM; >> + >> + xattr_transmute->value_len = TRANS_TRUE_SIZE; >> + xattr_transmute->name = XATTR_SMACK_TRANSMUTE; >> + } >> } >> >> xattr->value = kstrdup(isp->smk_known, GFP_NOFS); >> @@ -3518,20 +3535,12 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode) >> * If there is a transmute attribute on the >> * directory mark the inode. >> */ >> - if (isp->smk_flags & SMK_INODE_CHANGED) { >> - isp->smk_flags &= ~SMK_INODE_CHANGED; >> - rc = __vfs_setxattr(&nop_mnt_idmap, dp, inode, >> - XATTR_NAME_SMACKTRANSMUTE, >> - TRANS_TRUE, TRANS_TRUE_SIZE, >> - 0); >> - } else { >> - rc = __vfs_getxattr(dp, inode, >> - XATTR_NAME_SMACKTRANSMUTE, trattr, >> - TRANS_TRUE_SIZE); >> - if (rc >= 0 && strncmp(trattr, TRANS_TRUE, >> - TRANS_TRUE_SIZE) != 0) >> - rc = -EINVAL; >> - } >> + rc = __vfs_getxattr(dp, inode, >> + XATTR_NAME_SMACKTRANSMUTE, trattr, >> + TRANS_TRUE_SIZE); >> + if (rc >= 0 && strncmp(trattr, TRANS_TRUE, >> + TRANS_TRUE_SIZE) != 0) >> + rc = -EINVAL; >> if (rc >= 0) >> transflag = SMK_INODE_TRANSMUTE; >> } > >