| Message ID | 20210415232336.199462-3-cascardo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Xenial] UBUNTU: SAUCE: vfs_setxattr: free converted value if xattr_permission returns error | expand |
On 16/04/2021 01:23, Thadeu Lima de Souza Cascardo wrote: > 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@canonical.com> > --- > fs/xattr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Just in case there is any other derivative work from this kernel: Fixes: f9e973d2d1ca ("vfs: move cap_convert_nscap() call into vfs_setxattr()") Best regards, Krzysztof
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);
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@canonical.com> --- fs/xattr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)