[SRU Xenial] UBUNTU: SAUCE: vfs_setxattr: free converted value if xattr_permission returns error

Thadeu Lima de Souza Cascardo cascardo at canonical.com
Thu Apr 15 23:23:36 UTC 2021


BugLink: https://bugs.launchpad.net/bugs/1924611

The backport of commit 7c03e2cda4a5 ("vfs: move cap_convert_nscap() call
into vfs_setxattr()") did not consider that vfs_setxattr had other exit
paths that would require a converted value to be freed.

If xattr_permission returns a failure, it would cause a memory leak. In the
case of security.capability attribute, which is the only that can allocate
a new value, xattr_permission will return a failure in case of
HAS_UNMAPPED_ID(inode), which would already be caught by cap_convert_nscap,
at !capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

However, if the file IS_IMMUTABLE or IS_APPEND, the failure will be
returned and the leak will happen.

Though setting a file as immutable or append is restricted to
CAP_FILE_IMMUTABLE, the leak was still shown to happen when trying to
setcap on an immutable file after doing a mount unshare.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>
---
 fs/xattr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 6f38106d08dc..53086977db5c 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -142,7 +142,7 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 
 	error = xattr_permission(inode, name, MAY_WRITE);
 	if (error)
-		return error;
+		goto out_free;
 
 	mutex_lock(&inode->i_mutex);
 	error = security_inode_setxattr(dentry, name, value, size, flags);
@@ -153,6 +153,7 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 
 out:
 	mutex_unlock(&inode->i_mutex);
+out_free:
 	if (value != orig_value)
 		kfree(value);
 
-- 
2.27.0




More information about the kernel-team mailing list