Message ID | 20230603191518.1397490-3-roberto.sassu@huaweicloud.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp1811691vqr; Sat, 3 Jun 2023 12:21:15 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4XGIKe9wE3ALem3UoynoLyEqDzZ9mJPrPWGFbUfjXlxUYgu5R6LFHHbRp0u7yYmkeL/KCw X-Received: by 2002:a17:902:d355:b0:1ac:482e:ed4d with SMTP id l21-20020a170902d35500b001ac482eed4dmr2291131plk.18.1685820075328; Sat, 03 Jun 2023 12:21:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685820075; cv=none; d=google.com; s=arc-20160816; b=USCczXh/HocUkw6Rp65liLP/aUwfp7Dj8HHbRU20nJ7IDTJCi3+9A6ldt6by0f1BXd m1BW6qq0tlxJfvAq26gSFFjnkfN9jo0iPPAtpquDi1fcB8QfMJWrE0BUwqk1h6sp3Ept gqMF59MPXYOlrVAKdpFJ+RIfK4qjBsw3D2qRl6y8CUyHpF+FBmYmZR0NU+Rj9AoU27bo a8Vz/gpCoe+0k70FO/64O/updcqhvsHydw9mV4HkiKg715p22N1+VnihVA2c0Me//4tO jtOkEzBQVnxweCfCV05NGqn0A2FnWLSkh39tSYaRgHW8csDjN7R7t7KM9cUL5yWCHK/U V9aw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=/xyzE4HkNRxrDfT4TrDBF454r5Pty2WwGqabXCGK1qw=; b=KSKPKGXb1/aWslykCU1HuRxZwGP3VN0QDlc1loXvY19sQ9cbrGrezGlzZS+r2BiiOb zJqf9TJ3ktagJSgYhMeOOfZf5xtIRipR3zIvBH+AQoolZXIePPXdJYAwNkLmQumgbbJv 1is7hJH+x2wC2vpqhEFDt6T+/X37gJWQM6w9He/dktIxEwqITK/JEmYzq3PdqZ4FNOtv fVsiGQMbTDY+F7HFVrfWulF1NOfVCW5KnYzPnjSQtjdS06O/w0ZaULamwAylNmHW3xOv uXNPaGeFs1nbforzKtAsw1hAOYhpJoZGzSaXLNV9U7/rmOOtvgKld2LfZD0dJt6CRLt8 ao2g== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id e1-20020a170902744100b001a64fa4eb27si2989943plt.230.2023.06.03.12.21.03; Sat, 03 Jun 2023 12:21:15 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229897AbjFCTQt (ORCPT <rfc822;stefanalexe802@gmail.com> + 99 others); Sat, 3 Jun 2023 15:16:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40818 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229879AbjFCTQd (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 3 Jun 2023 15:16:33 -0400 Received: from frasgout12.his.huawei.com (unknown [14.137.139.154]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0EAA91AD; Sat, 3 Jun 2023 12:16:29 -0700 (PDT) Received: from mail02.huawei.com (unknown [172.18.147.229]) by frasgout12.his.huawei.com (SkyGuard) with ESMTP id 4QYTnh3RZ5z9v7Yc; Sun, 4 Jun 2023 03:04:40 +0800 (CST) Received: from huaweicloud.com (unknown [10.204.63.22]) by APP1 (Coremail) with SMTP id LxC2BwCnCuZXkXtkAEoJAw--.3607S4; Sat, 03 Jun 2023 20:16:05 +0100 (CET) From: Roberto Sassu <roberto.sassu@huaweicloud.com> To: zohar@linux.ibm.com, 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 <roberto.sassu@huawei.com> Subject: [PATCH v11 2/4] smack: Set the SMACK64TRANSMUTE xattr in smack_inode_init_security() Date: Sat, 3 Jun 2023 21:15:16 +0200 Message-Id: <20230603191518.1397490-3-roberto.sassu@huaweicloud.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230603191518.1397490-1-roberto.sassu@huaweicloud.com> References: <20230603191518.1397490-1-roberto.sassu@huaweicloud.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: LxC2BwCnCuZXkXtkAEoJAw--.3607S4 X-Coremail-Antispam: 1UD129KBjvJXoWxAr4UCr17KFyDurW8Kw43GFg_yoWruF48pF WUK3ZxKrs5tF1DWrWFyF4UW3yaka1rGrWUWr9xWrsav3ZrXw1xKFWkXr1YkF17Xrykur9Y qF4jqry5XFn0y37anT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUBFb4IE77IF4wAFF20E14v26rWj6s0DM7CY07I20VC2zVCF04k2 6cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28IrcIa0xkI8VA2jI8067AKxVWUXw A2048vs2IY020Ec7CjxVAFwI0_Xr0E3s1l8cAvFVAK0II2c7xJM28CjxkF64kEwVA0rcxS w2x7M28EF7xvwVC0I7IYx2IY67AKxVWUJVWUCwA2z4x0Y4vE2Ix0cI8IcVCY1x0267AKxV WxJVW8Jr1l84ACjcxK6I8E87Iv67AKxVW8JVWxJwA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_ Gr1j6F4UJwAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG6I80ew Av7VC0I7IYx2IY67AKxVWUJVWUGwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFVCjc4AY 6r1j6r4UM4x0Y48IcxkI7VAKI48JM4IIrI8v6xkF7I0E8cxan2IY04v7MxAIw28IcxkI7V AKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI8I3I0E5I8CrVAFwI0_Jr0_Jr4lx2IqxVCj r7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVW8ZVWrXwCIc40Y0x0EwIxGrwCI42IY6x IIjxv20xvE14v26r1j6r1xMIIF0xvE2Ix0cI8IcVCY1x0267AKxVWxJVW8Jr1lIxAIcVCF 04k26cxKx2IYs7xG6r1j6r1xMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2jsIEc7 CjxVAFwI0_Gr0_Gr1UYxBIdaVFxhVjvjDU0xZFpf9x07UC9aPUUUUU= X-CM-SenderInfo: purev21wro2thvvxqx5xdzvxpfor3voofrz/1tbiAQARBF1jj44QwwAAs4 X-CFilter-Loop: Reflected X-Spam-Status: No, score=-0.5 required=5.0 tests=BAYES_00,KHOP_HELO_FCRDNS, PDS_RDNS_DYNAMIC_FP,RCVD_IN_MSPIKE_BL,RCVD_IN_MSPIKE_L3,RDNS_DYNAMIC, SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1767710471180825623?= X-GMAIL-MSGID: =?utf-8?q?1767710471180825623?= |
Series |
evm: Do HMAC of multiple per LSM xattrs for new inodes
|
|
Commit Message
Roberto Sassu
June 3, 2023, 7:15 p.m. UTC
From: Roberto Sassu <roberto.sassu@huawei.com> 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 <roberto.sassu@huawei.com> --- security/smack/smack.h | 2 +- security/smack/smack_lsm.c | 43 +++++++++++++++++++++++--------------- 2 files changed, 27 insertions(+), 18 deletions(-)
Comments
On Sat, 2023-06-03 at 21:15 +0200, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > 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. Hi Mengchi could you please redo your tests with this patch set applied? https://lore.kernel.org/linux-integrity/20230603191518.1397490-1-roberto.sassu@huaweicloud.com/ You need: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git/log/?h=next https://github.com/cschaufler/smack-next/commits/next Thanks Roberto > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > 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; > + > /* > * 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); > + 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; > }
On Mon Jun 5, 2023 at 11:38 AM EEST, Roberto Sassu wrote: > On Sat, 2023-06-03 at 21:15 +0200, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > With the newly added ability of LSMs to supply multiple xattrs, set > > SMACK64TRASMUTE in smack_inode_init_security(), instead of d_instantiate(). nit: TRANSMUTE Sorry, just hit into my eye. I skimmed it because I implemented original feature :-) BR, Jarkko
Hi Roberto, On Sat, 2023-06-03 at 21:15 +0200, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > 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 <roberto.sassu@huawei.com> 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. Is there a reason for beginning the function with "if (xattr) {" instead "if (!xattr) return 0;". This causes unnecessary indenting. > /* > * 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. > + 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; > }
On 6/9/2023 9:26 AM, Jarkko Sakkinen wrote: > On Mon Jun 5, 2023 at 11:38 AM EEST, Roberto Sassu wrote: >> On Sat, 2023-06-03 at 21:15 +0200, Roberto Sassu wrote: >>> From: Roberto Sassu <roberto.sassu@huawei.com> >>> >>> With the newly added ability of LSMs to supply multiple xattrs, set >>> SMACK64TRASMUTE in smack_inode_init_security(), instead of d_instantiate(). > > nit: TRANSMUTE > > Sorry, just hit into my eye. I skimmed it because I implemented original > feature :-) Cool! Currently the transmute xattr is defined as: #define XATTR_SMACK_TRANSMUTE "SMACK64TRANSMUTE" so, should be good to say the full xattr name, right? Thanks Roberto
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 <roberto.sassu@huawei.com> >> >> 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 <roberto.sassu@huawei.com> > > 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; >> } > >
On Mon, 2023-06-05 08:38:29 +0000, Roberto Sassu wrote: > > On Sat, 2023-06-03 at 21:15 +0200, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > 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. > > Hi Mengchi > > could you please redo your tests with this patch set applied? > > https://lore.kernel.org/linux-integrity/20230603191518.1397490-1-roberto.sassu@huaweicloud.com/ > > You need: > > https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git/log/?h=next > > https://github.com/cschaufler/smack-next/commits/next > > Thanks > > Roberto Sorry for the later reply. It turned out lsm.git repo needs your previous two overlay fs fixes before applying these four patches. With v12 I did not see the issue I reported anymore. Best, Mengchi > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > 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; > > + > > /* > > * 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); > > + 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; > > } > >
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; + /* * 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); + 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; }