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

[Christian Brauner <brauner@kernel.org>]

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.