All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
       [not found] <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcas1p2.samsung.com>
@ 2018-05-31  9:28 ` CHANDAN VN
  2018-05-31 15:26   ` Casey Schaufler
  2018-05-31 15:39     ` Tejun Heo
  0 siblings, 2 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-05-31  9:28 UTC (permalink / raw)
  To: gregkh, tj, bfields, jlayton, linux-kernel, linux-nfs, casey
  Cc: cpgs, chandan.vn, sireesha.t

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
  2018-05-31  9:28 ` [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set CHANDAN VN
@ 2018-05-31 15:26   ` Casey Schaufler
  2018-05-31 20:57     ` Eric W. Biederman
  2018-05-31 15:39     ` Tejun Heo
  1 sibling, 1 reply; 33+ messages in thread
From: Casey Schaufler @ 2018-05-31 15:26 UTC (permalink / raw)
  To: CHANDAN VN, gregkh, tj, bfields, jlayton, linux-kernel, linux-nfs
  Cc: cpgs, sireesha.t, Casey Schaufler

On 5/31/2018 2:28 AM, 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>

Why not:

 static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
 {
-	int len = 0;
-	len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, true);
+	int len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, false);
 
 	if (len < 0)
 		return len;

> ---
>  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) {

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
  2018-05-31  9:28 ` [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set CHANDAN VN
@ 2018-05-31 15:39     ` Tejun Heo
  2018-05-31 15:39     ` Tejun Heo
  1 sibling, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2018-05-31 15:39 UTC (permalink / raw)
  To: CHANDAN VN
  Cc: gregkh, bfields, jlayton, linux-kernel, linux-nfs, casey, cpgs,
	sireesha.t, Chris Wright, linux-security-module

(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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-05-31 15:39     ` Tejun Heo
  0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2018-05-31 15:39 UTC (permalink / raw)
  To: linux-security-module

(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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
  2018-05-31 15:39     ` Tejun Heo
@ 2018-05-31 16:04       ` Casey Schaufler
  -1 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-05-31 16:04 UTC (permalink / raw)
  To: Tejun Heo, CHANDAN VN
  Cc: gregkh, bfields, jlayton, linux-kernel, linux-nfs, cpgs,
	sireesha.t, Chris Wright, linux-security-module

On 5/31/2018 8:39 AM, Tejun Heo wrote:
> (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?

security_inode_getsecctx() provides a security context. Technically,
this is a data blob, although both provider provide a null terminated
string. security_inode_getsecurity(), on the other hand, provides a
string to match an attribute name. The former releases the security
context with security_release_secctx(), where the later releases the
string with kfree().

When the Smack hook smack_inode_getsecctx() was added in 2009
for use by labeled NFS the alloc value passed to
smack_inode_getsecurity() was set incorrectly. This wasn't a
major issue, since labeled NFS is a fringe case. When kernfs
started using the hook, it became the issue you discovered.

The reason that we have all this confusion is that SELinux
generates security contexts as needed, while Smack keeps them
around all the time. Releasing an SELinux context frees memory,
while releasing a Smack context is a null operation.

>
> 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
>>

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-05-31 16:04       ` Casey Schaufler
  0 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-05-31 16:04 UTC (permalink / raw)
  To: linux-security-module

On 5/31/2018 8:39 AM, Tejun Heo wrote:
> (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?

security_inode_getsecctx() provides a security context. Technically,
this is a data blob, although both provider provide a null terminated
string. security_inode_getsecurity(), on the other hand, provides a
string to match an attribute name. The former releases the security
context with security_release_secctx(), where the later releases the
string with kfree().

When the Smack hook smack_inode_getsecctx() was added in 2009
for use by labeled NFS the alloc value passed to
smack_inode_getsecurity() was set incorrectly. This wasn't a
major issue, since labeled NFS is a fringe case. When kernfs
started using the hook, it became the issue you discovered.

The reason that we have all this confusion is that SELinux
generates security contexts as needed, while Smack keeps them
around all the time. Releasing an SELinux context frees memory,
while releasing a Smack context is a null operation.

>
> 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
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
  2018-05-31 16:04       ` Casey Schaufler
@ 2018-05-31 16:11         ` Tejun Heo
  -1 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2018-05-31 16:11 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: CHANDAN VN, gregkh, bfields, jlayton, linux-kernel, linux-nfs,
	cpgs, sireesha.t, Chris Wright, linux-security-module

On Thu, May 31, 2018 at 09:04:25AM -0700, Casey Schaufler wrote:
> On 5/31/2018 8:39 AM, Tejun Heo wrote:
> > (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?
> 
> security_inode_getsecctx() provides a security context. Technically,
> this is a data blob, although both provider provide a null terminated
> string. security_inode_getsecurity(), on the other hand, provides a
> string to match an attribute name. The former releases the security
> context with security_release_secctx(), where the later releases the
> string with kfree().
> 
> When the Smack hook smack_inode_getsecctx() was added in 2009
> for use by labeled NFS the alloc value passed to
> smack_inode_getsecurity() was set incorrectly. This wasn't a
> major issue, since labeled NFS is a fringe case. When kernfs
> started using the hook, it became the issue you discovered.
> 
> The reason that we have all this confusion is that SELinux
> generates security contexts as needed, while Smack keeps them
> around all the time. Releasing an SELinux context frees memory,
> while releasing a Smack context is a null operation.

Any chance this detail can be hidden behind security api?  This looks
pretty error-prone, no?

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-05-31 16:11         ` Tejun Heo
  0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2018-05-31 16:11 UTC (permalink / raw)
  To: linux-security-module

On Thu, May 31, 2018 at 09:04:25AM -0700, Casey Schaufler wrote:
> On 5/31/2018 8:39 AM, Tejun Heo wrote:
> > (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?
> 
> security_inode_getsecctx() provides a security context. Technically,
> this is a data blob, although both provider provide a null terminated
> string. security_inode_getsecurity(), on the other hand, provides a
> string to match an attribute name. The former releases the security
> context with security_release_secctx(), where the later releases the
> string with kfree().
> 
> When the Smack hook smack_inode_getsecctx() was added in 2009
> for use by labeled NFS the alloc value passed to
> smack_inode_getsecurity() was set incorrectly. This wasn't a
> major issue, since labeled NFS is a fringe case. When kernfs
> started using the hook, it became the issue you discovered.
> 
> The reason that we have all this confusion is that SELinux
> generates security contexts as needed, while Smack keeps them
> around all the time. Releasing an SELinux context frees memory,
> while releasing a Smack context is a null operation.

Any chance this detail can be hidden behind security api?  This looks
pretty error-prone, no?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
  2018-05-31 16:11         ` Tejun Heo
@ 2018-05-31 16:22           ` Casey Schaufler
  -1 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-05-31 16:22 UTC (permalink / raw)
  To: Tejun Heo
  Cc: CHANDAN VN, gregkh, bfields, jlayton, linux-kernel, linux-nfs,
	cpgs, sireesha.t, Chris Wright, linux-security-module,
	Casey Schaufler

On 5/31/2018 9:11 AM, Tejun Heo wrote:
> On Thu, May 31, 2018 at 09:04:25AM -0700, Casey Schaufler wrote:
>> On 5/31/2018 8:39 AM, Tejun Heo wrote:
>>> (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?
>> security_inode_getsecctx() provides a security context. Technically,
>> this is a data blob, although both provider provide a null terminated
>> string. security_inode_getsecurity(), on the other hand, provides a
>> string to match an attribute name. The former releases the security
>> context with security_release_secctx(), where the later releases the
>> string with kfree().
>>
>> When the Smack hook smack_inode_getsecctx() was added in 2009
>> for use by labeled NFS the alloc value passed to
>> smack_inode_getsecurity() was set incorrectly. This wasn't a
>> major issue, since labeled NFS is a fringe case. When kernfs
>> started using the hook, it became the issue you discovered.
>>
>> The reason that we have all this confusion is that SELinux
>> generates security contexts as needed, while Smack keeps them
>> around all the time. Releasing an SELinux context frees memory,
>> while releasing a Smack context is a null operation.
> Any chance this detail can be hidden behind security api?  This looks
> pretty error-prone, no?

It *is* hidden behind the security API. The problem is strictly
within the Smack code, where the implementer of smack_inode_getsecctx()
made an error.

>
> Thanks.
>


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-05-31 16:22           ` Casey Schaufler
  0 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-05-31 16:22 UTC (permalink / raw)
  To: linux-security-module

On 5/31/2018 9:11 AM, Tejun Heo wrote:
> On Thu, May 31, 2018 at 09:04:25AM -0700, Casey Schaufler wrote:
>> On 5/31/2018 8:39 AM, Tejun Heo wrote:
>>> (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?
>> security_inode_getsecctx() provides a security context. Technically,
>> this is a data blob, although both provider provide a null terminated
>> string. security_inode_getsecurity(), on the other hand, provides a
>> string to match an attribute name. The former releases the security
>> context with security_release_secctx(), where the later releases the
>> string with kfree().
>>
>> When the Smack hook smack_inode_getsecctx() was added in 2009
>> for use by labeled NFS the alloc value passed to
>> smack_inode_getsecurity() was set incorrectly. This wasn't a
>> major issue, since labeled NFS is a fringe case. When kernfs
>> started using the hook, it became the issue you discovered.
>>
>> The reason that we have all this confusion is that SELinux
>> generates security contexts as needed, while Smack keeps them
>> around all the time. Releasing an SELinux context frees memory,
>> while releasing a Smack context is a null operation.
> Any chance this detail can be hidden behind security api?  This looks
> pretty error-prone, no?

It *is* hidden behind the security API. The problem is strictly
within the Smack code, where the implementer of smack_inode_getsecctx()
made an error.

>
> Thanks.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
  2018-05-31 15:26   ` Casey Schaufler
@ 2018-05-31 20:57     ` Eric W. Biederman
  2018-05-31 21:08       ` Casey Schaufler
  0 siblings, 1 reply; 33+ messages in thread
From: Eric W. Biederman @ 2018-05-31 20:57 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: CHANDAN VN, gregkh, tj, bfields, jlayton, linux-kernel,
	linux-nfs, cpgs, sireesha.t

Casey Schaufler <casey@schaufler-ca.com> writes:

> On 5/31/2018 2:28 AM, 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>
>
> Why not:
>
>  static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>  {
> -	int len = 0;
> -	len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, true);
> +	int len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, false);
>
The practical difference here is the true vs the false in the call
to smack_inode_getsecurity?

>  	if (len < 0)
>  		return len;
>

Eric

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
  2018-05-31 20:57     ` Eric W. Biederman
@ 2018-05-31 21:08       ` Casey Schaufler
  0 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-05-31 21:08 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: CHANDAN VN, gregkh, tj, bfields, jlayton, linux-kernel,
	linux-nfs, cpgs, sireesha.t, Casey Schaufler

On 5/31/2018 1:57 PM, Eric W. Biederman wrote:
> Casey Schaufler <casey@schaufler-ca.com> writes:
>
>> On 5/31/2018 2:28 AM, 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>
>> Why not:
>>
>>  static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>>  {
>> -	int len = 0;
>> -	len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, true);
>> +	int len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, false);
>>
> The practical difference here is the true vs the false in the call
> to smack_inode_getsecurity?

That is correct. The author of smack_inode_getsecctx() has a SELinux
background and appears to have missed that Smack is careful not to
allocate memory and make copies of labels when it doesn't need to.

>
>>  	if (len < 0)
>>  		return len;
>>
> Eric
>

^ permalink raw reply	[flat|nested] 33+ messages in thread

* RE: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
       [not found]         ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p5>
  2018-06-01  8:56             ` CHANDAN VN
@ 2018-06-01  8:56             ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-01  8:56 UTC (permalink / raw)
  To: Tejun Heo, Casey Schaufler
  Cc: gregkh, bfields, jlayton, linux-kernel, linux-nfs, CPGS,
	Sireesha Talluri, Chris Wright, linux-security-module

Hi
 \r
\r
>On 5/31/2018 9:11 AM, Tejun Heo wrote:\r
> On Thu, May 31, 2018 at 09:04:25AM -0700, Casey Schaufler wrote:\r
>>> On 5/31/2018 8:39 AM, Tejun Heo wrote:\r
>>>> (cc'ing more security folks and copying whole body)\r
>>>>\r
>>>> So, I'm sure the patch fixes the memory leak but API wise it looks\r
>>>> super confusing.  Can security folks chime in here?  Is this the right\r
>>>> fix?\r
>>>> security_inode_getsecctx() provides a security context. Technically,\r
>>>> this is a data blob, although both provider provide a null terminated\r
>>>> string. security_inode_getsecurity(), on the other hand, provides a\r
>>>> string to match an attribute name. The former releases the security\r
>>>> context with security_release_secctx(), where the later releases the\r
>>>> string with kfree().\r
>>>>\r
>>>> When the Smack hook smack_inode_getsecctx() was added in 2009\r
>>>> for use by labeled NFS the alloc value passed to\r
>>> smack_inode_getsecurity() was set incorrectly. This wasn't a\r
>>> major issue, since labeled NFS is a fringe case. When kernfs\r
>>> started using the hook, it became the issue you discovered.\r
>>>\r
>>> The reason that we have all this confusion is that SELinux\r
>>> generates security contexts as needed, while Smack keeps them\r
>>> around all the time. Releasing an SELinux context frees memory,\r
>>> while releasing a Smack context is a null operation.\r
>> Any chance this detail can be hidden behind security api?  This looks\r
>> pretty error-prone, no?\r
 \r
>>It *is* hidden behind the security API. The problem is strictly\r
>>within the Smack code, where the implementer of smack_inode_getsecctx()\r
>>made an error.\r
\r
I agree that the fix can be done simply by using "false" for \r
smack_inode_getsecurity(), but what happens with kernfs_node_setsecdata()\r
and smack_inode_notifysecctx(). kernfs_node_setsecdata() is probably ignorable\r
but smack_inode_notifysecctx() is sending the "ctx" to smack_inode_setsecurity()\r
and since "ctx" would be NULL because we used "false", smack_inode_setsecurity()\r
becomes dummy.\r
\r


^ permalink raw reply	[flat|nested] 33+ messages in thread

* RE: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-06-01  8:56             ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-01  8:56 UTC (permalink / raw)
  To: Tejun Heo, Casey Schaufler
  Cc: gregkh, bfields, jlayton, linux-kernel, linux-nfs, CPGS,
	Sireesha Talluri, Chris Wright, linux-security-module

Hi
=C2=A0=0D=0A=0D=0A>On=C2=A05/31/2018=C2=A09:11=C2=A0AM,=C2=A0Tejun=C2=A0Heo=
=C2=A0wrote:=0D=0A>=C2=A0On=C2=A0Thu,=C2=A0May=C2=A031,=C2=A02018=C2=A0at=
=C2=A009:04:25AM=C2=A0-0700,=C2=A0Casey=C2=A0Schaufler=C2=A0wrote:=0D=0A>>>=
=C2=A0On=C2=A05/31/2018=C2=A08:39=C2=A0AM,=C2=A0Tejun=C2=A0Heo=C2=A0wrote:=
=0D=0A>>>>=C2=A0(cc'ing=C2=A0more=C2=A0security=C2=A0folks=C2=A0and=C2=A0co=
pying=C2=A0whole=C2=A0body)=0D=0A>>>>=0D=0A>>>>=C2=A0So,=C2=A0I'm=C2=A0sure=
=C2=A0the=C2=A0patch=C2=A0fixes=C2=A0the=C2=A0memory=C2=A0leak=C2=A0but=C2=
=A0API=C2=A0wise=C2=A0it=C2=A0looks=0D=0A>>>>=C2=A0super=C2=A0confusing.=C2=
=A0=C2=A0Can=C2=A0security=C2=A0folks=C2=A0chime=C2=A0in=C2=A0here?=C2=A0=
=C2=A0Is=C2=A0this=C2=A0the=C2=A0right=0D=0A>>>>=C2=A0fix?=0D=0A>>>>=C2=A0s=
ecurity_inode_getsecctx()=C2=A0provides=C2=A0a=C2=A0security=C2=A0context.=
=C2=A0Technically,=0D=0A>>>>=C2=A0this=C2=A0is=C2=A0a=C2=A0data=C2=A0blob,=
=C2=A0although=C2=A0both=C2=A0provider=C2=A0provide=C2=A0a=C2=A0null=C2=A0t=
erminated=0D=0A>>>>=C2=A0string.=C2=A0security_inode_getsecurity(),=C2=A0on=
=C2=A0the=C2=A0other=C2=A0hand,=C2=A0provides=C2=A0a=0D=0A>>>>=C2=A0string=
=C2=A0to=C2=A0match=C2=A0an=C2=A0attribute=C2=A0name.=C2=A0The=C2=A0former=
=C2=A0releases=C2=A0the=C2=A0security=0D=0A>>>>=C2=A0context=C2=A0with=C2=
=A0security_release_secctx(),=C2=A0where=C2=A0the=C2=A0later=C2=A0releases=
=C2=A0the=0D=0A>>>>=C2=A0string=C2=A0with=C2=A0kfree().=0D=0A>>>>=0D=0A>>>>=
=C2=A0When=C2=A0the=C2=A0Smack=C2=A0hook=C2=A0smack_inode_getsecctx()=C2=A0=
was=C2=A0added=C2=A0in=C2=A02009=0D=0A>>>>=C2=A0for=C2=A0use=C2=A0by=C2=A0l=
abeled=C2=A0NFS=C2=A0the=C2=A0alloc=C2=A0value=C2=A0passed=C2=A0to=0D=0A>>>=
=C2=A0smack_inode_getsecurity()=C2=A0was=C2=A0set=C2=A0incorrectly.=C2=A0Th=
is=C2=A0wasn't=C2=A0a=0D=0A>>>=C2=A0major=C2=A0issue,=C2=A0since=C2=A0label=
ed=C2=A0NFS=C2=A0is=C2=A0a=C2=A0fringe=C2=A0case.=C2=A0When=C2=A0kernfs=0D=
=0A>>>=C2=A0started=C2=A0using=C2=A0the=C2=A0hook,=C2=A0it=C2=A0became=C2=
=A0the=C2=A0issue=C2=A0you=C2=A0discovered.=0D=0A>>>=0D=0A>>>=C2=A0The=C2=
=A0reason=C2=A0that=C2=A0we=C2=A0have=C2=A0all=C2=A0this=C2=A0confusion=C2=
=A0is=C2=A0that=C2=A0SELinux=0D=0A>>>=C2=A0generates=C2=A0security=C2=A0con=
texts=C2=A0as=C2=A0needed,=C2=A0while=C2=A0Smack=C2=A0keeps=C2=A0them=0D=0A=
>>>=C2=A0around=C2=A0all=C2=A0the=C2=A0time.=C2=A0Releasing=C2=A0an=C2=A0SE=
Linux=C2=A0context=C2=A0frees=C2=A0memory,=0D=0A>>>=C2=A0while=C2=A0releasi=
ng=C2=A0a=C2=A0Smack=C2=A0context=C2=A0is=C2=A0a=C2=A0null=C2=A0operation.=
=0D=0A>>=C2=A0Any=C2=A0chance=C2=A0this=C2=A0detail=C2=A0can=C2=A0be=C2=A0h=
idden=C2=A0behind=C2=A0security=C2=A0api?=C2=A0=C2=A0This=C2=A0looks=0D=0A>=
>=C2=A0pretty=C2=A0error-prone,=C2=A0no?=0D=0A=C2=A0=0D=0A>>It=C2=A0*is*=C2=
=A0hidden=C2=A0behind=C2=A0the=C2=A0security=C2=A0API.=C2=A0The=C2=A0proble=
m=C2=A0is=C2=A0strictly=0D=0A>>within=C2=A0the=C2=A0Smack=C2=A0code,=C2=A0w=
here=C2=A0the=C2=A0implementer=C2=A0of=C2=A0smack_inode_getsecctx()=0D=0A>>=
made=C2=A0an=C2=A0error.=0D=0A=0D=0AI=20agree=20that=20the=20fix=20can=20be=
=20done=20simply=20by=20using=20=22false=22=20for=20=0D=0Asmack_inode_getse=
curity(),=20but=20what=20happens=20with=20kernfs_node_setsecdata()=0D=0Aand=
=20smack_inode_notifysecctx().=20kernfs_node_setsecdata()=20is=20probably=
=20ignorable=0D=0Abut=20smack_inode_notifysecctx()=20is=20sending=20the=20=
=22ctx=22=20to=20smack_inode_setsecurity()=0D=0Aand=20since=20=22ctx=22=20w=
ould=20be=20NULL=20because=20we=20used=20=22false=22,=20smack_inode_setsecu=
rity()=0D=0Abecomes=20dummy.=0D=0A=0D=0A

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-06-01  8:56             ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-01  8:56 UTC (permalink / raw)
  To: linux-security-module

Hi
?

>On?5/31/2018?9:11?AM,?Tejun?Heo?wrote:
>?On?Thu,?May?31,?2018?at?09:04:25AM?-0700,?Casey?Schaufler?wrote:
>>>?On?5/31/2018?8:39?AM,?Tejun?Heo?wrote:
>>>>?(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?
>>>>?security_inode_getsecctx()?provides?a?security?context.?Technically,
>>>>?this?is?a?data?blob,?although?both?provider?provide?a?null?terminated
>>>>?string.?security_inode_getsecurity(),?on?the?other?hand,?provides?a
>>>>?string?to?match?an?attribute?name.?The?former?releases?the?security
>>>>?context?with?security_release_secctx(),?where?the?later?releases?the
>>>>?string?with?kfree().
>>>>
>>>>?When?the?Smack?hook?smack_inode_getsecctx()?was?added?in?2009
>>>>?for?use?by?labeled?NFS?the?alloc?value?passed?to
>>>?smack_inode_getsecurity()?was?set?incorrectly.?This?wasn't?a
>>>?major?issue,?since?labeled?NFS?is?a?fringe?case.?When?kernfs
>>>?started?using?the?hook,?it?became?the?issue?you?discovered.
>>>
>>>?The?reason?that?we?have?all?this?confusion?is?that?SELinux
>>>?generates?security?contexts?as?needed,?while?Smack?keeps?them
>>>?around?all?the?time.?Releasing?an?SELinux?context?frees?memory,
>>>?while?releasing?a?Smack?context?is?a?null?operation.
>>?Any?chance?this?detail?can?be?hidden?behind?security?api???This?looks
>>?pretty?error-prone,?no?
?
>>It?*is*?hidden?behind?the?security?API.?The?problem?is?strictly
>>within?the?Smack?code,?where?the?implementer?of?smack_inode_getsecctx()
>>made?an?error.

I agree that the fix can be done simply by using "false" for 
smack_inode_getsecurity(), but what happens with kernfs_node_setsecdata()
and smack_inode_notifysecctx(). kernfs_node_setsecdata() is probably ignorable
but smack_inode_notifysecctx() is sending the "ctx" to smack_inode_setsecurity()
and since "ctx" would be NULL because we used "false", smack_inode_setsecurity()
becomes dummy.


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
  2018-06-01  8:56             ` CHANDAN VN
@ 2018-06-01 16:22               ` Casey Schaufler
  -1 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-06-01 16:22 UTC (permalink / raw)
  To: chandan.vn, Tejun Heo
  Cc: gregkh, bfields, jlayton, linux-kernel, linux-nfs, CPGS,
	Sireesha Talluri, Chris Wright, linux-security-module,
	Casey Schaufler

On 6/1/2018 1:56 AM, CHANDAN VN wrote:
> Hi
>  
>
>> On 5/31/2018 9:11 AM, Tejun Heo wrote:
>>  On Thu, May 31, 2018 at 09:04:25AM -0700, Casey Schaufler wrote:
>>>>  On 5/31/2018 8:39 AM, Tejun Heo wrote:
>>>>>  (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?
>>>>>  security_inode_getsecctx() provides a security context. Technically,
>>>>>  this is a data blob, although both provider provide a null terminated
>>>>>  string. security_inode_getsecurity(), on the other hand, provides a
>>>>>  string to match an attribute name. The former releases the security
>>>>>  context with security_release_secctx(), where the later releases the
>>>>>  string with kfree().
>>>>>
>>>>>  When the Smack hook smack_inode_getsecctx() was added in 2009
>>>>>  for use by labeled NFS the alloc value passed to
>>>>  smack_inode_getsecurity() was set incorrectly. This wasn't a
>>>>  major issue, since labeled NFS is a fringe case. When kernfs
>>>>  started using the hook, it became the issue you discovered.
>>>>
>>>>  The reason that we have all this confusion is that SELinux
>>>>  generates security contexts as needed, while Smack keeps them
>>>>  around all the time. Releasing an SELinux context frees memory,
>>>>  while releasing a Smack context is a null operation.
>>>  Any chance this detail can be hidden behind security api?  This looks
>>>  pretty error-prone, no?
>  
>>> It *is* hidden behind the security API. The problem is strictly
>>> within the Smack code, where the implementer of smack_inode_getsecctx()
>>> made an error.
> I agree that the fix can be done simply by using "false" for 
> smack_inode_getsecurity(), but what happens with kernfs_node_setsecdata()
> and smack_inode_notifysecctx(). kernfs_node_setsecdata() is probably ignorable
> but smack_inode_notifysecctx() is sending the "ctx" to smack_inode_setsecurity()
> and since "ctx" would be NULL because we used "false", smack_inode_setsecurity()
> becomes dummy.

Thank you for pointing this out. You're right, there's more
at issue here than changing the alloc flag will fix. I think
that calling smack_inode_getsecurity() from smack_inode_getsecctx()
is making the code more complicated than it needs to be. I will
have a patch shortly.



^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-06-01 16:22               ` Casey Schaufler
  0 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-06-01 16:22 UTC (permalink / raw)
  To: linux-security-module

On 6/1/2018 1:56 AM, CHANDAN VN wrote:
> Hi
> ?
>
>> On?5/31/2018?9:11?AM,?Tejun?Heo?wrote:
>> ?On?Thu,?May?31,?2018?at?09:04:25AM?-0700,?Casey?Schaufler?wrote:
>>>> ?On?5/31/2018?8:39?AM,?Tejun?Heo?wrote:
>>>>> ?(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?
>>>>> ?security_inode_getsecctx()?provides?a?security?context.?Technically,
>>>>> ?this?is?a?data?blob,?although?both?provider?provide?a?null?terminated
>>>>> ?string.?security_inode_getsecurity(),?on?the?other?hand,?provides?a
>>>>> ?string?to?match?an?attribute?name.?The?former?releases?the?security
>>>>> ?context?with?security_release_secctx(),?where?the?later?releases?the
>>>>> ?string?with?kfree().
>>>>>
>>>>> ?When?the?Smack?hook?smack_inode_getsecctx()?was?added?in?2009
>>>>> ?for?use?by?labeled?NFS?the?alloc?value?passed?to
>>>> ?smack_inode_getsecurity()?was?set?incorrectly.?This?wasn't?a
>>>> ?major?issue,?since?labeled?NFS?is?a?fringe?case.?When?kernfs
>>>> ?started?using?the?hook,?it?became?the?issue?you?discovered.
>>>>
>>>> ?The?reason?that?we?have?all?this?confusion?is?that?SELinux
>>>> ?generates?security?contexts?as?needed,?while?Smack?keeps?them
>>>> ?around?all?the?time.?Releasing?an?SELinux?context?frees?memory,
>>>> ?while?releasing?a?Smack?context?is?a?null?operation.
>>> ?Any?chance?this?detail?can?be?hidden?behind?security?api???This?looks
>>> ?pretty?error-prone,?no?
> ?
>>> It?*is*?hidden?behind?the?security?API.?The?problem?is?strictly
>>> within?the?Smack?code,?where?the?implementer?of?smack_inode_getsecctx()
>>> made?an?error.
> I agree that the fix can be done simply by using "false" for 
> smack_inode_getsecurity(), but what happens with kernfs_node_setsecdata()
> and smack_inode_notifysecctx(). kernfs_node_setsecdata() is probably ignorable
> but smack_inode_notifysecctx() is sending the "ctx" to smack_inode_setsecurity()
> and since "ctx" would be NULL because we used "false", smack_inode_setsecurity()
> becomes dummy.

Thank you for pointing this out. You're right, there's more
at issue here than changing the alloc flag will fix. I think
that calling smack_inode_getsecurity() from smack_inode_getsecctx()
is making the code more complicated than it needs to be. I will
have a patch shortly.


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* RE: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
       [not found]             ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p7>
  2018-06-01 16:29                 ` CHANDAN VN
@ 2018-06-01 16:29                 ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-01 16:29 UTC (permalink / raw)
  To: linux-security-module
  Cc: Tejun Heo, gregkh, bfields, jlayton, linux-kernel, linux-nfs,
	CPGS, Sireesha Talluri, Chris Wright, Casey Schaufler


>> I agree that the fix can be done simply by using "false" for \r
>> smack_inode_getsecurity(), but what happens with kernfs_node_setsecdata()\r
>> and smack_inode_notifysecctx(). kernfs_node_setsecdata() is probably ignorable\r
>> but smack_inode_notifysecctx() is sending the "ctx" to smack_inode_setsecurity()\r
>> and since "ctx" would be NULL because we used "false", smack_inode_setsecurity()\r
>> becomes dummy.\r
 \r
>Thank you for pointing this out. You're right, there's more\r
>at issue here than changing the alloc flag will fix. I think\r
>that calling smack_inode_getsecurity() from smack_inode_getsecctx()\r
>is making the code more complicated than it needs to be. I will\r
>have a patch shortly.\r
\r
If you think the patch would take time or is complicated, I suggest that the kfree() fix should go\r
to fix the leaks for now. \r
 \r
 \r
 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* RE: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-06-01 16:29                 ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-01 16:29 UTC (permalink / raw)
  To: linux-security-module
  Cc: Tejun Heo, gregkh, bfields, jlayton, linux-kernel, linux-nfs,
	CPGS, Sireesha Talluri, Chris Wright, Casey Schaufler


>>=C2=A0I=C2=A0agree=C2=A0that=C2=A0the=C2=A0fix=C2=A0can=C2=A0be=C2=A0done=
=C2=A0simply=C2=A0by=C2=A0using=C2=A0=22false=22=C2=A0for=C2=A0=0D=0A>>=C2=
=A0smack_inode_getsecurity(),=C2=A0but=C2=A0what=C2=A0happens=C2=A0with=C2=
=A0kernfs_node_setsecdata()=0D=0A>>=C2=A0and=C2=A0smack_inode_notifysecctx(=
).=C2=A0kernfs_node_setsecdata()=C2=A0is=C2=A0probably=C2=A0ignorable=0D=0A=
>>=C2=A0but=C2=A0smack_inode_notifysecctx()=C2=A0is=C2=A0sending=C2=A0the=
=C2=A0=22ctx=22=C2=A0to=C2=A0smack_inode_setsecurity()=0D=0A>>=C2=A0and=C2=
=A0since=C2=A0=22ctx=22=C2=A0would=C2=A0be=C2=A0NULL=C2=A0because=C2=A0we=
=C2=A0used=C2=A0=22false=22,=C2=A0smack_inode_setsecurity()=0D=0A>>=C2=A0be=
comes=C2=A0dummy.=0D=0A=C2=A0=0D=0A>Thank=C2=A0you=C2=A0for=C2=A0pointing=
=C2=A0this=C2=A0out.=C2=A0You're=C2=A0right,=C2=A0there's=C2=A0more=0D=0A>a=
t=C2=A0issue=C2=A0here=C2=A0than=C2=A0changing=C2=A0the=C2=A0alloc=C2=A0fla=
g=C2=A0will=C2=A0fix.=C2=A0I=C2=A0think=0D=0A>that=C2=A0calling=C2=A0smack_=
inode_getsecurity()=C2=A0from=C2=A0smack_inode_getsecctx()=0D=0A>is=C2=A0ma=
king=C2=A0the=C2=A0code=C2=A0more=C2=A0complicated=C2=A0than=C2=A0it=C2=A0n=
eeds=C2=A0to=C2=A0be.=C2=A0I=C2=A0will=0D=0A>have=C2=A0a=C2=A0patch=C2=A0sh=
ortly.=0D=0A=0D=0AIf=20you=20think=20the=20patch=20would=20take=20time=20or=
=20is=20complicated,=20I=20suggest=20that=20the=20kfree()=20fix=20should=20=
go=0D=0Ato=20fix=20the=20leaks=20for=20now.=C2=A0=0D=0A=C2=A0=0D=0A=C2=A0=
=0D=0A=C2=A0

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-06-01 16:29                 ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-01 16:29 UTC (permalink / raw)
  To: linux-security-module


>>?I?agree?that?the?fix?can?be?done?simply?by?using?"false"?for?
>>?smack_inode_getsecurity(),?but?what?happens?with?kernfs_node_setsecdata()
>>?and?smack_inode_notifysecctx().?kernfs_node_setsecdata()?is?probably?ignorable
>>?but?smack_inode_notifysecctx()?is?sending?the?"ctx"?to?smack_inode_setsecurity()
>>?and?since?"ctx"?would?be?NULL?because?we?used?"false",?smack_inode_setsecurity()
>>?becomes?dummy.
?
>Thank?you?for?pointing?this?out.?You're?right,?there's?more
>at?issue?here?than?changing?the?alloc?flag?will?fix.?I?think
>that?calling?smack_inode_getsecurity()?from?smack_inode_getsecctx()
>is?making?the?code?more?complicated?than?it?needs?to?be.?I?will
>have?a?patch?shortly.

If you think the patch would take time or is complicated, I suggest that the kfree() fix should go
to fix the leaks for now.?
?
?
?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
  2018-06-01 16:29                 ` CHANDAN VN
@ 2018-06-01 16:41                   ` Casey Schaufler
  -1 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-06-01 16:41 UTC (permalink / raw)
  To: chandan.vn, linux-security-module
  Cc: Tejun Heo, gregkh, bfields, jlayton, linux-kernel, linux-nfs,
	CPGS, Sireesha Talluri, Chris Wright, Casey Schaufler

On 6/1/2018 9:29 AM, CHANDAN VN wrote:
>>>  I agree that the fix can be done simply by using "false" for 
>>>  smack_inode_getsecurity(), but what happens with kernfs_node_setsecdata()
>>>  and smack_inode_notifysecctx(). kernfs_node_setsecdata() is probably ignorable
>>>  but smack_inode_notifysecctx() is sending the "ctx" to smack_inode_setsecurity()
>>>  and since "ctx" would be NULL because we used "false", smack_inode_setsecurity()
>>>  becomes dummy.
>  
>> Thank you for pointing this out. You're right, there's more
>> at issue here than changing the alloc flag will fix. I think
>> that calling smack_inode_getsecurity() from smack_inode_getsecctx()
>> is making the code more complicated than it needs to be. I will
>> have a patch shortly.
> If you think the patch would take time or is complicated, I suggest that the kfree() fix should go
> to fix the leaks for now.

Heavens no! The patch is very simple. I'm building a kernel with
it now, and should have it tested and posted within a few hours.
The implementation of smack_inode_getsecctx() that's there is
understandable, but wrong. There's a much better way to do the
job.


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set
@ 2018-06-01 16:41                   ` Casey Schaufler
  0 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-06-01 16:41 UTC (permalink / raw)
  To: linux-security-module

On 6/1/2018 9:29 AM, CHANDAN VN wrote:
>>> ?I?agree?that?the?fix?can?be?done?simply?by?using?"false"?for?
>>> ?smack_inode_getsecurity(),?but?what?happens?with?kernfs_node_setsecdata()
>>> ?and?smack_inode_notifysecctx().?kernfs_node_setsecdata()?is?probably?ignorable
>>> ?but?smack_inode_notifysecctx()?is?sending?the?"ctx"?to?smack_inode_setsecurity()
>>> ?and?since?"ctx"?would?be?NULL?because?we?used?"false",?smack_inode_setsecurity()
>>> ?becomes?dummy.
> ?
>> Thank?you?for?pointing?this?out.?You're?right,?there's?more
>> at?issue?here?than?changing?the?alloc?flag?will?fix.?I?think
>> that?calling?smack_inode_getsecurity()?from?smack_inode_getsecctx()
>> is?making?the?code?more?complicated?than?it?needs?to?be.?I?will
>> have?a?patch?shortly.
> If you think the patch would take time or is complicated, I suggest that the kfree() fix should go
> to fix the leaks for now.

Heavens no! The patch is very simple. I'm building a kernel with
it now, and should have it tested and posted within a few hours.
The implementation of smack_inode_getsecctx() that's there is
understandable, but wrong. There's a much better way to do the
job.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
  2018-06-01 16:41                   ` Casey Schaufler
@ 2018-06-01 17:45                     ` Casey Schaufler
  -1 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-06-01 17:45 UTC (permalink / raw)
  To: chandan.vn, linux-security-module
  Cc: Tejun Heo, gregkh, bfields, jlayton, linux-kernel, linux-nfs,
	CPGS, Sireesha Talluri, Chris Wright, Casey Schaufler

Fix memory leak in smack_inode_getsecctx

The implementation of smack_inode_getsecctx() made
incorrect assumptions about how Smack presents a security
context. Smack does not need to allocate memory to support
security contexts, so "releasing" a Smack context is a no-op.
The code made an unnecessary copy and returned that as a
context, which was never freed. The revised implementation
returns the context correctly.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 security/smack/smack_lsm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0b414836bebd..5e3beae334a8 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1545,9 +1545,9 @@ static int smack_inode_listsecurity(struct inode *inode, char *buffer,
  */
 static void smack_inode_getsecid(struct inode *inode, u32 *secid)
 {
-	struct inode_smack *isp = inode->i_security;
+	struct smack_known *skp = smk_of_inode(inode);
 
-	*secid = isp->smk_inode->smk_secid;
+	*secid = skp->smk_secid;
 }
 
 /*
@@ -4538,12 +4538,10 @@ static int smack_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
 
 static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
 {
-	int len = 0;
-	len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, true);
+	struct smack_known *skp = smk_of_inode(inode);
 
-	if (len < 0)
-		return len;
-	*ctxlen = len;
+	*ctx = skp->smk_known;
+	*ctxlen = strlen(skp->smk_known);
 	return 0;
 }
 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
@ 2018-06-01 17:45                     ` Casey Schaufler
  0 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-06-01 17:45 UTC (permalink / raw)
  To: linux-security-module

Fix memory leak in smack_inode_getsecctx

The implementation of smack_inode_getsecctx() made
incorrect assumptions about how Smack presents a security
context. Smack does not need to allocate memory to support
security contexts, so "releasing" a Smack context is a no-op.
The code made an unnecessary copy and returned that as a
context, which was never freed. The revised implementation
returns the context correctly.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 security/smack/smack_lsm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0b414836bebd..5e3beae334a8 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1545,9 +1545,9 @@ static int smack_inode_listsecurity(struct inode *inode, char *buffer,
  */
 static void smack_inode_getsecid(struct inode *inode, u32 *secid)
 {
-	struct inode_smack *isp = inode->i_security;
+	struct smack_known *skp = smk_of_inode(inode);
 
-	*secid = isp->smk_inode->smk_secid;
+	*secid = skp->smk_secid;
 }
 
 /*
@@ -4538,12 +4538,10 @@ static int smack_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
 
 static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
 {
-	int len = 0;
-	len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, true);
+	struct smack_known *skp = smk_of_inode(inode);
 
-	if (len < 0)
-		return len;
-	*ctxlen = len;
+	*ctx = skp->smk_known;
+	*ctxlen = strlen(skp->smk_known);
 	return 0;
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
  2018-06-01 17:45                     ` Casey Schaufler
@ 2018-06-04 21:01                       ` Casey Schaufler
  -1 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-06-04 21:01 UTC (permalink / raw)
  To: linux-security-module, Tejun Heo
  Cc: chandan.vn, gregkh, bfields, jlayton, linux-kernel, linux-nfs,
	CPGS, Sireesha Talluri, Chris Wright

On 6/1/2018 10:45 AM, Casey Schaufler wrote:
> Fix memory leak in smack_inode_getsecctx
>
> The implementation of smack_inode_getsecctx() made
> incorrect assumptions about how Smack presents a security
> context. Smack does not need to allocate memory to support
> security contexts, so "releasing" a Smack context is a no-op.
> The code made an unnecessary copy and returned that as a
> context, which was never freed. The revised implementation
> returns the context correctly.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

Tejun, does this pass your tests?

> ---
>  security/smack/smack_lsm.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0b414836bebd..5e3beae334a8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1545,9 +1545,9 @@ static int smack_inode_listsecurity(struct inode *inode, char *buffer,
>   */
>  static void smack_inode_getsecid(struct inode *inode, u32 *secid)
>  {
> -	struct inode_smack *isp = inode->i_security;
> +	struct smack_known *skp = smk_of_inode(inode);
>  
> -	*secid = isp->smk_inode->smk_secid;
> +	*secid = skp->smk_secid;
>  }
>  
>  /*
> @@ -4538,12 +4538,10 @@ static int smack_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
>  
>  static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>  {
> -	int len = 0;
> -	len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, true);
> +	struct smack_known *skp = smk_of_inode(inode);
>  
> -	if (len < 0)
> -		return len;
> -	*ctxlen = len;
> +	*ctx = skp->smk_known;
> +	*ctxlen = strlen(skp->smk_known);
>  	return 0;
>  }
>  
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
@ 2018-06-04 21:01                       ` Casey Schaufler
  0 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-06-04 21:01 UTC (permalink / raw)
  To: linux-security-module

On 6/1/2018 10:45 AM, Casey Schaufler wrote:
> Fix memory leak in smack_inode_getsecctx
>
> The implementation of smack_inode_getsecctx() made
> incorrect assumptions about how Smack presents a security
> context. Smack does not need to allocate memory to support
> security contexts, so "releasing" a Smack context is a no-op.
> The code made an unnecessary copy and returned that as a
> context, which was never freed. The revised implementation
> returns the context correctly.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

Tejun, does this pass your tests?

> ---
>  security/smack/smack_lsm.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0b414836bebd..5e3beae334a8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1545,9 +1545,9 @@ static int smack_inode_listsecurity(struct inode *inode, char *buffer,
>   */
>  static void smack_inode_getsecid(struct inode *inode, u32 *secid)
>  {
> -	struct inode_smack *isp = inode->i_security;
> +	struct smack_known *skp = smk_of_inode(inode);
>  
> -	*secid = isp->smk_inode->smk_secid;
> +	*secid = skp->smk_secid;
>  }
>  
>  /*
> @@ -4538,12 +4538,10 @@ static int smack_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
>  
>  static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>  {
> -	int len = 0;
> -	len = smack_inode_getsecurity(inode, XATTR_SMACK_SUFFIX, ctx, true);
> +	struct smack_known *skp = smk_of_inode(inode);
>  
> -	if (len < 0)
> -		return len;
> -	*ctxlen = len;
> +	*ctx = skp->smk_known;
> +	*ctxlen = strlen(skp->smk_known);
>  	return 0;
>  }
>  
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
  2018-06-04 21:01                       ` Casey Schaufler
@ 2018-06-04 21:27                         ` Tejun Heo
  -1 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2018-06-04 21:27 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: linux-security-module, chandan.vn, gregkh, bfields, jlayton,
	linux-kernel, linux-nfs, CPGS, Sireesha Talluri, Chris Wright

On Mon, Jun 04, 2018 at 02:01:34PM -0700, Casey Schaufler wrote:
> On 6/1/2018 10:45 AM, Casey Schaufler wrote:
> > Fix memory leak in smack_inode_getsecctx
> >
> > The implementation of smack_inode_getsecctx() made
> > incorrect assumptions about how Smack presents a security
> > context. Smack does not need to allocate memory to support
> > security contexts, so "releasing" a Smack context is a no-op.
> > The code made an unnecessary copy and returned that as a
> > context, which was never freed. The revised implementation
> > returns the context correctly.
> >
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> 
> Tejun, does this pass your tests?

Oh, I'm not the one who reported.  Chandan?

-- 
tejun

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
@ 2018-06-04 21:27                         ` Tejun Heo
  0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2018-06-04 21:27 UTC (permalink / raw)
  To: linux-security-module

On Mon, Jun 04, 2018 at 02:01:34PM -0700, Casey Schaufler wrote:
> On 6/1/2018 10:45 AM, Casey Schaufler wrote:
> > Fix memory leak in smack_inode_getsecctx
> >
> > The implementation of smack_inode_getsecctx() made
> > incorrect assumptions about how Smack presents a security
> > context. Smack does not need to allocate memory to support
> > security contexts, so "releasing" a Smack context is a no-op.
> > The code made an unnecessary copy and returned that as a
> > context, which was never freed. The revised implementation
> > returns the context correctly.
> >
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> 
> Tejun, does this pass your tests?

Oh, I'm not the one who reported.  Chandan?

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* RE: Re: [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
       [not found]                       ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p3>
  2018-06-05  7:04                           ` CHANDAN VN
@ 2018-06-05  7:04                           ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-05  7:04 UTC (permalink / raw)
  To: Tejun Heo, Casey Schaufler
  Cc: linux-security-module, gregkh, bfields, jlayton, linux-kernel,
	linux-nfs, CPGS, Sireesha Talluri, Chris Wright

 \r
\r
>On Mon, Jun 04, 2018 at 02:01:34PM -0700, Casey Schaufler wrote:\r
>> On 6/1/2018 10:45 AM, Casey Schaufler wrote:\r
>> > Fix memory leak in smack_inode_getsecctx\r
>> >\r
>> > The implementation of smack_inode_getsecctx() made\r
>> > incorrect assumptions about how Smack presents a security\r
>> > context. Smack does not need to allocate memory to support\r
>> > security contexts, so "releasing" a Smack context is a no-op.\r
>> > The code made an unnecessary copy and returned that as a\r
>> > context, which was never freed. The revised implementation\r
>> > returns the context correctly.\r
>> >\r
>> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>\r
>> \r
>> Tejun, does this pass your tests?\r
 \r
>Oh, I'm not the one who reported.  Chandan?\r
Looks good to me. Leak not found.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* RE: Re: [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
@ 2018-06-05  7:04                           ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-05  7:04 UTC (permalink / raw)
  To: Tejun Heo, Casey Schaufler
  Cc: linux-security-module, gregkh, bfields, jlayton, linux-kernel,
	linux-nfs, CPGS, Sireesha Talluri, Chris Wright

=C2=A0=0D=0A=0D=0A>On=C2=A0Mon,=C2=A0Jun=C2=A004,=C2=A02018=C2=A0at=C2=A002=
:01:34PM=C2=A0-0700,=C2=A0Casey=C2=A0Schaufler=C2=A0wrote:=0D=0A>>=C2=A0On=
=C2=A06/1/2018=C2=A010:45=C2=A0AM,=C2=A0Casey=C2=A0Schaufler=C2=A0wrote:=0D=
=0A>>=C2=A0>=C2=A0Fix=C2=A0memory=C2=A0leak=C2=A0in=C2=A0smack_inode_getsec=
ctx=0D=0A>>=C2=A0>=0D=0A>>=C2=A0>=C2=A0The=C2=A0implementation=C2=A0of=C2=
=A0smack_inode_getsecctx()=C2=A0made=0D=0A>>=C2=A0>=C2=A0incorrect=C2=A0ass=
umptions=C2=A0about=C2=A0how=C2=A0Smack=C2=A0presents=C2=A0a=C2=A0security=
=0D=0A>>=C2=A0>=C2=A0context.=C2=A0Smack=C2=A0does=C2=A0not=C2=A0need=C2=A0=
to=C2=A0allocate=C2=A0memory=C2=A0to=C2=A0support=0D=0A>>=C2=A0>=C2=A0secur=
ity=C2=A0contexts,=C2=A0so=C2=A0=22releasing=22=C2=A0a=C2=A0Smack=C2=A0cont=
ext=C2=A0is=C2=A0a=C2=A0no-op.=0D=0A>>=C2=A0>=C2=A0The=C2=A0code=C2=A0made=
=C2=A0an=C2=A0unnecessary=C2=A0copy=C2=A0and=C2=A0returned=C2=A0that=C2=A0a=
s=C2=A0a=0D=0A>>=C2=A0>=C2=A0context,=C2=A0which=C2=A0was=C2=A0never=C2=A0f=
reed.=C2=A0The=C2=A0revised=C2=A0implementation=0D=0A>>=C2=A0>=C2=A0returns=
=C2=A0the=C2=A0context=C2=A0correctly.=0D=0A>>=C2=A0>=0D=0A>>=C2=A0>=C2=A0S=
igned-off-by:=C2=A0Casey=C2=A0Schaufler=C2=A0<casey=40schaufler-ca.com>=0D=
=0A>>=C2=A0=0D=0A>>=C2=A0Tejun,=C2=A0does=C2=A0this=C2=A0pass=C2=A0your=C2=
=A0tests?=0D=0A=C2=A0=0D=0A>Oh,=C2=A0I'm=C2=A0not=C2=A0the=C2=A0one=C2=A0wh=
o=C2=A0reported.=C2=A0=C2=A0Chandan?=0D=0ALooks=20good=20to=20me.=20Leak=20=
not=20found.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
@ 2018-06-05  7:04                           ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-05  7:04 UTC (permalink / raw)
  To: linux-security-module

?

>On?Mon,?Jun?04,?2018?at?02:01:34PM?-0700,?Casey?Schaufler?wrote:
>>?On?6/1/2018?10:45?AM,?Casey?Schaufler?wrote:
>>?>?Fix?memory?leak?in?smack_inode_getsecctx
>>?>
>>?>?The?implementation?of?smack_inode_getsecctx()?made
>>?>?incorrect?assumptions?about?how?Smack?presents?a?security
>>?>?context.?Smack?does?not?need?to?allocate?memory?to?support
>>?>?security?contexts,?so?"releasing"?a?Smack?context?is?a?no-op.
>>?>?The?code?made?an?unnecessary?copy?and?returned?that?as?a
>>?>?context,?which?was?never?freed.?The?revised?implementation
>>?>?returns?the?context?correctly.
>>?>
>>?>?Signed-off-by:?Casey?Schaufler?<casey@schaufler-ca.com>
>>?
>>?Tejun,?does?this?pass?your?tests?
?
>Oh,?I'm?not?the?one?who?reported.??Chandan?
Looks good to me. Leak not found.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
  2018-06-05  7:04                           ` CHANDAN VN
  (?)
  (?)
@ 2018-06-05 14:29                           ` Casey Schaufler
  -1 siblings, 0 replies; 33+ messages in thread
From: Casey Schaufler @ 2018-06-05 14:29 UTC (permalink / raw)
  To: linux-security-module

On 6/5/2018 12:04 AM, CHANDAN VN wrote:
> ?
>
>> On?Mon,?Jun?04,?2018?at?02:01:34PM?-0700,?Casey?Schaufler?wrote:
>>> ?On?6/1/2018?10:45?AM,?Casey?Schaufler?wrote:
>>> ?>?Fix?memory?leak?in?smack_inode_getsecctx
>>> ?>
>>> ?>?The?implementation?of?smack_inode_getsecctx()?made
>>> ?>?incorrect?assumptions?about?how?Smack?presents?a?security
>>> ?>?context.?Smack?does?not?need?to?allocate?memory?to?support
>>> ?>?security?contexts,?so?"releasing"?a?Smack?context?is?a?no-op.
>>> ?>?The?code?made?an?unnecessary?copy?and?returned?that?as?a
>>> ?>?context,?which?was?never?freed.?The?revised?implementation
>>> ?>?returns?the?context?correctly.
>>> ?>
>>> ?>?Signed-off-by:?Casey?Schaufler?<casey@schaufler-ca.com>
>>> ?
>>> ?Tejun,?does?this?pass?your?tests?
> ?
>> Oh,?I'm?not?the?one?who?reported.??Chandan?
> Looks good to me. Leak not found.

Thanks. Can I add your "Tested-by:" on the commit?

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH] Smack: Fix memory leak in smack_inode_getsecctx
       [not found]                           ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p4>
@ 2018-06-05 14:46                             ` CHANDAN VN
  0 siblings, 0 replies; 33+ messages in thread
From: CHANDAN VN @ 2018-06-05 14:46 UTC (permalink / raw)
  To: linux-security-module

?
?
>On?6/5/2018?12:04?AM,?CHANDAN?VN?wrote:
>>??
>>
>>>?On?Mon,?Jun?04,?2018?at?02:01:34PM?-0700,?Casey?Schaufler?wrote:
>>>>??On?6/1/2018?10:45?AM,?Casey?Schaufler?wrote:
>>>>??>?Fix?memory?leak?in?smack_inode_getsecctx
>>>>??>
>>>>??>?The?implementation?of?smack_inode_getsecctx()?made
>>>>??>?incorrect?assumptions?about?how?Smack?presents?a?security
>>>>??>?context.?Smack?does?not?need?to?allocate?memory?to?support
>>>>??>?security?contexts,?so?"releasing"?a?Smack?context?is?a?no-op.
>>>>??>?The?code?made?an?unnecessary?copy?and?returned?that?as?a
>>>>??>?context,?which?was?never?freed.?The?revised?implementation
>>>>??>?returns?the?context?correctly.
>>>>??>
>>>>??>?Signed-off-by:?Casey?Schaufler?<casey@schaufler-ca.com>
>>>>??
>>>>??Tejun,?does?this?pass?your?tests?
>>??
>>>?Oh,?I'm?not?the?one?who?reported.??Chandan?
>>?Looks?good?to?me.?Leak?not?found.
>?
>Thanks.?Can?I?add?your?"Tested-by:"?on?the?commit?

Sure. I think "Reported-by:" would also be ok.
?
?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2018-06-05 14:46 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcas1p2.samsung.com>
2018-05-31  9:28 ` [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set CHANDAN VN
2018-05-31 15:26   ` Casey Schaufler
2018-05-31 20:57     ` Eric W. Biederman
2018-05-31 21:08       ` Casey Schaufler
2018-05-31 15:39   ` Tejun Heo
2018-05-31 15:39     ` Tejun Heo
2018-05-31 16:04     ` Casey Schaufler
2018-05-31 16:04       ` Casey Schaufler
2018-05-31 16:11       ` Tejun Heo
2018-05-31 16:11         ` Tejun Heo
2018-05-31 16:22         ` Casey Schaufler
2018-05-31 16:22           ` Casey Schaufler
     [not found]         ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p5>
2018-06-01  8:56           ` CHANDAN VN
2018-06-01  8:56             ` CHANDAN VN
2018-06-01  8:56             ` CHANDAN VN
2018-06-01 16:22             ` Casey Schaufler
2018-06-01 16:22               ` Casey Schaufler
     [not found]             ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p7>
2018-06-01 16:29               ` CHANDAN VN
2018-06-01 16:29                 ` CHANDAN VN
2018-06-01 16:29                 ` CHANDAN VN
2018-06-01 16:41                 ` Casey Schaufler
2018-06-01 16:41                   ` Casey Schaufler
2018-06-01 17:45                   ` [PATCH] Smack: Fix memory leak in smack_inode_getsecctx Casey Schaufler
2018-06-01 17:45                     ` Casey Schaufler
2018-06-04 21:01                     ` Casey Schaufler
2018-06-04 21:01                       ` Casey Schaufler
2018-06-04 21:27                       ` Tejun Heo
2018-06-04 21:27                         ` Tejun Heo
     [not found]                       ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p3>
2018-06-05  7:04                         ` CHANDAN VN
2018-06-05  7:04                           ` CHANDAN VN
2018-06-05  7:04                           ` CHANDAN VN
2018-06-05 14:29                           ` Casey Schaufler
     [not found]                           ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p4>
2018-06-05 14:46                             ` CHANDAN VN

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.