[PATCH] security: commoncap: fix potential memleak on error path from vfs_getxattr_alloc

[PATCH] security: commoncap: fix potential memleak on error path from vfs_getxattr_alloc

[Gaosheng Cui]

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(-)

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;
-- 
2.25.1

Re: [PATCH] security: commoncap: fix potential memleak on error path from vfs_getxattr_alloc

[Christian Brauner]

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.

Re: [PATCH] security: commoncap: fix potential memleak on error path from vfs_getxattr_alloc

[Seth Forshee]

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

Re: [PATCH] security: commoncap: fix potential memleak on error path from vfs_getxattr_alloc

[cuigaosheng]

> 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.
> .