| Message ID | 20241023212158.18718-5-casey@schaufler-ca.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Oct 23, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote: > > Replace the (secctx,seclen) pointer pair with a single lsm_context > pointer to allow return of the LSM identifier along with the context > and context length. This allows security_release_secctx() to know how > to release the context. Callers have been modified to use or save the > returned data from the new structure. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > Cc: ceph-devel@vger.kernel.org > Cc: linux-nfs@vger.kernel.org > --- > fs/ceph/super.h | 3 +-- > fs/ceph/xattr.c | 16 ++++++---------- > fs/fuse/dir.c | 35 ++++++++++++++++++----------------- > fs/nfs/nfs4proc.c | 20 ++++++++++++-------- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/security.h | 26 +++----------------------- > security/security.c | 9 ++++----- > security/selinux/hooks.c | 9 +++++---- > 8 files changed, 50 insertions(+), 70 deletions(-) See my note on patch 1/5, merging into lsm/dev. -- paul-moore.com
On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > Replace the (secctx,seclen) pointer pair with a single lsm_context > pointer to allow return of the LSM identifier along with the context > and context length. This allows security_release_secctx() to know how > to release the context. Callers have been modified to use or save the > returned data from the new structure. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > Cc: ceph-devel@vger.kernel.org > Cc: linux-nfs@vger.kernel.org > --- > fs/ceph/super.h | 3 +-- > fs/ceph/xattr.c | 16 ++++++---------- > fs/fuse/dir.c | 35 ++++++++++++++++++----------------- > fs/nfs/nfs4proc.c | 20 ++++++++++++-------- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/security.h | 26 +++----------------------- > security/security.c | 9 ++++----- > security/selinux/hooks.c | 9 +++++---- > 8 files changed, 50 insertions(+), 70 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 76776d716744..0b116ef3a752 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -114,6 +114,7 @@ static inline struct nfs4_label * > nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > struct iattr *sattr, struct nfs4_label *label) > { > + struct lsm_context shim; > int err; > > if (label == NULL) > @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > label->label = NULL; > > err = security_dentry_init_security(dentry, sattr->ia_mode, > - &dentry->d_name, NULL, > - (void **)&label->label, &label->len); > - if (err == 0) > - return label; > + &dentry->d_name, NULL, &shim); > + if (err) > + return NULL; > > - return NULL; > + label->label = shim.context; > + label->len = shim.len; > + return label; > } > static inline void > nfs4_label_release_security(struct nfs4_label *label) > { > - struct lsm_context scaff; /* scaffolding */ > + struct lsm_context shim; > > if (label) { > - lsmcontext_init(&scaff, label->label, label->len, 0); > - security_release_secctx(&scaff); > + shim.context = label->label; > + shim.len = label->len; > + shim.id = LSM_ID_UNDEF; Is there a patch that follows this one to fix this? Otherwise, setting this to UNDEF causes SELinux to NOT free the context, which produces a memory leak for every NFS inode security context. Reported by kmemleak when running the selinux-testsuite NFS tests. > + security_release_secctx(&shim); > } > } > static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > Replace the (secctx,seclen) pointer pair with a single lsm_context > > pointer to allow return of the LSM identifier along with the context > > and context length. This allows security_release_secctx() to know how > > to release the context. Callers have been modified to use or save the > > returned data from the new structure. > > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > Cc: ceph-devel@vger.kernel.org > > Cc: linux-nfs@vger.kernel.org > > --- > > fs/ceph/super.h | 3 +-- > > fs/ceph/xattr.c | 16 ++++++---------- > > fs/fuse/dir.c | 35 ++++++++++++++++++----------------- > > fs/nfs/nfs4proc.c | 20 ++++++++++++-------- > > include/linux/lsm_hook_defs.h | 2 +- > > include/linux/security.h | 26 +++----------------------- > > security/security.c | 9 ++++----- > > security/selinux/hooks.c | 9 +++++---- > > 8 files changed, 50 insertions(+), 70 deletions(-) > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > index 76776d716744..0b116ef3a752 100644 > > --- a/fs/nfs/nfs4proc.c > > +++ b/fs/nfs/nfs4proc.c > > @@ -114,6 +114,7 @@ static inline struct nfs4_label * > > nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > > struct iattr *sattr, struct nfs4_label *label) > > { > > + struct lsm_context shim; > > int err; > > > > if (label == NULL) > > @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > > label->label = NULL; > > > > err = security_dentry_init_security(dentry, sattr->ia_mode, > > - &dentry->d_name, NULL, > > - (void **)&label->label, &label->len); > > - if (err == 0) > > - return label; > > + &dentry->d_name, NULL, &shim); > > + if (err) > > + return NULL; > > > > - return NULL; > > + label->label = shim.context; > > + label->len = shim.len; > > + return label; > > } > > static inline void > > nfs4_label_release_security(struct nfs4_label *label) > > { > > - struct lsm_context scaff; /* scaffolding */ > > + struct lsm_context shim; > > > > if (label) { > > - lsmcontext_init(&scaff, label->label, label->len, 0); > > - security_release_secctx(&scaff); > > + shim.context = label->label; > > + shim.len = label->len; > > + shim.id = LSM_ID_UNDEF; > > Is there a patch that follows this one to fix this? Otherwise, setting > this to UNDEF causes SELinux to NOT free the context, which produces a > memory leak for every NFS inode security context. Reported by kmemleak > when running the selinux-testsuite NFS tests. I don't recall seeing anything related to this, but patches are definitely welcome.
On 2/20/2025 9:40 AM, Paul Moore wrote: > On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: >> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>> Replace the (secctx,seclen) pointer pair with a single lsm_context >>> pointer to allow return of the LSM identifier along with the context >>> and context length. This allows security_release_secctx() to know how >>> to release the context. Callers have been modified to use or save the >>> returned data from the new structure. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> Cc: ceph-devel@vger.kernel.org >>> Cc: linux-nfs@vger.kernel.org >>> --- >>> fs/ceph/super.h | 3 +-- >>> fs/ceph/xattr.c | 16 ++++++---------- >>> fs/fuse/dir.c | 35 ++++++++++++++++++----------------- >>> fs/nfs/nfs4proc.c | 20 ++++++++++++-------- >>> include/linux/lsm_hook_defs.h | 2 +- >>> include/linux/security.h | 26 +++----------------------- >>> security/security.c | 9 ++++----- >>> security/selinux/hooks.c | 9 +++++---- >>> 8 files changed, 50 insertions(+), 70 deletions(-) >>> >>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>> index 76776d716744..0b116ef3a752 100644 >>> --- a/fs/nfs/nfs4proc.c >>> +++ b/fs/nfs/nfs4proc.c >>> @@ -114,6 +114,7 @@ static inline struct nfs4_label * >>> nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>> struct iattr *sattr, struct nfs4_label *label) >>> { >>> + struct lsm_context shim; >>> int err; >>> >>> if (label == NULL) >>> @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>> label->label = NULL; >>> >>> err = security_dentry_init_security(dentry, sattr->ia_mode, >>> - &dentry->d_name, NULL, >>> - (void **)&label->label, &label->len); >>> - if (err == 0) >>> - return label; >>> + &dentry->d_name, NULL, &shim); >>> + if (err) >>> + return NULL; >>> >>> - return NULL; >>> + label->label = shim.context; >>> + label->len = shim.len; >>> + return label; >>> } >>> static inline void >>> nfs4_label_release_security(struct nfs4_label *label) >>> { >>> - struct lsm_context scaff; /* scaffolding */ >>> + struct lsm_context shim; >>> >>> if (label) { >>> - lsmcontext_init(&scaff, label->label, label->len, 0); >>> - security_release_secctx(&scaff); >>> + shim.context = label->label; >>> + shim.len = label->len; >>> + shim.id = LSM_ID_UNDEF; >> Is there a patch that follows this one to fix this? Otherwise, setting >> this to UNDEF causes SELinux to NOT free the context, which produces a >> memory leak for every NFS inode security context. Reported by kmemleak >> when running the selinux-testsuite NFS tests. > I don't recall seeing anything related to this, but patches are > definitely welcome. I'm looking into this but as you well know the NFS tests aren't always especially cooperative.
On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > > > Replace the (secctx,seclen) pointer pair with a single lsm_context > > > pointer to allow return of the LSM identifier along with the context > > > and context length. This allows security_release_secctx() to know how > > > to release the context. Callers have been modified to use or save the > > > returned data from the new structure. > > > > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > > Cc: ceph-devel@vger.kernel.org > > > Cc: linux-nfs@vger.kernel.org > > > --- > > > fs/ceph/super.h | 3 +-- > > > fs/ceph/xattr.c | 16 ++++++---------- > > > fs/fuse/dir.c | 35 ++++++++++++++++++----------------- > > > fs/nfs/nfs4proc.c | 20 ++++++++++++-------- > > > include/linux/lsm_hook_defs.h | 2 +- > > > include/linux/security.h | 26 +++----------------------- > > > security/security.c | 9 ++++----- > > > security/selinux/hooks.c | 9 +++++---- > > > 8 files changed, 50 insertions(+), 70 deletions(-) > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > index 76776d716744..0b116ef3a752 100644 > > > --- a/fs/nfs/nfs4proc.c > > > +++ b/fs/nfs/nfs4proc.c > > > @@ -114,6 +114,7 @@ static inline struct nfs4_label * > > > nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > > > struct iattr *sattr, struct nfs4_label *label) > > > { > > > + struct lsm_context shim; > > > int err; > > > > > > if (label == NULL) > > > @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > > > label->label = NULL; > > > > > > err = security_dentry_init_security(dentry, sattr->ia_mode, > > > - &dentry->d_name, NULL, > > > - (void **)&label->label, &label->len); > > > - if (err == 0) > > > - return label; > > > + &dentry->d_name, NULL, &shim); > > > + if (err) > > > + return NULL; > > > > > > - return NULL; > > > + label->label = shim.context; > > > + label->len = shim.len; > > > + return label; > > > } > > > static inline void > > > nfs4_label_release_security(struct nfs4_label *label) > > > { > > > - struct lsm_context scaff; /* scaffolding */ > > > + struct lsm_context shim; > > > > > > if (label) { > > > - lsmcontext_init(&scaff, label->label, label->len, 0); > > > - security_release_secctx(&scaff); > > > + shim.context = label->label; > > > + shim.len = label->len; > > > + shim.id = LSM_ID_UNDEF; > > > > Is there a patch that follows this one to fix this? Otherwise, setting > > this to UNDEF causes SELinux to NOT free the context, which produces a > > memory leak for every NFS inode security context. Reported by kmemleak > > when running the selinux-testsuite NFS tests. > > I don't recall seeing anything related to this, but patches are > definitely welcome. Looking at this quickly, this is an interesting problem as I don't believe we have enough context in nfs4_label_release_security() to correctly set the shim.id value. If there is a positive, it is that lsm_context is really still just a string wrapped up with some metadata, e.g. length/ID, so we kfree()'ing shim.context is going to be okay-ish, at least for the foreseeable future. I can think of two ways to fix this, but I'd love to hear other ideas too. 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx() and skip any individual LSM processing. 2. Define a new LSM_ID_ANY value and update all of the LSMs to also process the ANY case as well as their own. I'm not finding either option very exciting, but option #2 looks particularly ugly, so I think I'd prefer to see someone draft a patch for option #1 assuming nothing better is presented.
On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > > > > > Replace the (secctx,seclen) pointer pair with a single lsm_context > > > > pointer to allow return of the LSM identifier along with the context > > > > and context length. This allows security_release_secctx() to know how > > > > to release the context. Callers have been modified to use or save the > > > > returned data from the new structure. > > > > > > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > > > Cc: ceph-devel@vger.kernel.org > > > > Cc: linux-nfs@vger.kernel.org > > > > --- > > > > fs/ceph/super.h | 3 +-- > > > > fs/ceph/xattr.c | 16 ++++++---------- > > > > fs/fuse/dir.c | 35 ++++++++++++++++++----------------- > > > > fs/nfs/nfs4proc.c | 20 ++++++++++++-------- > > > > include/linux/lsm_hook_defs.h | 2 +- > > > > include/linux/security.h | 26 +++----------------------- > > > > security/security.c | 9 ++++----- > > > > security/selinux/hooks.c | 9 +++++---- > > > > 8 files changed, 50 insertions(+), 70 deletions(-) > > > > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > > index 76776d716744..0b116ef3a752 100644 > > > > --- a/fs/nfs/nfs4proc.c > > > > +++ b/fs/nfs/nfs4proc.c > > > > @@ -114,6 +114,7 @@ static inline struct nfs4_label * > > > > nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > > > > struct iattr *sattr, struct nfs4_label *label) > > > > { > > > > + struct lsm_context shim; > > > > int err; > > > > > > > > if (label == NULL) > > > > @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > > > > label->label = NULL; > > > > > > > > err = security_dentry_init_security(dentry, sattr->ia_mode, > > > > - &dentry->d_name, NULL, > > > > - (void **)&label->label, &label->len); > > > > - if (err == 0) > > > > - return label; > > > > + &dentry->d_name, NULL, &shim); > > > > + if (err) > > > > + return NULL; > > > > > > > > - return NULL; > > > > + label->label = shim.context; > > > > + label->len = shim.len; > > > > + return label; > > > > } > > > > static inline void > > > > nfs4_label_release_security(struct nfs4_label *label) > > > > { > > > > - struct lsm_context scaff; /* scaffolding */ > > > > + struct lsm_context shim; > > > > > > > > if (label) { > > > > - lsmcontext_init(&scaff, label->label, label->len, 0); > > > > - security_release_secctx(&scaff); > > > > + shim.context = label->label; > > > > + shim.len = label->len; > > > > + shim.id = LSM_ID_UNDEF; > > > > > > Is there a patch that follows this one to fix this? Otherwise, setting > > > this to UNDEF causes SELinux to NOT free the context, which produces a > > > memory leak for every NFS inode security context. Reported by kmemleak > > > when running the selinux-testsuite NFS tests. > > > > I don't recall seeing anything related to this, but patches are > > definitely welcome. > > Looking at this quickly, this is an interesting problem as I don't > believe we have enough context in nfs4_label_release_security() to > correctly set the shim.id value. If there is a positive, it is that > lsm_context is really still just a string wrapped up with some > metadata, e.g. length/ID, so we kfree()'ing shim.context is going to > be okay-ish, at least for the foreseeable future. > > I can think of two ways to fix this, but I'd love to hear other ideas too. > > 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx() > and skip any individual LSM processing. > > 2. Define a new LSM_ID_ANY value and update all of the LSMs to also > process the ANY case as well as their own. > > I'm not finding either option very exciting, but option #2 looks > particularly ugly, so I think I'd prefer to see someone draft a patch > for option #1 assuming nothing better is presented. We could perhaps add a u32 lsmid to struct nfs4_label, save it from the shim.id obtained in nfs4_label_init_security(), and use it in nfs4_label_release_security(). Not sure why that wasn't done in the first place.
On 2/20/2025 10:02 AM, Stephen Smalley wrote: > On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote: >> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: >>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley >>> <stephen.smalley.work@gmail.com> wrote: >>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>> Replace the (secctx,seclen) pointer pair with a single lsm_context >>>>> pointer to allow return of the LSM identifier along with the context >>>>> and context length. This allows security_release_secctx() to know how >>>>> to release the context. Callers have been modified to use or save the >>>>> returned data from the new structure. >>>>> >>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>>> Cc: ceph-devel@vger.kernel.org >>>>> Cc: linux-nfs@vger.kernel.org >>>>> --- >>>>> fs/ceph/super.h | 3 +-- >>>>> fs/ceph/xattr.c | 16 ++++++---------- >>>>> fs/fuse/dir.c | 35 ++++++++++++++++++----------------- >>>>> fs/nfs/nfs4proc.c | 20 ++++++++++++-------- >>>>> include/linux/lsm_hook_defs.h | 2 +- >>>>> include/linux/security.h | 26 +++----------------------- >>>>> security/security.c | 9 ++++----- >>>>> security/selinux/hooks.c | 9 +++++---- >>>>> 8 files changed, 50 insertions(+), 70 deletions(-) >>>>> >>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>>>> index 76776d716744..0b116ef3a752 100644 >>>>> --- a/fs/nfs/nfs4proc.c >>>>> +++ b/fs/nfs/nfs4proc.c >>>>> @@ -114,6 +114,7 @@ static inline struct nfs4_label * >>>>> nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>>>> struct iattr *sattr, struct nfs4_label *label) >>>>> { >>>>> + struct lsm_context shim; >>>>> int err; >>>>> >>>>> if (label == NULL) >>>>> @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>>>> label->label = NULL; >>>>> >>>>> err = security_dentry_init_security(dentry, sattr->ia_mode, >>>>> - &dentry->d_name, NULL, >>>>> - (void **)&label->label, &label->len); >>>>> - if (err == 0) >>>>> - return label; >>>>> + &dentry->d_name, NULL, &shim); >>>>> + if (err) >>>>> + return NULL; >>>>> >>>>> - return NULL; >>>>> + label->label = shim.context; >>>>> + label->len = shim.len; >>>>> + return label; >>>>> } >>>>> static inline void >>>>> nfs4_label_release_security(struct nfs4_label *label) >>>>> { >>>>> - struct lsm_context scaff; /* scaffolding */ >>>>> + struct lsm_context shim; >>>>> >>>>> if (label) { >>>>> - lsmcontext_init(&scaff, label->label, label->len, 0); >>>>> - security_release_secctx(&scaff); >>>>> + shim.context = label->label; >>>>> + shim.len = label->len; >>>>> + shim.id = LSM_ID_UNDEF; >>>> Is there a patch that follows this one to fix this? Otherwise, setting >>>> this to UNDEF causes SELinux to NOT free the context, which produces a >>>> memory leak for every NFS inode security context. Reported by kmemleak >>>> when running the selinux-testsuite NFS tests. >>> I don't recall seeing anything related to this, but patches are >>> definitely welcome. >> Looking at this quickly, this is an interesting problem as I don't >> believe we have enough context in nfs4_label_release_security() to >> correctly set the shim.id value. If there is a positive, it is that >> lsm_context is really still just a string wrapped up with some >> metadata, e.g. length/ID, so we kfree()'ing shim.context is going to >> be okay-ish, at least for the foreseeable future. >> >> I can think of two ways to fix this, but I'd love to hear other ideas too. >> >> 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx() >> and skip any individual LSM processing. >> >> 2. Define a new LSM_ID_ANY value and update all of the LSMs to also >> process the ANY case as well as their own. >> >> I'm not finding either option very exciting, but option #2 looks >> particularly ugly, so I think I'd prefer to see someone draft a patch >> for option #1 assuming nothing better is presented. > We could perhaps add a u32 lsmid to struct nfs4_label, save it from > the shim.id obtained in nfs4_label_init_security(), and use it in > nfs4_label_release_security(). Not sure why that wasn't done in the > first place. This is all an artifact of breaking up and rearranging the full stacking patches. My bad. I have to find where the logic for releasing this got lost.
On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley > > > <stephen.smalley.work@gmail.com> wrote: > > > > On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > > > > > > > Replace the (secctx,seclen) pointer pair with a single lsm_context > > > > > pointer to allow return of the LSM identifier along with the context > > > > > and context length. This allows security_release_secctx() to know how > > > > > to release the context. Callers have been modified to use or save the > > > > > returned data from the new structure. > > > > > > > > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > > > > Cc: ceph-devel@vger.kernel.org > > > > > Cc: linux-nfs@vger.kernel.org > > > > > --- > > > > > fs/ceph/super.h | 3 +-- > > > > > fs/ceph/xattr.c | 16 ++++++---------- > > > > > fs/fuse/dir.c | 35 ++++++++++++++++++----------------- > > > > > fs/nfs/nfs4proc.c | 20 ++++++++++++-------- > > > > > include/linux/lsm_hook_defs.h | 2 +- > > > > > include/linux/security.h | 26 +++----------------------- > > > > > security/security.c | 9 ++++----- > > > > > security/selinux/hooks.c | 9 +++++---- > > > > > 8 files changed, 50 insertions(+), 70 deletions(-) > > > > > > > > > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > > > index 76776d716744..0b116ef3a752 100644 > > > > > --- a/fs/nfs/nfs4proc.c > > > > > +++ b/fs/nfs/nfs4proc.c > > > > > @@ -114,6 +114,7 @@ static inline struct nfs4_label * > > > > > nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > > > > > struct iattr *sattr, struct nfs4_label *label) > > > > > { > > > > > + struct lsm_context shim; > > > > > int err; > > > > > > > > > > if (label == NULL) > > > > > @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > > > > > label->label = NULL; > > > > > > > > > > err = security_dentry_init_security(dentry, sattr->ia_mode, > > > > > - &dentry->d_name, NULL, > > > > > - (void **)&label->label, &label->len); > > > > > - if (err == 0) > > > > > - return label; > > > > > + &dentry->d_name, NULL, &shim); > > > > > + if (err) > > > > > + return NULL; > > > > > > > > > > - return NULL; > > > > > + label->label = shim.context; > > > > > + label->len = shim.len; > > > > > + return label; > > > > > } > > > > > static inline void > > > > > nfs4_label_release_security(struct nfs4_label *label) > > > > > { > > > > > - struct lsm_context scaff; /* scaffolding */ > > > > > + struct lsm_context shim; > > > > > > > > > > if (label) { > > > > > - lsmcontext_init(&scaff, label->label, label->len, 0); > > > > > - security_release_secctx(&scaff); > > > > > + shim.context = label->label; > > > > > + shim.len = label->len; > > > > > + shim.id = LSM_ID_UNDEF; > > > > > > > > Is there a patch that follows this one to fix this? Otherwise, setting > > > > this to UNDEF causes SELinux to NOT free the context, which produces a > > > > memory leak for every NFS inode security context. Reported by kmemleak > > > > when running the selinux-testsuite NFS tests. > > > > > > I don't recall seeing anything related to this, but patches are > > > definitely welcome. > > > > Looking at this quickly, this is an interesting problem as I don't > > believe we have enough context in nfs4_label_release_security() to > > correctly set the shim.id value. If there is a positive, it is that > > lsm_context is really still just a string wrapped up with some > > metadata, e.g. length/ID, so we kfree()'ing shim.context is going to > > be okay-ish, at least for the foreseeable future. > > > > I can think of two ways to fix this, but I'd love to hear other ideas too. > > > > 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx() > > and skip any individual LSM processing. > > > > 2. Define a new LSM_ID_ANY value and update all of the LSMs to also > > process the ANY case as well as their own. > > > > I'm not finding either option very exciting, but option #2 looks > > particularly ugly, so I think I'd prefer to see someone draft a patch > > for option #1 assuming nothing better is presented. > > We could perhaps add a u32 lsmid to struct nfs4_label, save it from > the shim.id obtained in nfs4_label_init_security(), and use it in > nfs4_label_release_security(). Not sure why that wasn't done in the > first place. Something like this (not tested yet). If this looks sane, will submit separately. commit b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") did not preserve the lsm id for subsequent release calls, which results in a memory leak. Fix it by saving the lsm id in the nfs4_label and providing it on the subsequent release call. Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> --- fs/nfs/nfs4proc.c | 7 ++++--- include/linux/nfs4.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index df9669d4ded7..c0caaec7bd20 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -133,6 +133,7 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, if (err) return NULL; + label->lsmid = shim.id; label->label = shim.context; label->len = shim.len; return label; @@ -145,7 +146,7 @@ nfs4_label_release_security(struct nfs4_label *label) if (label) { shim.context = label->label; shim.len = label->len; - shim.id = LSM_ID_UNDEF; + shim.id = label->lsmid; security_release_secctx(&shim); } } @@ -6269,7 +6270,7 @@ static int _nfs4_get_security_label(struct inode *inode, void *buf, size_t buflen) { struct nfs_server *server = NFS_SERVER(inode); - struct nfs4_label label = {0, 0, buflen, buf}; + struct nfs4_label label = {0, 0, 0, buflen, buf}; u32 bitmask[3] = { 0, 0, FATTR4_WORD2_SECURITY_LABEL }; struct nfs_fattr fattr = { @@ -6374,7 +6375,7 @@ static int nfs4_do_set_security_label(struct inode *inode, static int nfs4_set_security_label(struct inode *inode, const void *buf, size_t buflen) { - struct nfs4_label ilabel = {0, 0, buflen, (char *)buf }; + struct nfs4_label ilabel = {0, 0, 0, buflen, (char *)buf }; struct nfs_fattr *fattr; int status; diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h index 71fbebfa43c7..9ac83ca88326 100644 --- a/include/linux/nfs4.h +++ b/include/linux/nfs4.h @@ -47,6 +47,7 @@ struct nfs4_acl { struct nfs4_label { uint32_t lfs; uint32_t pi; + u32 lsmid; u32 len; char *label; };
On 2/20/2025 10:16 AM, Stephen Smalley wrote: > On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: >> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote: >>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: >>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley >>>> <stephen.smalley.work@gmail.com> wrote: >>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>>> Replace the (secctx,seclen) pointer pair with a single lsm_context >>>>>> pointer to allow return of the LSM identifier along with the context >>>>>> and context length. This allows security_release_secctx() to know how >>>>>> to release the context. Callers have been modified to use or save the >>>>>> returned data from the new structure. >>>>>> >>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>>>> Cc: ceph-devel@vger.kernel.org >>>>>> Cc: linux-nfs@vger.kernel.org >>>>>> --- >>>>>> fs/ceph/super.h | 3 +-- >>>>>> fs/ceph/xattr.c | 16 ++++++---------- >>>>>> fs/fuse/dir.c | 35 ++++++++++++++++++----------------- >>>>>> fs/nfs/nfs4proc.c | 20 ++++++++++++-------- >>>>>> include/linux/lsm_hook_defs.h | 2 +- >>>>>> include/linux/security.h | 26 +++----------------------- >>>>>> security/security.c | 9 ++++----- >>>>>> security/selinux/hooks.c | 9 +++++---- >>>>>> 8 files changed, 50 insertions(+), 70 deletions(-) >>>>>> >>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>>>>> index 76776d716744..0b116ef3a752 100644 >>>>>> --- a/fs/nfs/nfs4proc.c >>>>>> +++ b/fs/nfs/nfs4proc.c >>>>>> @@ -114,6 +114,7 @@ static inline struct nfs4_label * >>>>>> nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>>>>> struct iattr *sattr, struct nfs4_label *label) >>>>>> { >>>>>> + struct lsm_context shim; >>>>>> int err; >>>>>> >>>>>> if (label == NULL) >>>>>> @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>>>>> label->label = NULL; >>>>>> >>>>>> err = security_dentry_init_security(dentry, sattr->ia_mode, >>>>>> - &dentry->d_name, NULL, >>>>>> - (void **)&label->label, &label->len); >>>>>> - if (err == 0) >>>>>> - return label; >>>>>> + &dentry->d_name, NULL, &shim); >>>>>> + if (err) >>>>>> + return NULL; >>>>>> >>>>>> - return NULL; >>>>>> + label->label = shim.context; >>>>>> + label->len = shim.len; >>>>>> + return label; >>>>>> } >>>>>> static inline void >>>>>> nfs4_label_release_security(struct nfs4_label *label) >>>>>> { >>>>>> - struct lsm_context scaff; /* scaffolding */ >>>>>> + struct lsm_context shim; >>>>>> >>>>>> if (label) { >>>>>> - lsmcontext_init(&scaff, label->label, label->len, 0); >>>>>> - security_release_secctx(&scaff); >>>>>> + shim.context = label->label; >>>>>> + shim.len = label->len; >>>>>> + shim.id = LSM_ID_UNDEF; >>>>> Is there a patch that follows this one to fix this? Otherwise, setting >>>>> this to UNDEF causes SELinux to NOT free the context, which produces a >>>>> memory leak for every NFS inode security context. Reported by kmemleak >>>>> when running the selinux-testsuite NFS tests. >>>> I don't recall seeing anything related to this, but patches are >>>> definitely welcome. >>> Looking at this quickly, this is an interesting problem as I don't >>> believe we have enough context in nfs4_label_release_security() to >>> correctly set the shim.id value. If there is a positive, it is that >>> lsm_context is really still just a string wrapped up with some >>> metadata, e.g. length/ID, so we kfree()'ing shim.context is going to >>> be okay-ish, at least for the foreseeable future. >>> >>> I can think of two ways to fix this, but I'd love to hear other ideas too. >>> >>> 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx() >>> and skip any individual LSM processing. >>> >>> 2. Define a new LSM_ID_ANY value and update all of the LSMs to also >>> process the ANY case as well as their own. >>> >>> I'm not finding either option very exciting, but option #2 looks >>> particularly ugly, so I think I'd prefer to see someone draft a patch >>> for option #1 assuming nothing better is presented. >> We could perhaps add a u32 lsmid to struct nfs4_label, save it from >> the shim.id obtained in nfs4_label_init_security(), and use it in >> nfs4_label_release_security(). Not sure why that wasn't done in the >> first place. > Something like this (not tested yet). If this looks sane, will submit > separately. > > commit b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") > did not preserve the lsm id for subsequent release calls, which results > in a memory leak. Fix it by saving the lsm id in the nfs4_label and > providing it on the subsequent release call. > > Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> I'm not a fan of adding secids into other subsystems, especially in cases where they've tried to avoid them in the past. The better solution, which I'm tracking down the patch for now, is for the individual LSMs to always do their release, and for security_release_secctx() to check the lsm_id and call the appropriate LSM specific hook. Until there are multiple LSMs with contexts, LSM_ID_UNDEF is as good as a match. Please don't use this patch. > --- > fs/nfs/nfs4proc.c | 7 ++++--- > include/linux/nfs4.h | 1 + > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index df9669d4ded7..c0caaec7bd20 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -133,6 +133,7 @@ nfs4_label_init_security(struct inode *dir, struct > dentry *dentry, > if (err) > return NULL; > > + label->lsmid = shim.id; > label->label = shim.context; > label->len = shim.len; > return label; > @@ -145,7 +146,7 @@ nfs4_label_release_security(struct nfs4_label *label) > if (label) { > shim.context = label->label; > shim.len = label->len; > - shim.id = LSM_ID_UNDEF; > + shim.id = label->lsmid; > security_release_secctx(&shim); > } > } > @@ -6269,7 +6270,7 @@ static int _nfs4_get_security_label(struct inode > *inode, void *buf, > size_t buflen) > { > struct nfs_server *server = NFS_SERVER(inode); > - struct nfs4_label label = {0, 0, buflen, buf}; > + struct nfs4_label label = {0, 0, 0, buflen, buf}; > > u32 bitmask[3] = { 0, 0, FATTR4_WORD2_SECURITY_LABEL }; > struct nfs_fattr fattr = { > @@ -6374,7 +6375,7 @@ static int nfs4_do_set_security_label(struct inode *inode, > static int > nfs4_set_security_label(struct inode *inode, const void *buf, size_t buflen) > { > - struct nfs4_label ilabel = {0, 0, buflen, (char *)buf }; > + struct nfs4_label ilabel = {0, 0, 0, buflen, (char *)buf }; > struct nfs_fattr *fattr; > int status; > > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > index 71fbebfa43c7..9ac83ca88326 100644 > --- a/include/linux/nfs4.h > +++ b/include/linux/nfs4.h > @@ -47,6 +47,7 @@ struct nfs4_acl { > struct nfs4_label { > uint32_t lfs; > uint32_t pi; > + u32 lsmid; > u32 len; > char *label; > };
On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 2/20/2025 10:16 AM, Stephen Smalley wrote: > > On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > >> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote: > >>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: > >>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley > >>>> <stephen.smalley.work@gmail.com> wrote: > >>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >>>>>> Replace the (secctx,seclen) pointer pair with a single lsm_context > >>>>>> pointer to allow return of the LSM identifier along with the context > >>>>>> and context length. This allows security_release_secctx() to know how > >>>>>> to release the context. Callers have been modified to use or save the > >>>>>> returned data from the new structure. > >>>>>> > >>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > >>>>>> Cc: ceph-devel@vger.kernel.org > >>>>>> Cc: linux-nfs@vger.kernel.org > >>>>>> --- > >>>>>> fs/ceph/super.h | 3 +-- > >>>>>> fs/ceph/xattr.c | 16 ++++++---------- > >>>>>> fs/fuse/dir.c | 35 ++++++++++++++++++----------------- > >>>>>> fs/nfs/nfs4proc.c | 20 ++++++++++++-------- > >>>>>> include/linux/lsm_hook_defs.h | 2 +- > >>>>>> include/linux/security.h | 26 +++----------------------- > >>>>>> security/security.c | 9 ++++----- > >>>>>> security/selinux/hooks.c | 9 +++++---- > >>>>>> 8 files changed, 50 insertions(+), 70 deletions(-) > >>>>>> > >>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > >>>>>> index 76776d716744..0b116ef3a752 100644 > >>>>>> --- a/fs/nfs/nfs4proc.c > >>>>>> +++ b/fs/nfs/nfs4proc.c > >>>>>> @@ -114,6 +114,7 @@ static inline struct nfs4_label * > >>>>>> nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > >>>>>> struct iattr *sattr, struct nfs4_label *label) > >>>>>> { > >>>>>> + struct lsm_context shim; > >>>>>> int err; > >>>>>> > >>>>>> if (label == NULL) > >>>>>> @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > >>>>>> label->label = NULL; > >>>>>> > >>>>>> err = security_dentry_init_security(dentry, sattr->ia_mode, > >>>>>> - &dentry->d_name, NULL, > >>>>>> - (void **)&label->label, &label->len); > >>>>>> - if (err == 0) > >>>>>> - return label; > >>>>>> + &dentry->d_name, NULL, &shim); > >>>>>> + if (err) > >>>>>> + return NULL; > >>>>>> > >>>>>> - return NULL; > >>>>>> + label->label = shim.context; > >>>>>> + label->len = shim.len; > >>>>>> + return label; > >>>>>> } > >>>>>> static inline void > >>>>>> nfs4_label_release_security(struct nfs4_label *label) > >>>>>> { > >>>>>> - struct lsm_context scaff; /* scaffolding */ > >>>>>> + struct lsm_context shim; > >>>>>> > >>>>>> if (label) { > >>>>>> - lsmcontext_init(&scaff, label->label, label->len, 0); > >>>>>> - security_release_secctx(&scaff); > >>>>>> + shim.context = label->label; > >>>>>> + shim.len = label->len; > >>>>>> + shim.id = LSM_ID_UNDEF; > >>>>> Is there a patch that follows this one to fix this? Otherwise, setting > >>>>> this to UNDEF causes SELinux to NOT free the context, which produces a > >>>>> memory leak for every NFS inode security context. Reported by kmemleak > >>>>> when running the selinux-testsuite NFS tests. > >>>> I don't recall seeing anything related to this, but patches are > >>>> definitely welcome. > >>> Looking at this quickly, this is an interesting problem as I don't > >>> believe we have enough context in nfs4_label_release_security() to > >>> correctly set the shim.id value. If there is a positive, it is that > >>> lsm_context is really still just a string wrapped up with some > >>> metadata, e.g. length/ID, so we kfree()'ing shim.context is going to > >>> be okay-ish, at least for the foreseeable future. > >>> > >>> I can think of two ways to fix this, but I'd love to hear other ideas too. > >>> > >>> 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx() > >>> and skip any individual LSM processing. > >>> > >>> 2. Define a new LSM_ID_ANY value and update all of the LSMs to also > >>> process the ANY case as well as their own. > >>> > >>> I'm not finding either option very exciting, but option #2 looks > >>> particularly ugly, so I think I'd prefer to see someone draft a patch > >>> for option #1 assuming nothing better is presented. > >> We could perhaps add a u32 lsmid to struct nfs4_label, save it from > >> the shim.id obtained in nfs4_label_init_security(), and use it in > >> nfs4_label_release_security(). Not sure why that wasn't done in the > >> first place. > > Something like this (not tested yet). If this looks sane, will submit > > separately. > > > > commit b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") > > did not preserve the lsm id for subsequent release calls, which results > > in a memory leak. Fix it by saving the lsm id in the nfs4_label and > > providing it on the subsequent release call. > > > > Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > I'm not a fan of adding secids into other subsystems, especially in cases > where they've tried to avoid them in the past. > > The better solution, which I'm tracking down the patch for now, is for > the individual LSMs to always do their release, and for security_release_secctx() > to check the lsm_id and call the appropriate LSM specific hook. Until there > are multiple LSMs with contexts, LSM_ID_UNDEF is as good as a match. > > Please don't use this patch. It doesn't add a secid; it just saves the LSM id obtained from lsm_context populated by the security_dentry_init_security() hook call and passes it back in the lsm_context to the security_release_secctx() call. > > > --- > > fs/nfs/nfs4proc.c | 7 ++++--- > > include/linux/nfs4.h | 1 + > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > index df9669d4ded7..c0caaec7bd20 100644 > > --- a/fs/nfs/nfs4proc.c > > +++ b/fs/nfs/nfs4proc.c > > @@ -133,6 +133,7 @@ nfs4_label_init_security(struct inode *dir, struct > > dentry *dentry, > > if (err) > > return NULL; > > > > + label->lsmid = shim.id; > > label->label = shim.context; > > label->len = shim.len; > > return label; > > @@ -145,7 +146,7 @@ nfs4_label_release_security(struct nfs4_label *label) > > if (label) { > > shim.context = label->label; > > shim.len = label->len; > > - shim.id = LSM_ID_UNDEF; > > + shim.id = label->lsmid; > > security_release_secctx(&shim); > > } > > } > > @@ -6269,7 +6270,7 @@ static int _nfs4_get_security_label(struct inode > > *inode, void *buf, > > size_t buflen) > > { > > struct nfs_server *server = NFS_SERVER(inode); > > - struct nfs4_label label = {0, 0, buflen, buf}; > > + struct nfs4_label label = {0, 0, 0, buflen, buf}; > > > > u32 bitmask[3] = { 0, 0, FATTR4_WORD2_SECURITY_LABEL }; > > struct nfs_fattr fattr = { > > @@ -6374,7 +6375,7 @@ static int nfs4_do_set_security_label(struct inode *inode, > > static int > > nfs4_set_security_label(struct inode *inode, const void *buf, size_t buflen) > > { > > - struct nfs4_label ilabel = {0, 0, buflen, (char *)buf }; > > + struct nfs4_label ilabel = {0, 0, 0, buflen, (char *)buf }; > > struct nfs_fattr *fattr; > > int status; > > > > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > > index 71fbebfa43c7..9ac83ca88326 100644 > > --- a/include/linux/nfs4.h > > +++ b/include/linux/nfs4.h > > @@ -47,6 +47,7 @@ struct nfs4_acl { > > struct nfs4_label { > > uint32_t lfs; > > uint32_t pi; > > + u32 lsmid; > > u32 len; > > char *label; > > };
On 2/20/2025 11:37 AM, Stephen Smalley wrote: > On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 2/20/2025 10:16 AM, Stephen Smalley wrote: >>> On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley >>> <stephen.smalley.work@gmail.com> wrote: >>>> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote: >>>>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: >>>>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley >>>>>> <stephen.smalley.work@gmail.com> wrote: >>>>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>>>>> Replace the (secctx,seclen) pointer pair with a single lsm_context >>>>>>>> pointer to allow return of the LSM identifier along with the context >>>>>>>> and context length. This allows security_release_secctx() to know how >>>>>>>> to release the context. Callers have been modified to use or save the >>>>>>>> returned data from the new structure. >>>>>>>> >>>>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>>>>>> Cc: ceph-devel@vger.kernel.org >>>>>>>> Cc: linux-nfs@vger.kernel.org >>>>>>>> --- >>>>>>>> fs/ceph/super.h | 3 +-- >>>>>>>> fs/ceph/xattr.c | 16 ++++++---------- >>>>>>>> fs/fuse/dir.c | 35 ++++++++++++++++++----------------- >>>>>>>> fs/nfs/nfs4proc.c | 20 ++++++++++++-------- >>>>>>>> include/linux/lsm_hook_defs.h | 2 +- >>>>>>>> include/linux/security.h | 26 +++----------------------- >>>>>>>> security/security.c | 9 ++++----- >>>>>>>> security/selinux/hooks.c | 9 +++++---- >>>>>>>> 8 files changed, 50 insertions(+), 70 deletions(-) >>>>>>>> >>>>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>>>>>>> index 76776d716744..0b116ef3a752 100644 >>>>>>>> --- a/fs/nfs/nfs4proc.c >>>>>>>> +++ b/fs/nfs/nfs4proc.c >>>>>>>> @@ -114,6 +114,7 @@ static inline struct nfs4_label * >>>>>>>> nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>>>>>>> struct iattr *sattr, struct nfs4_label *label) >>>>>>>> { >>>>>>>> + struct lsm_context shim; >>>>>>>> int err; >>>>>>>> >>>>>>>> if (label == NULL) >>>>>>>> @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>>>>>>> label->label = NULL; >>>>>>>> >>>>>>>> err = security_dentry_init_security(dentry, sattr->ia_mode, >>>>>>>> - &dentry->d_name, NULL, >>>>>>>> - (void **)&label->label, &label->len); >>>>>>>> - if (err == 0) >>>>>>>> - return label; >>>>>>>> + &dentry->d_name, NULL, &shim); >>>>>>>> + if (err) >>>>>>>> + return NULL; >>>>>>>> >>>>>>>> - return NULL; >>>>>>>> + label->label = shim.context; >>>>>>>> + label->len = shim.len; >>>>>>>> + return label; >>>>>>>> } >>>>>>>> static inline void >>>>>>>> nfs4_label_release_security(struct nfs4_label *label) >>>>>>>> { >>>>>>>> - struct lsm_context scaff; /* scaffolding */ >>>>>>>> + struct lsm_context shim; >>>>>>>> >>>>>>>> if (label) { >>>>>>>> - lsmcontext_init(&scaff, label->label, label->len, 0); >>>>>>>> - security_release_secctx(&scaff); >>>>>>>> + shim.context = label->label; >>>>>>>> + shim.len = label->len; >>>>>>>> + shim.id = LSM_ID_UNDEF; >>>>>>> Is there a patch that follows this one to fix this? Otherwise, setting >>>>>>> this to UNDEF causes SELinux to NOT free the context, which produces a >>>>>>> memory leak for every NFS inode security context. Reported by kmemleak >>>>>>> when running the selinux-testsuite NFS tests. >>>>>> I don't recall seeing anything related to this, but patches are >>>>>> definitely welcome. >>>>> Looking at this quickly, this is an interesting problem as I don't >>>>> believe we have enough context in nfs4_label_release_security() to >>>>> correctly set the shim.id value. If there is a positive, it is that >>>>> lsm_context is really still just a string wrapped up with some >>>>> metadata, e.g. length/ID, so we kfree()'ing shim.context is going to >>>>> be okay-ish, at least for the foreseeable future. >>>>> >>>>> I can think of two ways to fix this, but I'd love to hear other ideas too. >>>>> >>>>> 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx() >>>>> and skip any individual LSM processing. >>>>> >>>>> 2. Define a new LSM_ID_ANY value and update all of the LSMs to also >>>>> process the ANY case as well as their own. >>>>> >>>>> I'm not finding either option very exciting, but option #2 looks >>>>> particularly ugly, so I think I'd prefer to see someone draft a patch >>>>> for option #1 assuming nothing better is presented. >>>> We could perhaps add a u32 lsmid to struct nfs4_label, save it from >>>> the shim.id obtained in nfs4_label_init_security(), and use it in >>>> nfs4_label_release_security(). Not sure why that wasn't done in the >>>> first place. >>> Something like this (not tested yet). If this looks sane, will submit >>> separately. >>> >>> commit b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") >>> did not preserve the lsm id for subsequent release calls, which results >>> in a memory leak. Fix it by saving the lsm id in the nfs4_label and >>> providing it on the subsequent release call. >>> >>> Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") >>> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> >> I'm not a fan of adding secids into other subsystems, especially in cases >> where they've tried to avoid them in the past. >> >> The better solution, which I'm tracking down the patch for now, is for >> the individual LSMs to always do their release, and for security_release_secctx() >> to check the lsm_id and call the appropriate LSM specific hook. Until there >> are multiple LSMs with contexts, LSM_ID_UNDEF is as good as a match. >> >> Please don't use this patch. > It doesn't add a secid; it just saves the LSM id obtained from > lsm_context populated by the security_dentry_init_security() hook call > and passes it back in the lsm_context to the security_release_secctx() > call. Right. Sorry. If you're going to do that, the nfs_label struct should just include a lsm_context instead. But that hit opposition when proposed initially. The practical solution has to acknowledge that at this stage there can only be one LSM providing contexts, and each LSM can release the context if the LSM is matches the LSM or is LSM_ID_UNDEF. That will change before SELinux, AppArmor and Smack can co-exist, but that's not yet available. For now the check if (cp->id == LSM_ID_SELINUX) can either be removed or changed to if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF) In a system that respects LSM_FLAG_LEGACY_MAJOR the id isn't relevant with the context using LSMs all being thus identified. > >>> --- >>> fs/nfs/nfs4proc.c | 7 ++++--- >>> include/linux/nfs4.h | 1 + >>> 2 files changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>> index df9669d4ded7..c0caaec7bd20 100644 >>> --- a/fs/nfs/nfs4proc.c >>> +++ b/fs/nfs/nfs4proc.c >>> @@ -133,6 +133,7 @@ nfs4_label_init_security(struct inode *dir, struct >>> dentry *dentry, >>> if (err) >>> return NULL; >>> >>> + label->lsmid = shim.id; >>> label->label = shim.context; >>> label->len = shim.len; >>> return label; >>> @@ -145,7 +146,7 @@ nfs4_label_release_security(struct nfs4_label *label) >>> if (label) { >>> shim.context = label->label; >>> shim.len = label->len; >>> - shim.id = LSM_ID_UNDEF; >>> + shim.id = label->lsmid; >>> security_release_secctx(&shim); >>> } >>> } >>> @@ -6269,7 +6270,7 @@ static int _nfs4_get_security_label(struct inode >>> *inode, void *buf, >>> size_t buflen) >>> { >>> struct nfs_server *server = NFS_SERVER(inode); >>> - struct nfs4_label label = {0, 0, buflen, buf}; >>> + struct nfs4_label label = {0, 0, 0, buflen, buf}; >>> >>> u32 bitmask[3] = { 0, 0, FATTR4_WORD2_SECURITY_LABEL }; >>> struct nfs_fattr fattr = { >>> @@ -6374,7 +6375,7 @@ static int nfs4_do_set_security_label(struct inode *inode, >>> static int >>> nfs4_set_security_label(struct inode *inode, const void *buf, size_t buflen) >>> { >>> - struct nfs4_label ilabel = {0, 0, buflen, (char *)buf }; >>> + struct nfs4_label ilabel = {0, 0, 0, buflen, (char *)buf }; >>> struct nfs_fattr *fattr; >>> int status; >>> >>> diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h >>> index 71fbebfa43c7..9ac83ca88326 100644 >>> --- a/include/linux/nfs4.h >>> +++ b/include/linux/nfs4.h >>> @@ -47,6 +47,7 @@ struct nfs4_acl { >>> struct nfs4_label { >>> uint32_t lfs; >>> uint32_t pi; >>> + u32 lsmid; >>> u32 len; >>> char *label; >>> };
On Thu, Feb 20, 2025 at 3:31 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 2/20/2025 11:37 AM, Stephen Smalley wrote: > > On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 2/20/2025 10:16 AM, Stephen Smalley wrote: > >>> On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley > >>> <stephen.smalley.work@gmail.com> wrote: > >>>> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote: > >>>>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: > >>>>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley > >>>>>> <stephen.smalley.work@gmail.com> wrote: > >>>>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >>>>>>>> Replace the (secctx,seclen) pointer pair with a single lsm_context > >>>>>>>> pointer to allow return of the LSM identifier along with the context > >>>>>>>> and context length. This allows security_release_secctx() to know how > >>>>>>>> to release the context. Callers have been modified to use or save the > >>>>>>>> returned data from the new structure. > >>>>>>>> > >>>>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > >>>>>>>> Cc: ceph-devel@vger.kernel.org > >>>>>>>> Cc: linux-nfs@vger.kernel.org > >>>>>>>> --- > >>>>>>>> fs/ceph/super.h | 3 +-- > >>>>>>>> fs/ceph/xattr.c | 16 ++++++---------- > >>>>>>>> fs/fuse/dir.c | 35 ++++++++++++++++++----------------- > >>>>>>>> fs/nfs/nfs4proc.c | 20 ++++++++++++-------- > >>>>>>>> include/linux/lsm_hook_defs.h | 2 +- > >>>>>>>> include/linux/security.h | 26 +++----------------------- > >>>>>>>> security/security.c | 9 ++++----- > >>>>>>>> security/selinux/hooks.c | 9 +++++---- > >>>>>>>> 8 files changed, 50 insertions(+), 70 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > >>>>>>>> index 76776d716744..0b116ef3a752 100644 > >>>>>>>> --- a/fs/nfs/nfs4proc.c > >>>>>>>> +++ b/fs/nfs/nfs4proc.c > >>>>>>>> @@ -114,6 +114,7 @@ static inline struct nfs4_label * > >>>>>>>> nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > >>>>>>>> struct iattr *sattr, struct nfs4_label *label) > >>>>>>>> { > >>>>>>>> + struct lsm_context shim; > >>>>>>>> int err; > >>>>>>>> > >>>>>>>> if (label == NULL) > >>>>>>>> @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > >>>>>>>> label->label = NULL; > >>>>>>>> > >>>>>>>> err = security_dentry_init_security(dentry, sattr->ia_mode, > >>>>>>>> - &dentry->d_name, NULL, > >>>>>>>> - (void **)&label->label, &label->len); > >>>>>>>> - if (err == 0) > >>>>>>>> - return label; > >>>>>>>> + &dentry->d_name, NULL, &shim); > >>>>>>>> + if (err) > >>>>>>>> + return NULL; > >>>>>>>> > >>>>>>>> - return NULL; > >>>>>>>> + label->label = shim.context; > >>>>>>>> + label->len = shim.len; > >>>>>>>> + return label; > >>>>>>>> } > >>>>>>>> static inline void > >>>>>>>> nfs4_label_release_security(struct nfs4_label *label) > >>>>>>>> { > >>>>>>>> - struct lsm_context scaff; /* scaffolding */ > >>>>>>>> + struct lsm_context shim; > >>>>>>>> > >>>>>>>> if (label) { > >>>>>>>> - lsmcontext_init(&scaff, label->label, label->len, 0); > >>>>>>>> - security_release_secctx(&scaff); > >>>>>>>> + shim.context = label->label; > >>>>>>>> + shim.len = label->len; > >>>>>>>> + shim.id = LSM_ID_UNDEF; > >>>>>>> Is there a patch that follows this one to fix this? Otherwise, setting > >>>>>>> this to UNDEF causes SELinux to NOT free the context, which produces a > >>>>>>> memory leak for every NFS inode security context. Reported by kmemleak > >>>>>>> when running the selinux-testsuite NFS tests. > >>>>>> I don't recall seeing anything related to this, but patches are > >>>>>> definitely welcome. > >>>>> Looking at this quickly, this is an interesting problem as I don't > >>>>> believe we have enough context in nfs4_label_release_security() to > >>>>> correctly set the shim.id value. If there is a positive, it is that > >>>>> lsm_context is really still just a string wrapped up with some > >>>>> metadata, e.g. length/ID, so we kfree()'ing shim.context is going to > >>>>> be okay-ish, at least for the foreseeable future. > >>>>> > >>>>> I can think of two ways to fix this, but I'd love to hear other ideas too. > >>>>> > >>>>> 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx() > >>>>> and skip any individual LSM processing. > >>>>> > >>>>> 2. Define a new LSM_ID_ANY value and update all of the LSMs to also > >>>>> process the ANY case as well as their own. > >>>>> > >>>>> I'm not finding either option very exciting, but option #2 looks > >>>>> particularly ugly, so I think I'd prefer to see someone draft a patch > >>>>> for option #1 assuming nothing better is presented. > >>>> We could perhaps add a u32 lsmid to struct nfs4_label, save it from > >>>> the shim.id obtained in nfs4_label_init_security(), and use it in > >>>> nfs4_label_release_security(). Not sure why that wasn't done in the > >>>> first place. > >>> Something like this (not tested yet). If this looks sane, will submit > >>> separately. > >>> > >>> commit b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") > >>> did not preserve the lsm id for subsequent release calls, which results > >>> in a memory leak. Fix it by saving the lsm id in the nfs4_label and > >>> providing it on the subsequent release call. > >>> > >>> Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") > >>> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > >> I'm not a fan of adding secids into other subsystems, especially in cases > >> where they've tried to avoid them in the past. > >> > >> The better solution, which I'm tracking down the patch for now, is for > >> the individual LSMs to always do their release, and for security_release_secctx() > >> to check the lsm_id and call the appropriate LSM specific hook. Until there > >> are multiple LSMs with contexts, LSM_ID_UNDEF is as good as a match. > >> > >> Please don't use this patch. > > It doesn't add a secid; it just saves the LSM id obtained from > > lsm_context populated by the security_dentry_init_security() hook call > > and passes it back in the lsm_context to the security_release_secctx() > > call. > > Right. Sorry. If you're going to do that, the nfs_label struct should > just include a lsm_context instead. But that hit opposition when proposed > initially. > > The practical solution has to acknowledge that at this stage there can only > be one LSM providing contexts, and each LSM can release the context if the > LSM is matches the LSM or is LSM_ID_UNDEF. That will change before SELinux, > AppArmor and Smack can co-exist, but that's not yet available. For now the > check > > if (cp->id == LSM_ID_SELINUX) > > can either be removed or changed to > > if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF) > > In a system that respects LSM_FLAG_LEGACY_MAJOR the id isn't relevant > with the context using LSMs all being thus identified. Shrug. My patch seemed cleaner, but I don't really care as long as it is fixed, preferably before 6.14 goes final.
On 2/20/2025 12:33 PM, Stephen Smalley wrote: > On Thu, Feb 20, 2025 at 3:31 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 2/20/2025 11:37 AM, Stephen Smalley wrote: >>> On Thu, Feb 20, 2025 at 2:33 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> On 2/20/2025 10:16 AM, Stephen Smalley wrote: >>>>> On Thu, Feb 20, 2025 at 1:02 PM Stephen Smalley >>>>> <stephen.smalley.work@gmail.com> wrote: >>>>>> On Thu, Feb 20, 2025 at 12:54 PM Paul Moore <paul@paul-moore.com> wrote: >>>>>>> On Thu, Feb 20, 2025 at 12:40 PM Paul Moore <paul@paul-moore.com> wrote: >>>>>>>> On Thu, Feb 20, 2025 at 11:43 AM Stephen Smalley >>>>>>>> <stephen.smalley.work@gmail.com> wrote: >>>>>>>>> On Wed, Oct 23, 2024 at 5:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>>>>>>> Replace the (secctx,seclen) pointer pair with a single lsm_context >>>>>>>>>> pointer to allow return of the LSM identifier along with the context >>>>>>>>>> and context length. This allows security_release_secctx() to know how >>>>>>>>>> to release the context. Callers have been modified to use or save the >>>>>>>>>> returned data from the new structure. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>>>>>>>> Cc: ceph-devel@vger.kernel.org >>>>>>>>>> Cc: linux-nfs@vger.kernel.org >>>>>>>>>> --- >>>>>>>>>> fs/ceph/super.h | 3 +-- >>>>>>>>>> fs/ceph/xattr.c | 16 ++++++---------- >>>>>>>>>> fs/fuse/dir.c | 35 ++++++++++++++++++----------------- >>>>>>>>>> fs/nfs/nfs4proc.c | 20 ++++++++++++-------- >>>>>>>>>> include/linux/lsm_hook_defs.h | 2 +- >>>>>>>>>> include/linux/security.h | 26 +++----------------------- >>>>>>>>>> security/security.c | 9 ++++----- >>>>>>>>>> security/selinux/hooks.c | 9 +++++---- >>>>>>>>>> 8 files changed, 50 insertions(+), 70 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>>>>>>>>> index 76776d716744..0b116ef3a752 100644 >>>>>>>>>> --- a/fs/nfs/nfs4proc.c >>>>>>>>>> +++ b/fs/nfs/nfs4proc.c >>>>>>>>>> @@ -114,6 +114,7 @@ static inline struct nfs4_label * >>>>>>>>>> nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>>>>>>>>> struct iattr *sattr, struct nfs4_label *label) >>>>>>>>>> { >>>>>>>>>> + struct lsm_context shim; >>>>>>>>>> int err; >>>>>>>>>> >>>>>>>>>> if (label == NULL) >>>>>>>>>> @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>>>>>>>>> label->label = NULL; >>>>>>>>>> >>>>>>>>>> err = security_dentry_init_security(dentry, sattr->ia_mode, >>>>>>>>>> - &dentry->d_name, NULL, >>>>>>>>>> - (void **)&label->label, &label->len); >>>>>>>>>> - if (err == 0) >>>>>>>>>> - return label; >>>>>>>>>> + &dentry->d_name, NULL, &shim); >>>>>>>>>> + if (err) >>>>>>>>>> + return NULL; >>>>>>>>>> >>>>>>>>>> - return NULL; >>>>>>>>>> + label->label = shim.context; >>>>>>>>>> + label->len = shim.len; >>>>>>>>>> + return label; >>>>>>>>>> } >>>>>>>>>> static inline void >>>>>>>>>> nfs4_label_release_security(struct nfs4_label *label) >>>>>>>>>> { >>>>>>>>>> - struct lsm_context scaff; /* scaffolding */ >>>>>>>>>> + struct lsm_context shim; >>>>>>>>>> >>>>>>>>>> if (label) { >>>>>>>>>> - lsmcontext_init(&scaff, label->label, label->len, 0); >>>>>>>>>> - security_release_secctx(&scaff); >>>>>>>>>> + shim.context = label->label; >>>>>>>>>> + shim.len = label->len; >>>>>>>>>> + shim.id = LSM_ID_UNDEF; >>>>>>>>> Is there a patch that follows this one to fix this? Otherwise, setting >>>>>>>>> this to UNDEF causes SELinux to NOT free the context, which produces a >>>>>>>>> memory leak for every NFS inode security context. Reported by kmemleak >>>>>>>>> when running the selinux-testsuite NFS tests. >>>>>>>> I don't recall seeing anything related to this, but patches are >>>>>>>> definitely welcome. >>>>>>> Looking at this quickly, this is an interesting problem as I don't >>>>>>> believe we have enough context in nfs4_label_release_security() to >>>>>>> correctly set the shim.id value. If there is a positive, it is that >>>>>>> lsm_context is really still just a string wrapped up with some >>>>>>> metadata, e.g. length/ID, so we kfree()'ing shim.context is going to >>>>>>> be okay-ish, at least for the foreseeable future. >>>>>>> >>>>>>> I can think of two ways to fix this, but I'd love to hear other ideas too. >>>>>>> >>>>>>> 1. Handle the LSM_ID_UNDEF case directly in security_release_secctx() >>>>>>> and skip any individual LSM processing. >>>>>>> >>>>>>> 2. Define a new LSM_ID_ANY value and update all of the LSMs to also >>>>>>> process the ANY case as well as their own. >>>>>>> >>>>>>> I'm not finding either option very exciting, but option #2 looks >>>>>>> particularly ugly, so I think I'd prefer to see someone draft a patch >>>>>>> for option #1 assuming nothing better is presented. >>>>>> We could perhaps add a u32 lsmid to struct nfs4_label, save it from >>>>>> the shim.id obtained in nfs4_label_init_security(), and use it in >>>>>> nfs4_label_release_security(). Not sure why that wasn't done in the >>>>>> first place. >>>>> Something like this (not tested yet). If this looks sane, will submit >>>>> separately. >>>>> >>>>> commit b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") >>>>> did not preserve the lsm id for subsequent release calls, which results >>>>> in a memory leak. Fix it by saving the lsm id in the nfs4_label and >>>>> providing it on the subsequent release call. >>>>> >>>>> Fixes: b530104f50e8 ("lsm: lsm_context in security_dentry_init_security") >>>>> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> >>>> I'm not a fan of adding secids into other subsystems, especially in cases >>>> where they've tried to avoid them in the past. >>>> >>>> The better solution, which I'm tracking down the patch for now, is for >>>> the individual LSMs to always do their release, and for security_release_secctx() >>>> to check the lsm_id and call the appropriate LSM specific hook. Until there >>>> are multiple LSMs with contexts, LSM_ID_UNDEF is as good as a match. >>>> >>>> Please don't use this patch. >>> It doesn't add a secid; it just saves the LSM id obtained from >>> lsm_context populated by the security_dentry_init_security() hook call >>> and passes it back in the lsm_context to the security_release_secctx() >>> call. >> Right. Sorry. If you're going to do that, the nfs_label struct should >> just include a lsm_context instead. But that hit opposition when proposed >> initially. >> >> The practical solution has to acknowledge that at this stage there can only >> be one LSM providing contexts, and each LSM can release the context if the >> LSM is matches the LSM or is LSM_ID_UNDEF. That will change before SELinux, >> AppArmor and Smack can co-exist, but that's not yet available. For now the >> check >> >> if (cp->id == LSM_ID_SELINUX) >> >> can either be removed or changed to >> >> if (cp->id == LSM_ID_SELINUX || cp->id == LSM_ID_UNDEF) >> >> In a system that respects LSM_FLAG_LEGACY_MAJOR the id isn't relevant >> with the context using LSMs all being thus identified. > Shrug. My patch seemed cleaner, Adding the lsm_context was cleaner. Not worth yet another roadblock. I will have a patch asap. I'm dealing with a facilities issue at Smack Labs (whole site being painted, everything in disarray) that is slowing things down. > but I don't really care as long as it > is fixed, preferably before 6.14 goes final. >
On Thu, Feb 20, 2025 at 4:08 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > Adding the lsm_context was cleaner. Not worth yet another roadblock. > I will have a patch asap. I'm dealing with a facilities issue at > Smack Labs (whole site being painted, everything in disarray) that > is slowing things down. Sorry for the delay today, I was distracted by some meetings and wrestling with gcc v15 on my Fedora Rawhide dev/test system while I try to test the LSM init patchset. Anyway, I'll add my comments to Stephen's formal patch posting.
diff --git a/fs/ceph/super.h b/fs/ceph/super.h index 2508aa8950b7..c9fad8c825dd 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -1133,8 +1133,7 @@ struct ceph_acl_sec_ctx { void *acl; #endif #ifdef CONFIG_CEPH_FS_SECURITY_LABEL - void *sec_ctx; - u32 sec_ctxlen; + struct lsm_context lsmctx; #endif #ifdef CONFIG_FS_ENCRYPTION struct ceph_fscrypt_auth *fscrypt_auth; diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c index f7996770cc2c..0b9e1f385d31 100644 --- a/fs/ceph/xattr.c +++ b/fs/ceph/xattr.c @@ -1383,8 +1383,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode, int err; err = security_dentry_init_security(dentry, mode, &dentry->d_name, - &name, &as_ctx->sec_ctx, - &as_ctx->sec_ctxlen); + &name, &as_ctx->lsmctx); if (err < 0) { WARN_ON_ONCE(err != -EOPNOTSUPP); err = 0; /* do nothing */ @@ -1409,7 +1408,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode, */ name_len = strlen(name); err = ceph_pagelist_reserve(pagelist, - 4 * 2 + name_len + as_ctx->sec_ctxlen); + 4 * 2 + name_len + as_ctx->lsmctx.len); if (err) goto out; @@ -1432,8 +1431,9 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode, ceph_pagelist_encode_32(pagelist, name_len); ceph_pagelist_append(pagelist, name, name_len); - ceph_pagelist_encode_32(pagelist, as_ctx->sec_ctxlen); - ceph_pagelist_append(pagelist, as_ctx->sec_ctx, as_ctx->sec_ctxlen); + ceph_pagelist_encode_32(pagelist, as_ctx->lsmctx.len); + ceph_pagelist_append(pagelist, as_ctx->lsmctx.context, + as_ctx->lsmctx.len); err = 0; out: @@ -1446,16 +1446,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode, void ceph_release_acl_sec_ctx(struct ceph_acl_sec_ctx *as_ctx) { -#ifdef CONFIG_CEPH_FS_SECURITY_LABEL - struct lsm_context scaff; /* scaffolding */ -#endif #ifdef CONFIG_CEPH_FS_POSIX_ACL posix_acl_release(as_ctx->acl); posix_acl_release(as_ctx->default_acl); #endif #ifdef CONFIG_CEPH_FS_SECURITY_LABEL - lsmcontext_init(&scaff, as_ctx->sec_ctx, as_ctx->sec_ctxlen, 0); - security_release_secctx(&scaff); + security_release_secctx(&as_ctx->lsmctx); #endif #ifdef CONFIG_FS_ENCRYPTION kfree(as_ctx->fscrypt_auth); diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 54104dd48af7..eea4d0d27ce1 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -466,29 +466,29 @@ static int get_security_context(struct dentry *entry, umode_t mode, { struct fuse_secctx *fctx; struct fuse_secctx_header *header; - void *ctx = NULL, *ptr; - u32 ctxlen, total_len = sizeof(*header); + struct lsm_context lsmctx = { }; + void *ptr; + u32 total_len = sizeof(*header); int err, nr_ctx = 0; - const char *name; + const char *name = NULL; size_t namelen; err = security_dentry_init_security(entry, mode, &entry->d_name, - &name, &ctx, &ctxlen); - if (err) { - if (err != -EOPNOTSUPP) - goto out_err; - /* No LSM is supporting this security hook. Ignore error */ - ctxlen = 0; - ctx = NULL; - } + &name, &lsmctx); + + /* If no LSM is supporting this security hook ignore error */ + if (err && err != -EOPNOTSUPP) + goto out_err; - if (ctxlen) { + if (lsmctx.len) { nr_ctx = 1; namelen = strlen(name) + 1; err = -EIO; - if (WARN_ON(namelen > XATTR_NAME_MAX + 1 || ctxlen > S32_MAX)) + if (WARN_ON(namelen > XATTR_NAME_MAX + 1 || + lsmctx.len > S32_MAX)) goto out_err; - total_len += FUSE_REC_ALIGN(sizeof(*fctx) + namelen + ctxlen); + total_len += FUSE_REC_ALIGN(sizeof(*fctx) + namelen + + lsmctx.len); } err = -ENOMEM; @@ -501,19 +501,20 @@ static int get_security_context(struct dentry *entry, umode_t mode, ptr += sizeof(*header); if (nr_ctx) { fctx = ptr; - fctx->size = ctxlen; + fctx->size = lsmctx.len; ptr += sizeof(*fctx); strcpy(ptr, name); ptr += namelen; - memcpy(ptr, ctx, ctxlen); + memcpy(ptr, lsmctx.context, lsmctx.len); } ext->size = total_len; ext->value = header; err = 0; out_err: - kfree(ctx); + if (nr_ctx) + security_release_secctx(&lsmctx); return err; } diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 76776d716744..0b116ef3a752 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -114,6 +114,7 @@ static inline struct nfs4_label * nfs4_label_init_security(struct inode *dir, struct dentry *dentry, struct iattr *sattr, struct nfs4_label *label) { + struct lsm_context shim; int err; if (label == NULL) @@ -128,21 +129,24 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, label->label = NULL; err = security_dentry_init_security(dentry, sattr->ia_mode, - &dentry->d_name, NULL, - (void **)&label->label, &label->len); - if (err == 0) - return label; + &dentry->d_name, NULL, &shim); + if (err) + return NULL; - return NULL; + label->label = shim.context; + label->len = shim.len; + return label; } static inline void nfs4_label_release_security(struct nfs4_label *label) { - struct lsm_context scaff; /* scaffolding */ + struct lsm_context shim; if (label) { - lsmcontext_init(&scaff, label->label, label->len, 0); - security_release_secctx(&scaff); + shim.context = label->label; + shim.len = label->len; + shim.id = LSM_ID_UNDEF; + security_release_secctx(&shim); } } static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label) diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 69e1076448c6..e2f1ce37c41e 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -83,7 +83,7 @@ LSM_HOOK(int, 0, move_mount, const struct path *from_path, const struct path *to_path) LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry, int mode, const struct qstr *name, const char **xattr_name, - void **ctx, u32 *ctxlen) + struct lsm_context *cp) LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode, struct qstr *name, const struct cred *old, struct cred *new) diff --git a/include/linux/security.h b/include/linux/security.h index 7d0adc1833ab..3ad59666e56c 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -237,25 +237,6 @@ struct lsm_context { int id; /* Identifies the module */ }; -/** - * lsmcontext_init - initialize an lsmcontext structure. - * @cp: Pointer to the context to initialize - * @context: Initial context, or NULL - * @size: Size of context, or 0 - * @id: Which LSM provided the context - * - * Fill in the lsmcontext from the provided information. - * This is a scaffolding function that will be removed when - * lsm_context integration is complete. - */ -static inline void lsmcontext_init(struct lsm_context *cp, char *context, - u32 size, int id) -{ - cp->id = id; - cp->context = context; - cp->len = size; -} - /* * Values used in the task_security_ops calls */ @@ -409,8 +390,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, int security_move_mount(const struct path *from_path, const struct path *to_path); int security_dentry_init_security(struct dentry *dentry, int mode, const struct qstr *name, - const char **xattr_name, void **ctx, - u32 *ctxlen); + const char **xattr_name, + struct lsm_context *lsmcxt); int security_dentry_create_files_as(struct dentry *dentry, int mode, struct qstr *name, const struct cred *old, @@ -883,8 +864,7 @@ static inline int security_dentry_init_security(struct dentry *dentry, int mode, const struct qstr *name, const char **xattr_name, - void **ctx, - u32 *ctxlen) + struct lsm_context *lsmcxt) { return -EOPNOTSUPP; } diff --git a/security/security.c b/security/security.c index 4ca3c9e28b6f..1d57e4e1bceb 100644 --- a/security/security.c +++ b/security/security.c @@ -1734,8 +1734,7 @@ void security_inode_free(struct inode *inode) * @mode: mode used to determine resource type * @name: name of the last path component * @xattr_name: name of the security/LSM xattr - * @ctx: pointer to the resulting LSM context - * @ctxlen: length of @ctx + * @lsmctx: pointer to the resulting LSM context * * Compute a context for a dentry as the inode is not yet available since NFSv4 * has no label backed by an EA anyway. It is important to note that @@ -1745,11 +1744,11 @@ void security_inode_free(struct inode *inode) */ int security_dentry_init_security(struct dentry *dentry, int mode, const struct qstr *name, - const char **xattr_name, void **ctx, - u32 *ctxlen) + const char **xattr_name, + struct lsm_context *lsmctx) { return call_int_hook(dentry_init_security, dentry, mode, name, - xattr_name, ctx, ctxlen); + xattr_name, lsmctx); } EXPORT_SYMBOL(security_dentry_init_security); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index ce5e45abd8d3..79776a5e651d 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2869,8 +2869,8 @@ static void selinux_inode_free_security(struct inode *inode) static int selinux_dentry_init_security(struct dentry *dentry, int mode, const struct qstr *name, - const char **xattr_name, void **ctx, - u32 *ctxlen) + const char **xattr_name, + struct lsm_context *cp) { u32 newsid; int rc; @@ -2885,8 +2885,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, if (xattr_name) *xattr_name = XATTR_NAME_SELINUX; - return security_sid_to_context(newsid, (char **)ctx, - ctxlen); + cp->id = LSM_ID_SELINUX; + return security_sid_to_context(newsid, (char **)cp->context, + &cp->len); } static int selinux_dentry_create_files_as(struct dentry *dentry, int mode,
Replace the (secctx,seclen) pointer pair with a single lsm_context pointer to allow return of the LSM identifier along with the context and context length. This allows security_release_secctx() to know how to release the context. Callers have been modified to use or save the returned data from the new structure. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> Cc: ceph-devel@vger.kernel.org Cc: linux-nfs@vger.kernel.org --- fs/ceph/super.h | 3 +-- fs/ceph/xattr.c | 16 ++++++---------- fs/fuse/dir.c | 35 ++++++++++++++++++----------------- fs/nfs/nfs4proc.c | 20 ++++++++++++-------- include/linux/lsm_hook_defs.h | 2 +- include/linux/security.h | 26 +++----------------------- security/security.c | 9 ++++----- security/selinux/hooks.c | 9 +++++---- 8 files changed, 50 insertions(+), 70 deletions(-)