From: Roberto Sassu roberto.sassu@huawei.com
Reiserfs sets a security xattr at inode creation time in two stages: first, it calls reiserfs_security_init() to obtain the xattr from active LSMs; then, it calls reiserfs_security_write() to actually write that xattr.
Unfortunately, it seems there is a wrong expectation that LSMs provide the full xattr name in the form 'security.<suffix>'. However, LSMs always provided just the suffix, causing reiserfs to not write the xattr at all (if the suffix is shorter than the prefix), or to write an xattr with the wrong name.
Add a temporary buffer in reiserfs_security_write(), and write to it the full xattr name, before passing it to reiserfs_xattr_set_handle().
Since the 'security.' prefix is always prepended, remove the name length check.
Cc: stable@vger.kernel.org # v2.6.x Fixes: 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- fs/reiserfs/xattr_security.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c index 6bffdf9a4fd..b0c354ab113 100644 --- a/fs/reiserfs/xattr_security.c +++ b/fs/reiserfs/xattr_security.c @@ -95,11 +95,13 @@ int reiserfs_security_write(struct reiserfs_transaction_handle *th, struct inode *inode, struct reiserfs_security_handle *sec) { + char xattr_name[XATTR_NAME_MAX + 1]; int error; - if (strlen(sec->name) < sizeof(XATTR_SECURITY_PREFIX)) - return -EINVAL;
- error = reiserfs_xattr_set_handle(th, inode, sec->name, sec->value, + snprintf(xattr_name, sizeof(xattr_name), "%s%s", XATTR_SECURITY_PREFIX, + sec->name); + + error = reiserfs_xattr_set_handle(th, inode, xattr_name, sec->value, sec->length, XATTR_CREATE); if (error == -ENODATA || error == -EOPNOTSUPP) error = 0;
On Wed, Mar 29, 2023 at 9:05 AM Roberto Sassu roberto.sassu@huaweicloud.com wrote:
From: Roberto Sassu roberto.sassu@huawei.com
Reiserfs sets a security xattr at inode creation time in two stages: first, it calls reiserfs_security_init() to obtain the xattr from active LSMs; then, it calls reiserfs_security_write() to actually write that xattr.
Unfortunately, it seems there is a wrong expectation that LSMs provide the full xattr name in the form 'security.<suffix>'. However, LSMs always provided just the suffix, causing reiserfs to not write the xattr at all (if the suffix is shorter than the prefix), or to write an xattr with the wrong name.
Add a temporary buffer in reiserfs_security_write(), and write to it the full xattr name, before passing it to reiserfs_xattr_set_handle().
Since the 'security.' prefix is always prepended, remove the name length check.
Cc: stable@vger.kernel.org # v2.6.x Fixes: 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
fs/reiserfs/xattr_security.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c index 6bffdf9a4fd..b0c354ab113 100644 --- a/fs/reiserfs/xattr_security.c +++ b/fs/reiserfs/xattr_security.c @@ -95,11 +95,13 @@ int reiserfs_security_write(struct reiserfs_transaction_handle *th, struct inode *inode, struct reiserfs_security_handle *sec) {
char xattr_name[XATTR_NAME_MAX + 1]; int error;
if (strlen(sec->name) < sizeof(XATTR_SECURITY_PREFIX))return -EINVAL;
If one really wanted to be paranoid they could verify that 'XATTR_SECURITY_PREFIX_LEN + strlen(sec->name) <= XATTR_NAME_MAX' and return EINVAL, but that really shouldn't be an issue and if the concatenation does result in a xattr name that is too big, the snprintf() will safely truncate/managle it.
Regardless, this patch is fine with me, but it would be nice if at least of the reiserfs/VFS folks could provide an ACK/Reviewed-by tag, although I think we can still move forward on this without one of those.
error = reiserfs_xattr_set_handle(th, inode, sec->name, sec->value,
snprintf(xattr_name, sizeof(xattr_name), "%s%s", XATTR_SECURITY_PREFIX,sec->name);error = reiserfs_xattr_set_handle(th, inode, xattr_name, sec->value, sec->length, XATTR_CREATE); if (error == -ENODATA || error == -EOPNOTSUPP) error = 0;-- 2.25.1
On Thu, 2023-03-30 at 17:15 -0400, Paul Moore wrote:
On Wed, Mar 29, 2023 at 9:05 AM Roberto Sassu roberto.sassu@huaweicloud.com wrote:
From: Roberto Sassu roberto.sassu@huawei.com
Reiserfs sets a security xattr at inode creation time in two stages: first, it calls reiserfs_security_init() to obtain the xattr from active LSMs; then, it calls reiserfs_security_write() to actually write that xattr.
Unfortunately, it seems there is a wrong expectation that LSMs provide the full xattr name in the form 'security.<suffix>'. However, LSMs always provided just the suffix, causing reiserfs to not write the xattr at all (if the suffix is shorter than the prefix), or to write an xattr with the wrong name.
Add a temporary buffer in reiserfs_security_write(), and write to it the full xattr name, before passing it to reiserfs_xattr_set_handle().
Since the 'security.' prefix is always prepended, remove the name length check.
Cc: stable@vger.kernel.org # v2.6.x Fixes: 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
fs/reiserfs/xattr_security.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c index 6bffdf9a4fd..b0c354ab113 100644 --- a/fs/reiserfs/xattr_security.c +++ b/fs/reiserfs/xattr_security.c @@ -95,11 +95,13 @@ int reiserfs_security_write(struct reiserfs_transaction_handle *th, struct inode *inode, struct reiserfs_security_handle *sec) {
char xattr_name[XATTR_NAME_MAX + 1]; int error;
if (strlen(sec->name) < sizeof(XATTR_SECURITY_PREFIX))return -EINVAL;If one really wanted to be paranoid they could verify that 'XATTR_SECURITY_PREFIX_LEN + strlen(sec->name) <= XATTR_NAME_MAX' and return EINVAL, but that really shouldn't be an issue and if the concatenation does result in a xattr name that is too big, the snprintf() will safely truncate/managle it.
Ok, I could do it.
Thanks
Roberto
Regardless, this patch is fine with me, but it would be nice if at least of the reiserfs/VFS folks could provide an ACK/Reviewed-by tag, although I think we can still move forward on this without one of those.
error = reiserfs_xattr_set_handle(th, inode, sec->name, sec->value,
snprintf(xattr_name, sizeof(xattr_name), "%s%s", XATTR_SECURITY_PREFIX,sec->name);error = reiserfs_xattr_set_handle(th, inode, xattr_name, sec->value, sec->length, XATTR_CREATE); if (error == -ENODATA || error == -EOPNOTSUPP) error = 0;-- 2.25.1
linux-stable-mirror@lists.linaro.org
-
Paul Moore -
Roberto Sassu