[PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

[PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

[CHANDAN VN]

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

Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

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

Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

[Tejun Heo]

(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

Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

[Casey Schaufler]

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

Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

[Tejun Heo]

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

Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

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

Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

[Eric W. Biederman]

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

Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

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

RE: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

[CHANDAN VN]

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.

Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

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

RE: Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

[CHANDAN VN]

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

Re: [PATCH 1/1] Fix memory leak in kernfs_security_xattr_set and kernfs_security_xattr_set

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

[PATCH] Smack: Fix memory leak in smack_inode_getsecctx

[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;
 }
 

Re: [PATCH] Smack: Fix memory leak in smack_inode_getsecctx

[Casey Schaufler]

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
>

Re: [PATCH] Smack: Fix memory leak in smack_inode_getsecctx

[Tejun Heo]

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

RE: Re: [PATCH] Smack: Fix memory leak in smack_inode_getsecctx

[CHANDAN VN]

 

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