| Message ID | 20221025133357.2404086-1-cuigaosheng1@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] security: commoncap: fix potential memleak on error path from vfs_getxattr_alloc | expand |
On Tue, Oct 25, 2022 at 09:33:57PM +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. > > Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") Acked-by: Serge Hallyn <serge@hallyn.com> > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > --- > v2: > - Update the Fixes tag, from 71bc356f93a1 to 8db6c34f1dbc. Thanks! > 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
On Tue, Oct 25, 2022 at 9:34 AM Gaosheng Cui <cuigaosheng1@huawei.com> 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. > > Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > --- > v2: > - Update the Fixes tag, from 71bc356f93a1 to 8db6c34f1dbc. Thanks! > security/commoncap.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) Merged into lsm/stable-6.1 and plan on sending it up to Linus early next week (network connectivity permitting). Thanks!
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: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> --- v2: - Update the Fixes tag, from 71bc356f93a1 to 8db6c34f1dbc. Thanks! security/commoncap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)