| Message ID | 1527758911-18610-1-git-send-email-chandan.vn@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) { -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(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 >
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-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {