Return-Path: <linux-nfs-owner@vger.kernel.org>
Received: from mail-yw0-f194.google.com ([209.85.161.194]:42355 "EHLO
        mail-yw0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1755417AbeEaPjr (ORCPT
        <rfc822;linux-nfs@vger.kernel.org>); Thu, 31 May 2018 11:39:47 -0400
Date: Thu, 31 May 2018 08:39:43 -0700
From: Tejun Heo <tj@kernel.org>
To: CHANDAN VN <chandan.vn@samsung.com>
Cc: gregkh@linuxfoundation.org, bfields@fieldses.org,
        jlayton@kernel.org, linux-kernel@vger.kernel.org,
        linux-nfs@vger.kernel.org, casey@schaufler-ca.com,
        cpgs@samsung.com, sireesha.t@samsung.com,
        Chris Wright <chrisw@sous-sol.org>,
        linux-security-module@vger.kernel.org
Subject: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and
 kernfs_security_xattr_set
Message-ID: <20180531153943.GR1351649@devbig577.frc2.facebook.com>
References: <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcas1p2.samsung.com>
 <1527758911-18610-1-git-send-email-chandan.vn@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
In-Reply-To: <1527758911-18610-1-git-send-email-chandan.vn@samsung.com>
Sender: linux-nfs-owner@vger.kernel.org
List-ID: <linux-nfs.vger.kernel.org>

(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@samsung.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@samsung.com>
> Signed-off-by: CHANDAN VN <chandan.vn@samsung.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