| Message ID | d5fa8ace-8bc4-4261-bf34-c32e7c948bb8@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | lsm,nfs: fix NFS4 memory leak of lsm_context | expand |
On Fri, Feb 21, 2025 at 10:59 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > The NFS4 security label code does not support multiple labels, and > is intentionally unaware of which LSM is providing them. It is also > the case that currently only one LSM that use security contexts is > permitted to be active, as enforced by LSM_FLAG_EXCLUSIVE. Any LSM > that receives a release_secctx that is not explicitly designated as > for another LSM can safely carry out the release process. The NFS4 > code identifies the lsm_context as LSM_ID_UNDEF, so allowing the > called LSM to perform the release is safe. Additional sophistication > will be required when context using LSMs are allowed to be used > together. Shrug. This seems less safe to me but I will give it a try with SELinux labeled NFS tests. > > Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > security/apparmor/secid.c | 2 +- > security/selinux/hooks.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c > index 28caf66b9033..db484c214cda 100644 > --- a/security/apparmor/secid.c > +++ b/security/apparmor/secid.c > @@ -108,7 +108,7 @@ int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid) > > void apparmor_release_secctx(struct lsm_context *cp) > { > - if (cp->id == LSM_ID_APPARMOR) { > + if (cp->id == LSM_ID_APPARMOR || cp->id == LSM_ID_UNDEF) { > kfree(cp->context); > cp->context = NULL; > cp->id = LSM_ID_UNDEF; > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 7b867dfec88b..b89d3438b3df 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6673,7 +6673,7 @@ static int selinux_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid) > > static void selinux_release_secctx(struct lsm_context *cp) > { > - if (cp->id == LSM_ID_SELINUX) { > + if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF) { > kfree(cp->context); > cp->context = NULL; > cp->id = LSM_ID_UNDEF; >
On Fri, Feb 21, 2025 at 10:59 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > The NFS4 security label code does not support multiple labels, and > is intentionally unaware of which LSM is providing them. It is also > the case that currently only one LSM that use security contexts is > permitted to be active, as enforced by LSM_FLAG_EXCLUSIVE. Any LSM > that receives a release_secctx that is not explicitly designated as > for another LSM can safely carry out the release process. The NFS4 > code identifies the lsm_context as LSM_ID_UNDEF, so allowing the > called LSM to perform the release is safe. Additional sophistication > will be required when context using LSMs are allowed to be used > together. > > Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > security/apparmor/secid.c | 2 +- > security/selinux/hooks.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) I'm sorry Casey, but Stephen's patch seems like a much better approach to me. https://lore.kernel.org/linux-security-module/20250220192935.9014-2-stephen.smalley.work@gmail.com/
On Fri, Feb 21, 2025 at 12:49 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Fri, Feb 21, 2025 at 10:59 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > The NFS4 security label code does not support multiple labels, and > > is intentionally unaware of which LSM is providing them. It is also > > the case that currently only one LSM that use security contexts is > > permitted to be active, as enforced by LSM_FLAG_EXCLUSIVE. Any LSM > > that receives a release_secctx that is not explicitly designated as > > for another LSM can safely carry out the release process. The NFS4 > > code identifies the lsm_context as LSM_ID_UNDEF, so allowing the > > called LSM to perform the release is safe. Additional sophistication > > will be required when context using LSMs are allowed to be used > > together. > > Shrug. This seems less safe to me but I will give it a try with > SELinux labeled NFS tests. My kernel with this patch seems to be crashing during the NFS tests but not 100% sure yet if it is your patch's fault or something else. > > > > > Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > --- > > security/apparmor/secid.c | 2 +- > > security/selinux/hooks.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c > > index 28caf66b9033..db484c214cda 100644 > > --- a/security/apparmor/secid.c > > +++ b/security/apparmor/secid.c > > @@ -108,7 +108,7 @@ int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid) > > > > void apparmor_release_secctx(struct lsm_context *cp) > > { > > - if (cp->id == LSM_ID_APPARMOR) { > > + if (cp->id == LSM_ID_APPARMOR || cp->id == LSM_ID_UNDEF) { > > kfree(cp->context); > > cp->context = NULL; > > cp->id = LSM_ID_UNDEF; > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 7b867dfec88b..b89d3438b3df 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -6673,7 +6673,7 @@ static int selinux_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid) > > > > static void selinux_release_secctx(struct lsm_context *cp) > > { > > - if (cp->id == LSM_ID_SELINUX) { > > + if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF) { > > kfree(cp->context); > > cp->context = NULL; > > cp->id = LSM_ID_UNDEF; > >
diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c index 28caf66b9033..db484c214cda 100644 --- a/security/apparmor/secid.c +++ b/security/apparmor/secid.c @@ -108,7 +108,7 @@ int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid) void apparmor_release_secctx(struct lsm_context *cp) { - if (cp->id == LSM_ID_APPARMOR) { + if (cp->id == LSM_ID_APPARMOR || cp->id == LSM_ID_UNDEF) { kfree(cp->context); cp->context = NULL; cp->id = LSM_ID_UNDEF; diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 7b867dfec88b..b89d3438b3df 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6673,7 +6673,7 @@ static int selinux_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid) static void selinux_release_secctx(struct lsm_context *cp) { - if (cp->id == LSM_ID_SELINUX) { + if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF) { kfree(cp->context); cp->context = NULL; cp->id = LSM_ID_UNDEF;
The NFS4 security label code does not support multiple labels, and is intentionally unaware of which LSM is providing them. It is also the case that currently only one LSM that use security contexts is permitted to be active, as enforced by LSM_FLAG_EXCLUSIVE. Any LSM that receives a release_secctx that is not explicitly designated as for another LSM can safely carry out the release process. The NFS4 code identifies the lsm_context as LSM_ID_UNDEF, so allowing the called LSM to perform the release is safe. Additional sophistication will be required when context using LSMs are allowed to be used together. Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- security/apparmor/secid.c | 2 +- security/selinux/hooks.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)