linux-kernel - Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180531153943.GR1351649@devbig577.frc2.facebook.com>
Date:   Thu, 31 May 2018 08:39:43 -0700
From:   Tejun Heo <tj@...nel.org>
To:     CHANDAN VN <chandan.vn@...sung.com>
Cc:     gregkh@...uxfoundation.org, bfields@...ldses.org,
        jlayton@...nel.org, linux-kernel@...r.kernel.org,
        linux-nfs@...r.kernel.org, casey@...aufler-ca.com,
        cpgs@...sung.com, sireesha.t@...sung.com,
        Chris Wright <chrisw@...s-sol.org>,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and
 kernfs_security_xattr_set

(cc'ing more security folks and copying whole body)

So, I'm sure the patch fixes the memory leak but API wise it looks
super confusing.  Can security folks chime in here?  Is this the right
fix?

Thanks.

On Thu, May 31, 2018 at 02:58:31PM +0530, CHANDAN VN wrote:
> From: "sireesha.t" <sireesha.t@...sung.com>
> 
> Leak is caused because smack_inode_getsecurity() is allocating memory
> using kstrdup(). Though the security_release_secctx() is called, it
> would not free the allocated memory. Calling security_release_secctx is
> not relevant for this scenario as inode_getsecurity() does not provide a
> "secctx".
> 
> Similar fix has been mainlined:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=57e7ba04d422c3d41c8426380303ec9b7533ded9
> 
> The fix is to replace the security_release_secctx() with a kfree()
> 
> Below is the KMEMLEAK dump:
> unreferenced object 0xffffffc025e11c80 (size 64):
>   comm "systemd-tmpfile", pid 2452, jiffies 4294894464 (age 235587.492s)
>   hex dump (first 32 bytes):
>     53 79 73 74 65 6d 3a 3a 53 68 61 72 65 64 00 00  System::Shared..
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffff80081be770>] __save_stack_trace+0x28/0x34
>     [<ffffff80081bedb8>] create_object+0x130/0x25c
>     [<ffffff80088c82f8>] kmemleak_alloc+0x30/0x5c
>     [<ffffff80081b3ef0>] __kmalloc_track_caller+0x1cc/0x2a8
>     [<ffffff800818673c>] kstrdup+0x3c/0x6c
>     [<ffffff80082d78b0>] smack_inode_getsecurity+0xcc/0xec
>     [<ffffff80082d78f4>] smack_inode_getsecctx+0x24/0x44
>     [<ffffff80082d5ea0>] security_inode_getsecctx+0x50/0x70
>     [<ffffff800823bbcc>] kernfs_security_xattr_set+0x74/0xe0
>     [<ffffff80081eafec>] __vfs_setxattr+0x74/0x90
>     [<ffffff80081eb088>] __vfs_setxattr_noperm+0x80/0x1ac
>     [<ffffff80081eb238>] vfs_setxattr+0x84/0xac
>     [<ffffff80081eb374>] setxattr+0x114/0x178
>     [<ffffff80081eb44c>] path_setxattr+0x74/0xb8
>     [<ffffff80081ebdcc>] SyS_lsetxattr+0x10/0x1c
>     [<ffffff800808310c>] __sys_trace_return+0x0/0x4
> 
> Signed-off-by: sireesha.t <sireesha.t@...sung.com>
> Signed-off-by: CHANDAN VN <chandan.vn@...sung.com>
> ---
>  fs/kernfs/inode.c | 3 ++-
>  fs/nfsd/nfs4xdr.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index a343039..53befb8 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -369,7 +369,8 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
>  	mutex_unlock(&kernfs_mutex);
>  
>  	if (secdata)
> -		security_release_secctx(secdata, secdata_len);
> +		kfree(secdata);
> +
>  	return error;
>  }
>  
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index aaa88c1..1e0dbe9 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2911,7 +2911,7 @@ static int get_parent_attributes(struct svc_export *exp, struct kstat *stat)
>  out:
>  #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>  	if (context)
> -		security_release_secctx(context, contextlen);
> +		kfree(context);
>  #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
>  	kfree(acl);
>  	if (tempfh) {
> -- 
> 1.9.1
> 

-- 
tejun