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

[Seth Forshee <sforshee@kernel.org>]

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