From 3b0c2d3eaa83da259d7726192cf55a137769012f Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Fri, 12 Mar 2021 15:07:09 -0600 Subject: Revert 95ebabde382c ("capabilities: Don't allow writing ambiguous v3 file capabilities") It turns out that there are in fact userspace implementations that care and this recent change caused a regression. https://github.com/containers/buildah/issues/3071 As the motivation for the original change was future development, and the impact is existing real world code just revert this change and allow the ambiguity in v3 file caps. Cc: stable@vger.kernel.org Fixes: 95ebabde382c ("capabilities: Don't allow writing ambiguous v3 file capabilities") Signed-off-by: Eric W. Biederman --- security/commoncap.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index 28f4d25480df..1c519c875217 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -543,8 +543,7 @@ int cap_convert_nscap(struct user_namespace *mnt_userns, struct dentry *dentry, __u32 magic, nsmagic; struct inode *inode = d_backing_inode(dentry); struct user_namespace *task_ns = current_user_ns(), - *fs_ns = inode->i_sb->s_user_ns, - *ancestor; + *fs_ns = inode->i_sb->s_user_ns; kuid_t rootid; size_t newsize; @@ -567,15 +566,6 @@ int cap_convert_nscap(struct user_namespace *mnt_userns, struct dentry *dentry, if (nsrootid == -1) return -EINVAL; - /* - * Do not allow allow adding a v3 filesystem capability xattr - * if the rootid field is ambiguous. - */ - for (ancestor = task_ns->parent; ancestor; ancestor = ancestor->parent) { - if (from_kuid(ancestor, rootid) == 0) - return -EINVAL; - } - newsize = sizeof(struct vfs_ns_cap_data); nscap = kmalloc(newsize, GFP_ATOMIC); if (!nscap) -- cgit v1.2.3 From 82e5d8cc768b0c7b03c551a9ab1f8f3f68d5f83f Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Mon, 22 Mar 2021 17:02:41 +0100 Subject: security: commoncap: fix -Wstringop-overread warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gcc-11 introdces a harmless warning for cap_inode_getsecurity: security/commoncap.c: In function ‘cap_inode_getsecurity’: security/commoncap.c:440:33: error: ‘memcpy’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread] 440 | memcpy(&nscap->data, &cap->data, sizeof(__le32) * 2 * VFS_CAP_U32); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The problem here is that tmpbuf is initialized to NULL, so gcc assumes it is not accessible unless it gets set by vfs_getxattr_alloc(). This is a legitimate warning as far as I can tell, but the code is correct since it correctly handles the error when that function fails. Add a separate NULL check to tell gcc about it as well. Signed-off-by: Arnd Bergmann Acked-by: Christian Brauner Signed-off-by: James Morris --- security/commoncap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index 26c1cb725dcb..2bdeacd32e3f 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -391,7 +391,7 @@ int cap_inode_getsecurity(struct inode *inode, const char *name, void **buffer, &tmpbuf, size, GFP_NOFS); dput(dentry); - if (ret < 0) + if (ret < 0 || !tmpbuf) return ret; fs_ns = inode->i_sb->s_user_ns; -- cgit v1.2.3 From 049ae601f3fb3d5b1c1efdb434499770c96237f6 Mon Sep 17 00:00:00 2001 From: Randy Dunlap Date: Sun, 11 Apr 2021 17:55:28 -0700 Subject: security: commoncap: clean up kernel-doc comments Fix kernel-doc notation in commoncap.c. Use correct (matching) function name in comments as in code. Use correct function argument names in kernel-doc comments. Use kernel-doc's "Return:" format for function return values. Fixes these kernel-doc warnings: ../security/commoncap.c:1206: warning: expecting prototype for cap_task_ioprio(). Prototype was for cap_task_setioprio() instead ../security/commoncap.c:1219: warning: expecting prototype for cap_task_ioprio(). Prototype was for cap_task_setnice() instead Signed-off-by: Randy Dunlap Reviewed-by: Serge Hallyn Signed-off-by: James Morris --- security/commoncap.c | 50 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 17 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index 2bdeacd32e3f..b088bf002db5 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -50,7 +50,7 @@ static void warn_setuid_and_fcaps_mixed(const char *fname) /** * cap_capable - Determine whether a task has a particular effective capability * @cred: The credentials to use - * @ns: The user namespace in which we need the capability + * @targ_ns: The user namespace in which we need the capability * @cap: The capability to check for * @opts: Bitmask of options defined in include/linux/security.h * @@ -289,7 +289,7 @@ int cap_capset(struct cred *new, * affects the security markings on that inode, and if it is, should * inode_killpriv() be invoked or the change rejected. * - * Returns 1 if security.capability has a value, meaning inode_killpriv() + * Return: 1 if security.capability has a value, meaning inode_killpriv() * is required, 0 otherwise, meaning inode_killpriv() is not required. */ int cap_inode_need_killpriv(struct dentry *dentry) @@ -307,7 +307,7 @@ int cap_inode_need_killpriv(struct dentry *dentry) * * Erase the privilege-enhancing security markings on an inode. * - * Returns 0 if successful, -ve on error. + * Return: 0 if successful, -ve on error. */ int cap_inode_killpriv(struct dentry *dentry) { @@ -490,7 +490,7 @@ static bool validheader(size_t size, const struct vfs_cap_data *cap) * User requested a write of security.capability. If needed, update the * xattr to change from v2 to v3, or to fixup the v3 rootid. * - * If all is ok, we return the new size, on error return < 0. + * Return: On success, return the new size; on error, return < 0. */ int cap_convert_nscap(struct dentry *dentry, const void **ivalue, size_t size) { @@ -822,7 +822,9 @@ static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old, * * Set up the proposed credentials for a new execution context being * constructed by execve(). The proposed creds in @bprm->cred is altered, - * which won't take effect immediately. Returns 0 if successful, -ve on error. + * which won't take effect immediately. + * + * Return: 0 if successful, -ve on error. */ int cap_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file) { @@ -1049,7 +1051,9 @@ static inline void cap_emulate_setxuid(struct cred *new, const struct cred *old) * @flags: Indications of what has changed * * Fix up the results of setuid() call before the credential changes are - * actually applied, returning 0 to grant the changes, -ve to deny them. + * actually applied. + * + * Return: 0 to grant the changes, -ve to deny them. */ int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags) { @@ -1119,7 +1123,9 @@ static int cap_safe_nice(struct task_struct *p) * @p: The task to affect * * Detemine if the requested scheduler policy change is permitted for the - * specified task, returning 0 if permission is granted, -ve if denied. + * specified task. + * + * Return: 0 if permission is granted, -ve if denied. */ int cap_task_setscheduler(struct task_struct *p) { @@ -1127,12 +1133,14 @@ int cap_task_setscheduler(struct task_struct *p) } /** - * cap_task_ioprio - Detemine if I/O priority change is permitted + * cap_task_setioprio - Detemine if I/O priority change is permitted * @p: The task to affect * @ioprio: The I/O priority to set * * Detemine if the requested I/O priority change is permitted for the specified - * task, returning 0 if permission is granted, -ve if denied. + * task. + * + * Return: 0 if permission is granted, -ve if denied. */ int cap_task_setioprio(struct task_struct *p, int ioprio) { @@ -1140,12 +1148,14 @@ int cap_task_setioprio(struct task_struct *p, int ioprio) } /** - * cap_task_ioprio - Detemine if task priority change is permitted + * cap_task_setnice - Detemine if task priority change is permitted * @p: The task to affect * @nice: The nice value to set * * Detemine if the requested task priority change is permitted for the - * specified task, returning 0 if permission is granted, -ve if denied. + * specified task. + * + * Return: 0 if permission is granted, -ve if denied. */ int cap_task_setnice(struct task_struct *p, int nice) { @@ -1175,12 +1185,15 @@ static int cap_prctl_drop(unsigned long cap) /** * cap_task_prctl - Implement process control functions for this security module * @option: The process control function requested - * @arg2, @arg3, @arg4, @arg5: The argument data for this function + * @arg2: The argument data for this function + * @arg3: The argument data for this function + * @arg4: The argument data for this function + * @arg5: The argument data for this function * * Allow process control functions (sys_prctl()) to alter capabilities; may * also deny access to other functions not otherwise implemented here. * - * Returns 0 or +ve on success, -ENOSYS if this function is not implemented + * Return: 0 or +ve on success, -ENOSYS if this function is not implemented * here, other -ve on error. If -ENOSYS is returned, sys_prctl() and other LSM * modules will consider performing the function. */ @@ -1315,7 +1328,9 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, * @pages: The size of the mapping * * Determine whether the allocation of a new virtual mapping by the current - * task is permitted, returning 1 if permission is granted, 0 if not. + * task is permitted. + * + * Return: 1 if permission is granted, 0 if not. */ int cap_vm_enough_memory(struct mm_struct *mm, long pages) { @@ -1328,14 +1343,15 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages) return cap_sys_admin; } -/* +/** * cap_mmap_addr - check if able to map given addr * @addr: address attempting to be mapped * * If the process is attempting to map memory below dac_mmap_min_addr they need * CAP_SYS_RAWIO. The other parameters to this function are unused by the - * capability security module. Returns 0 if this mapping should be allowed - * -EPERM if not. + * capability security module. + * + * Return: 0 if this mapping should be allowed or -EPERM if not. */ int cap_mmap_addr(unsigned long addr) { -- cgit v1.2.3