| Message ID | 20221110094639.3086409-3-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | evm: Prepare for moving to the LSM infrastructure | expand |
Hi Roberto, On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Rewrite security_old_inode_init_security() to call > security_inode_init_security() before making changes to support multiple > LSMs providing xattrs. Do it so that the required changes are done only in > one place. Only security_inode_init_security() has support for EVM. Making security_old_inode_init_security() a wrapper for security_inode_init_security() could result in security.evm extended attributes being created that previously weren't created. In fact ocfs2 defines ocfs2_init_security_get() as a wrapper for both the old and new inode_init_security calls based on the caller's preference. Only mknod and symlink seem to use the old function. Wondering why do they differentiate between callers? (Cc'ing the ocfs2 mailing list as they're affected by this change.) "[PATCH v4 1/5] reiserfs: Add missing calls to reiserfs_security_free()" fixed a memory leak. I couldn't tell if there was a similar memory leak in ocfs2, the only other user of security_old_inode_init_security(). As ocfs2 already defines initxattrs, that leaves only reiserfs missing initxattrs(). A better, cleaner solution would be to define one. thanks, Mimi > > Define the security_initxattrs() callback and pass it to > security_inode_init_security() as argument, to obtain the first xattr > provided by LSMs. > > This behavior is a bit different from the current one. Before this patch > calling call_int_hook() could cause multiple LSMs to provide an xattr, > since call_int_hook() does not stop when an LSM returns zero. The caller of > security_old_inode_init_security() receives the last xattr set. The pointer > of the xattr value of previous LSMs is lost, causing memory leaks. > > However, in practice, this scenario does not happen as the only in-tree > LSMs providing an xattr at inode creation time are SELinux and Smack, which > are mutually exclusive. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>b
On 11/17/2022 2:03 PM, Mimi Zohar wrote: > Hi Roberto, > > On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote: >> From: Roberto Sassu <roberto.sassu@huawei.com> >> >> Rewrite security_old_inode_init_security() to call >> security_inode_init_security() before making changes to support multiple >> LSMs providing xattrs. Do it so that the required changes are done only in >> one place. > > Only security_inode_init_security() has support for EVM. Making > security_old_inode_init_security() a wrapper for > security_inode_init_security() could result in security.evm extended > attributes being created that previously weren't created. Hi Mimi yes, I thought about this problem. In fact, it should not matter too much. Since security_old_inode_init_security() supports setting only one xattr: if there is an LSM xattr, that one will be set, and the EVM one will be discarded; if there is no LSM xattr, EVM would not add one. > In fact ocfs2 defines ocfs2_init_security_get() as a wrapper for both > the old and new inode_init_security calls based on the caller's > preference. Only mknod and symlink seem to use the old function. > Wondering why do they differentiate between callers? (Cc'ing the ocfs2 > mailing list as they're affected by this change.) > > "[PATCH v4 1/5] reiserfs: Add missing calls to > reiserfs_security_free()" fixed a memory leak. I couldn't tell if > there was a similar memory leak in ocfs2, the only other user of > security_old_inode_init_security(). Will look into it. > As ocfs2 already defines initxattrs, that leaves only reiserfs missing > initxattrs(). A better, cleaner solution would be to define one. Yes, great idea! Thanks Roberto > thanks, > > Mimi > >> >> Define the security_initxattrs() callback and pass it to >> security_inode_init_security() as argument, to obtain the first xattr >> provided by LSMs. >> >> This behavior is a bit different from the current one. Before this patch >> calling call_int_hook() could cause multiple LSMs to provide an xattr, >> since call_int_hook() does not stop when an LSM returns zero. The caller of >> security_old_inode_init_security() receives the last xattr set. The pointer >> of the xattr value of previous LSMs is lost, causing memory leaks. >> >> However, in practice, this scenario does not happen as the only in-tree >> LSMs providing an xattr at inode creation time are SELinux and Smack, which >> are mutually exclusive. >> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>b
On Thu, 2022-11-17 at 08:03 -0500, Mimi Zohar wrote: > Hi Roberto, > > On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > Rewrite security_old_inode_init_security() to call > > security_inode_init_security() before making changes to support multiple > > LSMs providing xattrs. Do it so that the required changes are done only in > > one place. > > Only security_inode_init_security() has support for EVM. Making > security_old_inode_init_security() a wrapper for > security_inode_init_security() could result in security.evm extended > attributes being created that previously weren't created. > > In fact ocfs2 defines ocfs2_init_security_get() as a wrapper for both > the old and new inode_init_security calls based on the caller's > preference. Only mknod and symlink seem to use the old function. > Wondering why do they differentiate between callers? (Cc'ing the ocfs2 > mailing list as they're affected by this change.) > > "[PATCH v4 1/5] reiserfs: Add missing calls to > reiserfs_security_free()" fixed a memory leak. I couldn't tell if > there was a similar memory leak in ocfs2, the only other user of > security_old_inode_init_security(). From what I see, there is no memory leak there. > As ocfs2 already defines initxattrs, that leaves only reiserfs missing > initxattrs(). A better, cleaner solution would be to define one. If I understood why security_old_inode_init_security() is called instead of security_inode_init_security(), the reason seems that the filesystem code uses the length of the obtained xattr to make some calculations (e.g. reserve space). The xattr is written at a later time. Since for reiserfs there is a plan to deprecate it, it probably wouldn't be worth to support the creation of multiple xattrs. I would define a callback to take the first xattr and make a copy, so that calling security_inode_init_security() + reiserfs_initxattrs() is equivalent to calling security_old_inode_init_security(). But then, this is what anyway I was doing with the security_initxattrs() callback, for all callers of security_old_inode_i nit_security(). Also, security_old_inode_init_security() is exported to kernel modules. Maybe, it is used somewhere. So, unless we plan to remove it completely, it should be probably be fixed to avoid multiple LSMs successfully setting an xattr, and losing the memory of all except the last (which this patch fixes by calling security_inode_init_security()) . If there is still the preference, I will implement the reiserfs callback and make a fix for security_old_inode_init_security(). Thanks Roberto > thanks, > > Mimi > > > Define the security_initxattrs() callback and pass it to > > security_inode_init_security() as argument, to obtain the first xattr > > provided by LSMs. > > > > This behavior is a bit different from the current one. Before this patch > > calling call_int_hook() could cause multiple LSMs to provide an xattr, > > since call_int_hook() does not stop when an LSM returns zero. The caller of > > security_old_inode_init_security() receives the last xattr set. The pointer > > of the xattr value of previous LSMs is lost, causing memory leaks. > > > > However, in practice, this scenario does not happen as the only in-tree > > LSMs providing an xattr at inode creation time are SELinux and Smack, which > > are mutually exclusive. > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>b
On Mon, 2022-11-21 at 10:45 +0100, Roberto Sassu wrote: > > As ocfs2 already defines initxattrs, that leaves only reiserfs missing > > initxattrs(). A better, cleaner solution would be to define one. > > If I understood why security_old_inode_init_security() is called > instead of security_inode_init_security(), the reason seems that the > filesystem code uses the length of the obtained xattr to make some > calculations (e.g. reserve space). The xattr is written at a later > time. > > Since for reiserfs there is a plan to deprecate it, it probably > wouldn't be worth to support the creation of multiple xattrs. I would > define a callback to take the first xattr and make a copy, so that > calling security_inode_init_security() + reiserfs_initxattrs() is > equivalent to calling security_old_inode_init_security(). > > But then, this is what anyway I was doing with the > security_initxattrs() callback, for all callers of security_old_inode_i > nit_security(). > > Also, security_old_inode_init_security() is exported to kernel modules. > Maybe, it is used somewhere. So, unless we plan to remove it > completely, it should be probably be fixed to avoid multiple LSMs > successfully setting an xattr, and losing the memory of all except the > last (which this patch fixes by calling security_inode_init_security()) > . > > If there is still the preference, I will implement the reiserfs > callback and make a fix for security_old_inode_init_security(). There's no sense in doing both, as the purpose of defining a reiserfs initxattrs function was to clean up this code making it more readable. Mimi
On Mon, Nov 21, 2022 at 3:54 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Mon, 2022-11-21 at 10:45 +0100, Roberto Sassu wrote: > > > As ocfs2 already defines initxattrs, that leaves only reiserfs missing > > > initxattrs(). A better, cleaner solution would be to define one. > > > > If I understood why security_old_inode_init_security() is called > > instead of security_inode_init_security(), the reason seems that the > > filesystem code uses the length of the obtained xattr to make some > > calculations (e.g. reserve space). The xattr is written at a later > > time. > > > > Since for reiserfs there is a plan to deprecate it, it probably > > wouldn't be worth to support the creation of multiple xattrs. I would > > define a callback to take the first xattr and make a copy, so that > > calling security_inode_init_security() + reiserfs_initxattrs() is > > equivalent to calling security_old_inode_init_security(). FWIW, reiserfs isn't going to be removed until 2025, I'm hopeful we can remove the IMA/EVM special cases before then :) > > But then, this is what anyway I was doing with the > > security_initxattrs() callback, for all callers of security_old_inode_i > > nit_security(). > > > > Also, security_old_inode_init_security() is exported to kernel modules. > > Maybe, it is used somewhere. So, unless we plan to remove it > > completely, it should be probably be fixed to avoid multiple LSMs > > successfully setting an xattr, and losing the memory of all except the > > last (which this patch fixes by calling security_inode_init_security()). I would much rather remove security_old_inode_init_security() then worry about what out-of-tree modules might be using it. Hopefully we can resolve the ocfs2 usage and get ocfs2 exclusively on the new hook without too much trouble, which means all we have left is reiserfs ... how difficult would you expect the conversion to be for reiserfs? > > If there is still the preference, I will implement the reiserfs > > callback and make a fix for security_old_inode_init_security(). > > There's no sense in doing both, as the purpose of defining a reiserfs > initxattrs function was to clean up this code making it more readable. > > Mimi
On Mon, 2022-11-21 at 18:55 -0500, Paul Moore wrote: > On Mon, Nov 21, 2022 at 3:54 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Mon, 2022-11-21 at 10:45 +0100, Roberto Sassu wrote: > > > > As ocfs2 already defines initxattrs, that leaves only reiserfs missing > > > > initxattrs(). A better, cleaner solution would be to define one. > > > > > > If I understood why security_old_inode_init_security() is called > > > instead of security_inode_init_security(), the reason seems that the > > > filesystem code uses the length of the obtained xattr to make some > > > calculations (e.g. reserve space). The xattr is written at a later > > > time. > > > > > > Since for reiserfs there is a plan to deprecate it, it probably > > > wouldn't be worth to support the creation of multiple xattrs. I would > > > define a callback to take the first xattr and make a copy, so that > > > calling security_inode_init_security() + reiserfs_initxattrs() is > > > equivalent to calling security_old_inode_init_security(). > > FWIW, reiserfs isn't going to be removed until 2025, I'm hopeful we > can remove the IMA/EVM special cases before then :) Well, we are not that far... > > > But then, this is what anyway I was doing with the > > > security_initxattrs() callback, for all callers of security_old_inode_i > > > nit_security(). > > > > > > Also, security_old_inode_init_security() is exported to kernel modules. > > > Maybe, it is used somewhere. So, unless we plan to remove it > > > completely, it should be probably be fixed to avoid multiple LSMs > > > successfully setting an xattr, and losing the memory of all except the > > > last (which this patch fixes by calling security_inode_init_security()). > > I would much rather remove security_old_inode_init_security() then > worry about what out-of-tree modules might be using it. Hopefully we > can resolve the ocfs2 usage and get ocfs2 exclusively on the new hook > without too much trouble, which means all we have left is reiserfs ... > how difficult would you expect the conversion to be for reiserfs? Ok for removing security_old_inode_init_security(). For reiserfs, I guess maintaining the current behavior of setting only one xattr should be easy. For multiple xattrs, I need to understand exactly how many blocks need to be reserved. > > > If there is still the preference, I will implement the reiserfs > > > callback and make a fix for security_old_inode_init_security(). > > > > There's no sense in doing both, as the purpose of defining a reiserfs > > initxattrs function was to clean up this code making it more readable. The fix for security_old_inode_init_security(), stopping at the first LSM returning zero, was to avoid the memory leak. Will not be needed if the function is removed. Roberto
diff --git a/security/security.c b/security/security.c index 79d82cb6e469..a0e9b4ce2341 100644 --- a/security/security.c +++ b/security/security.c @@ -1089,20 +1089,34 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode, } EXPORT_SYMBOL(security_dentry_create_files_as); +static int security_initxattrs(struct inode *inode, const struct xattr *xattrs, + void *fs_info) +{ + struct xattr *dest = (struct xattr *)fs_info; + + dest->name = xattrs->name; + dest->value = xattrs->value; + dest->value_len = xattrs->value_len; + return 0; +} + int security_inode_init_security(struct inode *inode, struct inode *dir, const struct qstr *qstr, const initxattrs initxattrs, void *fs_data) { struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1]; struct xattr *lsm_xattr, *evm_xattr, *xattr; - int ret; + int ret = -EOPNOTSUPP; if (unlikely(IS_PRIVATE(inode))) - return 0; + goto out_exit; - if (!initxattrs) - return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, - dir, qstr, NULL, NULL, NULL); + if (!initxattrs || + (initxattrs == &security_initxattrs && !fs_data)) { + ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, + dir, qstr, NULL, NULL, NULL); + goto out_exit; + } memset(new_xattrs, 0, sizeof(new_xattrs)); lsm_xattr = new_xattrs; ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr, @@ -1118,8 +1132,19 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, goto out; ret = initxattrs(inode, new_xattrs, fs_data); out: - for (xattr = new_xattrs; xattr->value != NULL; xattr++) + for (xattr = new_xattrs; xattr->value != NULL; xattr++) { + /* + * Xattr value freed by the caller of + * security_old_inode_init_security(). + */ + if (xattr == new_xattrs && initxattrs == &security_initxattrs && + !ret) + continue; kfree(xattr->value); + } +out_exit: + if (initxattrs == &security_initxattrs) + return ret; return (ret == -EOPNOTSUPP) ? 0 : ret; } EXPORT_SYMBOL(security_inode_init_security); @@ -1136,10 +1161,23 @@ int security_old_inode_init_security(struct inode *inode, struct inode *dir, const struct qstr *qstr, const char **name, void **value, size_t *len) { - if (unlikely(IS_PRIVATE(inode))) - return -EOPNOTSUPP; - return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, - qstr, name, value, len); + struct xattr xattr = {}; + struct xattr *lsm_xattr = (value) ? &xattr : NULL; + int ret; + + ret = security_inode_init_security(inode, dir, qstr, + security_initxattrs, lsm_xattr); + if (ret) + return ret; + + if (name) + *name = lsm_xattr->name; + if (value) + *value = lsm_xattr->value; + if (len) + *len = lsm_xattr->value_len; + + return 0; } EXPORT_SYMBOL(security_old_inode_init_security);