* [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; 16+ 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 related [flat|nested] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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 15:39 ` Tejun Heo 2018-05-31 16:04 ` Casey Schaufler 1 sibling, 1 reply; 16+ 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] 16+ 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 2018-05-31 16:11 ` Tejun Heo 0 siblings, 1 reply; 16+ 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] 16+ 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 2018-05-31 16:22 ` Casey Schaufler [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p5> 0 siblings, 2 replies; 16+ 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] 16+ 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 [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p5> 1 sibling, 0 replies; 16+ 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] 16+ messages in thread
[parent not found: <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p5>]
* 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 16:22 ` Casey Schaufler [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p7> 0 siblings, 2 replies; 16+ 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] 16+ 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 [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p7> 1 sibling, 0 replies; 16+ 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] 16+ messages in thread
[parent not found: <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p7>]
* 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:41 ` Casey Schaufler 0 siblings, 1 reply; 16+ 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] 16+ 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 2018-06-01 17:45 ` [PATCH] Smack: Fix memory leak in smack_inode_getsecctx Casey Schaufler 0 siblings, 1 reply; 16+ 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] 16+ 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 2018-06-04 21:01 ` Casey Schaufler 0 siblings, 1 reply; 16+ 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 related [flat|nested] 16+ messages in thread
* Re: [PATCH] Smack: Fix memory leak in smack_inode_getsecctx 2018-06-01 17:45 ` [PATCH] Smack: Fix memory leak in smack_inode_getsecctx Casey Schaufler @ 2018-06-04 21:01 ` Casey Schaufler 2018-06-04 21:27 ` Tejun Heo [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p3> 0 siblings, 2 replies; 16+ 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] 16+ 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 [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p3> 1 sibling, 0 replies; 16+ 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] 16+ messages in thread
[parent not found: <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p3>]
* RE: Re: [PATCH] Smack: Fix memory leak in smack_inode_getsecctx [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p3> @ 2018-06-05 7:04 ` CHANDAN VN 0 siblings, 0 replies; 16+ 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] 16+ messages in thread
end of thread, other threads:[~2018-06-05 7:04 UTC | newest] Thread overview: 16+ 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 16:04 ` Casey Schaufler 2018-05-31 16:11 ` Tejun Heo 2018-05-31 16:22 ` Casey Schaufler [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p5> 2018-06-01 8:56 ` CHANDAN VN 2018-06-01 16:22 ` Casey Schaufler [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p7> 2018-06-01 16:29 ` CHANDAN VN 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-04 21:01 ` Casey Schaufler 2018-06-04 21:27 ` Tejun Heo [not found] ` <CGME20180531092848epcas1p24b638ccd6da00f1e039bdb64de7e1a5b@epcms5p3> 2018-06-05 7:04 ` CHANDAN VN
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).