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

[cuigaosheng <cuigaosheng1@huawei.com>]

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