| Message ID | 20221025104544.2298829-1-cuigaosheng1@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | security: commoncap: fix potential memleak on error path from vfs_getxattr_alloc | expand |
On Tue, Oct 25, 2022 at 06:45:44PM +0800, Gaosheng Cui wrote: > In cap_inode_getsecurity(), we will use vfs_getxattr_alloc() to > complete the memory allocation of tmpbuf, if we have completed > the memory allocation of tmpbuf, but failed to call handler->get(...), > there will be a memleak in below logic: > > |-- ret = (int)vfs_getxattr_alloc(mnt_userns, ...) <-- alloc for tmpbuf > |-- value = krealloc(*xattr_value, error + 1, flags) <-- alloc memory > |-- error = handler->get(handler, ...) <-- if error > |-- *xattr_value = value <-- xattr_value is &tmpbuf <-- memory leak > > So we will try to free(tmpbuf) after vfs_getxattr_alloc() fails to fix it. Hey Gaosheng, > > Fixes: 71bc356f93a1 ("commoncap: handle idmapped mounts") The Fixes: tag is wrong afaict. The control flow isn't changed in any way by the referenced commit. The logic changed the last time with 82e5d8cc768b ("security: commoncap: fix -Wstringop-overread warning") but even that commit wouldn't have introduced the bug. It would've been introduced by 8db6c34f1dbc ("Introduce v3 namespaced file capabilities"). So update the Fixes: tag with that reference, please. Otherwise I think you in principle have a point. Not sure if we have any filesystem that in practice would fail after permission checks have already passed with the first call to ->get() but then fail with the correct size passed in the second ->get() invocation. Sounds super unlikely but not impossible.
On Tue, Oct 25, 2022 at 03:04:59PM +0200, Christian Brauner wrote: > On Tue, Oct 25, 2022 at 06:45:44PM +0800, Gaosheng Cui wrote: > > In cap_inode_getsecurity(), we will use vfs_getxattr_alloc() to > > complete the memory allocation of tmpbuf, if we have completed > > the memory allocation of tmpbuf, but failed to call handler->get(...), > > there will be a memleak in below logic: > > > > |-- ret = (int)vfs_getxattr_alloc(mnt_userns, ...) <-- alloc for tmpbuf > > |-- value = krealloc(*xattr_value, error + 1, flags) <-- alloc memory > > |-- error = handler->get(handler, ...) <-- if error > > |-- *xattr_value = value <-- xattr_value is &tmpbuf <-- memory leak > > > > So we will try to free(tmpbuf) after vfs_getxattr_alloc() fails to fix it. > > Hey Gaosheng, > > > > > Fixes: 71bc356f93a1 ("commoncap: handle idmapped mounts") > > The Fixes: tag is wrong afaict. The control flow isn't changed in any > way by the referenced commit. > > The logic changed the last time with > 82e5d8cc768b ("security: commoncap: fix -Wstringop-overread warning") > but even that commit wouldn't have introduced the bug. It would've been > introduced by 8db6c34f1dbc ("Introduce v3 namespaced file > capabilities"). So update the Fixes: tag with that reference, please. > > Otherwise I think you in principle have a point. Not sure if we have any > filesystem that in practice would fail after permission checks have > already passed with the first call to ->get() but then fail with the > correct size passed in the second ->get() invocation. Sounds super > unlikely but not impossible. Looks like several other callers have the same problem -- evm_is_immutable(), evm_xattr_change(), ima_eventevmsig_init(). And ima_read_xattr() expects the caller to handle it which seems potentially problematic, though currently only current caller does handle it properly. The documention comment for vfs_getxattr_alloc() needs a warning that the caller needs to free memory even if an error is returned, because that is very counterintuitive. It probably should have been named vfs_getxattr_realloc() ... Seth
> The Fixes: tag is wrong afaict. The control flow isn't changed in any > way by the referenced commit. Thanks for taking time to review this patch, I have update the fixes tags. link: https://patchwork.kernel.org/project/linux-security-module/patch/20221025133357.2404086-1-cuigaosheng1@huawei.com/ > Otherwise I think you in principle have a point. Not sure if we have any > filesystem that in practice would fail after permission checks have > already passed with the first call to ->get() but then fail with the > correct size passed in the second ->get() invocation. Sounds super > unlikely but not impossible. This problem was discovered when I was reading the code, we can build the fault scenario by hard coding, but the probability of this problem occurring during use should be very small, I thought we can fix it, so I submit the patch, thanks again! On 2022/10/25 21:04, Christian Brauner wrote: > On Tue, Oct 25, 2022 at 06:45:44PM +0800, Gaosheng Cui wrote: >> In cap_inode_getsecurity(), we will use vfs_getxattr_alloc() to >> complete the memory allocation of tmpbuf, if we have completed >> the memory allocation of tmpbuf, but failed to call handler->get(...), >> there will be a memleak in below logic: >> >> |-- ret = (int)vfs_getxattr_alloc(mnt_userns, ...) <-- alloc for tmpbuf >> |-- value = krealloc(*xattr_value, error + 1, flags) <-- alloc memory >> |-- error = handler->get(handler, ...) <-- if error >> |-- *xattr_value = value <-- xattr_value is &tmpbuf <-- memory leak >> >> So we will try to free(tmpbuf) after vfs_getxattr_alloc() fails to fix it. > Hey Gaosheng, > >> Fixes: 71bc356f93a1 ("commoncap: handle idmapped mounts") > The Fixes: tag is wrong afaict. The control flow isn't changed in any > way by the referenced commit. > > The logic changed the last time with > 82e5d8cc768b ("security: commoncap: fix -Wstringop-overread warning") > but even that commit wouldn't have introduced the bug. It would've been > introduced by 8db6c34f1dbc ("Introduce v3 namespaced file > capabilities"). So update the Fixes: tag with that reference, please. > > Otherwise I think you in principle have a point. Not sure if we have any > filesystem that in practice would fail after permission checks have > already passed with the first call to ->get() but then fail with the > correct size passed in the second ->get() invocation. Sounds super > unlikely but not impossible. > .
diff --git a/security/commoncap.c b/security/commoncap.c index 5fc8986c3c77..bc751fa5adad 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -401,8 +401,10 @@ int cap_inode_getsecurity(struct user_namespace *mnt_userns, &tmpbuf, size, GFP_NOFS); dput(dentry); - if (ret < 0 || !tmpbuf) - return ret; + if (ret < 0 || !tmpbuf) { + size = ret; + goto out_free; + } fs_ns = inode->i_sb->s_user_ns; cap = (struct vfs_cap_data *) tmpbuf;
In cap_inode_getsecurity(), we will use vfs_getxattr_alloc() to complete the memory allocation of tmpbuf, if we have completed the memory allocation of tmpbuf, but failed to call handler->get(...), there will be a memleak in below logic: |-- ret = (int)vfs_getxattr_alloc(mnt_userns, ...) <-- alloc for tmpbuf |-- value = krealloc(*xattr_value, error + 1, flags) <-- alloc memory |-- error = handler->get(handler, ...) <-- if error |-- *xattr_value = value <-- xattr_value is &tmpbuf <-- memory leak So we will try to free(tmpbuf) after vfs_getxattr_alloc() fails to fix it. Fixes: 71bc356f93a1 ("commoncap: handle idmapped mounts") Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> --- security/commoncap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)