| Message ID | 20230411172337.340518-1-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Smack modifications for: security: Allow all LSMs to provide xattrs for inode_init_security hook | expand |
On 4/11/2023 10:23 AM, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Very very quick modification. Not tested. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > security/smack/smack.h | 2 +- > security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ > 2 files changed, 23 insertions(+), 21 deletions(-) > > diff --git a/security/smack/smack.h b/security/smack/smack.h > index e2239be7bd6..f00c8498c60 100644 > --- a/security/smack/smack.h > +++ b/security/smack/smack.h > @@ -127,7 +127,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) */ See below ... > #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 8392983334b..b43820bdbd0 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -54,12 +54,12 @@ > > /* > * Smack uses multiple xattrs. > - * SMACK64 - for access control, SMACK64EXEC - label for the program, > - * SMACK64MMAP - controls library loading, > + * SMACK64 - for access control, > * SMACK64TRANSMUTE - label initialization, > - * Not saved on files - SMACK64IPIN and SMACK64IPOUT > + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, > + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP > */ > -#define SMACK_INODE_INIT_XATTRS 4 > +#define SMACK_INODE_INIT_XATTRS 2 > > #ifdef SMACK_IPV6_PORT_LABELING > static DEFINE_MUTEX(smack_ipv6_lock); > @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > const struct qstr *qstr, > struct xattr *xattrs, int *xattr_count) > { > - struct inode_smack *issp = smack_inode(inode); > struct smack_known *skp = smk_of_current(); > struct smack_known *isp = smk_of_inode(inode); > struct smack_known *dsp = smk_of_inode(dir); > struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); > + struct xattr *xattr2; I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. It also looks like it could move to be declared in the if clause. > int may; > > if (xattr) { > @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > smk_inode_transmutable(dir)) { > isp = dsp; > - issp->smk_flags |= SMK_INODE_CHANGED; I think you need to keep this. More below. > + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); > + if (xattr2) { > + xattr2->value = kmemdup(TRANS_TRUE, > + TRANS_TRUE_SIZE, > + GFP_NOFS); > + if (xattr2->value == NULL) > + return -ENOMEM; > + > + xattr2->value_len = TRANS_TRUE_SIZE; > + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; > + } > } > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > @@ -3512,20 +3522,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; Where is the SMACK64_TRANSMUTE attribute going to get set on the file? It's not going to get set in smack_init_inode_security(). The inode will know it's transmuting, but it won't get to disk without the __vfs_setxattr() here in smack_d_instantiate(). Now, it's been a long time since that code was written, so I could be wrong, but I'm pretty sure about that. I think that you should be fine with the changes in smack_init_inode_security(), and leaving smack_d_instantiate() untouched. > if (rc >= 0) > transflag = SMK_INODE_TRANSMUTE; > }
On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote: > On 4/11/2023 10:23 AM, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > Very very quick modification. Not tested. > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > security/smack/smack.h | 2 +- > > security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ > > 2 files changed, 23 insertions(+), 21 deletions(-) > > > > diff --git a/security/smack/smack.h b/security/smack/smack.h > > index e2239be7bd6..f00c8498c60 100644 > > --- a/security/smack/smack.h > > +++ b/security/smack/smack.h > > @@ -127,7 +127,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) */ > > See below ... > > > #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 8392983334b..b43820bdbd0 100644 > > --- a/security/smack/smack_lsm.c > > +++ b/security/smack/smack_lsm.c > > @@ -54,12 +54,12 @@ > > > > /* > > * Smack uses multiple xattrs. > > - * SMACK64 - for access control, SMACK64EXEC - label for the program, > > - * SMACK64MMAP - controls library loading, > > + * SMACK64 - for access control, > > * SMACK64TRANSMUTE - label initialization, > > - * Not saved on files - SMACK64IPIN and SMACK64IPOUT > > + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, > > + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP > > */ > > -#define SMACK_INODE_INIT_XATTRS 4 > > +#define SMACK_INODE_INIT_XATTRS 2 > > > > #ifdef SMACK_IPV6_PORT_LABELING > > static DEFINE_MUTEX(smack_ipv6_lock); > > @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > const struct qstr *qstr, > > struct xattr *xattrs, int *xattr_count) > > { > > - struct inode_smack *issp = smack_inode(inode); > > struct smack_known *skp = smk_of_current(); > > struct smack_known *isp = smk_of_inode(inode); > > struct smack_known *dsp = smk_of_inode(dir); > > struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); > > + struct xattr *xattr2; > > I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. > It also looks like it could move to be declared in the if clause. > > > int may; > > > > if (xattr) { > > @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > > smk_inode_transmutable(dir)) { > > isp = dsp; > > - issp->smk_flags |= SMK_INODE_CHANGED; > > I think you need to keep this. More below. > > > + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); > > + if (xattr2) { > > + xattr2->value = kmemdup(TRANS_TRUE, > > + TRANS_TRUE_SIZE, > > + GFP_NOFS); > > + if (xattr2->value == NULL) > > + return -ENOMEM; > > + > > + xattr2->value_len = TRANS_TRUE_SIZE; > > + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; > > + } > > } > > > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > @@ -3512,20 +3522,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; > > Where is the SMACK64_TRANSMUTE attribute going to get set on the file? > It's not going to get set in smack_init_inode_security(). The inode will Isn't that the purpose of the inode_init_security hook? After all LSMs provide one or multiple xattrs, xattrs are going to be written to the disk with the initxattr() callback of filesystems. There is a small mistake above (XATTR_SMACK_TRANSMUTE instead of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix). After fixing that, Smack tests succeed: 95 Passed, 0 Failed, 100% Success rate There was a test failing in dir-transmute.sh, before I fixed the xattr name. Thanks Roberto > know it's transmuting, but it won't get to disk without the __vfs_setxattr() > here in smack_d_instantiate(). Now, it's been a long time since that code > was written, so I could be wrong, but I'm pretty sure about that. > > I think that you should be fine with the changes in smack_init_inode_security(), > and leaving smack_d_instantiate() untouched. > > > if (rc >= 0) > > transflag = SMK_INODE_TRANSMUTE; > > }
On 4/12/2023 12:22 AM, Roberto Sassu wrote: > On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote: >> On 4/11/2023 10:23 AM, Roberto Sassu wrote: >>> From: Roberto Sassu <roberto.sassu@huawei.com> >>> >>> Very very quick modification. Not tested. >>> >>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >>> --- >>> security/smack/smack.h | 2 +- >>> security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ >>> 2 files changed, 23 insertions(+), 21 deletions(-) >>> >>> diff --git a/security/smack/smack.h b/security/smack/smack.h >>> index e2239be7bd6..f00c8498c60 100644 >>> --- a/security/smack/smack.h >>> +++ b/security/smack/smack.h >>> @@ -127,7 +127,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) */ >> See below ... >> >>> #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 8392983334b..b43820bdbd0 100644 >>> --- a/security/smack/smack_lsm.c >>> +++ b/security/smack/smack_lsm.c >>> @@ -54,12 +54,12 @@ >>> >>> /* >>> * Smack uses multiple xattrs. >>> - * SMACK64 - for access control, SMACK64EXEC - label for the program, >>> - * SMACK64MMAP - controls library loading, >>> + * SMACK64 - for access control, >>> * SMACK64TRANSMUTE - label initialization, >>> - * Not saved on files - SMACK64IPIN and SMACK64IPOUT >>> + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, >>> + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP >>> */ >>> -#define SMACK_INODE_INIT_XATTRS 4 >>> +#define SMACK_INODE_INIT_XATTRS 2 >>> >>> #ifdef SMACK_IPV6_PORT_LABELING >>> static DEFINE_MUTEX(smack_ipv6_lock); >>> @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >>> const struct qstr *qstr, >>> struct xattr *xattrs, int *xattr_count) >>> { >>> - struct inode_smack *issp = smack_inode(inode); >>> struct smack_known *skp = smk_of_current(); >>> struct smack_known *isp = smk_of_inode(inode); >>> struct smack_known *dsp = smk_of_inode(dir); >>> struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); >>> + struct xattr *xattr2; >> I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. >> It also looks like it could move to be declared in the if clause. >> >>> int may; >>> >>> if (xattr) { >>> @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >>> if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && >>> smk_inode_transmutable(dir)) { >>> isp = dsp; >>> - issp->smk_flags |= SMK_INODE_CHANGED; >> I think you need to keep this. More below. >> >>> + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); >>> + if (xattr2) { >>> + xattr2->value = kmemdup(TRANS_TRUE, >>> + TRANS_TRUE_SIZE, >>> + GFP_NOFS); >>> + if (xattr2->value == NULL) >>> + return -ENOMEM; >>> + >>> + xattr2->value_len = TRANS_TRUE_SIZE; >>> + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; >>> + } >>> } >>> >>> xattr->value = kstrdup(isp->smk_known, GFP_NOFS); >>> @@ -3512,20 +3522,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; >> Where is the SMACK64_TRANSMUTE attribute going to get set on the file? >> It's not going to get set in smack_init_inode_security(). The inode will > Isn't that the purpose of the inode_init_security hook? No. It initializes the in-memory inode. > After all LSMs provide one or multiple xattrs, xattrs are going to be > written to the disk with the initxattr() callback of filesystems. > > There is a small mistake above (XATTR_SMACK_TRANSMUTE instead > of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix). > > After fixing that, Smack tests succeed: It's over a decade since I seriously looked at this code path, but I'm pretty sure the __vfs_setxattr() call is necessary to get the attribute written out. With your change the in-memory inode will get the attribute, but if you reboot it won't be on the directory. > > 95 Passed, 0 Failed, 100% Success rate > > There was a test failing in dir-transmute.sh, before I fixed the xattr > name. > > Thanks > > Roberto > >> know it's transmuting, but it won't get to disk without the __vfs_setxattr() >> here in smack_d_instantiate(). Now, it's been a long time since that code >> was written, so I could be wrong, but I'm pretty sure about that. >> >> I think that you should be fine with the changes in smack_init_inode_security(), >> and leaving smack_d_instantiate() untouched. >> >>> if (rc >= 0) >>> transflag = SMK_INODE_TRANSMUTE; >>> }
On Wed, 2023-04-12 at 13:29 -0700, Casey Schaufler wrote: > On 4/12/2023 12:22 AM, Roberto Sassu wrote: > > On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote: > > > On 4/11/2023 10:23 AM, Roberto Sassu wrote: > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > Very very quick modification. Not tested. > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > --- > > > > security/smack/smack.h | 2 +- > > > > security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ > > > > 2 files changed, 23 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/security/smack/smack.h b/security/smack/smack.h > > > > index e2239be7bd6..f00c8498c60 100644 > > > > --- a/security/smack/smack.h > > > > +++ b/security/smack/smack.h > > > > @@ -127,7 +127,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) */ > > > See below ... > > > > > > > #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 8392983334b..b43820bdbd0 100644 > > > > --- a/security/smack/smack_lsm.c > > > > +++ b/security/smack/smack_lsm.c > > > > @@ -54,12 +54,12 @@ > > > > > > > > /* > > > > * Smack uses multiple xattrs. > > > > - * SMACK64 - for access control, SMACK64EXEC - label for the program, > > > > - * SMACK64MMAP - controls library loading, > > > > + * SMACK64 - for access control, > > > > * SMACK64TRANSMUTE - label initialization, > > > > - * Not saved on files - SMACK64IPIN and SMACK64IPOUT > > > > + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, > > > > + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP > > > > */ > > > > -#define SMACK_INODE_INIT_XATTRS 4 > > > > +#define SMACK_INODE_INIT_XATTRS 2 > > > > > > > > #ifdef SMACK_IPV6_PORT_LABELING > > > > static DEFINE_MUTEX(smack_ipv6_lock); > > > > @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > > > const struct qstr *qstr, > > > > struct xattr *xattrs, int *xattr_count) > > > > { > > > > - struct inode_smack *issp = smack_inode(inode); > > > > struct smack_known *skp = smk_of_current(); > > > > struct smack_known *isp = smk_of_inode(inode); > > > > struct smack_known *dsp = smk_of_inode(dir); > > > > struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); > > > > + struct xattr *xattr2; > > > I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. > > > It also looks like it could move to be declared in the if clause. > > > > > > > int may; > > > > > > > > if (xattr) { > > > > @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > > > > smk_inode_transmutable(dir)) { > > > > isp = dsp; > > > > - issp->smk_flags |= SMK_INODE_CHANGED; > > > I think you need to keep this. More below. > > > > > > > + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); > > > > + if (xattr2) { > > > > + xattr2->value = kmemdup(TRANS_TRUE, > > > > + TRANS_TRUE_SIZE, > > > > + GFP_NOFS); > > > > + if (xattr2->value == NULL) > > > > + return -ENOMEM; > > > > + > > > > + xattr2->value_len = TRANS_TRUE_SIZE; > > > > + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; > > > > + } > > > > } > > > > > > > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > > > @@ -3512,20 +3522,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; > > > Where is the SMACK64_TRANSMUTE attribute going to get set on the file? > > > It's not going to get set in smack_init_inode_security(). The inode will > > Isn't that the purpose of the inode_init_security hook? > > No. It initializes the in-memory inode. I hope I'm not mistaken here... I make a small example. Filesystems call security_inode_init_security(). Ext4 does: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n54 security_inode_init_security() allocates new_xattrs. Each LSM fills new_xattrs. At the end of the loop, if there is at least one xattr filled, the initxattrs() callback passed by the caller of security_inode_init_security() is called. The ext4 initxattrs() callback is: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n35 which scans the xattr array and, for each xattr, calls ext4_xattr_set_handle(). Maybe I'm overlooking it, but ext4_xattr_set_handle() is setting xattrs on the disk. Am I wrong? Thanks Roberto > > After all LSMs provide one or multiple xattrs, xattrs are going to be > > written to the disk with the initxattr() callback of filesystems. > > > > There is a small mistake above (XATTR_SMACK_TRANSMUTE instead > > of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix). > > > > After fixing that, Smack tests succeed: > > It's over a decade since I seriously looked at this code path, > but I'm pretty sure the __vfs_setxattr() call is necessary to get > the attribute written out. With your change the in-memory inode will > get the attribute, but if you reboot it won't be on the directory. > > > 95 Passed, 0 Failed, 100% Success rate > > > > There was a test failing in dir-transmute.sh, before I fixed the xattr > > name. > > > > Thanks > > > > Roberto > > > > > know it's transmuting, but it won't get to disk without the __vfs_setxattr() > > > here in smack_d_instantiate(). Now, it's been a long time since that code > > > was written, so I could be wrong, but I'm pretty sure about that. > > > > > > I think that you should be fine with the changes in smack_init_inode_security(), > > > and leaving smack_d_instantiate() untouched. > > > > > > > if (rc >= 0) > > > > transflag = SMK_INODE_TRANSMUTE; > > > > }
On 4/13/2023 12:11 AM, Roberto Sassu wrote: > On Wed, 2023-04-12 at 13:29 -0700, Casey Schaufler wrote: >> On 4/12/2023 12:22 AM, Roberto Sassu wrote: >>> On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote: >>>> On 4/11/2023 10:23 AM, Roberto Sassu wrote: >>>>> From: Roberto Sassu <roberto.sassu@huawei.com> >>>>> >>>>> Very very quick modification. Not tested. >>>>> >>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >>>>> --- >>>>> security/smack/smack.h | 2 +- >>>>> security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ >>>>> 2 files changed, 23 insertions(+), 21 deletions(-) >>>>> >>>>> diff --git a/security/smack/smack.h b/security/smack/smack.h >>>>> index e2239be7bd6..f00c8498c60 100644 >>>>> --- a/security/smack/smack.h >>>>> +++ b/security/smack/smack.h >>>>> @@ -127,7 +127,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) */ >>>> See below ... >>>> >>>>> #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 8392983334b..b43820bdbd0 100644 >>>>> --- a/security/smack/smack_lsm.c >>>>> +++ b/security/smack/smack_lsm.c >>>>> @@ -54,12 +54,12 @@ >>>>> >>>>> /* >>>>> * Smack uses multiple xattrs. >>>>> - * SMACK64 - for access control, SMACK64EXEC - label for the program, >>>>> - * SMACK64MMAP - controls library loading, >>>>> + * SMACK64 - for access control, >>>>> * SMACK64TRANSMUTE - label initialization, >>>>> - * Not saved on files - SMACK64IPIN and SMACK64IPOUT >>>>> + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, >>>>> + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP >>>>> */ >>>>> -#define SMACK_INODE_INIT_XATTRS 4 >>>>> +#define SMACK_INODE_INIT_XATTRS 2 >>>>> >>>>> #ifdef SMACK_IPV6_PORT_LABELING >>>>> static DEFINE_MUTEX(smack_ipv6_lock); >>>>> @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >>>>> const struct qstr *qstr, >>>>> struct xattr *xattrs, int *xattr_count) >>>>> { >>>>> - struct inode_smack *issp = smack_inode(inode); >>>>> struct smack_known *skp = smk_of_current(); >>>>> struct smack_known *isp = smk_of_inode(inode); >>>>> struct smack_known *dsp = smk_of_inode(dir); >>>>> struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); >>>>> + struct xattr *xattr2; >>>> I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. >>>> It also looks like it could move to be declared in the if clause. >>>> >>>>> int may; >>>>> >>>>> if (xattr) { >>>>> @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >>>>> if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && >>>>> smk_inode_transmutable(dir)) { >>>>> isp = dsp; >>>>> - issp->smk_flags |= SMK_INODE_CHANGED; >>>> I think you need to keep this. More below. >>>> >>>>> + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); >>>>> + if (xattr2) { >>>>> + xattr2->value = kmemdup(TRANS_TRUE, >>>>> + TRANS_TRUE_SIZE, >>>>> + GFP_NOFS); >>>>> + if (xattr2->value == NULL) >>>>> + return -ENOMEM; >>>>> + >>>>> + xattr2->value_len = TRANS_TRUE_SIZE; >>>>> + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; >>>>> + } >>>>> } >>>>> >>>>> xattr->value = kstrdup(isp->smk_known, GFP_NOFS); >>>>> @@ -3512,20 +3522,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; >>>> Where is the SMACK64_TRANSMUTE attribute going to get set on the file? >>>> It's not going to get set in smack_init_inode_security(). The inode will >>> Isn't that the purpose of the inode_init_security hook? >> No. It initializes the in-memory inode. > I hope I'm not mistaken here... > > I make a small example. Filesystems call > security_inode_init_security(). Ext4 does: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n54 > > security_inode_init_security() allocates new_xattrs. Each LSM fills > new_xattrs. At the end of the loop, if there is at least one xattr > filled, the initxattrs() callback passed by the caller of > security_inode_init_security() is called. > > The ext4 initxattrs() callback is: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n35 > > which scans the xattr array and, for each xattr, > calls ext4_xattr_set_handle(). > > Maybe I'm overlooking it, but ext4_xattr_set_handle() is setting xattrs > on the disk. Am I wrong? Yes, you're wrong. I tried your change, and the SMACK64_TRANSMUTE isn't set on the sub-directory when it's created. The __vfs_setxattr() call really is necessary. > > Thanks > > Roberto > >>> After all LSMs provide one or multiple xattrs, xattrs are going to be >>> written to the disk with the initxattr() callback of filesystems. >>> >>> There is a small mistake above (XATTR_SMACK_TRANSMUTE instead >>> of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix). >>> >>> After fixing that, Smack tests succeed: >> It's over a decade since I seriously looked at this code path, >> but I'm pretty sure the __vfs_setxattr() call is necessary to get >> the attribute written out. With your change the in-memory inode will >> get the attribute, but if you reboot it won't be on the directory. >> >>> 95 Passed, 0 Failed, 100% Success rate >>> >>> There was a test failing in dir-transmute.sh, before I fixed the xattr >>> name. >>> >>> Thanks >>> >>> Roberto >>> >>>> know it's transmuting, but it won't get to disk without the __vfs_setxattr() >>>> here in smack_d_instantiate(). Now, it's been a long time since that code >>>> was written, so I could be wrong, but I'm pretty sure about that. >>>> >>>> I think that you should be fine with the changes in smack_init_inode_security(), >>>> and leaving smack_d_instantiate() untouched. >>>> >>>>> if (rc >= 0) >>>>> transflag = SMK_INODE_TRANSMUTE; >>>>> }
On Mon, 2023-04-17 at 09:41 -0700, Casey Schaufler wrote: > On 4/13/2023 12:11 AM, Roberto Sassu wrote: > > On Wed, 2023-04-12 at 13:29 -0700, Casey Schaufler wrote: > > > On 4/12/2023 12:22 AM, Roberto Sassu wrote: > > > > On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote: > > > > > On 4/11/2023 10:23 AM, Roberto Sassu wrote: > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > Very very quick modification. Not tested. > > > > > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > --- > > > > > > security/smack/smack.h | 2 +- > > > > > > security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ > > > > > > 2 files changed, 23 insertions(+), 21 deletions(-) > > > > > > > > > > > > diff --git a/security/smack/smack.h b/security/smack/smack.h > > > > > > index e2239be7bd6..f00c8498c60 100644 > > > > > > --- a/security/smack/smack.h > > > > > > +++ b/security/smack/smack.h > > > > > > @@ -127,7 +127,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) */ > > > > > See below ... > > > > > > > > > > > #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 8392983334b..b43820bdbd0 100644 > > > > > > --- a/security/smack/smack_lsm.c > > > > > > +++ b/security/smack/smack_lsm.c > > > > > > @@ -54,12 +54,12 @@ > > > > > > > > > > > > /* > > > > > > * Smack uses multiple xattrs. > > > > > > - * SMACK64 - for access control, SMACK64EXEC - label for the program, > > > > > > - * SMACK64MMAP - controls library loading, > > > > > > + * SMACK64 - for access control, > > > > > > * SMACK64TRANSMUTE - label initialization, > > > > > > - * Not saved on files - SMACK64IPIN and SMACK64IPOUT > > > > > > + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, > > > > > > + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP > > > > > > */ > > > > > > -#define SMACK_INODE_INIT_XATTRS 4 > > > > > > +#define SMACK_INODE_INIT_XATTRS 2 > > > > > > > > > > > > #ifdef SMACK_IPV6_PORT_LABELING > > > > > > static DEFINE_MUTEX(smack_ipv6_lock); > > > > > > @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > > > > > const struct qstr *qstr, > > > > > > struct xattr *xattrs, int *xattr_count) > > > > > > { > > > > > > - struct inode_smack *issp = smack_inode(inode); > > > > > > struct smack_known *skp = smk_of_current(); > > > > > > struct smack_known *isp = smk_of_inode(inode); > > > > > > struct smack_known *dsp = smk_of_inode(dir); > > > > > > struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); > > > > > > + struct xattr *xattr2; > > > > > I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. > > > > > It also looks like it could move to be declared in the if clause. > > > > > > > > > > > int may; > > > > > > > > > > > > if (xattr) { > > > > > > @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > > > > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > > > > > > smk_inode_transmutable(dir)) { > > > > > > isp = dsp; > > > > > > - issp->smk_flags |= SMK_INODE_CHANGED; > > > > > I think you need to keep this. More below. > > > > > > > > > > > + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); > > > > > > + if (xattr2) { > > > > > > + xattr2->value = kmemdup(TRANS_TRUE, > > > > > > + TRANS_TRUE_SIZE, > > > > > > + GFP_NOFS); > > > > > > + if (xattr2->value == NULL) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + xattr2->value_len = TRANS_TRUE_SIZE; > > > > > > + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; > > > > > > + } > > > > > > } > > > > > > > > > > > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > > > > > @@ -3512,20 +3522,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; > > > > > Where is the SMACK64_TRANSMUTE attribute going to get set on the file? > > > > > It's not going to get set in smack_init_inode_security(). The inode will > > > > Isn't that the purpose of the inode_init_security hook? > > > No. It initializes the in-memory inode. > > I hope I'm not mistaken here... > > > > I make a small example. Filesystems call > > security_inode_init_security(). Ext4 does: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n54 > > > > security_inode_init_security() allocates new_xattrs. Each LSM fills > > new_xattrs. At the end of the loop, if there is at least one xattr > > filled, the initxattrs() callback passed by the caller of > > security_inode_init_security() is called. > > > > The ext4 initxattrs() callback is: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n35 > > > > which scans the xattr array and, for each xattr, > > calls ext4_xattr_set_handle(). > > > > Maybe I'm overlooking it, but ext4_xattr_set_handle() is setting xattrs > > on the disk. Am I wrong? > > Yes, you're wrong. I tried your change, and the SMACK64_TRANSMUTE isn't > set on the sub-directory when it's created. The __vfs_setxattr() call really > is necessary. Could you please also check if there is any change with this fix: Replace: xattr2->name = XATTR_NAME_SMACKTRANSMUTE; with: xattr2->name = XATTR_SMACK_TRANSMUTE; Thanks Roberto > > Thanks > > > > Roberto > > > > > > After all LSMs provide one or multiple xattrs, xattrs are going to be > > > > written to the disk with the initxattr() callback of filesystems. > > > > > > > > There is a small mistake above (XATTR_SMACK_TRANSMUTE instead > > > > of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix). > > > but I'm pretty sure the __vfs_setxattr() call is necessary to get > > > the attribute written out. With your change the in-memory inode will > > > get the attribute, but if you reboot it won't be on the directory. > > > > > > > 95 Passed, 0 Failed, 100% Success rate > > > > > > > > There was a test failing in dir-transmute.sh, before I fixed the xattr > > > > name. > > > > > > > > Thanks > > > > > > > > Roberto > > > > > > > > > know it's transmuting, but it won't get to disk without the __vfs_setxattr() > > > > > here in smack_d_instantiate(). Now, it's been a long time since that code > > > > > was written, so I could be wrong, but I'm pretty sure about that. > > > > > > > > > > I think that you should be fine with the changes in smack_init_inode_security(), > > > > > and leaving smack_d_instantiate() untouched. > > > > > > > > > > > if (rc >= 0) > > > > > > transflag = SMK_INODE_TRANSMUTE; > > > > > > }
On 4/18/2023 12:05 AM, Roberto Sassu wrote: > On Mon, 2023-04-17 at 09:41 -0700, Casey Schaufler wrote: >> On 4/13/2023 12:11 AM, Roberto Sassu wrote: >>> On Wed, 2023-04-12 at 13:29 -0700, Casey Schaufler wrote: >>>> On 4/12/2023 12:22 AM, Roberto Sassu wrote: >>>>> On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote: >>>>>> On 4/11/2023 10:23 AM, Roberto Sassu wrote: >>>>>>> From: Roberto Sassu <roberto.sassu@huawei.com> >>>>>>> >>>>>>> Very very quick modification. Not tested. >>>>>>> >>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >>>>>>> --- >>>>>>> security/smack/smack.h | 2 +- >>>>>>> security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ >>>>>>> 2 files changed, 23 insertions(+), 21 deletions(-) >>>>>>> >>>>>>> diff --git a/security/smack/smack.h b/security/smack/smack.h >>>>>>> index e2239be7bd6..f00c8498c60 100644 >>>>>>> --- a/security/smack/smack.h >>>>>>> +++ b/security/smack/smack.h >>>>>>> @@ -127,7 +127,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) */ >>>>>> See below ... >>>>>> >>>>>>> #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 8392983334b..b43820bdbd0 100644 >>>>>>> --- a/security/smack/smack_lsm.c >>>>>>> +++ b/security/smack/smack_lsm.c >>>>>>> @@ -54,12 +54,12 @@ >>>>>>> >>>>>>> /* >>>>>>> * Smack uses multiple xattrs. >>>>>>> - * SMACK64 - for access control, SMACK64EXEC - label for the program, >>>>>>> - * SMACK64MMAP - controls library loading, >>>>>>> + * SMACK64 - for access control, >>>>>>> * SMACK64TRANSMUTE - label initialization, >>>>>>> - * Not saved on files - SMACK64IPIN and SMACK64IPOUT >>>>>>> + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, >>>>>>> + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP >>>>>>> */ >>>>>>> -#define SMACK_INODE_INIT_XATTRS 4 >>>>>>> +#define SMACK_INODE_INIT_XATTRS 2 >>>>>>> >>>>>>> #ifdef SMACK_IPV6_PORT_LABELING >>>>>>> static DEFINE_MUTEX(smack_ipv6_lock); >>>>>>> @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >>>>>>> const struct qstr *qstr, >>>>>>> struct xattr *xattrs, int *xattr_count) >>>>>>> { >>>>>>> - struct inode_smack *issp = smack_inode(inode); >>>>>>> struct smack_known *skp = smk_of_current(); >>>>>>> struct smack_known *isp = smk_of_inode(inode); >>>>>>> struct smack_known *dsp = smk_of_inode(dir); >>>>>>> struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); >>>>>>> + struct xattr *xattr2; >>>>>> I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. >>>>>> It also looks like it could move to be declared in the if clause. >>>>>> >>>>>>> int may; >>>>>>> >>>>>>> if (xattr) { >>>>>>> @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >>>>>>> if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && >>>>>>> smk_inode_transmutable(dir)) { >>>>>>> isp = dsp; >>>>>>> - issp->smk_flags |= SMK_INODE_CHANGED; >>>>>> I think you need to keep this. More below. >>>>>> >>>>>>> + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); >>>>>>> + if (xattr2) { >>>>>>> + xattr2->value = kmemdup(TRANS_TRUE, >>>>>>> + TRANS_TRUE_SIZE, >>>>>>> + GFP_NOFS); >>>>>>> + if (xattr2->value == NULL) >>>>>>> + return -ENOMEM; >>>>>>> + >>>>>>> + xattr2->value_len = TRANS_TRUE_SIZE; >>>>>>> + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; >>>>>>> + } >>>>>>> } >>>>>>> >>>>>>> xattr->value = kstrdup(isp->smk_known, GFP_NOFS); >>>>>>> @@ -3512,20 +3522,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; >>>>>> Where is the SMACK64_TRANSMUTE attribute going to get set on the file? >>>>>> It's not going to get set in smack_init_inode_security(). The inode will >>>>> Isn't that the purpose of the inode_init_security hook? >>>> No. It initializes the in-memory inode. >>> I hope I'm not mistaken here... >>> >>> I make a small example. Filesystems call >>> security_inode_init_security(). Ext4 does: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n54 >>> >>> security_inode_init_security() allocates new_xattrs. Each LSM fills >>> new_xattrs. At the end of the loop, if there is at least one xattr >>> filled, the initxattrs() callback passed by the caller of >>> security_inode_init_security() is called. >>> >>> The ext4 initxattrs() callback is: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n35 >>> >>> which scans the xattr array and, for each xattr, >>> calls ext4_xattr_set_handle(). >>> >>> Maybe I'm overlooking it, but ext4_xattr_set_handle() is setting xattrs >>> on the disk. Am I wrong? >> Yes, you're wrong. I tried your change, and the SMACK64_TRANSMUTE isn't >> set on the sub-directory when it's created. The __vfs_setxattr() call really >> is necessary. > Could you please also check if there is any change with this fix: > > Replace: > > xattr2->name = XATTR_NAME_SMACKTRANSMUTE; > > with: > > xattr2->name = XATTR_SMACK_TRANSMUTE; > > Thanks Unless I'm missing something really obvious there's no way that any of the evm/ima changes would obviate the need for the __vfs_setxattr() call. It's real easy to verify correct behavior, see the attached script. (you'll want to change the "notroot" value to a user on your system) > > Roberto > >>> Thanks >>> >>> Roberto >>> >>>>> After all LSMs provide one or multiple xattrs, xattrs are going to be >>>>> written to the disk with the initxattr() callback of filesystems. >>>>> >>>>> There is a small mistake above (XATTR_SMACK_TRANSMUTE instead >>>>> of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix). >>>> but I'm pretty sure the __vfs_setxattr() call is necessary to get >>>> the attribute written out. With your change the in-memory inode will >>>> get the attribute, but if you reboot it won't be on the directory. >>>> >>>>> 95 Passed, 0 Failed, 100% Success rate >>>>> >>>>> There was a test failing in dir-transmute.sh, before I fixed the xattr >>>>> name. >>>>> >>>>> Thanks >>>>> >>>>> Roberto >>>>> >>>>>> know it's transmuting, but it won't get to disk without the __vfs_setxattr() >>>>>> here in smack_d_instantiate(). Now, it's been a long time since that code >>>>>> was written, so I could be wrong, but I'm pretty sure about that. >>>>>> >>>>>> I think that you should be fine with the changes in smack_init_inode_security(), >>>>>> and leaving smack_d_instantiate() untouched. >>>>>> >>>>>>> if (rc >= 0) >>>>>>> transflag = SMK_INODE_TRANSMUTE; >>>>>>> } #! /bin/sh # # TESTDIR="/tmp/transmute-test" TESTFILE="$TESTDIR"/file TESTSUBDIR="$TESTDIR"/subdir TESTSUB2DIR="$TESTSUBDIR"/subdir TESTSUB3DIR="$TESTSUB2DIR"/subdir USERID=`/usr/bin/id -u` LOAD=/sys/fs/smackfs/load2 if [ "$USERID" != "0" ] then echo User ID $USERID is not root. exit 1 fi notroot="cschaufler" # NOTROOT=`tail -1 /etc/passwd | sed -e 's/:.*//'` # echo Using '"'$NOTROOT'"' as a test user. RUNAS=`cat /proc/self/attr/current` rm -rf $TESTDIR mkdir $TESTDIR grep `cat /proc/self/attr/current` $LOAD echo Dir: `attr -q -S -g SMACK64 $TESTDIR 2> /dev/null` `ls -id $TESTDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` # # Simple set/list/get tests # TLABEL=`attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` if [ "x""$TLABEL" != "x" ] then echo Initial transmute label is unexpectedly '"'"$TLABEL"'"'. exit 1 fi attr -q -S -s SMACK64TRANSMUTE -V WILL $TESTDIR 2> /dev/null TLABEL=`attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` if [ "x""$TLABEL" != "x" ] then echo Setting transmute label should not have gotten '"'"$TLABEL"'"'. exit 1 fi TLIST=`attr -q -S -l $TESTDIR | grep TRANSMUTE` if [ "x""$TLIST" != "x" ] then echo Setting transmute label listed incorrectly as '"'"$TLIST"'"'. exit 1 fi attr -q -S -s SMACK64TRANSMUTE -V TRUE $TESTDIR 2> /dev/null TLABEL=`attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` if [ "x""$TLABEL" != "xTRUE" ] then echo Setting transmute label should not have gotten '"'"$TLABEL"'"'. exit 1 fi TLIST=`attr -q -S -l $TESTDIR | grep TRANSMUTE` if [ "x""$TLIST" != "xSMACK64TRANSMUTE" ] then echo Setting transmute label listed incorrectly as '"'"$TLIST"'"'. exit 1 fi attr -q -S -r SMACK64TRANSMUTE $TESTDIR 2> /dev/null TLABEL=`attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` if [ "x""$TLABEL" != "x" ] then echo Removing transmute label left '"'"$TLABEL"'"' behind. exit 1 fi rm -rf $TESTDIR # # Rule setting tests # # 1 2 3 4 5 6 # 1234567890123456789012345678901234567890123456789012345678901234567890 # subject-label.......... object-label........... rwxat # # 1 2 3 # 123456789012345678901234567890 SUBJECT="Snap " SUBNAME="Snap" # 1 2 3 # 123456789012345678901234567890 OBJECT="Crackle " OBNAME="Crackle" OLDRULE=`grep '^'"$SUBNAME"' '"$OBNAME" $LOAD` echo -n "$SUBJECT"' '"$OBJECT"' ''rwxa' > $LOAD RULE=`grep '^'"$SUBNAME"' '"$OBNAME" $LOAD` if [ "x""$RULE" != "x""$SUBNAME"' '"$OBNAME"' rwxa' ] then echo Setting rule without '"'t'"' unexpectedly '"'"$RULE"'"'. exit 1 fi echo -n "$SUBJECT"' '"$OBJECT"' ''rwxat' > $LOAD RULE=`grep '^'"$SUBNAME"' '"$OBNAME" $LOAD` if [ "x""$RULE" != "x""$SUBNAME"' '"$OBNAME"' rwxat' ] then echo Setting rule with '"'t'"' unexpectedly '"'"$RULE"'"'. exit 1 fi # # Verify correct label behavior with neither a transmute # directory nor a transmute rule. # rm -rf $TESTDIR mkdir $TESTDIR grep `cat /proc/self/attr/current` $LOAD echo Dir: `attr -q -S -g SMACK64 $TESTDIR 2> /dev/null` `ls -id $TESTDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` chmod 777 $TESTDIR attr -q -S -s SMACK64 -V $OBNAME $TESTDIR echo -n "$SUBJECT"' '"$OBJECT"' ''rwxa' > $LOAD echo "$SUBNAME" > /proc/self/attr/current su - $NOTROOT -c "date > $TESTFILE" echo "$RUNAS" > /proc/self/attr/current GOT=`attr -q -S -g SMACK64 $TESTFILE` if [ 'x'"$SUBNAME" != 'x'"$GOT" ] then echo Expected '"'$SUBNAME'"' but got '"'$GOT'"'. exit 1 fi # # Verify correct label behavior with a transmute # directory but not a transmute rule. # rm -rf $TESTDIR mkdir $TESTDIR grep `cat /proc/self/attr/current` $LOAD echo Dir: `attr -q -S -g SMACK64 $TESTDIR 2> /dev/null` `ls -id $TESTDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` chmod 777 $TESTDIR attr -q -S -s SMACK64 -V $OBNAME $TESTDIR attr -q -S -s SMACK64TRANSMUTE -V TRUE $TESTDIR echo -n "$SUBJECT"' '"$OBJECT"' ''rwxa' > $LOAD echo "$SUBNAME" > /proc/self/attr/current su - $NOTROOT -c "date > $TESTFILE" echo "$RUNAS" > /proc/self/attr/current GOT=`attr -q -S -g SMACK64 $TESTFILE` if [ 'x'"$SUBNAME" != 'x'"$GOT" ] then echo Expected '"'$SUBNAME'"' but got '"'$GOT'"'. exit 1 fi # # Verify correct label behavior without a transmute # directory but with a transmute rule. # rm -rf $TESTDIR mkdir $TESTDIR grep `cat /proc/self/attr/current` $LOAD echo Dir: `attr -q -S -g SMACK64 $TESTDIR 2> /dev/null` `ls -id $TESTDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` chmod 777 $TESTDIR attr -q -S -s SMACK64 -V $OBNAME $TESTDIR echo -n "$SUBJECT"' '"$OBJECT"' ''rwxat' > $LOAD echo "$SUBNAME" > /proc/self/attr/current su - $NOTROOT -c "date > $TESTFILE" echo "$RUNAS" > /proc/self/attr/current GOT=`attr -q -S -g SMACK64 $TESTFILE` if [ 'x'"$SUBNAME" != 'x'"$GOT" ] then echo Expected '"'$SUBNAME'"' but got '"'$GOT'"'. exit 1 fi # # Verify correct label behavior with a transmute # directory and a transmute rule. # rm -rf $TESTDIR mkdir $TESTDIR grep `cat /proc/self/attr/current` $LOAD echo Dir: `attr -q -S -g SMACK64 $TESTDIR 2> /dev/null` `ls -id $TESTDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` chmod 777 $TESTDIR attr -q -S -s SMACK64 -V $OBNAME $TESTDIR attr -q -S -s SMACK64TRANSMUTE -V TRUE $TESTDIR grep `cat /proc/self/attr/current` $LOAD echo Dir: `attr -q -S -g SMACK64 $TESTDIR 2> /dev/null` `ls -id $TESTDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` echo -n "$SUBJECT"' '"$OBJECT"' ''rwxat' > $LOAD echo "$SUBNAME" > /proc/self/attr/current su - $NOTROOT -c "date > $TESTFILE" echo "$RUNAS" > /proc/self/attr/current GOT=`attr -q -S -g SMACK64 $TESTFILE` if [ 'x'"$OBNAME" != 'x'"$GOT" ] then echo Expected '"'$OBNAME'"' but got '"'$GOT'"'. attr -l -S $TESTDIR attr -S -g SMACK64 $TESTFILE grep '^'"$SUBNAME"' '"$OBNAME" $LOAD exit 1 else echo Expected '"'$OBNAME'"' and got '"'$GOT'"' successfully. fi # # Verify correct label behavior with a transmute # directory and subdirectories # echo "$SUBNAME" > /proc/self/attr/current rm -rf $TESTSUBDIR su - $NOTROOT -c "mkdir $TESTSUBDIR" grep `cat /proc/self/attr/current` $LOAD echo Dir: `attr -q -S -g SMACK64 $TESTDIR 2> /dev/null` `ls -id $TESTDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` echo Dir: `attr -q -S -g SMACK64 $TESTSUBDIR 2> /dev/null` `ls -id $TESTSUBDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTSUBDIR 2> /dev/null` echo "$RUNAS" > /proc/self/attr/current GOT=`attr -q -S -g SMACK64 $TESTSUBDIR` if [ 'x'"$OBNAME" != 'x'"$GOT" ] then echo Expected '"'$OBNAME'"' but got '"'$GOT'"'. else echo Expected '"'$OBNAME'"' and got '"'$GOT'"' successfully. fi echo "$SUBNAME" > /proc/self/attr/current rm -rf $TESTSUB2DIR su - $NOTROOT -c "mkdir $TESTSUB2DIR" grep `cat /proc/self/attr/current` $LOAD echo Dir: `attr -q -S -g SMACK64 $TESTDIR 2> /dev/null` `ls -id $TESTDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` echo Dir: `attr -q -S -g SMACK64 $TESTSUBDIR 2> /dev/null` `ls -id $TESTSUBDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTSUBDIR 2> /dev/null` echo Dir: `attr -q -S -g SMACK64 $TESTSUB2DIR 2> /dev/null` `ls -id $TESTSUB2DIR` `attr -q -S -g SMACK64TRANSMUTE $TESTSUB2DIR 2> /dev/null` echo "$RUNAS" > /proc/self/attr/current GOT=`attr -q -S -g SMACK64 $TESTSUB2DIR` if [ 'x'"$OBNAME" != 'x'"$GOT" ] then echo Expected '"'$OBNAME'"' but got '"'$GOT'"'. else echo Expected '"'$OBNAME'"' and got '"'$GOT'"' successfully. fi echo "$SUBNAME" > /proc/self/attr/current rm -rf $TESTSUB3DIR su - $NOTROOT -c "mkdir $TESTSUB3DIR" grep `cat /proc/self/attr/current` $LOAD echo Dir: `attr -q -S -g SMACK64 $TESTDIR 2> /dev/null` `ls -id $TESTDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTDIR 2> /dev/null` echo Dir: `attr -q -S -g SMACK64 $TESTSUBDIR 2> /dev/null` `ls -id $TESTSUBDIR` `attr -q -S -g SMACK64TRANSMUTE $TESTSUBDIR 2> /dev/null` echo Dir: `attr -q -S -g SMACK64 $TESTSUB2DIR 2> /dev/null` `ls -id $TESTSUB2DIR` `attr -q -S -g SMACK64TRANSMUTE $TESTSUB2DIR 2> /dev/null` echo Dir: `attr -q -S -g SMACK64 $TESTSUB3DIR 2> /dev/null` `ls -id $TESTSUB3DIR` `attr -q -S -g SMACK64TRANSMUTE $TESTSUB3DIR 2> /dev/null` echo "$RUNAS" > /proc/self/attr/current GOT=`attr -q -S -g SMACK64 $TESTSUB3DIR` if [ 'x'"$OBNAME" != 'x'"$GOT" ] then echo Expected '"'$OBNAME'"' but got '"'$GOT'"'. else echo Expected '"'$OBNAME'"' and got '"'$GOT'"' successfully. fi # # test SMACK64TRANSMUTE removal # echo test SMACK64TRANSMUTE removal attr -S -g SMACK64TRANSMUTE $TESTSUB3DIR rm -rf $TESTSUB3DIR attr -S -r SMACK64TRANSMUTE $TESTSUB2DIR su - $NOTROOT -c "mkdir $TESTSUB3DIR" attr -S -g SMACK64TRANSMUTE $TESTSUB2DIR attr -S -g SMACK64TRANSMUTE $TESTSUB3DIR rm -rf $TESTDIR # # Restore old label rule. # if [ "x""$OLDRULE" != "x" ] then RAWOLDMODE=`echo $OLDRULE | sed -e 's/.* //'` I=`echo $RAWOLDMODE | grep 'r'` if [ "x""$I" = "x" ] then OLDMODE="-" else OLDMODE="r" fi I=`echo $RAWOLDMODE | grep 'w'` if [ "x""$I" = "x" ] then OLDMODE="$OLDMODE""-" else OLDMODE="$OLDMODE""w" fi I=`echo $RAWOLDMODE | grep 'x'` if [ "x""$I" = "x" ] then OLDMODE="$OLDMODE""-" else OLDMODE="$OLDMODE""x" fi I=`echo $RAWOLDMODE | grep 'a'` if [ "x""$I" = "x" ] then OLDMODE="$OLDMODE""-" else OLDMODE="$OLDMODE""a" fi I=`echo $RAWOLDMODE | grep 't'` if [ "x""$I" = "x" ] then OLDMODE="$OLDMODE""-" else OLDMODE="$OLDMODE""t" fi echo -n "$SUBJECT"' '"$OBJECT"' '"$OLDMODE" > $LOAD # echo Rule reset to '"'"$SUBNAME"' '"$OBNAME"' '"$OLDMODE"'"' else echo -n "$SUBJECT"' '"$OBJECT"' ''-----' > $LOAD # echo Rule reset to '"'"$SUBNAME"' '"$OBNAME"' '"-----"'"' fi echo "$RUNAS" > /proc/self/attr/current rm -rf $TESTDIR exit 0
On Tue, 2023-04-18 at 09:02 -0700, Casey Schaufler wrote: > On 4/18/2023 12:05 AM, Roberto Sassu wrote: > > On Mon, 2023-04-17 at 09:41 -0700, Casey Schaufler wrote: > > > On 4/13/2023 12:11 AM, Roberto Sassu wrote: > > > > On Wed, 2023-04-12 at 13:29 -0700, Casey Schaufler wrote: > > > > > On 4/12/2023 12:22 AM, Roberto Sassu wrote: > > > > > > On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote: > > > > > > > On 4/11/2023 10:23 AM, Roberto Sassu wrote: > > > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > > > > > Very very quick modification. Not tested. > > > > > > > > > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > --- > > > > > > > > security/smack/smack.h | 2 +- > > > > > > > > security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ > > > > > > > > 2 files changed, 23 insertions(+), 21 deletions(-) > > > > > > > > > > > > > > > > diff --git a/security/smack/smack.h b/security/smack/smack.h > > > > > > > > index e2239be7bd6..f00c8498c60 100644 > > > > > > > > --- a/security/smack/smack.h > > > > > > > > +++ b/security/smack/smack.h > > > > > > > > @@ -127,7 +127,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) */ > > > > > > > See below ... > > > > > > > > > > > > > > > #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 8392983334b..b43820bdbd0 100644 > > > > > > > > --- a/security/smack/smack_lsm.c > > > > > > > > +++ b/security/smack/smack_lsm.c > > > > > > > > @@ -54,12 +54,12 @@ > > > > > > > > > > > > > > > > /* > > > > > > > > * Smack uses multiple xattrs. > > > > > > > > - * SMACK64 - for access control, SMACK64EXEC - label for the program, > > > > > > > > - * SMACK64MMAP - controls library loading, > > > > > > > > + * SMACK64 - for access control, > > > > > > > > * SMACK64TRANSMUTE - label initialization, > > > > > > > > - * Not saved on files - SMACK64IPIN and SMACK64IPOUT > > > > > > > > + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, > > > > > > > > + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP > > > > > > > > */ > > > > > > > > -#define SMACK_INODE_INIT_XATTRS 4 > > > > > > > > +#define SMACK_INODE_INIT_XATTRS 2 > > > > > > > > > > > > > > > > #ifdef SMACK_IPV6_PORT_LABELING > > > > > > > > static DEFINE_MUTEX(smack_ipv6_lock); > > > > > > > > @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > > > > > > > const struct qstr *qstr, > > > > > > > > struct xattr *xattrs, int *xattr_count) > > > > > > > > { > > > > > > > > - struct inode_smack *issp = smack_inode(inode); > > > > > > > > struct smack_known *skp = smk_of_current(); > > > > > > > > struct smack_known *isp = smk_of_inode(inode); > > > > > > > > struct smack_known *dsp = smk_of_inode(dir); > > > > > > > > struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); > > > > > > > > + struct xattr *xattr2; > > > > > > > I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. > > > > > > > It also looks like it could move to be declared in the if clause. > > > > > > > > > > > > > > > int may; > > > > > > > > > > > > > > > > if (xattr) { > > > > > > > > @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > > > > > > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > > > > > > > > smk_inode_transmutable(dir)) { > > > > > > > > isp = dsp; > > > > > > > > - issp->smk_flags |= SMK_INODE_CHANGED; > > > > > > > I think you need to keep this. More below. > > > > > > > > > > > > > > > + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); > > > > > > > > + if (xattr2) { > > > > > > > > + xattr2->value = kmemdup(TRANS_TRUE, > > > > > > > > + TRANS_TRUE_SIZE, > > > > > > > > + GFP_NOFS); > > > > > > > > + if (xattr2->value == NULL) > > > > > > > > + return -ENOMEM; > > > > > > > > + > > > > > > > > + xattr2->value_len = TRANS_TRUE_SIZE; > > > > > > > > + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; > > > > > > > > + } > > > > > > > > } > > > > > > > > > > > > > > > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > > > > > > > @@ -3512,20 +3522,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; > > > > > > > Where is the SMACK64_TRANSMUTE attribute going to get set on the file? > > > > > > > It's not going to get set in smack_init_inode_security(). The inode will > > > > > > Isn't that the purpose of the inode_init_security hook? > > > > > No. It initializes the in-memory inode. > > > > I hope I'm not mistaken here... > > > > > > > > I make a small example. Filesystems call > > > > security_inode_init_security(). Ext4 does: > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n54 > > > > > > > > security_inode_init_security() allocates new_xattrs. Each LSM fills > > > > new_xattrs. At the end of the loop, if there is at least one xattr > > > > filled, the initxattrs() callback passed by the caller of > > > > security_inode_init_security() is called. > > > > > > > > The ext4 initxattrs() callback is: > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n35 > > > > > > > > which scans the xattr array and, for each xattr, > > > > calls ext4_xattr_set_handle(). > > > > > > > > Maybe I'm overlooking it, but ext4_xattr_set_handle() is setting xattrs > > > > on the disk. Am I wrong? > > > Yes, you're wrong. I tried your change, and the SMACK64_TRANSMUTE isn't > > > set on the sub-directory when it's created. The __vfs_setxattr() call really > > > is necessary. > > Could you please also check if there is any change with this fix: > > > > Replace: > > > > xattr2->name = XATTR_NAME_SMACKTRANSMUTE; > > > > with: > > > > xattr2->name = XATTR_SMACK_TRANSMUTE; > > > > Thanks > > Unless I'm missing something really obvious there's no way that any > of the evm/ima changes would obviate the need for the __vfs_setxattr() call. > It's real easy to verify correct behavior, see the attached script. > (you'll want to change the "notroot" value to a user on your system) I got some errors during xattr removal, so not sure if my patch was working properly or not (it happened also without it, didn't investigate more). However, I saw another discussion related to transmute: https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@amazon.com/ I add the people in CC. The steps described were so easy to understand and executed, I tried without and with overlayfs. Without: # echo "_ system rwxatl" > /sys/fs/smackfs/load2 # mkdir /data # chsmack -a "system" /data # chsmack -t /data # mkdir -p /data/dir1/dir2 # chsmack /data/dir1 /data/dir1 access="system" transmute="TRUE" # chsmack /data/dir1/dir2 /data/dir1/dir2 access="system" transmute="TRUE" It seems to work, right? With overlay fs it didn't work, same result as the one Mengchi reported. Since Mengchi's solution was to set SMK_INODE_CHANGED, and I want to get rid of it, I thought to investigate more. Looking at smack_dentry_create_files_as(), I see that the label of the process is overwritten with the label of the transmuting directory. That causes smack_inode_init_security() to lookup the transmuting rule on the overridden credential, and not on the original one. In the example above, it means that, when overlayfs is creating the new inode, the label of the process is system, not _. So no transmute permission, and also the xattr will not be added, as observed by Mengchi. Hopefully I undertood the code, so in this particular case we would not need to override the label of the process in smack_dentry_create_files_ as(). If you see smack_inode_init_security(): struct smack_known *skp = smk_of_current(); struct smack_known *isp = smk_of_inode(inode); struct smack_known *dsp = smk_of_inode(dir); [...] if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && smk_inode_transmutable(dir)) { isp = dsp; [...] xattr->value = kstrdup(isp->smk_known, GFP_NOFS); This code is telling, if there is a transmute rule, and the directory is transmuting, set the label of the new inode to the label of the directory. That should be already the result that we wanted to obtain. The current code should have been doing it by overriding the label of the process in smack_dentry_create_files_as() with the label of the parent directory, and letting the inode being created with the overridden label of the process. The transmute xattr is not set due to the problem described above. So, as a quick test, I kept this patch with the change to xattr2->name, and skipped the label override in smack_dentry_create_files_as(). It worked, I get the same result as without overlayfs. Wondering if the process label override is necessary in other cases. Roberto > > Roberto > > > > > > Thanks > > > > > > > > Roberto > > > > > > > > > > After all LSMs provide one or multiple xattrs, xattrs are going to be > > > > > > written to the disk with the initxattr() callback of filesystems. > > > > > > > > > > > > There is a small mistake above (XATTR_SMACK_TRANSMUTE instead > > > > > > of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix). > > > > > but I'm pretty sure the __vfs_setxattr() call is necessary to get > > > > > the attribute written out. With your change the in-memory inode will > > > > > get the attribute, but if you reboot it won't be on the directory. > > > > > > > > > > > 95 Passed, 0 Failed, 100% Success rate > > > > > > > > > > > > There was a test failing in dir-transmute.sh, before I fixed the xattr > > > > > > name. > > > > > > > > > > > > Thanks > > > > > > > > > > > > Roberto > > > > > > > > > > > > > know it's transmuting, but it won't get to disk without the __vfs_setxattr() > > > > > > > here in smack_d_instantiate(). Now, it's been a long time since that code > > > > > > > was written, so I could be wrong, but I'm pretty sure about that. > > > > > > > > > > > > > > I think that you should be fine with the changes in smack_init_inode_security(), > > > > > > > and leaving smack_d_instantiate() untouched. > > > > > > > > > > > > > > > if (rc >= 0) > > > > > > > > transflag = SMK_INODE_TRANSMUTE; > > > > > > > > }
> > I got some errors during xattr removal, so not sure if my patch was > working properly or not (it happened also without it, didn't > investigate more). > > However, I saw another discussion related to transmute: > > https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@amazon.com/ > > I add the people in CC. > > The steps described were so easy to understand and executed, I tried > without and with overlayfs. > > Without: > > # echo "_ system rwxatl" > /sys/fs/smackfs/load2 > # mkdir /data > # chsmack -a "system" /data > # chsmack -t /data > # mkdir -p /data/dir1/dir2 > # chsmack /data/dir1 > /data/dir1 access="system" transmute="TRUE" > # chsmack /data/dir1/dir2 > /data/dir1/dir2 access="system" transmute="TRUE" > > It seems to work, right? > > With overlay fs it didn't work, same result as the one Mengchi > reported. Since Mengchi's solution was to set SMK_INODE_CHANGED, and I > want to get rid of it, I thought to investigate more. > > Looking at smack_dentry_create_files_as(), I see that the label of the > process is overwritten with the label of the transmuting directory. > > That causes smack_inode_init_security() to lookup the transmuting rule > on the overridden credential, and not on the original one. > > In the example above, it means that, when overlayfs is creating the new > inode, the label of the process is system, not _. So no transmute > permission, and also the xattr will not be added, as observed by > Mengchi. > > Hopefully I undertood the code, so in this particular case we would not > need to override the label of the process in smack_dentry_create_files_ > as(). > > If you see smack_inode_init_security(): > > struct smack_known *skp = smk_of_current(); > struct smack_known *isp = smk_of_inode(inode); > struct smack_known *dsp = smk_of_inode(dir); > > [...] > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > smk_inode_transmutable(dir)) { > isp = dsp; > [...] > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > This code is telling, if there is a transmute rule, and the directory > is transmuting, set the label of the new inode to the label of the > directory. That should be already the result that we wanted to obtain. > > The current code should have been doing it by overriding the label of > the process in smack_dentry_create_files_as() with the label of the > parent directory, and letting the inode being created with the > overridden label of the process. The transmute xattr is not set due to > the problem described above. > > So, as a quick test, I kept this patch with the change to xattr2->name, > and skipped the label override in smack_dentry_create_files_as(). It > worked, I get the same result as without overlayfs. Wondering if the > process label override is necessary in other cases. If I understand correctly, removing the if block below is what you suggested. diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index cfcbb748da25..a867288e9de9 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -4769,8 +4769,8 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode, * providing access is transmuting use the containing * directory label instead of the process label. */ - if (may > 0 && (may & MAY_TRANSMUTE)) - ntsp->smk_task = isp->smk_inode; +// if (may > 0 && (may & MAY_TRANSMUTE)) +// ntsp->smk_task = isp->smk_inode; } return 0; } This way will have issue in the following situation on the vanila kernel. data in the lowerdir has "_" label before overlay and dir1 is already created in the lowerdir. # chsmack /data /data access="_" # chsmack /data/dir1 /data/dir1 access="system" transmute="TRUE" Apply overlay on data directory and set the smack rule in the same way. data has the same smack label. # chsmack /data /data access="system" transmute="TRUE" After that, remove dir1 and mkdir dir1 again. dir1 did not get the correct label. # rm -r /data/dir1 # mkdir -p /data/dir1 # chsmack /data/dir1 /data/dir1 access="_" Since I am not very familiar your change. Could you help check with your patch will this issue also happen? Best, Mengchi > > Roberto
On 4/19/2023 6:46 AM, Roberto Sassu wrote: > On Tue, 2023-04-18 at 09:02 -0700, Casey Schaufler wrote: >> On 4/18/2023 12:05 AM, Roberto Sassu wrote: >>> On Mon, 2023-04-17 at 09:41 -0700, Casey Schaufler wrote: >>>> On 4/13/2023 12:11 AM, Roberto Sassu wrote: >>>>> On Wed, 2023-04-12 at 13:29 -0700, Casey Schaufler wrote: >>>>>> On 4/12/2023 12:22 AM, Roberto Sassu wrote: >>>>>>> On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote: >>>>>>>> On 4/11/2023 10:23 AM, Roberto Sassu wrote: >>>>>>>>> From: Roberto Sassu <roberto.sassu@huawei.com> >>>>>>>>> >>>>>>>>> Very very quick modification. Not tested. >>>>>>>>> >>>>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >>>>>>>>> --- >>>>>>>>> security/smack/smack.h | 2 +- >>>>>>>>> security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ >>>>>>>>> 2 files changed, 23 insertions(+), 21 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/security/smack/smack.h b/security/smack/smack.h >>>>>>>>> index e2239be7bd6..f00c8498c60 100644 >>>>>>>>> --- a/security/smack/smack.h >>>>>>>>> +++ b/security/smack/smack.h >>>>>>>>> @@ -127,7 +127,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) */ >>>>>>>> See below ... >>>>>>>> >>>>>>>>> #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 8392983334b..b43820bdbd0 100644 >>>>>>>>> --- a/security/smack/smack_lsm.c >>>>>>>>> +++ b/security/smack/smack_lsm.c >>>>>>>>> @@ -54,12 +54,12 @@ >>>>>>>>> >>>>>>>>> /* >>>>>>>>> * Smack uses multiple xattrs. >>>>>>>>> - * SMACK64 - for access control, SMACK64EXEC - label for the program, >>>>>>>>> - * SMACK64MMAP - controls library loading, >>>>>>>>> + * SMACK64 - for access control, >>>>>>>>> * SMACK64TRANSMUTE - label initialization, >>>>>>>>> - * Not saved on files - SMACK64IPIN and SMACK64IPOUT >>>>>>>>> + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, >>>>>>>>> + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP >>>>>>>>> */ >>>>>>>>> -#define SMACK_INODE_INIT_XATTRS 4 >>>>>>>>> +#define SMACK_INODE_INIT_XATTRS 2 >>>>>>>>> >>>>>>>>> #ifdef SMACK_IPV6_PORT_LABELING >>>>>>>>> static DEFINE_MUTEX(smack_ipv6_lock); >>>>>>>>> @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >>>>>>>>> const struct qstr *qstr, >>>>>>>>> struct xattr *xattrs, int *xattr_count) >>>>>>>>> { >>>>>>>>> - struct inode_smack *issp = smack_inode(inode); >>>>>>>>> struct smack_known *skp = smk_of_current(); >>>>>>>>> struct smack_known *isp = smk_of_inode(inode); >>>>>>>>> struct smack_known *dsp = smk_of_inode(dir); >>>>>>>>> struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); >>>>>>>>> + struct xattr *xattr2; >>>>>>>> I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. >>>>>>>> It also looks like it could move to be declared in the if clause. >>>>>>>> >>>>>>>>> int may; >>>>>>>>> >>>>>>>>> if (xattr) { >>>>>>>>> @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, >>>>>>>>> if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && >>>>>>>>> smk_inode_transmutable(dir)) { >>>>>>>>> isp = dsp; >>>>>>>>> - issp->smk_flags |= SMK_INODE_CHANGED; >>>>>>>> I think you need to keep this. More below. >>>>>>>> >>>>>>>>> + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); >>>>>>>>> + if (xattr2) { >>>>>>>>> + xattr2->value = kmemdup(TRANS_TRUE, >>>>>>>>> + TRANS_TRUE_SIZE, >>>>>>>>> + GFP_NOFS); >>>>>>>>> + if (xattr2->value == NULL) >>>>>>>>> + return -ENOMEM; >>>>>>>>> + >>>>>>>>> + xattr2->value_len = TRANS_TRUE_SIZE; >>>>>>>>> + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; >>>>>>>>> + } >>>>>>>>> } >>>>>>>>> >>>>>>>>> xattr->value = kstrdup(isp->smk_known, GFP_NOFS); >>>>>>>>> @@ -3512,20 +3522,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; >>>>>>>> Where is the SMACK64_TRANSMUTE attribute going to get set on the file? >>>>>>>> It's not going to get set in smack_init_inode_security(). The inode will >>>>>>> Isn't that the purpose of the inode_init_security hook? >>>>>> No. It initializes the in-memory inode. >>>>> I hope I'm not mistaken here... >>>>> >>>>> I make a small example. Filesystems call >>>>> security_inode_init_security(). Ext4 does: >>>>> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n54 >>>>> >>>>> security_inode_init_security() allocates new_xattrs. Each LSM fills >>>>> new_xattrs. At the end of the loop, if there is at least one xattr >>>>> filled, the initxattrs() callback passed by the caller of >>>>> security_inode_init_security() is called. >>>>> >>>>> The ext4 initxattrs() callback is: >>>>> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n35 >>>>> >>>>> which scans the xattr array and, for each xattr, >>>>> calls ext4_xattr_set_handle(). >>>>> >>>>> Maybe I'm overlooking it, but ext4_xattr_set_handle() is setting xattrs >>>>> on the disk. Am I wrong? >>>> Yes, you're wrong. I tried your change, and the SMACK64_TRANSMUTE isn't >>>> set on the sub-directory when it's created. The __vfs_setxattr() call really >>>> is necessary. >>> Could you please also check if there is any change with this fix: >>> >>> Replace: >>> >>> xattr2->name = XATTR_NAME_SMACKTRANSMUTE; >>> >>> with: >>> >>> xattr2->name = XATTR_SMACK_TRANSMUTE; >>> >>> Thanks >> Unless I'm missing something really obvious there's no way that any >> of the evm/ima changes would obviate the need for the __vfs_setxattr() call. >> It's real easy to verify correct behavior, see the attached script. >> (you'll want to change the "notroot" value to a user on your system) > I got some errors during xattr removal, so not sure if my patch was > working properly or not (it happened also without it, didn't > investigate more). The script is demonstrating that those xattrs don't exist when they shouldn't, si all is good there. > > However, I saw another discussion related to transmute: > > https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@amazon.com/ > > I add the people in CC. > > The steps described were so easy to understand and executed, I tried > without and with overlayfs. > > Without: > > # echo "_ system rwxatl" > /sys/fs/smackfs/load2 > # mkdir /data > # chsmack -a "system" /data > # chsmack -t /data > # mkdir -p /data/dir1/dir2 > # chsmack /data/dir1 > /data/dir1 access="system" transmute="TRUE" > # chsmack /data/dir1/dir2 > /data/dir1/dir2 access="system" transmute="TRUE" > > It seems to work, right? > > With overlay fs it didn't work, same result as the one Mengchi > reported. Since Mengchi's solution was to set SMK_INODE_CHANGED, and I > want to get rid of it, I thought to investigate more. > > Looking at smack_dentry_create_files_as(), I see that the label of the > process is overwritten with the label of the transmuting directory. > > That causes smack_inode_init_security() to lookup the transmuting rule > on the overridden credential, and not on the original one. > > In the example above, it means that, when overlayfs is creating the new > inode, the label of the process is system, not _. So no transmute > permission, and also the xattr will not be added, as observed by > Mengchi. OK, I see that. Looks like the original implementation was poorly thought out/tested. > Hopefully I undertood the code, so in this particular case we would not > need to override the label of the process in smack_dentry_create_files_ > as(). > > If you see smack_inode_init_security(): > > struct smack_known *skp = smk_of_current(); > struct smack_known *isp = smk_of_inode(inode); > struct smack_known *dsp = smk_of_inode(dir); > > [...] > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > smk_inode_transmutable(dir)) { > isp = dsp; > [...] > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > This code is telling, if there is a transmute rule, and the directory > is transmuting, set the label of the new inode to the label of the > directory. That should be already the result that we wanted to obtain. > > The current code should have been doing it by overriding the label of > the process in smack_dentry_create_files_as() with the label of the > parent directory, and letting the inode being created with the > overridden label of the process. The transmute xattr is not set due to > the problem described above. That would explain the observed behavior. > So, as a quick test, I kept this patch with the change to xattr2->name, > and skipped the label override in smack_dentry_create_files_as(). It > worked, I get the same result as without overlayfs. Wondering if the > process label override is necessary in other cases. It's possible. It's been a long time since I've looked at this. I'm tempted to take a change to make overlayfs work upstream and then worry about the ima changes. There seems to be a lot more going on with the ima changes than is obvious from what's in the Smack code. > > Roberto > >>> Roberto >>> >>>>> Thanks >>>>> >>>>> Roberto >>>>> >>>>>>> After all LSMs provide one or multiple xattrs, xattrs are going to be >>>>>>> written to the disk with the initxattr() callback of filesystems. >>>>>>> >>>>>>> There is a small mistake above (XATTR_SMACK_TRANSMUTE instead >>>>>>> of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix). >>>>>> but I'm pretty sure the __vfs_setxattr() call is necessary to get >>>>>> the attribute written out. With your change the in-memory inode will >>>>>> get the attribute, but if you reboot it won't be on the directory. >>>>>> >>>>>>> 95 Passed, 0 Failed, 100% Success rate >>>>>>> >>>>>>> There was a test failing in dir-transmute.sh, before I fixed the xattr >>>>>>> name. >>>>>>> >>>>>>> Thanks >>>>>>> >>>>>>> Roberto >>>>>>> >>>>>>>> know it's transmuting, but it won't get to disk without the __vfs_setxattr() >>>>>>>> here in smack_d_instantiate(). Now, it's been a long time since that code >>>>>>>> was written, so I could be wrong, but I'm pretty sure about that. >>>>>>>> >>>>>>>> I think that you should be fine with the changes in smack_init_inode_security(), >>>>>>>> and leaving smack_d_instantiate() untouched. >>>>>>>> >>>>>>>>> if (rc >= 0) >>>>>>>>> transflag = SMK_INODE_TRANSMUTE; >>>>>>>>> }
On Wed, 2023-04-19 at 12:25 -0700, Mengchi Cheng wrote: > > I got some errors during xattr removal, so not sure if my patch was > > working properly or not (it happened also without it, didn't > > investigate more). > > > > However, I saw another discussion related to transmute: > > > > https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@amazon.com/ > > > > I add the people in CC. > > > > The steps described were so easy to understand and executed, I tried > > without and with overlayfs. > > > > Without: > > > > # echo "_ system rwxatl" > /sys/fs/smackfs/load2 > > # mkdir /data > > # chsmack -a "system" /data > > # chsmack -t /data > > # mkdir -p /data/dir1/dir2 > > # chsmack /data/dir1 > > /data/dir1 access="system" transmute="TRUE" > > # chsmack /data/dir1/dir2 > > /data/dir1/dir2 access="system" transmute="TRUE" > > > > It seems to work, right? > > > > With overlay fs it didn't work, same result as the one Mengchi > > reported. Since Mengchi's solution was to set SMK_INODE_CHANGED, and I > > want to get rid of it, I thought to investigate more. > > > > Looking at smack_dentry_create_files_as(), I see that the label of the > > process is overwritten with the label of the transmuting directory. > > > > That causes smack_inode_init_security() to lookup the transmuting rule > > on the overridden credential, and not on the original one. > > > > In the example above, it means that, when overlayfs is creating the new > > inode, the label of the process is system, not _. So no transmute > > permission, and also the xattr will not be added, as observed by > > Mengchi. > > > > Hopefully I undertood the code, so in this particular case we would not > > need to override the label of the process in smack_dentry_create_files_ > > as(). > > > > If you see smack_inode_init_security(): > > > > struct smack_known *skp = smk_of_current(); > > struct smack_known *isp = smk_of_inode(inode); > > struct smack_known *dsp = smk_of_inode(dir); > > > > [...] > > > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > > smk_inode_transmutable(dir)) { > > isp = dsp; > > [...] > > > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > > > This code is telling, if there is a transmute rule, and the directory > > is transmuting, set the label of the new inode to the label of the > > directory. That should be already the result that we wanted to obtain. > > > > The current code should have been doing it by overriding the label of > > the process in smack_dentry_create_files_as() with the label of the > > parent directory, and letting the inode being created with the > > overridden label of the process. The transmute xattr is not set due to > > the problem described above. > > > > So, as a quick test, I kept this patch with the change to xattr2->name, > > and skipped the label override in smack_dentry_create_files_as(). It > > worked, I get the same result as without overlayfs. Wondering if the > > process label override is necessary in other cases. > > If I understand correctly, removing the if block below is what you suggested. Yes, more or less is what I did. > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index cfcbb748da25..a867288e9de9 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -4769,8 +4769,8 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode, > * providing access is transmuting use the containing > * directory label instead of the process label. > */ > - if (may > 0 && (may & MAY_TRANSMUTE)) > - ntsp->smk_task = isp->smk_inode; > +// if (may > 0 && (may & MAY_TRANSMUTE)) > +// ntsp->smk_task = isp->smk_inode; > } > return 0; > } > > This way will have issue in the following situation on the vanila kernel. > data in the lowerdir has "_" label before overlay and dir1 is already > created in the lowerdir. > # chsmack /data > /data access="_" > # chsmack /data/dir1 > /data/dir1 access="system" transmute="TRUE" > Apply overlay on data directory and set the smack rule in the same way. > data has the same smack label. > # chsmack /data > /data access="system" transmute="TRUE" I'm using an older kernel, but I get _ instead of system. > After that, remove dir1 and mkdir dir1 again. dir1 did not get the correct > label. > # rm -r /data/dir1 > # mkdir -p /data/dir1 > # chsmack /data/dir1 > /data/dir1 access="_" Unfortunately, it cannot work: Thread 3 hit Breakpoint 1, smack_inode_init_security (...) at security/smack/smack_lsm.c:959 959 { (gdb) p dir->i_ino $12 = 9169116 (gdb) p dsp $13 = (struct smack_known *) 0xffffffff831fc0a0 <smack_known_floor> ls -i /home/root/data_work/ 9169116 work So, transmuting is decided on the working directory. If I do: # chsmack -a system -t /home/root/data_work/work/ # mkdir /data/dir1 # chsmack /data/dir1 /data/dir1 access="system" transmute="TRUE" I obtain the expected result. However, this problem is due to how overlayfs works: static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, struct ovl_cattr *cattr) { [...] newdentry = ovl_create_temp(ofs, workdir, cattr); err = PTR_ERR(newdentry); if (IS_ERR(newdentry)) goto out_dput; The good news seems to be that, once you set the label to the correct directory, transmuting works with the changes I proposed. Roberto > Since I am not very familiar your change. Could you help check with your > patch will this issue also happen? > > > Best, > Mengchi > > > > > Roberto
On Wed, 2023-04-19 at 14:00 -0700, Casey Schaufler wrote: > On 4/19/2023 6:46 AM, Roberto Sassu wrote: > > On Tue, 2023-04-18 at 09:02 -0700, Casey Schaufler wrote: > > > On 4/18/2023 12:05 AM, Roberto Sassu wrote: > > > > On Mon, 2023-04-17 at 09:41 -0700, Casey Schaufler wrote: > > > > > On 4/13/2023 12:11 AM, Roberto Sassu wrote: > > > > > > On Wed, 2023-04-12 at 13:29 -0700, Casey Schaufler wrote: > > > > > > > On 4/12/2023 12:22 AM, Roberto Sassu wrote: > > > > > > > > On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote: > > > > > > > > > On 4/11/2023 10:23 AM, Roberto Sassu wrote: > > > > > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > > > > > > > > > Very very quick modification. Not tested. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > --- > > > > > > > > > > security/smack/smack.h | 2 +- > > > > > > > > > > security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------ > > > > > > > > > > 2 files changed, 23 insertions(+), 21 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/security/smack/smack.h b/security/smack/smack.h > > > > > > > > > > index e2239be7bd6..f00c8498c60 100644 > > > > > > > > > > --- a/security/smack/smack.h > > > > > > > > > > +++ b/security/smack/smack.h > > > > > > > > > > @@ -127,7 +127,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) */ > > > > > > > > > See below ... > > > > > > > > > > > > > > > > > > > #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 8392983334b..b43820bdbd0 100644 > > > > > > > > > > --- a/security/smack/smack_lsm.c > > > > > > > > > > +++ b/security/smack/smack_lsm.c > > > > > > > > > > @@ -54,12 +54,12 @@ > > > > > > > > > > > > > > > > > > > > /* > > > > > > > > > > * Smack uses multiple xattrs. > > > > > > > > > > - * SMACK64 - for access control, SMACK64EXEC - label for the program, > > > > > > > > > > - * SMACK64MMAP - controls library loading, > > > > > > > > > > + * SMACK64 - for access control, > > > > > > > > > > * SMACK64TRANSMUTE - label initialization, > > > > > > > > > > - * Not saved on files - SMACK64IPIN and SMACK64IPOUT > > > > > > > > > > + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, > > > > > > > > > > + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP > > > > > > > > > > */ > > > > > > > > > > -#define SMACK_INODE_INIT_XATTRS 4 > > > > > > > > > > +#define SMACK_INODE_INIT_XATTRS 2 > > > > > > > > > > > > > > > > > > > > #ifdef SMACK_IPV6_PORT_LABELING > > > > > > > > > > static DEFINE_MUTEX(smack_ipv6_lock); > > > > > > > > > > @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > > > > > > > > > const struct qstr *qstr, > > > > > > > > > > struct xattr *xattrs, int *xattr_count) > > > > > > > > > > { > > > > > > > > > > - struct inode_smack *issp = smack_inode(inode); > > > > > > > > > > struct smack_known *skp = smk_of_current(); > > > > > > > > > > struct smack_known *isp = smk_of_inode(inode); > > > > > > > > > > struct smack_known *dsp = smk_of_inode(dir); > > > > > > > > > > struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); > > > > > > > > > > + struct xattr *xattr2; > > > > > > > > > I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2. > > > > > > > > > It also looks like it could move to be declared in the if clause. > > > > > > > > > > > > > > > > > > > int may; > > > > > > > > > > > > > > > > > > > > if (xattr) { > > > > > > > > > > @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, > > > > > > > > > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > > > > > > > > > > smk_inode_transmutable(dir)) { > > > > > > > > > > isp = dsp; > > > > > > > > > > - issp->smk_flags |= SMK_INODE_CHANGED; > > > > > > > > > I think you need to keep this. More below. > > > > > > > > > > > > > > > > > > > + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); > > > > > > > > > > + if (xattr2) { > > > > > > > > > > + xattr2->value = kmemdup(TRANS_TRUE, > > > > > > > > > > + TRANS_TRUE_SIZE, > > > > > > > > > > + GFP_NOFS); > > > > > > > > > > + if (xattr2->value == NULL) > > > > > > > > > > + return -ENOMEM; > > > > > > > > > > + > > > > > > > > > > + xattr2->value_len = TRANS_TRUE_SIZE; > > > > > > > > > > + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; > > > > > > > > > > + } > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > > > > > > > > > @@ -3512,20 +3522,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; > > > > > > > > > Where is the SMACK64_TRANSMUTE attribute going to get set on the file? > > > > > > > > > It's not going to get set in smack_init_inode_security(). The inode will > > > > > > > > Isn't that the purpose of the inode_init_security hook? > > > > > > > No. It initializes the in-memory inode. > > > > > > I hope I'm not mistaken here... > > > > > > > > > > > > I make a small example. Filesystems call > > > > > > security_inode_init_security(). Ext4 does: > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n54 > > > > > > > > > > > > security_inode_init_security() allocates new_xattrs. Each LSM fills > > > > > > new_xattrs. At the end of the loop, if there is at least one xattr > > > > > > filled, the initxattrs() callback passed by the caller of > > > > > > security_inode_init_security() is called. > > > > > > > > > > > > The ext4 initxattrs() callback is: > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n35 > > > > > > > > > > > > which scans the xattr array and, for each xattr, > > > > > > calls ext4_xattr_set_handle(). > > > > > > > > > > > > Maybe I'm overlooking it, but ext4_xattr_set_handle() is setting xattrs > > > > > > on the disk. Am I wrong? > > > > > Yes, you're wrong. I tried your change, and the SMACK64_TRANSMUTE isn't > > > > > set on the sub-directory when it's created. The __vfs_setxattr() call really > > > > > is necessary. > > > > Could you please also check if there is any change with this fix: > > > > > > > > Replace: > > > > > > > > xattr2->name = XATTR_NAME_SMACKTRANSMUTE; > > > > > > > > with: > > > > > > > > xattr2->name = XATTR_SMACK_TRANSMUTE; > > > > > > > > Thanks > > > Unless I'm missing something really obvious there's no way that any > > > of the evm/ima changes would obviate the need for the __vfs_setxattr() call. > > > It's real easy to verify correct behavior, see the attached script. > > > (you'll want to change the "notroot" value to a user on your system) > > I got some errors during xattr removal, so not sure if my patch was > > working properly or not (it happened also without it, didn't > > investigate more). > > The script is demonstrating that those xattrs don't exist when they > shouldn't, si all is good there. > > > However, I saw another discussion related to transmute: > > > > https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@amazon.com/ > > > > I add the people in CC. > > > > The steps described were so easy to understand and executed, I tried > > without and with overlayfs. > > > > Without: > > > > # echo "_ system rwxatl" > /sys/fs/smackfs/load2 > > # mkdir /data > > # chsmack -a "system" /data > > # chsmack -t /data > > # mkdir -p /data/dir1/dir2 > > # chsmack /data/dir1 > > /data/dir1 access="system" transmute="TRUE" > > # chsmack /data/dir1/dir2 > > /data/dir1/dir2 access="system" transmute="TRUE" > > > > It seems to work, right? > > > > With overlay fs it didn't work, same result as the one Mengchi > > reported. Since Mengchi's solution was to set SMK_INODE_CHANGED, and I > > want to get rid of it, I thought to investigate more. > > > > Looking at smack_dentry_create_files_as(), I see that the label of the > > process is overwritten with the label of the transmuting directory. > > > > That causes smack_inode_init_security() to lookup the transmuting rule > > on the overridden credential, and not on the original one. > > > > In the example above, it means that, when overlayfs is creating the new > > inode, the label of the process is system, not _. So no transmute > > permission, and also the xattr will not be added, as observed by > > Mengchi. > > OK, I see that. Looks like the original implementation was poorly > thought out/tested. > > > Hopefully I undertood the code, so in this particular case we would not > > need to override the label of the process in smack_dentry_create_files_ > > as(). > > > > If you see smack_inode_init_security(): > > > > struct smack_known *skp = smk_of_current(); > > struct smack_known *isp = smk_of_inode(inode); > > struct smack_known *dsp = smk_of_inode(dir); > > > > [...] > > > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > > smk_inode_transmutable(dir)) { > > isp = dsp; > > [...] > > > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > > > This code is telling, if there is a transmute rule, and the directory > > is transmuting, set the label of the new inode to the label of the > > directory. That should be already the result that we wanted to obtain. > > > > The current code should have been doing it by overriding the label of > > the process in smack_dentry_create_files_as() with the label of the > > parent directory, and letting the inode being created with the > > overridden label of the process. The transmute xattr is not set due to > > the problem described above. > > That would explain the observed behavior. > > > So, as a quick test, I kept this patch with the change to xattr2->name, > > and skipped the label override in smack_dentry_create_files_as(). It > > worked, I get the same result as without overlayfs. Wondering if the > > process label override is necessary in other cases. > > It's possible. It's been a long time since I've looked at this. > I'm tempted to take a change to make overlayfs work upstream and > then worry about the ima changes. There seems to be a lot more > going on with the ima changes than is obvious from what's in the > Smack code. We could also set only SMACK64 in smack_inode_init_security(), and move SMACKTRANSMUTE64 later, when we figure out how to fix the case of overlayfs. IMA and EVM would work in both cases. Roberto > > Roberto > > > > > > Roberto > > > > > > > > > > Thanks > > > > > > > > > > > > Roberto > > > > > > > > > > > > > > After all LSMs provide one or multiple xattrs, xattrs are going to be > > > > > > > > written to the disk with the initxattr() callback of filesystems. > > > > > > > > > > > > > > > > There is a small mistake above (XATTR_SMACK_TRANSMUTE instead > > > > > > > > of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix). > > > > > > > but I'm pretty sure the __vfs_setxattr() call is necessary to get > > > > > > > the attribute written out. With your change the in-memory inode will > > > > > > > get the attribute, but if you reboot it won't be on the directory. > > > > > > > > > > > > > > > 95 Passed, 0 Failed, 100% Success rate > > > > > > > > > > > > > > > > There was a test failing in dir-transmute.sh, before I fixed the xattr > > > > > > > > name. > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > Roberto > > > > > > > > > > > > > > > > > know it's transmuting, but it won't get to disk without the __vfs_setxattr() > > > > > > > > > here in smack_d_instantiate(). Now, it's been a long time since that code > > > > > > > > > was written, so I could be wrong, but I'm pretty sure about that. > > > > > > > > > > > > > > > > > > I think that you should be fine with the changes in smack_init_inode_security(), > > > > > > > > > and leaving smack_d_instantiate() untouched. > > > > > > > > > > > > > > > > > > > if (rc >= 0) > > > > > > > > > > transflag = SMK_INODE_TRANSMUTE; > > > > > > > > > > }
On Thu, 2023-04-20 at 10:50 +0200, Roberto Sassu wrote: > > > > It's possible. It's been a long time since I've looked at this. > > I'm tempted to take a change to make overlayfs work upstream and > > then worry about the ima changes. There seems to be a lot more > > going on with the ima changes than is obvious from what's in the > > Smack code. It doesn't sound like the patch set introduces the overlayfs bug. The security_inode_init_security() change to initialize multiple LSMs and IMA xattrs and include them in the EVM hmac calculation is straight forward. In addition, the patch set creates the infrastructure for allowing multiple per LSM xattrs, as requested, to be initialized in security_inode_init_security() and included in the EVM hmac. Mimi > We could also set only SMACK64 in smack_inode_init_security(), and move > SMACKTRANSMUTE64 later, when we figure out how to fix the case of > overlayfs. > > IMA and EVM would work in both cases.
On Thu, 2023-04-20 at 06:44 -0400, Mimi Zohar wrote: > On Thu, 2023-04-20 at 10:50 +0200, Roberto Sassu wrote: > > > It's possible. It's been a long time since I've looked at this. > > > I'm tempted to take a change to make overlayfs work upstream and > > > then worry about the ima changes. There seems to be a lot more > > > going on with the ima changes than is obvious from what's in the > > > Smack code. > > It doesn't sound like the patch set introduces the overlayfs bug. Correct. The first problem of overlayfs is that smack_dentry_create_files_as() override the credentials in a way that transmuting is not detected correctly anymore in smack_inode_init_security(). The process label is already overwritten with the directory label, at the time smack_inode_i nit_security() calls smk_access_entry(), so the latter will not find the transmuting rule that refers to the original process label. The second problem is that overlayfs temporarily creates the new directory in the working directory, that does not necessarily have the same label of the parent directory the new file/directory will be added to, causing the transmuting to be computed incorrectly. > The security_inode_init_security() change to initialize multiple LSMs > and IMA xattrs and include them in the EVM hmac calculation is straight > forward. > > In addition, the patch set creates the infrastructure for allowing > multiple per LSM xattrs, as requested, to be initialized in > security_inode_init_security() and included in the EVM hmac. > > Mimi > > > We could also set only SMACK64 in smack_inode_init_security(), and move > > SMACKTRANSMUTE64 later, when we figure out how to fix the case of > > overlayfs. > > > > IMA and EVM would work in both cases. Thanks to Mimi, I realized that adding SMACKTRANSMUTE64 in smack_inode_init_security() is actually necessary. Calling __vfs_getxattr() in smack_d_instantiate() causes the xattr to be added without EVM updating the HMAC (thus, making the HMAC invalid). Thanks Roberto
On Thu, 2023-04-20 at 10:48 +0200, Roberto Sassu wrote: > On Wed, 2023-04-19 at 12:25 -0700, Mengchi Cheng wrote: > > > I got some errors during xattr removal, so not sure if my patch was > > > working properly or not (it happened also without it, didn't > > > investigate more). > > > > > > However, I saw another discussion related to transmute: > > > > > > https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@amazon.com/ > > > > > > I add the people in CC. > > > > > > The steps described were so easy to understand and executed, I tried > > > without and with overlayfs. > > > > > > Without: > > > > > > # echo "_ system rwxatl" > /sys/fs/smackfs/load2 > > > # mkdir /data > > > # chsmack -a "system" /data > > > # chsmack -t /data > > > # mkdir -p /data/dir1/dir2 > > > # chsmack /data/dir1 > > > /data/dir1 access="system" transmute="TRUE" > > > # chsmack /data/dir1/dir2 > > > /data/dir1/dir2 access="system" transmute="TRUE" > > > > > > It seems to work, right? > > > > > > With overlay fs it didn't work, same result as the one Mengchi > > > reported. Since Mengchi's solution was to set SMK_INODE_CHANGED, and I > > > want to get rid of it, I thought to investigate more. > > > > > > Looking at smack_dentry_create_files_as(), I see that the label of the > > > process is overwritten with the label of the transmuting directory. > > > > > > That causes smack_inode_init_security() to lookup the transmuting rule > > > on the overridden credential, and not on the original one. > > > > > > In the example above, it means that, when overlayfs is creating the new > > > inode, the label of the process is system, not _. So no transmute > > > permission, and also the xattr will not be added, as observed by > > > Mengchi. > > > > > > Hopefully I undertood the code, so in this particular case we would not > > > need to override the label of the process in smack_dentry_create_files_ > > > as(). > > > > > > If you see smack_inode_init_security(): > > > > > > struct smack_known *skp = smk_of_current(); > > > struct smack_known *isp = smk_of_inode(inode); > > > struct smack_known *dsp = smk_of_inode(dir); > > > > > > [...] > > > > > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > > > smk_inode_transmutable(dir)) { > > > isp = dsp; > > > [...] > > > > > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > > > > > This code is telling, if there is a transmute rule, and the directory > > > is transmuting, set the label of the new inode to the label of the > > > directory. That should be already the result that we wanted to obtain. > > > > > > The current code should have been doing it by overriding the label of > > > the process in smack_dentry_create_files_as() with the label of the > > > parent directory, and letting the inode being created with the > > > overridden label of the process. The transmute xattr is not set due to > > > the problem described above. > > > > > > So, as a quick test, I kept this patch with the change to xattr2->name, > > > and skipped the label override in smack_dentry_create_files_as(). It > > > worked, I get the same result as without overlayfs. Wondering if the > > > process label override is necessary in other cases. > > > > If I understand correctly, removing the if block below is what you suggested. > > Yes, more or less is what I did. > > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > > index cfcbb748da25..a867288e9de9 100644 > > --- a/security/smack/smack_lsm.c > > +++ b/security/smack/smack_lsm.c > > @@ -4769,8 +4769,8 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode, > > * providing access is transmuting use the containing > > * directory label instead of the process label. > > */ > > - if (may > 0 && (may & MAY_TRANSMUTE)) > > - ntsp->smk_task = isp->smk_inode; > > +// if (may > 0 && (may & MAY_TRANSMUTE)) > > +// ntsp->smk_task = isp->smk_inode; > > } > > return 0; > > } > > > > This way will have issue in the following situation on the vanila kernel. > > data in the lowerdir has "_" label before overlay and dir1 is already > > created in the lowerdir. > > # chsmack /data > > /data access="_" > > # chsmack /data/dir1 > > /data/dir1 access="system" transmute="TRUE" > > Apply overlay on data directory and set the smack rule in the same way. > > data has the same smack label. > > # chsmack /data > > /data access="system" transmute="TRUE" > > I'm using an older kernel, but I get _ instead of system. > > > After that, remove dir1 and mkdir dir1 again. dir1 did not get the correct > > label. > > # rm -r /data/dir1 > > # mkdir -p /data/dir1 > > # chsmack /data/dir1 > > /data/dir1 access="_" > > Unfortunately, it cannot work: > > Thread 3 hit Breakpoint 1, smack_inode_init_security (...) at security/smack/smack_lsm.c:959 > 959 { > (gdb) p dir->i_ino > $12 = 9169116 > (gdb) p dsp > $13 = (struct smack_known *) 0xffffffff831fc0a0 <smack_known_floor> > > > ls -i /home/root/data_work/ > 9169116 work > > So, transmuting is decided on the working directory. Actually, after studying the code better, this is what security_dentry_create_files_as() is useful for. * Compute a context for a dentry as the inode is not yet available and set * that context in passed in creds so that new files are created using that * context. Context is calculated using the passed in creds and not the creds * of the caller. And Smack is doing: if (may > 0 && (may & MAY_TRANSMUTE)) { ntsp->smk_task = isp->smk_inode; The new inode will be created with the label of the current task, that was replaced with the label of the parent directory (see above) in smac k_dentry_create_files_as(). I think the reason why Mengchi was not obtaining the desired label when replacing /data/dir1 was because /data is incorrectly labeled. To ensure that /data has label 'system' and transmute is true, I added smackfstransmute=system to the mount options. However, at the beginning, it seemed that it didn't work: # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data # chsmack /data /data access="system" I found that the reason for this is that smack_inode_getsecurity() retrieves metadata from the inode only for SMACK64, and the rest from xattrs (which would not work for mount options). I just made a patch to handle SMACK64TRANSMUTE too. With the patch applied, I correctly get: # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data # chsmack /data /data access="system" transmute="TRUE" With the root inode correctly labeled, I get: # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data # rm -Rf /data/dir1 # mkdir /data/dir1 # chsmack /data/dir1 /data/dir1 access="system" This is partially correct, transmute="TRUE" is missing. Judging from smk_task, we cannot determine in smack_inode_init_security () if transmuting was successful in smack_dentry_create_files_as(). We need an extra information. Mengchi's solution was to add the new inode as parameter to security_dentry_create_files_as(), so that SMK_INODE_CHANGED can be set in smk_flags, and SMACK64TRANSMUTE is set in smack_d_instantiate(). One concern could be that preallocating the inode maybe is overlayfs- specific? A comment also says that we might not use that one: err = ovl_create_or_link(dentry, inode, &attr, false); /* Did we end up using the preallocated inode? */ if (inode != d_inode(dentry)) We could achieve the same goal without adding a new parameter to security_dentry_create_files_as() and, instead, by adding a new field in the task_smack structure, smk_transmuted, that is set to smk_task when transmuting is successful. Then, if smk_task == smk_transmuted, smack_inode_init_security() would set SMK_INODE_CHANGED. On top of that, I would instead just provide the second xattr SMACK64TRANSMUTE, in addition to SMACK64. Will send the patches for upstream first, and then switch to providing SMACK64TRANSMUTE in smack_inode_init_security(), in a new version of the EVM patch set. Thanks Roberto > If I do: > > # chsmack -a system -t /home/root/data_work/work/ > # mkdir /data/dir1 > # chsmack /data/dir1 > /data/dir1 access="system" transmute="TRUE" > > I obtain the expected result. However, this problem is due to how overlayfs works: > > static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, > struct ovl_cattr *cattr) > { > > [...] > > newdentry = ovl_create_temp(ofs, workdir, cattr); > err = PTR_ERR(newdentry); > if (IS_ERR(newdentry)) > goto out_dput; > > > The good news seems to be that, once you set the label to the correct > directory, transmuting works with the changes I proposed. > > Roberto > > > Since I am not very familiar your change. Could you help check with your > > patch will this issue also happen? > > > > > > Best, > > Mengchi > > > > > > > > Roberto
On Mon, 2023-05-08 12:29:42 +0000, Roberto Sassu wrote: > > On Thu, 2023-04-20 at 10:48 +0200, Roberto Sassu wrote: > > On Wed, 2023-04-19 at 12:25 -0700, Mengchi Cheng wrote: > > > > I got some errors during xattr removal, so not sure if my patch was > > > > working properly or not (it happened also without it, didn't > > > > investigate more). > > > > > > > > However, I saw another discussion related to transmute: > > > > > > > > https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@amazon.com/ > > > > > > > > I add the people in CC. > > > > > > > > The steps described were so easy to understand and executed, I tried > > > > without and with overlayfs. > > > > > > > > Without: > > > > > > > > # echo "_ system rwxatl" > /sys/fs/smackfs/load2 > > > > # mkdir /data > > > > # chsmack -a "system" /data > > > > # chsmack -t /data > > > > # mkdir -p /data/dir1/dir2 > > > > # chsmack /data/dir1 > > > > /data/dir1 access="system" transmute="TRUE" > > > > # chsmack /data/dir1/dir2 > > > > /data/dir1/dir2 access="system" transmute="TRUE" > > > > > > > > It seems to work, right? > > > > > > > > With overlay fs it didn't work, same result as the one Mengchi > > > > reported. Since Mengchi's solution was to set SMK_INODE_CHANGED, and I > > > > want to get rid of it, I thought to investigate more. > > > > > > > > Looking at smack_dentry_create_files_as(), I see that the label of the > > > > process is overwritten with the label of the transmuting directory. > > > > > > > > That causes smack_inode_init_security() to lookup the transmuting rule > > > > on the overridden credential, and not on the original one. > > > > > > > > In the example above, it means that, when overlayfs is creating the new > > > > inode, the label of the process is system, not _. So no transmute > > > > permission, and also the xattr will not be added, as observed by > > > > Mengchi. > > > > > > > > Hopefully I undertood the code, so in this particular case we would not > > > > need to override the label of the process in smack_dentry_create_files_ > > > > as(). > > > > > > > > If you see smack_inode_init_security(): > > > > > > > > struct smack_known *skp = smk_of_current(); > > > > struct smack_known *isp = smk_of_inode(inode); > > > > struct smack_known *dsp = smk_of_inode(dir); > > > > > > > > [...] > > > > > > > > if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && > > > > smk_inode_transmutable(dir)) { > > > > isp = dsp; > > > > [...] > > > > > > > > xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > > > > > > > This code is telling, if there is a transmute rule, and the directory > > > > is transmuting, set the label of the new inode to the label of the > > > > directory. That should be already the result that we wanted to obtain. > > > > > > > > The current code should have been doing it by overriding the label of > > > > the process in smack_dentry_create_files_as() with the label of the > > > > parent directory, and letting the inode being created with the > > > > overridden label of the process. The transmute xattr is not set due to > > > > the problem described above. > > > > > > > > So, as a quick test, I kept this patch with the change to xattr2->name, > > > > and skipped the label override in smack_dentry_create_files_as(). It > > > > worked, I get the same result as without overlayfs. Wondering if the > > > > process label override is necessary in other cases. > > > > > > If I understand correctly, removing the if block below is what you suggested. > > > > Yes, more or less is what I did. > > > > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > > > index cfcbb748da25..a867288e9de9 100644 > > > --- a/security/smack/smack_lsm.c > > > +++ b/security/smack/smack_lsm.c > > > @@ -4769,8 +4769,8 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode, > > > * providing access is transmuting use the containing > > > * directory label instead of the process label. > > > */ > > > - if (may > 0 && (may & MAY_TRANSMUTE)) > > > - ntsp->smk_task = isp->smk_inode; > > > +// if (may > 0 && (may & MAY_TRANSMUTE)) > > > +// ntsp->smk_task = isp->smk_inode; > > > } > > > return 0; > > > } > > > > > > This way will have issue in the following situation on the vanila kernel. > > > data in the lowerdir has "_" label before overlay and dir1 is already > > > created in the lowerdir. > > > # chsmack /data > > > /data access="_" > > > # chsmack /data/dir1 > > > /data/dir1 access="system" transmute="TRUE" > > > Apply overlay on data directory and set the smack rule in the same way. > > > data has the same smack label. > > > # chsmack /data > > > /data access="system" transmute="TRUE" > > > > I'm using an older kernel, but I get _ instead of system. > > > > > After that, remove dir1 and mkdir dir1 again. dir1 did not get the correct > > > label. > > > # rm -r /data/dir1 > > > # mkdir -p /data/dir1 > > > # chsmack /data/dir1 > > > /data/dir1 access="_" > > > > Unfortunately, it cannot work: > > > > Thread 3 hit Breakpoint 1, smack_inode_init_security (...) at security/smack/smack_lsm.c:959 > > 959 { > > (gdb) p dir->i_ino > > $12 = 9169116 > > (gdb) p dsp > > $13 = (struct smack_known *) 0xffffffff831fc0a0 <smack_known_floor> > > > > > > ls -i /home/root/data_work/ > > 9169116 work > > > > So, transmuting is decided on the working directory. > > Actually, after studying the code better, this is > what security_dentry_create_files_as() is useful for. > > * Compute a context for a dentry as the inode is not yet available and set > * that context in passed in creds so that new files are created using that > * context. Context is calculated using the passed in creds and not the creds > * of the caller. > > And Smack is doing: > > if (may > 0 && (may & MAY_TRANSMUTE)) { > ntsp->smk_task = isp->smk_inode; > > The new inode will be created with the label of the current task, that > was replaced with the label of the parent directory (see above) in smac > k_dentry_create_files_as(). > > I think the reason why Mengchi was not obtaining the desired label when > replacing /data/dir1 was because /data is incorrectly labeled. > > To ensure that /data has label 'system' and transmute is true, I added > smackfstransmute=system to the mount options. > > However, at the beginning, it seemed that it didn't work: > > # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data > # chsmack /data > /data access="system" > > I found that the reason for this is that smack_inode_getsecurity() > retrieves metadata from the inode only for SMACK64, and the rest from > xattrs (which would not work for mount options). I just made a patch to > handle SMACK64TRANSMUTE too. > > With the patch applied, I correctly get: > > # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data > # chsmack /data > /data access="system" transmute="TRUE" > > With the root inode correctly labeled, I get: > > # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data > # rm -Rf /data/dir1 > # mkdir /data/dir1 > # chsmack /data/dir1 > /data/dir1 access="system" > > This is partially correct, transmute="TRUE" is missing. > > Judging from smk_task, we cannot determine in smack_inode_init_security > () if transmuting was successful in smack_dentry_create_files_as(). We > need an extra information. > > Mengchi's solution was to add the new inode as parameter > to security_dentry_create_files_as(), so that SMK_INODE_CHANGED can be > set in smk_flags, and SMACK64TRANSMUTE is set in smack_d_instantiate(). > > One concern could be that preallocating the inode maybe is overlayfs- > specific? A comment also says that we might not use that one: security_dentry_create_files_as() is only referred in the fs/overlayfs/dir.c. inode is for sure preallocated in this case. I am glad you find a way without modifying security api. I did some tests with your patch on our system. It works as expected. I am ok to drop mine. Best, Mengchi > > err = ovl_create_or_link(dentry, inode, &attr, false); > /* Did we end up using the preallocated inode? */ > if (inode != d_inode(dentry)) > > We could achieve the same goal without adding a new parameter to security_dentry_create_files_as() and, instead, by adding a new field > in the task_smack structure, smk_transmuted, that is set to smk_task > when transmuting is successful. > > Then, if smk_task == smk_transmuted, smack_inode_init_security() would > set SMK_INODE_CHANGED. On top of that, I would instead just provide the > second xattr SMACK64TRANSMUTE, in addition to SMACK64. > > Will send the patches for upstream first, and then switch to providing > SMACK64TRANSMUTE in smack_inode_init_security(), in a new version of > the EVM patch set. > > Thanks > > Roberto > > > If I do: > > > > # chsmack -a system -t /home/root/data_work/work/ > > # mkdir /data/dir1 > > # chsmack /data/dir1 > > /data/dir1 access="system" transmute="TRUE" > > > > I obtain the expected result. However, this problem is due to how overlayfs works: > > > > static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, > > struct ovl_cattr *cattr) > > { > > > > [...] > > > > newdentry = ovl_create_temp(ofs, workdir, cattr); > > err = PTR_ERR(newdentry); > > if (IS_ERR(newdentry)) > > goto out_dput; > > > > > > The good news seems to be that, once you set the label to the correct > > directory, transmuting works with the changes I proposed. > > > > Roberto > > > > > Since I am not very familiar your change. Could you help check with your > > > patch will this issue also happen? > > > > > > > > > Best, > > > Mengchi > > > > > > > > > > > Roberto > >
On 5/9/2023 4:44 PM, Mengchi Cheng wrote: > On Mon, 2023-05-08 12:29:42 +0000, Roberto Sassu wrote: >> On Thu, 2023-04-20 at 10:48 +0200, Roberto Sassu wrote: >>> On Wed, 2023-04-19 at 12:25 -0700, Mengchi Cheng wrote: >>>>> I got some errors during xattr removal, so not sure if my patch was >>>>> working properly or not (it happened also without it, didn't >>>>> investigate more). >>>>> >>>>> However, I saw another discussion related to transmute: >>>>> >>>>> https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@amazon.com/ >>>>> >>>>> I add the people in CC. >>>>> >>>>> The steps described were so easy to understand and executed, I tried >>>>> without and with overlayfs. >>>>> >>>>> Without: >>>>> >>>>> # echo "_ system rwxatl" > /sys/fs/smackfs/load2 >>>>> # mkdir /data >>>>> # chsmack -a "system" /data >>>>> # chsmack -t /data >>>>> # mkdir -p /data/dir1/dir2 >>>>> # chsmack /data/dir1 >>>>> /data/dir1 access="system" transmute="TRUE" >>>>> # chsmack /data/dir1/dir2 >>>>> /data/dir1/dir2 access="system" transmute="TRUE" >>>>> >>>>> It seems to work, right? >>>>> >>>>> With overlay fs it didn't work, same result as the one Mengchi >>>>> reported. Since Mengchi's solution was to set SMK_INODE_CHANGED, and I >>>>> want to get rid of it, I thought to investigate more. >>>>> >>>>> Looking at smack_dentry_create_files_as(), I see that the label of the >>>>> process is overwritten with the label of the transmuting directory. >>>>> >>>>> That causes smack_inode_init_security() to lookup the transmuting rule >>>>> on the overridden credential, and not on the original one. >>>>> >>>>> In the example above, it means that, when overlayfs is creating the new >>>>> inode, the label of the process is system, not _. So no transmute >>>>> permission, and also the xattr will not be added, as observed by >>>>> Mengchi. >>>>> >>>>> Hopefully I undertood the code, so in this particular case we would not >>>>> need to override the label of the process in smack_dentry_create_files_ >>>>> as(). >>>>> >>>>> If you see smack_inode_init_security(): >>>>> >>>>> struct smack_known *skp = smk_of_current(); >>>>> struct smack_known *isp = smk_of_inode(inode); >>>>> struct smack_known *dsp = smk_of_inode(dir); >>>>> >>>>> [...] >>>>> >>>>> if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && >>>>> smk_inode_transmutable(dir)) { >>>>> isp = dsp; >>>>> [...] >>>>> >>>>> xattr->value = kstrdup(isp->smk_known, GFP_NOFS); >>>>> >>>>> This code is telling, if there is a transmute rule, and the directory >>>>> is transmuting, set the label of the new inode to the label of the >>>>> directory. That should be already the result that we wanted to obtain. >>>>> >>>>> The current code should have been doing it by overriding the label of >>>>> the process in smack_dentry_create_files_as() with the label of the >>>>> parent directory, and letting the inode being created with the >>>>> overridden label of the process. The transmute xattr is not set due to >>>>> the problem described above. >>>>> >>>>> So, as a quick test, I kept this patch with the change to xattr2->name, >>>>> and skipped the label override in smack_dentry_create_files_as(). It >>>>> worked, I get the same result as without overlayfs. Wondering if the >>>>> process label override is necessary in other cases. >>>> If I understand correctly, removing the if block below is what you suggested. >>> Yes, more or less is what I did. >>> >>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >>>> index cfcbb748da25..a867288e9de9 100644 >>>> --- a/security/smack/smack_lsm.c >>>> +++ b/security/smack/smack_lsm.c >>>> @@ -4769,8 +4769,8 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode, >>>> * providing access is transmuting use the containing >>>> * directory label instead of the process label. >>>> */ >>>> - if (may > 0 && (may & MAY_TRANSMUTE)) >>>> - ntsp->smk_task = isp->smk_inode; >>>> +// if (may > 0 && (may & MAY_TRANSMUTE)) >>>> +// ntsp->smk_task = isp->smk_inode; >>>> } >>>> return 0; >>>> } >>>> >>>> This way will have issue in the following situation on the vanila kernel. >>>> data in the lowerdir has "_" label before overlay and dir1 is already >>>> created in the lowerdir. >>>> # chsmack /data >>>> /data access="_" >>>> # chsmack /data/dir1 >>>> /data/dir1 access="system" transmute="TRUE" >>>> Apply overlay on data directory and set the smack rule in the same way. >>>> data has the same smack label. >>>> # chsmack /data >>>> /data access="system" transmute="TRUE" >>> I'm using an older kernel, but I get _ instead of system. >>> >>>> After that, remove dir1 and mkdir dir1 again. dir1 did not get the correct >>>> label. >>>> # rm -r /data/dir1 >>>> # mkdir -p /data/dir1 >>>> # chsmack /data/dir1 >>>> /data/dir1 access="_" >>> Unfortunately, it cannot work: >>> >>> Thread 3 hit Breakpoint 1, smack_inode_init_security (...) at security/smack/smack_lsm.c:959 >>> 959 { >>> (gdb) p dir->i_ino >>> $12 = 9169116 >>> (gdb) p dsp >>> $13 = (struct smack_known *) 0xffffffff831fc0a0 <smack_known_floor> >>> >>> >>> ls -i /home/root/data_work/ >>> 9169116 work >>> >>> So, transmuting is decided on the working directory. >> Actually, after studying the code better, this is >> what security_dentry_create_files_as() is useful for. >> >> * Compute a context for a dentry as the inode is not yet available and set >> * that context in passed in creds so that new files are created using that >> * context. Context is calculated using the passed in creds and not the creds >> * of the caller. >> >> And Smack is doing: >> >> if (may > 0 && (may & MAY_TRANSMUTE)) { >> ntsp->smk_task = isp->smk_inode; >> >> The new inode will be created with the label of the current task, that >> was replaced with the label of the parent directory (see above) in smac >> k_dentry_create_files_as(). >> >> I think the reason why Mengchi was not obtaining the desired label when >> replacing /data/dir1 was because /data is incorrectly labeled. >> >> To ensure that /data has label 'system' and transmute is true, I added >> smackfstransmute=system to the mount options. >> >> However, at the beginning, it seemed that it didn't work: >> >> # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data >> # chsmack /data >> /data access="system" >> >> I found that the reason for this is that smack_inode_getsecurity() >> retrieves metadata from the inode only for SMACK64, and the rest from >> xattrs (which would not work for mount options). I just made a patch to >> handle SMACK64TRANSMUTE too. >> >> With the patch applied, I correctly get: >> >> # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data >> # chsmack /data >> /data access="system" transmute="TRUE" >> >> With the root inode correctly labeled, I get: >> >> # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data >> # rm -Rf /data/dir1 >> # mkdir /data/dir1 >> # chsmack /data/dir1 >> /data/dir1 access="system" >> >> This is partially correct, transmute="TRUE" is missing. >> >> Judging from smk_task, we cannot determine in smack_inode_init_security >> () if transmuting was successful in smack_dentry_create_files_as(). We >> need an extra information. >> >> Mengchi's solution was to add the new inode as parameter >> to security_dentry_create_files_as(), so that SMK_INODE_CHANGED can be >> set in smk_flags, and SMACK64TRANSMUTE is set in smack_d_instantiate(). >> >> One concern could be that preallocating the inode maybe is overlayfs- >> specific? A comment also says that we might not use that one: > security_dentry_create_files_as() is only referred in the fs/overlayfs/dir.c. > inode is for sure preallocated in this case. > I am glad you find a way without modifying security api. I did some tests > with your patch on our system. It works as expected. I am ok to drop mine. Excellent. Thank you for verifying the patches. I have a few tests of my own to complete, but I expect to move it into next shortly. > > Best, > Mengchi > >> err = ovl_create_or_link(dentry, inode, &attr, false); >> /* Did we end up using the preallocated inode? */ >> if (inode != d_inode(dentry)) >> >> We could achieve the same goal without adding a new parameter to security_dentry_create_files_as() and, instead, by adding a new field >> in the task_smack structure, smk_transmuted, that is set to smk_task >> when transmuting is successful. >> >> Then, if smk_task == smk_transmuted, smack_inode_init_security() would >> set SMK_INODE_CHANGED. On top of that, I would instead just provide the >> second xattr SMACK64TRANSMUTE, in addition to SMACK64. >> >> Will send the patches for upstream first, and then switch to providing >> SMACK64TRANSMUTE in smack_inode_init_security(), in a new version of >> the EVM patch set. >> >> Thanks >> >> Roberto >> >>> If I do: >>> >>> # chsmack -a system -t /home/root/data_work/work/ >>> # mkdir /data/dir1 >>> # chsmack /data/dir1 >>> /data/dir1 access="system" transmute="TRUE" >>> >>> I obtain the expected result. However, this problem is due to how overlayfs works: >>> >>> static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, >>> struct ovl_cattr *cattr) >>> { >>> >>> [...] >>> >>> newdentry = ovl_create_temp(ofs, workdir, cattr); >>> err = PTR_ERR(newdentry); >>> if (IS_ERR(newdentry)) >>> goto out_dput; >>> >>> >>> The good news seems to be that, once you set the label to the correct >>> directory, transmuting works with the changes I proposed. >>> >>> Roberto >>> >>>> Since I am not very familiar your change. Could you help check with your >>>> patch will this issue also happen? >>>> >>>> >>>> Best, >>>> Mengchi >>>> >>>>> >>>>> Roberto >>
diff --git a/security/smack/smack.h b/security/smack/smack.h index e2239be7bd6..f00c8498c60 100644 --- a/security/smack/smack.h +++ b/security/smack/smack.h @@ -127,7 +127,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 8392983334b..b43820bdbd0 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -54,12 +54,12 @@ /* * Smack uses multiple xattrs. - * SMACK64 - for access control, SMACK64EXEC - label for the program, - * SMACK64MMAP - controls library loading, + * SMACK64 - for access control, * SMACK64TRANSMUTE - label initialization, - * Not saved on files - SMACK64IPIN and SMACK64IPOUT + * Not saved on files - SMACK64IPIN and SMACK64IPOUT, + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP */ -#define SMACK_INODE_INIT_XATTRS 4 +#define SMACK_INODE_INIT_XATTRS 2 #ifdef SMACK_IPV6_PORT_LABELING static DEFINE_MUTEX(smack_ipv6_lock); @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, const struct qstr *qstr, struct xattr *xattrs, int *xattr_count) { - struct inode_smack *issp = smack_inode(inode); struct smack_known *skp = smk_of_current(); struct smack_known *isp = smk_of_inode(inode); struct smack_known *dsp = smk_of_inode(dir); struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count); + struct xattr *xattr2; int may; if (xattr) { @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir, if (may > 0 && ((may & MAY_TRANSMUTE) != 0) && smk_inode_transmutable(dir)) { isp = dsp; - issp->smk_flags |= SMK_INODE_CHANGED; + xattr2 = lsm_get_xattr_slot(xattrs, xattr_count); + if (xattr2) { + xattr2->value = kmemdup(TRANS_TRUE, + TRANS_TRUE_SIZE, + GFP_NOFS); + if (xattr2->value == NULL) + return -ENOMEM; + + xattr2->value_len = TRANS_TRUE_SIZE; + xattr2->name = XATTR_NAME_SMACKTRANSMUTE; + } } xattr->value = kstrdup(isp->smk_known, GFP_NOFS); @@ -3512,20 +3522,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; }